Re: [RFC PATCH v2 0/5] Media Device Allocator API

[Mauro Carvalho Chehab]

Em Tue, 5 Apr 2016 08:10:11 +0200
Takashi Iwai  escreveu:

> On Tue, 05 Apr 2016 05:35:55 +0200,
> Shuah Khan wrote:
> > 
> > There are known problems with media device life time management. When media
> > device is released while an media ioctl is in progress, ioctls fail with
> > use-after-free errors and kernel hangs in some cases.
> > 
> > Media Device can be in any the following states:
> > 
> > - Allocated
> > - Registered (could be tied to more than one driver)
> > - Unregistered, not in use (media device file is not open)
> > - Unregistered, in use (media device file is not open)
> > - Released
> > 
> > When media device belongs to  more than one driver, registrations should be
> > tracked to avoid unregistering when one of the drivers does unregister. A 
> > new
> > num_drivers field in the struct media_device covers this case. The media 
> > device
> > should be unregistered only when the last unregister occurs with num_drivers
> > count zero.
> > 
> > When a media device is in use when it is unregistered, it should not be
> > released until the application exits when it detects the unregistered
> > status. Media device that is in use when it is unregistered is moved to
> > to_delete_list. When the last unregister occurs, media device is 
> > unregistered
> > and becomes an unregistered, still allocated device. Unregister marks the
> > device to be deleted.
> > 
> > When media device belongs to more than one driver, as both drivers could be
> > unbound/bound, driver should not end up getting stale media device that is
> > on its way out. Moving the unregistered media device to to_delete_list helps
> > this case as well.
> > 
> > I ran bind/unbind loop tests on uvcvideo, au0828, and snd-usb-audio while
> > running application that does ioctls. Didn't see any use-after-free errors
> > on media device. A couple of known issues seen:
> > 
> > 1. When application exits, cdev_put() gets called after media device is
> >released. This is a known issue to resolve and Media Device Allocator
> >can't solve this one.
> > 2. When au0828 module is removed and then ioctls fail when cdev_get() looks
> >for the owning module as au0828 is very often the module that owns the
> >media devnode. This is a cdev related issue that needs to be resolved and
> >Media Device Allocator can't solve this one.
> > 
> > Shuah Khan (5):
> >   media: Add Media Device Allocator API
> >   media: Add driver count to keep track of media device registrations
> >   media: uvcvideo change to use Media Device Allocator API
> >   media: au0828 change to use Media Device Allocator API
> >   sound/usb: Use Media Controller API to share media resources  
> 
> I don't think we need to include usb-audio patch at this stage yet.

I agree. Let's first fix MC races first, then address the multi-driver
issues. Only after having those fixed, we should look at the sound/usb
patch.

Ok, we could keep it on some testing tree, but, IMHO,  it doesn't make any
sense to submit it to review, while the core is not fixed.

> The most important thing for now is to improve / stabilize the API
> itself so that other drivers can use it as is.  Once when the API is
> really stabilized, we create a solid git branch that may be based for
> multiple subsystems, and I'll merge usb-audio stuff through sound git
> tree.

Works for me. After we have this properly fixed and stabilized on media,
I'll pass you a stable topic branch for you. This way, you can test a
new version of the sound/usb patch and apply on your tree when it fits
well for you.

> 
> Also, the previous usb-audio MC implementation had a few serious bugs,
> including quirk NULL dereference.  See the bugzilla below for some fix
> patches to 4.6-rc1:
>   https://bugzilla.kernel.org/show_bug.cgi?id=115561
> Feel free to fold them in, if they are still valid.
> 
> 
> thanks,
> 
> Takashi


-- 

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

Re: [RFC PATCH v2 0/5] Media Device Allocator API

[Shuah Khan]

On 04/05/2016 12:10 AM, Takashi Iwai wrote:
> On Tue, 05 Apr 2016 05:35:55 +0200,
> Shuah Khan wrote:
>>
>> There are known problems with media device life time management. When media
>> device is released while an media ioctl is in progress, ioctls fail with
>> use-after-free errors and kernel hangs in some cases.
>>
>> Media Device can be in any the following states:
>>
>> - Allocated
>> - Registered (could be tied to more than one driver)
>> - Unregistered, not in use (media device file is not open)
>> - Unregistered, in use (media device file is not open)
>> - Released
>>
>> When media device belongs to  more than one driver, registrations should be
>> tracked to avoid unregistering when one of the drivers does unregister. A new
>> num_drivers field in the struct media_device covers this case. The media 
>> device
>> should be unregistered only when the last unregister occurs with num_drivers
>> count zero.
>>
>> When a media device is in use when it is unregistered, it should not be
>> released until the application exits when it detects the unregistered
>> status. Media device that is in use when it is unregistered is moved to
>> to_delete_list. When the last unregister occurs, media device is unregistered
>> and becomes an unregistered, still allocated device. Unregister marks the
>> device to be deleted.
>>
>> When media device belongs to more than one driver, as both drivers could be
>> unbound/bound, driver should not end up getting stale media device that is
>> on its way out. Moving the unregistered media device to to_delete_list helps
>> this case as well.
>>
>> I ran bind/unbind loop tests on uvcvideo, au0828, and snd-usb-audio while
>> running application that does ioctls. Didn't see any use-after-free errors
>> on media device. A couple of known issues seen:
>>
>> 1. When application exits, cdev_put() gets called after media device is
>>released. This is a known issue to resolve and Media Device Allocator
>>can't solve this one.
>> 2. When au0828 module is removed and then ioctls fail when cdev_get() looks
>>for the owning module as au0828 is very often the module that owns the
>>media devnode. This is a cdev related issue that needs to be resolved and
>>Media Device Allocator can't solve this one.
>>
>> Shuah Khan (5):
>>   media: Add Media Device Allocator API
>>   media: Add driver count to keep track of media device registrations
>>   media: uvcvideo change to use Media Device Allocator API
>>   media: au0828 change to use Media Device Allocator API
>>   sound/usb: Use Media Controller API to share media resources
> 
> I don't think we need to include usb-audio patch at this stage yet.
> The most important thing for now is to improve / stabilize the API
> itself so that other drivers can use it as is.  Once when the API is
> really stabilized, we create a solid git branch that may be based for
> multiple subsystems, and I'll merge usb-audio stuff through sound git
> tree.

Agreed. I included snd-usb-audio as it provides a good test case for
multiple driver use-case. Yes it is a good idea to have a git branch
for wider testing.

> 
> Also, the previous usb-audio MC implementation had a few serious bugs,
> including quirk NULL dereference.  See the bugzilla below for some fix
> patches to 4.6-rc1:
>   https://bugzilla.kernel.org/show_bug.cgi?id=115561
> Feel free to fold them in, if they are still valid.

I folded them in. It is unfortunate that these bugs were introduced towards
the end when I was making changes to address review comments and I didn't
catch them, especially the quirk NULL dereference.

thanks,
-- Shuah

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

Re: [RFC PATCH v2 0/5] Media Device Allocator API

[Takashi Iwai]

On Tue, 05 Apr 2016 05:35:55 +0200,
Shuah Khan wrote:
> 
> There are known problems with media device life time management. When media
> device is released while an media ioctl is in progress, ioctls fail with
> use-after-free errors and kernel hangs in some cases.
> 
> Media Device can be in any the following states:
> 
> - Allocated
> - Registered (could be tied to more than one driver)
> - Unregistered, not in use (media device file is not open)
> - Unregistered, in use (media device file is not open)
> - Released
> 
> When media device belongs to  more than one driver, registrations should be
> tracked to avoid unregistering when one of the drivers does unregister. A new
> num_drivers field in the struct media_device covers this case. The media 
> device
> should be unregistered only when the last unregister occurs with num_drivers
> count zero.
> 
> When a media device is in use when it is unregistered, it should not be
> released until the application exits when it detects the unregistered
> status. Media device that is in use when it is unregistered is moved to
> to_delete_list. When the last unregister occurs, media device is unregistered
> and becomes an unregistered, still allocated device. Unregister marks the
> device to be deleted.
> 
> When media device belongs to more than one driver, as both drivers could be
> unbound/bound, driver should not end up getting stale media device that is
> on its way out. Moving the unregistered media device to to_delete_list helps
> this case as well.
> 
> I ran bind/unbind loop tests on uvcvideo, au0828, and snd-usb-audio while
> running application that does ioctls. Didn't see any use-after-free errors
> on media device. A couple of known issues seen:
> 
> 1. When application exits, cdev_put() gets called after media device is
>released. This is a known issue to resolve and Media Device Allocator
>can't solve this one.
> 2. When au0828 module is removed and then ioctls fail when cdev_get() looks
>for the owning module as au0828 is very often the module that owns the
>media devnode. This is a cdev related issue that needs to be resolved and
>Media Device Allocator can't solve this one.
> 
> Shuah Khan (5):
>   media: Add Media Device Allocator API
>   media: Add driver count to keep track of media device registrations
>   media: uvcvideo change to use Media Device Allocator API
>   media: au0828 change to use Media Device Allocator API
>   sound/usb: Use Media Controller API to share media resources

I don't think we need to include usb-audio patch at this stage yet.
The most important thing for now is to improve / stabilize the API
itself so that other drivers can use it as is.  Once when the API is
really stabilized, we create a solid git branch that may be based for
multiple subsystems, and I'll merge usb-audio stuff through sound git
tree.

Also, the previous usb-audio MC implementation had a few serious bugs,
including quirk NULL dereference.  See the bugzilla below for some fix
patches to 4.6-rc1:
  https://bugzilla.kernel.org/show_bug.cgi?id=115561
Feel free to fold them in, if they are still valid.


thanks,

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