• Hans de Goede's avatar
    [media] v4l2-event: Don't set sev->fh to NULL on unsubscribe · e3e72f39
    Hans de Goede authored
    Setting sev->fh to NULL causes problems for the del op added in the next
    patch of this series, since this op needs a way to get to its own data
    structures, and typically this will be done by using container_of on an
    embedded v4l2_fh struct.
    
    The reason the original code is setting sev->fh to NULL is to signal
    to users of the event framework that the unsubscription has happened,
    but since their is no shared lock between the event framework and users
    of it, this is inherently racy, and it also turns out to be unnecessary
    as long as both the event framework and the user of the framework do their
    own locking properly and the user guarantees that it holds no references
    to the subcribed_event structure after its del operation has been called.
    
    This is best explained by looking at the only code currently checking for
    sev->fh being set to NULL on unsubscribe, which is the v4l2-ctrls.c send_event
    function. 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
    
    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
    Signed-off-by: default avatarHans de Goede <hdegoede@redhat.com>
    Acked-by: default avatarHans Verkuil <hans.verkuil@cisco.com>
    Signed-off-by: default avatarMauro Carvalho Chehab <mchehab@redhat.com>
    e3e72f39
v4l2-event.c 7.5 KB