v4l2-event: Remove pending events from fh event queue when unsubscribing

Hi,

hverkuil wrote:

Ah, yes. You're right v4l2-ctrls.c may still hold a reference after releasing the spinlock.

*But* setting sev->fh to NULL and checking for this in v4l2-ctrls: send_event(), or doing something similar, is not only not needed it is outright wrong. v4l2_event_unsubscribe() and v4l2-ctrls: send_event() don't hold any shared lock, so any form of test then use in v4l2-ctrls: send_event() is inherent racy.

Here is the relevant code from v4l2-ctrls: send_event():

	if (sev->fh && (sev->fh != fh ||
			(sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK)))
		v4l2_event_queue_fh(sev->fh, &ev);

Now lets say that v4l2_event_unsubscribe and v4l2-ctrls: send_event() race on the same sev, then the following could happens:

1) send_event checks sev->fh, finds it is not NULL
<thread switch>
2) v4l2_event_unsubscribe sets sev->fh NULL
3) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks as the thread calling send_event holds the ctrl_lock
<thread switch>
4) send_event calls v4l2_event_queue_fh(sev->fh, &ev) which not is equivalent to calling: v4l2_event_queue_fh(NULL, &ev)
5) oops, NULL pointer deref.

Now again without setting sev->fh to NULL in v4l2_event_unsubscribe and without the (now senseless since always true) sev->fh != NULL check in send_event:

1) send_event is about to call v4l2_event_queue_fh(sev->fh, &ev) <thread switch>
2) v4l2_event_unsubscribe removes sev->list from the fh->subscribed list <thread switch>
3) send_event calls v4l2_event_queue_fh(sev->fh, &ev)
4) v4l2_event_queue_fh blocks on the fh_lock spinlock
<thread switch>
5) v4l2_event_unsubscribe unlocks the fh_lock spinlock
6) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks as the thread calling send_event holds the ctrl_lock
<thread switch>
8) v4l2_event_queue_fh takes the fh_lock
7) v4l2_event_queue_fh calls v4l2_event_subscribed, does not find it since sev->list has been removed from fh->subscribed already -> does nothing
9) v4l2_event_queue_fh releases the fh_lock
10) the caller of send_event releases the ctrl lock (mutex) <thread switch>
11) v4l2_ctrls del_event takes the ctrl lock
12) v4l2_ctrls del_event removes sev->node from the ev_subs list
13) v4l2_ctrls del_event releases the ctrl lock
14) v4l2_event_unsubscribe frees the sev, to which no references are being held anymore

Actually as explained above the fix is to not do any checks and let both "subsystems" take care of their own locking / consistency without any interactions (other then that v4l2_ctrls should not hold any references to the sev after its del op has completed).

I'll update the patch to also remove the sev->fh check from v4l2_ctrls:
send_event() and update the commit message.

Regards,

Hans