Re: [PATCH 4/6] v4l2-event: Don't set sev->fh to NULL on unsubscribe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



<resend with correct subject, sorry for the confusing wrong subject with the previous mail>

Hi,

hverkuil wrote:

>On Thursday, October 27, 2011 13:18:01 Hans de Goede wrote:
1: There is no reason for this after v4l2_event_unsubscribe releases the
spinlock nothing is holding a reference to the sev anymore except for the
local reference in the v4l2_event_unsubscribe function.

Not true. v4l2-ctrls.c may still have a reference to the sev through the
ev_subs list in struct v4l2_ctrl. The send_event() function checks for a
non-zero fh.

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

All that is needed is to find some different way of letting send_event()
know that this sev is no longer used. Perhaps by making sev->list empty?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux