Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans de Goede]

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

Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans de Goede]

Hi,

hverkuil wrote:

 > > This patch fixes these dangling pointers in the available queue by removing
 > > all matching pending events on unsubscription.
 >
 > The idea is fine, but the implementation is inefficient.
 >
 > Instead of the list_for_each_entry_safe you can just do:
 >
 >	for (i = 0; i < sev->in_use; i++) {
 >		list_del(&sev->events[sev_pos(sev, i)].list);
 >		fh->navailable--;
 >	}
 >
 > It's untested, but this should do the trick.

Agreed, I've modified my patch to use this construction instead.

Regards,

Hans

Various ctrl and event frame work patches (version 2)

[Hans de Goede]

Hi All,

This patch set obsoletes my previous "add v4l2_subscribed_event_ops" set,
while working on adding support for ctrl-events to the uvc driver I found
a few bugs in the event code, which this patchset fixes.

Changes since version 1:
-Added a documentation update (update v4l2-framework.txt) to:
 "v4l2-event: Add v4l2_subscribed_event_ops"

Regards,

Hans

[PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans de Goede]

The kev pointers inside the pending events queue (the available queue) of the
fh point to data inside the sev, unsubscribing frees the sev, thus making these
pointers point to freed memory!

This patch fixes these dangling pointers in the available queue by removing
all matching pending events on unsubscription.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-event.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9f56f18..01cbb7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			   struct v4l2_event_subscription *sub)
 {
 	struct v4l2_subscribed_event *sev;
+	struct v4l2_kevent *kev, *next;
 	unsigned long flags;
 
 	if (sub->type == V4L2_EVENT_ALL) {
@@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
 	if (sev != NULL) {
+		/* Remove any pending events for this subscription */
+		list_for_each_entry_safe(kev, next, &fh->available, list) {
+			if (kev->sev == sev) {
+				list_del(&kev->list);
+				fh->navailable--;
+			}
+		}
 		list_del(&sev->list);
 		sev->fh = NULL;
 	}
-- 
1.7.7

Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Sakari Ailus]

Hi Hans,

On Mon, Oct 31, 2011 at 04:16:46PM +0100, Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of the
> fh point to data inside the sev, unsubscribing frees the sev, thus making these
> pointers point to freed memory!
> 
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/video/v4l2-event.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)

Thanks for the patch! I think this should go in rather soon as this is an
important bugfix.

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Various ctrl and event frame work patches

[Hans de Goede]

Hi All,

This patch set obsoletes my previous "add v4l2_subscribed_event_ops" set,
while working on adding support for ctrl-events to the uvc driver I found
a few bugs in the event code, which this patchset fixes.

Regards,

Hans

[PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans de Goede]

The kev pointers inside the pending events queue (the available queue) of the
fh point to data inside the sev, unsubscribing frees the sev, thus making these
pointers point to freed memory!

This patch fixes these dangling pointers in the available queue by removing
all matching pending events on unsubscription.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-event.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9f56f18..01cbb7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			   struct v4l2_event_subscription *sub)
 {
 	struct v4l2_subscribed_event *sev;
+	struct v4l2_kevent *kev, *next;
 	unsigned long flags;
 
 	if (sub->type == V4L2_EVENT_ALL) {
@@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
 	if (sev != NULL) {
+		/* Remove any pending events for this subscription */
+		list_for_each_entry_safe(kev, next, &fh->available, list) {
+			if (kev->sev == sev) {
+				list_del(&kev->list);
+				fh->navailable--;
+			}
+		}
 		list_del(&sev->list);
 		sev->fh = NULL;
 	}
-- 
1.7.7

Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Laurent Pinchart]

Hi Hans,

On Thursday 27 October 2011 13:18:00 Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of
> the fh point to data inside the sev, unsubscribing frees the sev, thus
> making these pointers point to freed memory!
> 
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/video/v4l2-event.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 9f56f18..01cbb7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			   struct v4l2_event_subscription *sub)
>  {
>  	struct v4l2_subscribed_event *sev;
> +	struct v4l2_kevent *kev, *next;
>  	unsigned long flags;
> 
>  	if (sub->type == V4L2_EVENT_ALL) {
> @@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> 
>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>  	if (sev != NULL) {
> +		/* Remove any pending events for this subscription */
> +		list_for_each_entry_safe(kev, next, &fh->available, list) {
> +			if (kev->sev == sev) {
> +				list_del(&kev->list);
> +				fh->navailable--;
> +			}
> +		}
>  		list_del(&sev->list);
>  		sev->fh = NULL;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans Verkuil]

On Thursday, October 27, 2011 13:18:00 Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of the
> fh point to data inside the sev, unsubscribing frees the sev, thus making these
> pointers point to freed memory!
> 
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.

The idea is fine, but the implementation is inefficient.

Instead of the list_for_each_entry_safe you can just do:

	for (i = 0; i < sev->in_use; i++) {
		list_del(&sev->events[sev_pos(sev, i)].list);
		fh->navailable--;
	}

It's untested, but this should do the trick.

Regards,

	Hans

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/video/v4l2-event.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 9f56f18..01cbb7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -284,6 +284,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			   struct v4l2_event_subscription *sub)
>  {
>  	struct v4l2_subscribed_event *sev;
> +	struct v4l2_kevent *kev, *next;
>  	unsigned long flags;
>  
>  	if (sub->type == V4L2_EVENT_ALL) {
> @@ -295,6 +296,13 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  
>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>  	if (sev != NULL) {
> +		/* Remove any pending events for this subscription */
> +		list_for_each_entry_safe(kev, next, &fh->available, list) {
> +			if (kev->sev == sev) {
> +				list_del(&kev->list);
> +				fh->navailable--;
> +			}
> +		}
>  		list_del(&sev->list);
>  		sev->fh = NULL;
>  	}
>