VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

[Dave Stevenson]

Hi All

I'm testing with 4.19 and finding that testEvents in v4l2-compliance
is failing with ""failed to find event for control '%s' type %u", ie
it hasn't got the event for the inital values. This is with the
various BCM2835 drivers that I'm involved with.

Having looked at the v4l2-core history I tried reverting "ad608fb
media: v4l: event: Prevent freeing event subscriptions while
accessed". The test passes again.

Enabling all logging, and adding a couple of logging messages at the
beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
error path, I get the following logs:

[   90.629999] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
6, flags 0001
[   90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1
[   91.630166] videodev: v4l2_poll: video0: poll: 00000000
[   91.630311] videodev: v4l2_poll: video0: poll: 00000000
[   91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
id=0x980001, flags=0x1
[   91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
flags 0001
[   91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001
[   91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
- initial values queued
[   91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1
[   92.630513] videodev: v4l2_poll: video0: poll: 00000000
[   92.630660] videodev: v4l2_poll: video0: poll: 00000000
[   92.630729] videodev: v4l2_release: video0: release

So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
at the point that the initial values are queued.

Sorry, I don't have any other devices that support subscribing for
events to hand (uvcvideo passes the test as it reports unsupported). I
don't have a media tree build immediately available either, but I
can't see anything related to this in the recent history. I can go
down that route if needed.
v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
wake-up on Active Source is warn for <2.0"

Could someone test on other hardware to confirm that it's not the
drivers I'm using? I'm fairly certain it isn't as that patch does call
sev->ops->add(sev, elems); before list_add(&sev->list,
&fh->subscribed), and that is guaranteed to fail if sev->ops->add
immediately queues an event.

Thanks
  Dave

Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

[Hans Verkuil]

On 11/05/2018 01:21 PM, Dave Stevenson wrote:
> Hi All
> 
> I'm testing with 4.19 and finding that testEvents in v4l2-compliance
> is failing with ""failed to find event for control '%s' type %u", ie
> it hasn't got the event for the inital values. This is with the
> various BCM2835 drivers that I'm involved with.
> 
> Having looked at the v4l2-core history I tried reverting "ad608fb
> media: v4l: event: Prevent freeing event subscriptions while
> accessed". The test passes again.
> 
> Enabling all logging, and adding a couple of logging messages at the
> beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
> error path, I get the following logs:
> 
> [   90.629999] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
> 6, flags 0001
> [   90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1
> [   91.630166] videodev: v4l2_poll: video0: poll: 00000000
> [   91.630311] videodev: v4l2_poll: video0: poll: 00000000
> [   91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
> id=0x980001, flags=0x1
> [   91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
> flags 0001
> [   91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001
> [   91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
> - initial values queued
> [   91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1
> [   92.630513] videodev: v4l2_poll: video0: poll: 00000000
> [   92.630660] videodev: v4l2_poll: video0: poll: 00000000
> [   92.630729] videodev: v4l2_release: video0: release
> 
> So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
> at the point that the initial values are queued.
> 
> Sorry, I don't have any other devices that support subscribing for
> events to hand (uvcvideo passes the test as it reports unsupported). I
> don't have a media tree build immediately available either, but I
> can't see anything related to this in the recent history. I can go
> down that route if needed.
> v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
> wake-up on Active Source is warn for <2.0"
> 
> Could someone test on other hardware to confirm that it's not the
> drivers I'm using? I'm fairly certain it isn't as that patch does call
> sev->ops->add(sev, elems); before list_add(&sev->list,
> &fh->subscribed), and that is guaranteed to fail if sev->ops->add
> immediately queues an event.

Just to pitch in, I got similar issues when I tried to apply that commit
to our Cisco code base. I've been away for a week and had no time to look
into the cause, but it really appears that that commit was bad.

Sakari, can you take a look at this?

Regards,

	Hans

Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

[Hans Verkuil]

On 11/05/2018 02:18 PM, Hans Verkuil wrote:
> On 11/05/2018 01:21 PM, Dave Stevenson wrote:
>> Hi All
>>
>> I'm testing with 4.19 and finding that testEvents in v4l2-compliance
>> is failing with ""failed to find event for control '%s' type %u", ie
>> it hasn't got the event for the inital values. This is with the
>> various BCM2835 drivers that I'm involved with.
>>
>> Having looked at the v4l2-core history I tried reverting "ad608fb
>> media: v4l: event: Prevent freeing event subscriptions while
>> accessed". The test passes again.
>>
>> Enabling all logging, and adding a couple of logging messages at the
>> beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
>> error path, I get the following logs:
>>
>> [   90.629999] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
>> 6, flags 0001
>> [   90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1
>> [   91.630166] videodev: v4l2_poll: video0: poll: 00000000
>> [   91.630311] videodev: v4l2_poll: video0: poll: 00000000
>> [   91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
>> id=0x980001, flags=0x1
>> [   91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
>> flags 0001
>> [   91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001
>> [   91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
>> - initial values queued
>> [   91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1
>> [   92.630513] videodev: v4l2_poll: video0: poll: 00000000
>> [   92.630660] videodev: v4l2_poll: video0: poll: 00000000
>> [   92.630729] videodev: v4l2_release: video0: release
>>
>> So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
>> at the point that the initial values are queued.
>>
>> Sorry, I don't have any other devices that support subscribing for
>> events to hand (uvcvideo passes the test as it reports unsupported). I
>> don't have a media tree build immediately available either, but I
>> can't see anything related to this in the recent history. I can go
>> down that route if needed.
>> v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
>> wake-up on Active Source is warn for <2.0"
>>
>> Could someone test on other hardware to confirm that it's not the
>> drivers I'm using? I'm fairly certain it isn't as that patch does call
>> sev->ops->add(sev, elems); before list_add(&sev->list,
>> &fh->subscribed), and that is guaranteed to fail if sev->ops->add
>> immediately queues an event.
> 
> Just to pitch in, I got similar issues when I tried to apply that commit
> to our Cisco code base. I've been away for a week and had no time to look
> into the cause, but it really appears that that commit was bad.
> 
> Sakari, can you take a look at this?

Note: running v4l2-compliance with vivid (modprobe vivid no_error_inj=1) will
fail with this patch:

Control ioctls (Input 0):
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
                fail: v4l2-test-controls.cpp(823): failed to find event for control 'Brightness'
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 15 Private Controls: 39

We really need regular vivid tests to find these regressions :-(

Regards,

	Hans

Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

[Dave Stevenson]

Hi Hans

On Mon, 5 Nov 2018 at 13:18, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/05/2018 01:21 PM, Dave Stevenson wrote:
> > Hi All
> >
> > I'm testing with 4.19 and finding that testEvents in v4l2-compliance
> > is failing with ""failed to find event for control '%s' type %u", ie
> > it hasn't got the event for the inital values. This is with the
> > various BCM2835 drivers that I'm involved with.
> >
> > Having looked at the v4l2-core history I tried reverting "ad608fb
> > media: v4l: event: Prevent freeing event subscriptions while
> > accessed". The test passes again.
> >
> > Enabling all logging, and adding a couple of logging messages at the
> > beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
> > error path, I get the following logs:
> >
> > [   90.629999] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
> > 6, flags 0001
> > [   90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1
> > [   91.630166] videodev: v4l2_poll: video0: poll: 00000000
> > [   91.630311] videodev: v4l2_poll: video0: poll: 00000000
> > [   91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
> > id=0x980001, flags=0x1
> > [   91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
> > flags 0001
> > [   91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001
> > [   91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
> > - initial values queued
> > [   91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1
> > [   92.630513] videodev: v4l2_poll: video0: poll: 00000000
> > [   92.630660] videodev: v4l2_poll: video0: poll: 00000000
> > [   92.630729] videodev: v4l2_release: video0: release
> >
> > So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
> > at the point that the initial values are queued.
> >
> > Sorry, I don't have any other devices that support subscribing for
> > events to hand (uvcvideo passes the test as it reports unsupported). I
> > don't have a media tree build immediately available either, but I
> > can't see anything related to this in the recent history. I can go
> > down that route if needed.
> > v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
> > wake-up on Active Source is warn for <2.0"
> >
> > Could someone test on other hardware to confirm that it's not the
> > drivers I'm using? I'm fairly certain it isn't as that patch does call
> > sev->ops->add(sev, elems); before list_add(&sev->list,
> > &fh->subscribed), and that is guaranteed to fail if sev->ops->add
> > immediately queues an event.
>
> Just to pitch in, I got similar issues when I tried to apply that commit
> to our Cisco code base. I've been away for a week and had no time to look
> into the cause, but it really appears that that commit was bad.
>
> Sakari, can you take a look at this?

Thanks for confirming that it's not just me!

Swapping around the order of the list_add and ops->add fixes the
issue, but I'm not clear enough on whether there is a chance for
events to have been raised and need clearing to propose a full patch.
I'm currently running with:
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f5..a997d2e 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -232,18 +234,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
                goto out_unlock;
        }

+       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+       list_add(&sev->list, &fh->subscribed);
+       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
        if (sev->ops && sev->ops->add) {
                ret = sev->ops->add(sev, elems);
                if (ret) {
+                       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+                       list_del(&sev->list);
+                       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
                        kvfree(sev);
-                       goto out_unlock;
                }
        }

-       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
-       list_add(&sev->list, &fh->subscribed);
-       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-
 out_unlock:
        mutex_unlock(&fh->subscribe_lock);

Is there a need to iterate the sev->events list and free any
potentially pending events there in the ops->add error path?
We still have the subscribe_lock held, so I don't think that it
reintroduces the issue that the patch was fixing of unsubscribing
before subscribed.

This patch has also been merged to stable, so 4.14 is affected as well.

  Dave

Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

[Sakari Ailus]

Hi Dave, Hans,

Thanks for reporting the issue!

On Mon, Nov 05, 2018 at 01:56:40PM +0000, Dave Stevenson wrote:
> Hi Hans
> 
> On Mon, 5 Nov 2018 at 13:18, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 11/05/2018 01:21 PM, Dave Stevenson wrote:
> > > Hi All
> > >
> > > I'm testing with 4.19 and finding that testEvents in v4l2-compliance
> > > is failing with ""failed to find event for control '%s' type %u", ie
> > > it hasn't got the event for the inital values. This is with the
> > > various BCM2835 drivers that I'm involved with.
> > >
> > > Having looked at the v4l2-core history I tried reverting "ad608fb
> > > media: v4l: event: Prevent freeing event subscriptions while
> > > accessed". The test passes again.
> > >
> > > Enabling all logging, and adding a couple of logging messages at the
> > > beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
> > > error path, I get the following logs:
> > >
> > > [   90.629999] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
> > > 6, flags 0001
> > > [   90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1
> > > [   91.630166] videodev: v4l2_poll: video0: poll: 00000000
> > > [   91.630311] videodev: v4l2_poll: video0: poll: 00000000
> > > [   91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
> > > id=0x980001, flags=0x1
> > > [   91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
> > > flags 0001
> > > [   91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001
> > > [   91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
> > > - initial values queued
> > > [   91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1
> > > [   92.630513] videodev: v4l2_poll: video0: poll: 00000000
> > > [   92.630660] videodev: v4l2_poll: video0: poll: 00000000
> > > [   92.630729] videodev: v4l2_release: video0: release
> > >
> > > So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
> > > at the point that the initial values are queued.
> > >
> > > Sorry, I don't have any other devices that support subscribing for
> > > events to hand (uvcvideo passes the test as it reports unsupported). I
> > > don't have a media tree build immediately available either, but I
> > > can't see anything related to this in the recent history. I can go
> > > down that route if needed.
> > > v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
> > > wake-up on Active Source is warn for <2.0"
> > >
> > > Could someone test on other hardware to confirm that it's not the
> > > drivers I'm using? I'm fairly certain it isn't as that patch does call
> > > sev->ops->add(sev, elems); before list_add(&sev->list,
> > > &fh->subscribed), and that is guaranteed to fail if sev->ops->add
> > > immediately queues an event.
> >
> > Just to pitch in, I got similar issues when I tried to apply that commit
> > to our Cisco code base. I've been away for a week and had no time to look
> > into the cause, but it really appears that that commit was bad.
> >
> > Sakari, can you take a look at this?

Yes.

> 
> Thanks for confirming that it's not just me!
> 
> Swapping around the order of the list_add and ops->add fixes the
> issue, but I'm not clear enough on whether there is a chance for
> events to have been raised and need clearing to propose a full patch.
> I'm currently running with:
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f5..a997d2e 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -232,18 +234,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>                 goto out_unlock;
>         }
> 
> +       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +       list_add(&sev->list, &fh->subscribed);
> +       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
>         if (sev->ops && sev->ops->add) {
>                 ret = sev->ops->add(sev, elems);
>                 if (ret) {
> +                       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +                       list_del(&sev->list);
> +                       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>                         kvfree(sev);
> -                       goto out_unlock;
>                 }
>         }
> 
> -       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> -       list_add(&sev->list, &fh->subscribed);
> -       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> -
>  out_unlock:
>         mutex_unlock(&fh->subscribe_lock);
> 
> Is there a need to iterate the sev->events list and free any
> potentially pending events there in the ops->add error path?
> We still have the subscribe_lock held, so I don't think that it
> reintroduces the issue that the patch was fixing of unsubscribing
> before subscribed.

The events queued during that time need to be removed. Indeed the issue
appears to be that the add op is called before the event is added to the
list, and therefore the check whether it's subscribed fails.

> 
> This patch has also been merged to stable, so 4.14 is affected as well.

Yes, and the earlier stable kernels as well.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation

[Sakari Ailus]

Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event (and possibly other events arriving soon
afterwards) to be lost.

Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.

Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions while accessed")

Reported-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Dave, Hans,

This should address the issue.

The functions can (and probably should) be re-arranged later but let's get
the fix right first.

I haven't tested this using vivid yet as it crashes where I was testing
it. I'll see later if it works elsewhere but I'm sending the patch for
review in the meantime.

 drivers/media/v4l2-core/v4l2-event.c | 39 +++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f50a4b3..2b6651aeb89b 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_event_pending);
 
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+	struct v4l2_fh *fh = sev->fh;
+	unsigned int i;
+
+	lockdep_assert_held(&fh->subscribe_lock);
+	assert_spin_locked(&fh->vdev->fh_lock);
+
+	/* Remove any pending events for this subscription */
+	for (i = 0; i < sev->in_use; i++) {
+		list_del(&sev->events[sev_pos(sev, i)].list);
+		fh->navailable--;
+	}
+	list_del(&sev->list);
+}
+
 int v4l2_event_subscribe(struct v4l2_fh *fh,
 			 const struct v4l2_event_subscription *sub, unsigned elems,
 			 const struct v4l2_subscribed_event_ops *ops)
@@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 		goto out_unlock;
 	}
 
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	list_add(&sev->list, &fh->subscribed);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
 	if (sev->ops && sev->ops->add) {
 		ret = sev->ops->add(sev, elems);
 		if (ret) {
+			spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+			__v4l2_event_unsubscribe(sev);
+			spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 			kvfree(sev);
-			goto out_unlock;
 		}
 	}
 
-	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
-	list_add(&sev->list, &fh->subscribed);
-	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-
 out_unlock:
 	mutex_unlock(&fh->subscribe_lock);
 
@@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 {
 	struct v4l2_subscribed_event *sev;
 	unsigned long flags;
-	int i;
 
 	if (sub->type == V4L2_EVENT_ALL) {
 		v4l2_event_unsubscribe_all(fh);
@@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
-	if (sev != NULL) {
-		/* Remove any pending events for this subscription */
-		for (i = 0; i < sev->in_use; i++) {
-			list_del(&sev->events[sev_pos(sev, i)].list);
-			fh->navailable--;
-		}
-		list_del(&sev->list);
-	}
+	if (sev != NULL)
+		__v4l2_event_unsubscribe(sev);
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
-- 
2.11.0

Re: [PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation

[Hans Verkuil]

On 11/05/2018 04:46 PM, Sakari Ailus wrote:
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event (and possibly other events arriving soon
> afterwards) to be lost.
> 
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
> 
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions while accessed")
> 
> Reported-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Dave, Hans,
> 
> This should address the issue.
> 
> The functions can (and probably should) be re-arranged later but let's get
> the fix right first.
> 
> I haven't tested this using vivid yet as it crashes where I was testing
> it. I'll see later if it works elsewhere but I'm sending the patch for
> review in the meantime.

Did a quick vivid test and it passes the tests now.

I'll review the code tomorrow.

Regards,

	Hans

> 
>  drivers/media/v4l2-core/v4l2-event.c | 39 +++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f50a4b3..2b6651aeb89b 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_pending);
>  
> +static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
> +{
> +	struct v4l2_fh *fh = sev->fh;
> +	unsigned int i;
> +
> +	lockdep_assert_held(&fh->subscribe_lock);
> +	assert_spin_locked(&fh->vdev->fh_lock);
> +
> +	/* Remove any pending events for this subscription */
> +	for (i = 0; i < sev->in_use; i++) {
> +		list_del(&sev->events[sev_pos(sev, i)].list);
> +		fh->navailable--;
> +	}
> +	list_del(&sev->list);
> +}
> +
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
>  			 const struct v4l2_event_subscription *sub, unsigned elems,
>  			 const struct v4l2_subscribed_event_ops *ops)
> @@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  		goto out_unlock;
>  	}
>  
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	list_add(&sev->list, &fh->subscribed);
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
>  	if (sev->ops && sev->ops->add) {
>  		ret = sev->ops->add(sev, elems);
>  		if (ret) {
> +			spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +			__v4l2_event_unsubscribe(sev);
> +			spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  			kvfree(sev);
> -			goto out_unlock;
>  		}
>  	}
>  
> -	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> -	list_add(&sev->list, &fh->subscribed);
> -	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> -
>  out_unlock:
>  	mutex_unlock(&fh->subscribe_lock);
>  
> @@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  {
>  	struct v4l2_subscribed_event *sev;
>  	unsigned long flags;
> -	int i;
>  
>  	if (sub->type == V4L2_EVENT_ALL) {
>  		v4l2_event_unsubscribe_all(fh);
> @@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>  
>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> -	if (sev != NULL) {
> -		/* Remove any pending events for this subscription */
> -		for (i = 0; i < sev->in_use; i++) {
> -			list_del(&sev->events[sev_pos(sev, i)].list);
> -			fh->navailable--;
> -		}
> -		list_del(&sev->list);
> -	}
> +	if (sev != NULL)
> +		__v4l2_event_unsubscribe(sev);
>  
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  
> 

Re: [PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation

[Dave Stevenson]

Hi Sakari

On Mon, 5 Nov 2018 at 15:46, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event (and possibly other events arriving soon
> afterwards) to be lost.
>
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
>
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions while accessed")
>
> Reported-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Dave, Hans,
>
> This should address the issue.
>
> The functions can (and probably should) be re-arranged later but let's get
> the fix right first.
>
> I haven't tested this using vivid yet as it crashes where I was testing
> it. I'll see later if it works elsewhere but I'm sending the patch for
> review in the meantime.

Thanks. That seems to fix the failure for my 3 drivers.

For what it's worth:
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.org>

I haven't tested the failure path, but it looks sane.

>  drivers/media/v4l2-core/v4l2-event.c | 39 +++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f50a4b3..2b6651aeb89b 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> +static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
> +{
> +       struct v4l2_fh *fh = sev->fh;
> +       unsigned int i;
> +
> +       lockdep_assert_held(&fh->subscribe_lock);
> +       assert_spin_locked(&fh->vdev->fh_lock);
> +
> +       /* Remove any pending events for this subscription */
> +       for (i = 0; i < sev->in_use; i++) {
> +               list_del(&sev->events[sev_pos(sev, i)].list);
> +               fh->navailable--;
> +       }
> +       list_del(&sev->list);
> +}
> +
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
>                          const struct v4l2_event_subscription *sub, unsigned elems,
>                          const struct v4l2_subscribed_event_ops *ops)
> @@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>                 goto out_unlock;
>         }
>
> +       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +       list_add(&sev->list, &fh->subscribed);
> +       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
>         if (sev->ops && sev->ops->add) {
>                 ret = sev->ops->add(sev, elems);
>                 if (ret) {
> +                       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +                       __v4l2_event_unsubscribe(sev);
> +                       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>                         kvfree(sev);
> -                       goto out_unlock;
>                 }
>         }
>
> -       spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> -       list_add(&sev->list, &fh->subscribed);
> -       spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> -
>  out_unlock:
>         mutex_unlock(&fh->subscribe_lock);
>
> @@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  {
>         struct v4l2_subscribed_event *sev;
>         unsigned long flags;
> -       int i;
>
>         if (sub->type == V4L2_EVENT_ALL) {
>                 v4l2_event_unsubscribe_all(fh);
> @@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>         spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>
>         sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> -       if (sev != NULL) {
> -               /* Remove any pending events for this subscription */
> -               for (i = 0; i < sev->in_use; i++) {
> -                       list_del(&sev->events[sev_pos(sev, i)].list);
> -                       fh->navailable--;
> -               }
> -               list_del(&sev->list);
> -       }
> +       if (sev != NULL)
> +               __v4l2_event_unsubscribe(sev);
>
>         spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
> --
> 2.11.0
>