[RFC PATCH] radio-mr800: locking fixes

[RFC PATCH] radio-mr800: locking fixes

[Hans Verkuil]

- serialize the suspend and resume functions using the global lock.
- do not call usb_autopm_put_interface after a disconnect.
- fix a race when disconnecting the device.

Reported-by: David Ellingsworth <david@identd.dyndns.org>
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
index 2f56b26..b540e80 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct usb_interface *intf)
 	struct amradio_device *radio = to_amradio_dev(usb_get_intfdata(intf));
 
 	mutex_lock(&radio->lock);
+	/* increase the device node's refcount */
+	get_device(&radio->videodev.dev);
 	v4l2_device_disconnect(&radio->v4l2_dev);
-	mutex_unlock(&radio->lock);
 	video_unregister_device(&radio->videodev);
+	mutex_unlock(&radio->lock);
+	/* decrease the device node's refcount, allowing it to be released */
+	put_device(&radio->videodev.dev);
 }
 
 /* vidioc_querycap - query device capabilities */
@@ -515,7 +519,8 @@ static int usb_amradio_close(struct file *file)
 {
 	struct amradio_device *radio = file->private_data;
 
-	usb_autopm_put_interface(radio->intf);
+	if (video_is_registered(&radio->videodev))
+		usb_autopm_put_interface(radio->intf);
 	return 0;
 }
 
@@ -524,10 +529,12 @@ static int usb_amradio_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct amradio_device *radio = to_amradio_dev(usb_get_intfdata(intf));
 
+	mutex_lock(&radio->lock);
 	if (!radio->muted && radio->initialized) {
 		amradio_set_mute(radio, AMRADIO_STOP);
 		radio->muted = 0;
 	}
+	mutex_unlock(&radio->lock);
 
 	dev_info(&intf->dev, "going into suspend..\n");
 	return 0;
@@ -538,8 +545,9 @@ static int usb_amradio_resume(struct usb_interface *intf)
 {
 	struct amradio_device *radio = to_amradio_dev(usb_get_intfdata(intf));
 
+	mutex_lock(&radio->lock);
 	if (unlikely(!radio->initialized))
-		return 0;
+		goto unlock;
 
 	if (radio->stereo)
 		amradio_set_stereo(radio, WANT_STEREO);
@@ -551,6 +559,9 @@ static int usb_amradio_resume(struct usb_interface *intf)
 	if (!radio->muted)
 		amradio_set_mute(radio, AMRADIO_START);
 
+unlock:
+	mutex_unlock(&radio->lock);
+
 	dev_info(&intf->dev, "coming out of suspend..\n");
 	return 0;
 }
-- 
1.7.0.4

Re: [RFC PATCH] radio-mr800: locking fixes

[Hans Verkuil]

On Sunday, October 17, 2010 14:26:18 Hans Verkuil wrote:
> - serialize the suspend and resume functions using the global lock.
> - do not call usb_autopm_put_interface after a disconnect.
> - fix a race when disconnecting the device.

Regarding autosuspend: something seems to work since the power/runtime_status
attribute goes from 'suspended' to 'active' whenever the radio handle is open.
But the suspend and resume functions are never called. I can't figure out
why not. I don't see anything strange.

The whole autopm stuff is highly suspect anyway on a device like this since
it is perfectly reasonable to just set a frequency and exit. The audio is
just going to the line-in anyway. In other words: not having the device node
open does not mean that the device is idle and can be suspended.

My proposal would be to rip out the whole autosuspend business from this
driver. I've no idea why it is here at all.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCH] radio-mr800: locking fixes

[David Ellingsworth]

On Sun, Oct 17, 2010 at 8:52 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Sunday, October 17, 2010 14:26:18 Hans Verkuil wrote:
>> - serialize the suspend and resume functions using the global lock.
>> - do not call usb_autopm_put_interface after a disconnect.
>> - fix a race when disconnecting the device.
>
> Regarding autosuspend: something seems to work since the power/runtime_status
> attribute goes from 'suspended' to 'active' whenever the radio handle is open.
> But the suspend and resume functions are never called. I can't figure out
> why not. I don't see anything strange.
>
> The whole autopm stuff is highly suspect anyway on a device like this since
> it is perfectly reasonable to just set a frequency and exit. The audio is
> just going to the line-in anyway. In other words: not having the device node
> open does not mean that the device is idle and can be suspended.
>
> My proposal would be to rip out the whole autosuspend business from this
> driver. I've no idea why it is here at all.
>
> Regards,
>
>        Hans

Hans, I highly agree with that analysis. The original author put that
code in. But like you, I'm not sure if it was ever really valid. Since
I didn't have anything to test with, I left it untouched.

Regards,

David Ellingsworth

Re: [RFC PATCH] radio-mr800: locking fixes

[Hans Verkuil]

> On Sun, Oct 17, 2010 at 8:52 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On Sunday, October 17, 2010 14:26:18 Hans Verkuil wrote:
>>> - serialize the suspend and resume functions using the global lock.
>>> - do not call usb_autopm_put_interface after a disconnect.
>>> - fix a race when disconnecting the device.
>>
>> Regarding autosuspend: something seems to work since the
>> power/runtime_status
>> attribute goes from 'suspended' to 'active' whenever the radio handle is
>> open.
>> But the suspend and resume functions are never called. I can't figure
>> out
>> why not. I don't see anything strange.
>>
>> The whole autopm stuff is highly suspect anyway on a device like this
>> since
>> it is perfectly reasonable to just set a frequency and exit. The audio
>> is
>> just going to the line-in anyway. In other words: not having the device
>> node
>> open does not mean that the device is idle and can be suspended.
>>
>> My proposal would be to rip out the whole autosuspend business from this
>> driver. I've no idea why it is here at all.
>>
>> Regards,
>>
>>        Hans
>
> Hans, I highly agree with that analysis. The original author put that
> code in. But like you, I'm not sure if it was ever really valid. Since
> I didn't have anything to test with, I left it untouched.
>
> Regards,
>
> David Ellingsworth
>
>

OK, then I'll make a new patch that just rips out autosuspend support.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCH] radio-mr800: locking fixes

[David Ellingsworth]

On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> - serialize the suspend and resume functions using the global lock.
> - do not call usb_autopm_put_interface after a disconnect.
> - fix a race when disconnecting the device.
>
> Reported-by: David Ellingsworth <david@identd.dyndns.org>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
>  1 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/radio/radio-mr800.c b/drivers/media/radio/radio-mr800.c
> index 2f56b26..b540e80 100644
> --- a/drivers/media/radio/radio-mr800.c
> +++ b/drivers/media/radio/radio-mr800.c
> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct usb_interface *intf)
>        struct amradio_device *radio = to_amradio_dev(usb_get_intfdata(intf));
>
>        mutex_lock(&radio->lock);
> +       /* increase the device node's refcount */
> +       get_device(&radio->videodev.dev);
>        v4l2_device_disconnect(&radio->v4l2_dev);
> -       mutex_unlock(&radio->lock);
>        video_unregister_device(&radio->videodev);
> +       mutex_unlock(&radio->lock);
> +       /* decrease the device node's refcount, allowing it to be released */
> +       put_device(&radio->videodev.dev);
>  }

Hans, I understand the use of get/put_device here.. but can you
explain to me what issue you are trying to solve?
video_unregister_device does not have to be synchronized with anything
else. Thus, it is perfectly safe to call video_unregister_device while
not holding the device lock. Your prior implementation here was
correct.

Regards,

David Ellingsworth

Re: [RFC PATCH] radio-mr800: locking fixes

[Hans Verkuil]

> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> - serialize the suspend and resume functions using the global lock.
>> - do not call usb_autopm_put_interface after a disconnect.
>> - fix a race when disconnecting the device.
>>
>> Reported-by: David Ellingsworth <david@identd.dyndns.org>
>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>> ---
>>  drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
>>  1 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/radio/radio-mr800.c
>> b/drivers/media/radio/radio-mr800.c
>> index 2f56b26..b540e80 100644
>> --- a/drivers/media/radio/radio-mr800.c
>> +++ b/drivers/media/radio/radio-mr800.c
>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct
>> usb_interface *intf)
>>        struct amradio_device *radio =
>> to_amradio_dev(usb_get_intfdata(intf));
>>
>>        mutex_lock(&radio->lock);
>> +       /* increase the device node's refcount */
>> +       get_device(&radio->videodev.dev);
>>        v4l2_device_disconnect(&radio->v4l2_dev);
>> -       mutex_unlock(&radio->lock);
>>        video_unregister_device(&radio->videodev);
>> +       mutex_unlock(&radio->lock);
>> +       /* decrease the device node's refcount, allowing it to be
>> released */
>> +       put_device(&radio->videodev.dev);
>>  }
>
> Hans, I understand the use of get/put_device here.. but can you
> explain to me what issue you are trying to solve?
> video_unregister_device does not have to be synchronized with anything
> else. Thus, it is perfectly safe to call video_unregister_device while
> not holding the device lock. Your prior implementation here was
> correct.

This the original sequence:

       mutex_lock(&radio->lock);
       v4l2_device_disconnect(&radio->v4l2_dev);
       mutex_unlock(&radio->lock);
       video_unregister_device(&radio->videodev);

The problem with this is that userspace can call open or ioctl after the
unlock and before the device node is marked unregistered by
video_unregister_device.

Once you disconnect you want to block all access (except the release call).
What my patch does is to move the video_unregister_device call inside the
lock, but then I have to guard against the release being called before the
unlock by increasing the refcount.

I have ideas to improve on this as this gets hairy when you have multiple
device nodes, but I wait with that until the next kernel cycle.

Regards,

         Hans

>
> Regards,
>
> David Ellingsworth
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCH] radio-mr800: locking fixes

[Hans Verkuil]

> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> - serialize the suspend and resume functions using the global lock.
>> - do not call usb_autopm_put_interface after a disconnect.
>> - fix a race when disconnecting the device.
>>
>> Reported-by: David Ellingsworth <david@identd.dyndns.org>
>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>> ---
>>  drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
>>  1 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/radio/radio-mr800.c
>> b/drivers/media/radio/radio-mr800.c
>> index 2f56b26..b540e80 100644
>> --- a/drivers/media/radio/radio-mr800.c
>> +++ b/drivers/media/radio/radio-mr800.c
>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct
>> usb_interface *intf)
>>        struct amradio_device *radio =
>> to_amradio_dev(usb_get_intfdata(intf));
>>
>>        mutex_lock(&radio->lock);
>> +       /* increase the device node's refcount */
>> +       get_device(&radio->videodev.dev);
>>        v4l2_device_disconnect(&radio->v4l2_dev);
>> -       mutex_unlock(&radio->lock);
>>        video_unregister_device(&radio->videodev);
>> +       mutex_unlock(&radio->lock);
>> +       /* decrease the device node's refcount, allowing it to be
>> released */
>> +       put_device(&radio->videodev.dev);
>>  }
>
> Hans, I understand the use of get/put_device here.. but can you
> explain to me what issue you are trying to solve?
> video_unregister_device does not have to be synchronized with anything
> else. Thus, it is perfectly safe to call video_unregister_device while
> not holding the device lock. Your prior implementation here was
> correct.

This the original sequence:

       mutex_lock(&radio->lock);
       v4l2_device_disconnect(&radio->v4l2_dev);
       mutex_unlock(&radio->lock);
       video_unregister_device(&radio->videodev);

The problem with this is that userspace can call open or ioctl after the
unlock and before the device node is marked unregistered by
video_unregister_device.

Once you disconnect you want to block all access (except the release call).
What my patch does is to move the video_unregister_device call inside the
lock, but then I have to guard against the release being called before the
unlock by increasing the refcount.

I have ideas to improve on this as this gets hairy when you have multiple
device nodes, but I wait with that until the next kernel cycle.

Regards,

         Hans

>
> Regards,
>
> David Ellingsworth
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCH] radio-mr800: locking fixes

[David Ellingsworth]

On Mon, Oct 18, 2010 at 9:38 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> - serialize the suspend and resume functions using the global lock.
>>> - do not call usb_autopm_put_interface after a disconnect.
>>> - fix a race when disconnecting the device.
>>>
>>> Reported-by: David Ellingsworth <david@identd.dyndns.org>
>>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>>> ---
>>>  drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
>>>  1 files changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/radio/radio-mr800.c
>>> b/drivers/media/radio/radio-mr800.c
>>> index 2f56b26..b540e80 100644
>>> --- a/drivers/media/radio/radio-mr800.c
>>> +++ b/drivers/media/radio/radio-mr800.c
>>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct
>>> usb_interface *intf)
>>>        struct amradio_device *radio =
>>> to_amradio_dev(usb_get_intfdata(intf));
>>>
>>>        mutex_lock(&radio->lock);
>>> +       /* increase the device node's refcount */
>>> +       get_device(&radio->videodev.dev);
>>>        v4l2_device_disconnect(&radio->v4l2_dev);
>>> -       mutex_unlock(&radio->lock);
>>>        video_unregister_device(&radio->videodev);
>>> +       mutex_unlock(&radio->lock);
>>> +       /* decrease the device node's refcount, allowing it to be
>>> released */
>>> +       put_device(&radio->videodev.dev);
>>>  }
>>
>> Hans, I understand the use of get/put_device here.. but can you
>> explain to me what issue you are trying to solve?
>> video_unregister_device does not have to be synchronized with anything
>> else. Thus, it is perfectly safe to call video_unregister_device while
>> not holding the device lock. Your prior implementation here was
>> correct.
>
> This the original sequence:
>
>       mutex_lock(&radio->lock);
>       v4l2_device_disconnect(&radio->v4l2_dev);
>       mutex_unlock(&radio->lock);
>       video_unregister_device(&radio->videodev);
>
> The problem with this is that userspace can call open or ioctl after the
> unlock and before the device node is marked unregistered by
> video_unregister_device.
>
> Once you disconnect you want to block all access (except the release call).
> What my patch does is to move the video_unregister_device call inside the
> lock, but then I have to guard against the release being called before the
> unlock by increasing the refcount.
>
> I have ideas to improve on this as this gets hairy when you have multiple
> device nodes, but I wait with that until the next kernel cycle.
>
> Regards,
>
>         Hans
>

I think you're trying to solve a problem that doesn't exist.
To be a little more specific we have the following:

1. video_register_device - increments device refcount
2. video_unregister_device - decrements device refcount
3. v4l2_open - increments device refcount
4. v4l2_release - decrements device refcount

Keeping this in mind, the release callback of video_device is called
only when the device count reaches 0.

So under normal operation we have:

1. video_register_device -> device refcount incremented to 1
2. v4l2_open -> device refcount incremented to 2
3. v4l2_release -> device refcount decremented to 1
4. disconnect callback: video_unregister_device -> device refcount
decremented to 0 & release callback called.

If the user disconnects the device while it's open we have the following:

1. video_register_device -> device refcount incremented to 1
2. v4l2_open -> device refcount incremented to 2
3. disconnect callback: video_unregister_device -> device refcount
decremented to 1
4. v4l2_release -> device refcount decremented to 0 & release callback called.

In the above case, once video_unregister_device has been called, calls
to open no longer will work. However, the user holding the currently
open file handle can still call ioctl and other callbacks, but those
should be met with an -EIO, forcing them to close the open handle. The
original code did this by using the usb device as an indicator to see
if the device was still connected, as this functionality was not in
v4l2_core. On the other hand, v4l2_core could do this for us, just by
checking if the device is still registered.

As you can see from the above, there are no race conditions here.

Regards,

David Ellingsworth

Re: [RFC PATCH] radio-mr800: locking fixes

[Hans Verkuil]

> On Mon, Oct 18, 2010 at 9:38 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>>> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil <hverkuil@xs4all.nl>
>>> wrote:
>>>> - serialize the suspend and resume functions using the global lock.
>>>> - do not call usb_autopm_put_interface after a disconnect.
>>>> - fix a race when disconnecting the device.
>>>>
>>>> Reported-by: David Ellingsworth <david@identd.dyndns.org>
>>>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>>>> ---
>>>>  drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
>>>>  1 files changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/radio/radio-mr800.c
>>>> b/drivers/media/radio/radio-mr800.c
>>>> index 2f56b26..b540e80 100644
>>>> --- a/drivers/media/radio/radio-mr800.c
>>>> +++ b/drivers/media/radio/radio-mr800.c
>>>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct
>>>> usb_interface *intf)
>>>>        struct amradio_device *radio =
>>>> to_amradio_dev(usb_get_intfdata(intf));
>>>>
>>>>        mutex_lock(&radio->lock);
>>>> +       /* increase the device node's refcount */
>>>> +       get_device(&radio->videodev.dev);
>>>>        v4l2_device_disconnect(&radio->v4l2_dev);
>>>> -       mutex_unlock(&radio->lock);
>>>>        video_unregister_device(&radio->videodev);
>>>> +       mutex_unlock(&radio->lock);
>>>> +       /* decrease the device node's refcount, allowing it to be
>>>> released */
>>>> +       put_device(&radio->videodev.dev);
>>>>  }
>>>
>>> Hans, I understand the use of get/put_device here.. but can you
>>> explain to me what issue you are trying to solve?
>>> video_unregister_device does not have to be synchronized with anything
>>> else. Thus, it is perfectly safe to call video_unregister_device while
>>> not holding the device lock. Your prior implementation here was
>>> correct.
>>
>> This the original sequence:
>>
>>       mutex_lock(&radio->lock);
>>       v4l2_device_disconnect(&radio->v4l2_dev);
>>       mutex_unlock(&radio->lock);
>>       video_unregister_device(&radio->videodev);
>>
>> The problem with this is that userspace can call open or ioctl after the
>> unlock and before the device node is marked unregistered by
>> video_unregister_device.
>>
>> Once you disconnect you want to block all access (except the release
>> call).
>> What my patch does is to move the video_unregister_device call inside
>> the
>> lock, but then I have to guard against the release being called before
>> the
>> unlock by increasing the refcount.
>>
>> I have ideas to improve on this as this gets hairy when you have
>> multiple
>> device nodes, but I wait with that until the next kernel cycle.
>>
>> Regards,
>>
>>         Hans
>>
>
> I think you're trying to solve a problem that doesn't exist.
> To be a little more specific we have the following:
>
> 1. video_register_device - increments device refcount
> 2. video_unregister_device - decrements device refcount
> 3. v4l2_open - increments device refcount
> 4. v4l2_release - decrements device refcount
>
> Keeping this in mind, the release callback of video_device is called
> only when the device count reaches 0.
>
> So under normal operation we have:
>
> 1. video_register_device -> device refcount incremented to 1
> 2. v4l2_open -> device refcount incremented to 2
> 3. v4l2_release -> device refcount decremented to 1
> 4. disconnect callback: video_unregister_device -> device refcount
> decremented to 0 & release callback called.
>
> If the user disconnects the device while it's open we have the following:
>
> 1. video_register_device -> device refcount incremented to 1
> 2. v4l2_open -> device refcount incremented to 2
> 3. disconnect callback: video_unregister_device -> device refcount
> decremented to 1
> 4. v4l2_release -> device refcount decremented to 0 & release callback
> called.
>
> In the above case, once video_unregister_device has been called, calls
> to open no longer will work. However, the user holding the currently
> open file handle can still call ioctl and other callbacks, but those
> should be met with an -EIO, forcing them to close the open handle. The
> original code did this by using the usb device as an indicator to see
> if the device was still connected, as this functionality was not in
> v4l2_core. On the other hand, v4l2_core could do this for us, just by
> checking if the device is still registered.
>
> As you can see from the above, there are no race conditions here.

Yes there is a race: what you want is that in the disconnect function you
can safely clean up any data structures or whatever in the knowledge that
once the device nodes are unregistered only the release call can reach the
driver. All other calls (open, ioctl, etc) are blocked by the v4l core.

In order to do this you have to unregister the device nodes while the lock
is held. In the original code the device node was unregistered without the
lock being held, so that would allow userspace to sneak in a e.g. open()
call just before the unregister call.

BTW, note that this is probably not relevant for this particular driver.
Even if this race happens, it will most likely not be a problem for this
simple device.

Regards,

        Hans


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCH] radio-mr800: locking fixes

[David Ellingsworth]

On Mon, Oct 18, 2010 at 10:18 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> On Mon, Oct 18, 2010 at 9:38 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>
>>>> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil <hverkuil@xs4all.nl>
>>>> wrote:
>>>>> - serialize the suspend and resume functions using the global lock.
>>>>> - do not call usb_autopm_put_interface after a disconnect.
>>>>> - fix a race when disconnecting the device.
>>>>>
>>>>> Reported-by: David Ellingsworth <david@identd.dyndns.org>
>>>>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>>>>> ---
>>>>>  drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
>>>>>  1 files changed, 14 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/radio/radio-mr800.c
>>>>> b/drivers/media/radio/radio-mr800.c
>>>>> index 2f56b26..b540e80 100644
>>>>> --- a/drivers/media/radio/radio-mr800.c
>>>>> +++ b/drivers/media/radio/radio-mr800.c
>>>>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct
>>>>> usb_interface *intf)
>>>>>        struct amradio_device *radio =
>>>>> to_amradio_dev(usb_get_intfdata(intf));
>>>>>
>>>>>        mutex_lock(&radio->lock);
>>>>> +       /* increase the device node's refcount */
>>>>> +       get_device(&radio->videodev.dev);
>>>>>        v4l2_device_disconnect(&radio->v4l2_dev);
>>>>> -       mutex_unlock(&radio->lock);
>>>>>        video_unregister_device(&radio->videodev);
>>>>> +       mutex_unlock(&radio->lock);
>>>>> +       /* decrease the device node's refcount, allowing it to be
>>>>> released */
>>>>> +       put_device(&radio->videodev.dev);
>>>>>  }
>>>>
>>>> Hans, I understand the use of get/put_device here.. but can you
>>>> explain to me what issue you are trying to solve?
>>>> video_unregister_device does not have to be synchronized with anything
>>>> else. Thus, it is perfectly safe to call video_unregister_device while
>>>> not holding the device lock. Your prior implementation here was
>>>> correct.
>>>
>>> This the original sequence:
>>>
>>>       mutex_lock(&radio->lock);
>>>       v4l2_device_disconnect(&radio->v4l2_dev);
>>>       mutex_unlock(&radio->lock);
>>>       video_unregister_device(&radio->videodev);
>>>
>>> The problem with this is that userspace can call open or ioctl after the
>>> unlock and before the device node is marked unregistered by
>>> video_unregister_device.
>>>
>>> Once you disconnect you want to block all access (except the release
>>> call).
>>> What my patch does is to move the video_unregister_device call inside
>>> the
>>> lock, but then I have to guard against the release being called before
>>> the
>>> unlock by increasing the refcount.
>>>
>>> I have ideas to improve on this as this gets hairy when you have
>>> multiple
>>> device nodes, but I wait with that until the next kernel cycle.
>>>
>>> Regards,
>>>
>>>         Hans
>>>
>>
>> I think you're trying to solve a problem that doesn't exist.
>> To be a little more specific we have the following:
>>
>> 1. video_register_device - increments device refcount
>> 2. video_unregister_device - decrements device refcount
>> 3. v4l2_open - increments device refcount
>> 4. v4l2_release - decrements device refcount
>>
>> Keeping this in mind, the release callback of video_device is called
>> only when the device count reaches 0.
>>
>> So under normal operation we have:
>>
>> 1. video_register_device -> device refcount incremented to 1
>> 2. v4l2_open -> device refcount incremented to 2
>> 3. v4l2_release -> device refcount decremented to 1
>> 4. disconnect callback: video_unregister_device -> device refcount
>> decremented to 0 & release callback called.
>>
>> If the user disconnects the device while it's open we have the following:
>>
>> 1. video_register_device -> device refcount incremented to 1
>> 2. v4l2_open -> device refcount incremented to 2
>> 3. disconnect callback: video_unregister_device -> device refcount
>> decremented to 1
>> 4. v4l2_release -> device refcount decremented to 0 & release callback
>> called.
>>
>> In the above case, once video_unregister_device has been called, calls
>> to open no longer will work. However, the user holding the currently
>> open file handle can still call ioctl and other callbacks, but those
>> should be met with an -EIO, forcing them to close the open handle. The
>> original code did this by using the usb device as an indicator to see
>> if the device was still connected, as this functionality was not in
>> v4l2_core. On the other hand, v4l2_core could do this for us, just by
>> checking if the device is still registered.
>>
>> As you can see from the above, there are no race conditions here.
>
> Yes there is a race: what you want is that in the disconnect function you
> can safely clean up any data structures or whatever in the knowledge that
> once the device nodes are unregistered only the release call can reach the
> driver. All other calls (open, ioctl, etc) are blocked by the v4l core.
>
> In order to do this you have to unregister the device nodes while the lock
> is held. In the original code the device node was unregistered without the
> lock being held, so that would allow userspace to sneak in a e.g. open()
> call just before the unregister call.
>
> BTW, note that this is probably not relevant for this particular driver.
> Even if this race happens, it will most likely not be a problem for this
> simple device.
>
> Regards,
>
>        Hans
>

Hans, right I understand what you're getting at.. between the
v4l2_device_disconnect and video_unregister_device a call to open
could succeed. In the original code, the usb device of the object was
set to NULL. In the open callback if the usb device was NULL, the open
would fail with -EIO, thus preventing the race you are referring to.

Subsequently, all ioctl callbacks had the same check applied. So yes,
while there is a race here now, there wasn't one before. It was
already handled by the driver. The addition get/put_device calls do
nothing to prevent this race. Not even locking around
video_unregister_device prevents this race. Consider the following:

1. The lock is taken in the disconnect callback.
2. While the disconnect callback is being processed, a call to open happens.
3. Since the lock is already taken the open waits on the lock to be released.
4. The lock is finally released, the call to open acquires the lock
and succeeds.

As you can see.. there's nothing you can do to prevent this from
happening. This is the reason the original code set the usb device to
NULL while holding the lock, and checked to see if it was NULL in open
and the ioctl callbacks with the lock held. It's the only way to
prevent this race from happening.

Regards,

David Ellingsworth

Re: [RFC PATCH] radio-mr800: locking fixes

[David Ellingsworth]

On Mon, Oct 18, 2010 at 10:35 AM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> On Mon, Oct 18, 2010 at 10:18 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>>> On Mon, Oct 18, 2010 at 9:38 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>>> On Sun, Oct 17, 2010 at 8:26 AM, Hans Verkuil <hverkuil@xs4all.nl>
>>>>> wrote:
>>>>>> - serialize the suspend and resume functions using the global lock.
>>>>>> - do not call usb_autopm_put_interface after a disconnect.
>>>>>> - fix a race when disconnecting the device.
>>>>>>
>>>>>> Reported-by: David Ellingsworth <david@identd.dyndns.org>
>>>>>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>>>>>> ---
>>>>>>  drivers/media/radio/radio-mr800.c |   17 ++++++++++++++---
>>>>>>  1 files changed, 14 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/radio/radio-mr800.c
>>>>>> b/drivers/media/radio/radio-mr800.c
>>>>>> index 2f56b26..b540e80 100644
>>>>>> --- a/drivers/media/radio/radio-mr800.c
>>>>>> +++ b/drivers/media/radio/radio-mr800.c
>>>>>> @@ -284,9 +284,13 @@ static void usb_amradio_disconnect(struct
>>>>>> usb_interface *intf)
>>>>>>        struct amradio_device *radio =
>>>>>> to_amradio_dev(usb_get_intfdata(intf));
>>>>>>
>>>>>>        mutex_lock(&radio->lock);
>>>>>> +       /* increase the device node's refcount */
>>>>>> +       get_device(&radio->videodev.dev);
>>>>>>        v4l2_device_disconnect(&radio->v4l2_dev);
>>>>>> -       mutex_unlock(&radio->lock);
>>>>>>        video_unregister_device(&radio->videodev);
>>>>>> +       mutex_unlock(&radio->lock);
>>>>>> +       /* decrease the device node's refcount, allowing it to be
>>>>>> released */
>>>>>> +       put_device(&radio->videodev.dev);
>>>>>>  }
>>>>>
>>>>> Hans, I understand the use of get/put_device here.. but can you
>>>>> explain to me what issue you are trying to solve?
>>>>> video_unregister_device does not have to be synchronized with anything
>>>>> else. Thus, it is perfectly safe to call video_unregister_device while
>>>>> not holding the device lock. Your prior implementation here was
>>>>> correct.
>>>>
>>>> This the original sequence:
>>>>
>>>>       mutex_lock(&radio->lock);
>>>>       v4l2_device_disconnect(&radio->v4l2_dev);
>>>>       mutex_unlock(&radio->lock);
>>>>       video_unregister_device(&radio->videodev);
>>>>
>>>> The problem with this is that userspace can call open or ioctl after the
>>>> unlock and before the device node is marked unregistered by
>>>> video_unregister_device.
>>>>
>>>> Once you disconnect you want to block all access (except the release
>>>> call).
>>>> What my patch does is to move the video_unregister_device call inside
>>>> the
>>>> lock, but then I have to guard against the release being called before
>>>> the
>>>> unlock by increasing the refcount.
>>>>
>>>> I have ideas to improve on this as this gets hairy when you have
>>>> multiple
>>>> device nodes, but I wait with that until the next kernel cycle.
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>
>>> I think you're trying to solve a problem that doesn't exist.
>>> To be a little more specific we have the following:
>>>
>>> 1. video_register_device - increments device refcount
>>> 2. video_unregister_device - decrements device refcount
>>> 3. v4l2_open - increments device refcount
>>> 4. v4l2_release - decrements device refcount
>>>
>>> Keeping this in mind, the release callback of video_device is called
>>> only when the device count reaches 0.
>>>
>>> So under normal operation we have:
>>>
>>> 1. video_register_device -> device refcount incremented to 1
>>> 2. v4l2_open -> device refcount incremented to 2
>>> 3. v4l2_release -> device refcount decremented to 1
>>> 4. disconnect callback: video_unregister_device -> device refcount
>>> decremented to 0 & release callback called.
>>>
>>> If the user disconnects the device while it's open we have the following:
>>>
>>> 1. video_register_device -> device refcount incremented to 1
>>> 2. v4l2_open -> device refcount incremented to 2
>>> 3. disconnect callback: video_unregister_device -> device refcount
>>> decremented to 1
>>> 4. v4l2_release -> device refcount decremented to 0 & release callback
>>> called.
>>>
>>> In the above case, once video_unregister_device has been called, calls
>>> to open no longer will work. However, the user holding the currently
>>> open file handle can still call ioctl and other callbacks, but those
>>> should be met with an -EIO, forcing them to close the open handle. The
>>> original code did this by using the usb device as an indicator to see
>>> if the device was still connected, as this functionality was not in
>>> v4l2_core. On the other hand, v4l2_core could do this for us, just by
>>> checking if the device is still registered.
>>>
>>> As you can see from the above, there are no race conditions here.
>>
>> Yes there is a race: what you want is that in the disconnect function you
>> can safely clean up any data structures or whatever in the knowledge that
>> once the device nodes are unregistered only the release call can reach the
>> driver. All other calls (open, ioctl, etc) are blocked by the v4l core.
>>
>> In order to do this you have to unregister the device nodes while the lock
>> is held. In the original code the device node was unregistered without the
>> lock being held, so that would allow userspace to sneak in a e.g. open()
>> call just before the unregister call.
>>
>> BTW, note that this is probably not relevant for this particular driver.
>> Even if this race happens, it will most likely not be a problem for this
>> simple device.
>>
>> Regards,
>>
>>        Hans
>>
>
> Hans, right I understand what you're getting at.. between the
> v4l2_device_disconnect and video_unregister_device a call to open
> could succeed. In the original code, the usb device of the object was
> set to NULL. In the open callback if the usb device was NULL, the open
> would fail with -EIO, thus preventing the race you are referring to.
>
> Subsequently, all ioctl callbacks had the same check applied. So yes,
> while there is a race here now, there wasn't one before. It was
> already handled by the driver. The addition get/put_device calls do
> nothing to prevent this race. Not even locking around
> video_unregister_device prevents this race. Consider the following:
>
> 1. The lock is taken in the disconnect callback.
> 2. While the disconnect callback is being processed, a call to open happens.
> 3. Since the lock is already taken the open waits on the lock to be released.
> 4. The lock is finally released, the call to open acquires the lock
> and succeeds.
>
> As you can see.. there's nothing you can do to prevent this from
> happening. This is the reason the original code set the usb device to
> NULL while holding the lock, and checked to see if it was NULL in open
> and the ioctl callbacks with the lock held. It's the only way to
> prevent this race from happening.

OK, I see how this fixes it now.. You added the video_is_registered
check inside v4l2_open after acquiring the device lock. So while it
fixes that race it's rather ugly and very difficult to follow. In this
case, the original code provided an easier to follow sequence. If
possible it would be nice if v4l2_open and v4l2_unlocked_ioctl relied
on the device being connected rather than registered. Then this would
have been a non-issue and it would be much easier to follow.

Regards,

David Ellingsworth

Re: [RFC PATCH] radio-mr800: locking fixes

[Hans Verkuil]

On Monday, October 18, 2010 16:55:40 David Ellingsworth wrote:
> OK, I see how this fixes it now.. You added the video_is_registered
> check inside v4l2_open after acquiring the device lock. So while it
> fixes that race it's rather ugly and very difficult to follow. In this
> case, the original code provided an easier to follow sequence. If
> possible it would be nice if v4l2_open and v4l2_unlocked_ioctl relied
> on the device being connected rather than registered. Then this would
> have been a non-issue and it would be much easier to follow.

I agree that it isn't the prettiest code. My goal is to improve the v4l
core to make such cases easier to handle. But I'm waiting for the next
kernel cycle before I continue with this.

Working on this driver definitely helped give me a better understanding of
what is involved.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCH] radio-mr800: locking fixes

[David Ellingsworth]

On Mon, Oct 18, 2010 at 9:20 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> On Sun, Oct 17, 2010 at 8:52 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> On Sunday, October 17, 2010 14:26:18 Hans Verkuil wrote:
>>>> - serialize the suspend and resume functions using the global lock.
>>>> - do not call usb_autopm_put_interface after a disconnect.
>>>> - fix a race when disconnecting the device.
>>>
>>> Regarding autosuspend: something seems to work since the
>>> power/runtime_status
>>> attribute goes from 'suspended' to 'active' whenever the radio handle is
>>> open.
>>> But the suspend and resume functions are never called. I can't figure
>>> out
>>> why not. I don't see anything strange.
>>>
>>> The whole autopm stuff is highly suspect anyway on a device like this
>>> since
>>> it is perfectly reasonable to just set a frequency and exit. The audio
>>> is
>>> just going to the line-in anyway. In other words: not having the device
>>> node
>>> open does not mean that the device is idle and can be suspended.
>>>
>>> My proposal would be to rip out the whole autosuspend business from this
>>> driver. I've no idea why it is here at all.
>>>
>>> Regards,
>>>
>>>        Hans
>>
>> Hans, I highly agree with that analysis. The original author put that
>> code in. But like you, I'm not sure if it was ever really valid. Since
>> I didn't have anything to test with, I left it untouched.
>>
>> Regards,
>>
>> David Ellingsworth
>>
>>
>
> OK, then I'll make a new patch that just rips out autosuspend support.

I thought about this a little more. I think this driver could benefit
from auto-suspend, but it's current implementation is indeed flawed.
The calls to usb_autopm_put/get_interface could occur whenever the
device is muted/unmuted respectively after the device has been
initialized. Thus, the suspend should not happen while the device is
in use per se, but could occur after it's been muted.

Regards,

David Ellingsworth

Re: [RFC PATCH] radio-mr800: locking fixes

[Mauro Carvalho Chehab]

Em 18-10-2010 13:51, Hans Verkuil escreveu:
> On Monday, October 18, 2010 16:55:40 David Ellingsworth wrote:
>> OK, I see how this fixes it now.. You added the video_is_registered
>> check inside v4l2_open after acquiring the device lock. So while it
>> fixes that race it's rather ugly and very difficult to follow. In this
>> case, the original code provided an easier to follow sequence. If
>> possible it would be nice if v4l2_open and v4l2_unlocked_ioctl relied
>> on the device being connected rather than registered. Then this would
>> have been a non-issue and it would be much easier to follow.
> 
> I agree that it isn't the prettiest code. My goal is to improve the v4l
> core to make such cases easier to handle. But I'm waiting for the next
> kernel cycle before I continue with this.
> 
> Working on this driver definitely helped give me a better understanding of
> what is involved.
> 
> Regards,
>
Any conclusion about the locking fixes patch?

Cheers,
Mauro

Re: [RFC PATCH] radio-mr800: locking fixes

[David Ellingsworth]

> Any conclusion about the locking fixes patch?
>

I don't like it, but it at least fixes the problem. You can add my ack.

Acked-by: David Ellingsworth<david@identd.dyndns.org>