From e3e72f39b68ec2563d8ef22f9704a66b7f71b638 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 26 Oct 2011 05:52:47 -0300 Subject: [PATCH] [media] v4l2-event: Don't set sev->fh to NULL on unsubscribe 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 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 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) 2) v4l2_event_unsubscribe removes sev->list from the fh->subscribed list 3) send_event calls v4l2_event_queue_fh(sev->fh, &ev) 4) v4l2_event_queue_fh blocks on the fh_lock spinlock 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 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) 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: Hans de Goede Acked-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/video/v4l2-ctrls.c | 4 ++-- drivers/media/video/v4l2-event.c | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c index 5552f8137571..c58c91bbcfbd 100644 --- a/drivers/media/video/v4l2-ctrls.c +++ b/drivers/media/video/v4l2-ctrls.c @@ -820,8 +820,8 @@ static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes) fill_event(&ev, ctrl, changes); list_for_each_entry(sev, &ctrl->ev_subs, node) - if (sev->fh && (sev->fh != fh || - (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK))) + if (sev->fh != fh || + (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK)) v4l2_event_queue_fh(sev->fh, &ev); } diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c index 4d01f17497f6..3d93251f292e 100644 --- a/drivers/media/video/v4l2-event.c +++ b/drivers/media/video/v4l2-event.c @@ -302,7 +302,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, fh->navailable--; } list_del(&sev->list); - sev->fh = NULL; } spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); -- GitLab