kqueue: Avoid enqueuing an already-enqueued knote

[markj]

Summary

knotes with a non-trivial f_copy implementation may be activated before
kqueue_fork_copy_knote() is finished. In particular, it may be enqueued
at the time that kqueue_fork_copy_knote() calls knote_enqueue(). Guard
against this and add a test case which triggers the race. Note, the race
can be exploited to obtain root privileges.

Also make sure that the new knote does not inherit the KN_SCAN flag.

[kevans]

sys/kern/kern_event.c

3149

I wonder if kqueue(2) should talk about the race here at all. It seems that an application using KQUEUE_CPONFORK would need to take care to quiesce activities with it during a fork if the child may be sensitive to losing events, since we could seemingly race against a concurent scan that peeled the event off.

It doesn't really matter for timers since those are unconditionally re-armed in their f_copy, but maybe of interest for some others. The current wording leaves it kind of ambguous whether we try to handle this at all, I think

[kib]

sys/kern/kern_event.c

3114–3116

This is right, of course.

3149

I do not quite see why the race should be handled? Why not just clear KN_QUEUED together with other flags?

[kevans]

sys/kern/kern_event.c

3149

To be clear: why not just drop these lines entirely and clear KN_QUEUED / KN_SCAN if they because of a race, or something else?

I would agree if the suggestion is to stop activating any of them altogether, and we just document that pending events in the child are deactivated.

[kib]

sys/kern/kern_event.c

3149

kn1->kn_status &= ~(KN_QUEUED | KN_SCAN | KN_QUEUED);

is what I mean precisely.

[markj]

sys/kern/kern_event.c

3149

Suppose the parent has a EVFILT_READ knote for a pipe. Suppose some data arrives on the pipe, so pipeselwakeup() activates the knote. What happens if this races with a fork? Specifically, suppose the child knote is added to the pipe just after the pipeselwakeup() call finishes. Then the parent knote will be queued, but the child knote will not, so the child will not observe the event during a kqueue scan.

To me it makes sense that the child knote should be enqueued in this scenario. Kyle, do you mean that the child process should manually enable each event after the fork?

[kib]

sys/kern/kern_event.c

3149

And in fact I think that not activating the knotes in child is more correct behavior. Suppose that the knote was activated before the fork(), and it is provable by the code. E.g. the action that activates was performed in the same thread that does fork(). Then I argue that the child' kqueue knote should be not activated.

We cannot distinguish between racy activation vs. happens-before.

[kib]

sys/kern/kern_event.c

3149

And what if the racing events were tilted slightly differently and fork occured after the pipeselwakeup()? How could an app prove that we mis-handled the activation?

[kevans]

sys/kern/kern_event.c

3149

Sorry, bad choice of wording: not deactivated, but actively dequeud.

[markj]

sys/kern/kern_event.c

3149

And what if the racing events were tilted slightly differently and fork occured after the pipeselwakeup()? How could an app prove that we mis-handled the activation?

I don't understand how it is mis-handled. The child knote should be enqueued in that case: the pipe has data available for reading. The event is level-triggered, so if we do not enqueue the knote here, when will it be enqueued? In particular, what if the child is responsible for reading data from the pipe?

[kib]

sys/kern/kern_event.c

3149

But then the right choice is arguably per-knote type. Would it be more correct to do something like this (copied from the scan loop):

	event = kn1->kn_fop->f_event(kn, 0);
	if (event != 0)
		kn1->kn_status |= KN_ACTIVE;
	if ((kn1->kn_status & (KN_ACTIVE | KN_DISABLED) == KN_ACTIVE)
		knote_enqueue(kn1);

Probably f_touch() is needed as well.

[markj]

sys/kern/kern_event.c

3149

Yes, it somewhat makes sense to behave as though kqueue_register() was just called.

But, shouldn't we also be calling the original filter's f_attach method too? They are mostly approximated by calling knlist_add() directly, but that might not be sufficient...

[markj]

Test the knote in order to decide whether to enqueue it.

Hold locks when copying the source knote.

Skip marker knotes.

Do not copy kqueue knotes, they cannot be copied safely. A kqueue must
be private to the process, e.g., because struct kqueue carries a pointer
to the process' fd table.

Add some assertions.

[kib]

sys/kern/kern_event.c

3094

The intent there was that kqueue cannot be referenced indeed, but it could be copied into the child. Then, if another kqueue is also copied and has a knote referencing the first kqueue, then we copy the knote pointing it to the kqueue copy.

[markj]

sys/kern/kern_event.c

3094

I think it could be done, but for the purpose of this patch (which is needed for an SA) I suspect it would be better to avoid this. Do you want me to implement that as a follow up?

[kib]

sys/kern/kern_event.c

3094

Ok, we (I) can handle it later.