Re: [PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Sakari Ailus]

Hi Mauro,

On Tue, Jun 07, 2016 at 10:11:33AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 6 Jun 2016 17:13:12 -0600
> Shuah Khan  escreveu:
> 
> > > A few general comments on the patch --- I agree we've had the problem from
> > > the day one, but it's really started showing up recently. I agree with the
> > > taken approach of separating the lifetimes of both media device and the
> > > devnode. However, I don't think the patch as such is enough.
> 
> Do you or Laurent has an alternative patchset to fix those issues? From 
> where I sit, we have a serious bug that it is already known for a while,
> but nobody really tried to fix so far, except for Shuah and myself.

If I had, I would have posted it. :-I

> So, if you don't have any alternative patch ready to be merged, I'll
> apply what we have later today, together with the patch that fixes cdev
> livetime management:
>   https://patchwork.linuxtv.org/patch/34201/
> 
> This will allow it to be tested to a broader audience and check if
> the known issues will be fixed. I'll add a C/C stable, but my plan is
> to not send it as a fix for 4.7. Instead, to keep the fixes on our tree
> up to the next merge window, giving us ~5 weeks for testing.
> 
> As this is a Kernel only issue, it can be changed later if someone pops
> up with a better approach.

Ok. I haven't had time to review Shuah's patch but the direction taken sound
good.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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: [PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Mauro Carvalho Chehab]

Em Mon, 6 Jun 2016 17:13:12 -0600
Shuah Khan  escreveu:

> > A few general comments on the patch --- I agree we've had the problem from
> > the day one, but it's really started showing up recently. I agree with the
> > taken approach of separating the lifetimes of both media device and the
> > devnode. However, I don't think the patch as such is enough.

Do you or Laurent has an alternative patchset to fix those issues? From 
where I sit, we have a serious bug that it is already known for a while,
but nobody really tried to fix so far, except for Shuah and myself.

So, if you don't have any alternative patch ready to be merged, I'll
apply what we have later today, together with the patch that fixes cdev
livetime management:
https://patchwork.linuxtv.org/patch/34201/

This will allow it to be tested to a broader audience and check if
the known issues will be fixed. I'll add a C/C stable, but my plan is
to not send it as a fix for 4.7. Instead, to keep the fixes on our tree
up to the next merge window, giving us ~5 weeks for testing.

As this is a Kernel only issue, it can be changed later if someone pops
up with a better approach.

Regards,
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: [PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Shuah Khan]

Hi Sakari,

On 06/06/2016 02:45 AM, Sakari Ailus wrote:
> Hi Mauro,
> 
> On Sat, May 07, 2016 at 12:12:09PM -0300, Mauro Carvalho Chehab wrote:
>> struct media_devnode is currently embedded at struct media_device.
>>
>> While this works fine during normal usage, it leads to a race
>> condition during devnode unregister. the problem is that drivers
>> assume that, after calling media_device_unregister(), the struct
>> that contains media_device can be freed. This is not true, as it
>> can't be freed until userspace closes all opened /dev/media devnodes.
>>
>> In other words, if the media devnode is still open, and media_device
>> gets freed, any call to an ioctl will make the core to try to access
>> struct media_device, with will cause an use-after-free and even GPF.
>>
>> Fix this by dynamically allocating the struct media_devnode and only
>> freeing it when it is safe.
> 
> A few general comments on the patch --- I agree we've had the problem from
> the day one, but it's really started showing up recently. I agree with the
> taken approach of separating the lifetimes of both media device and the
> devnode. However, I don't think the patch as such is enough.

It is more like, we started actively testing for this problem. I added a new
test under selftests/media for this problem. Laurent brought this up at our
Finland media summit and I have been looking to solve this since then starting
with writing a test to reliably reproduce the problem.

You are right that this patch alone isn't enough and I sent in another patch 
that
sets cdev kref parent to media_devnode struct device kref.

https://patchwork.linuxtv.org/patch/34201/

> 
> For one, there are some issue that remain. In particular, access to the data
> structures (i.e. media_device and media_devnode) aren't serialised: the
> IOCTL or a system call passes media-devnode framework asynchronously with a
> possible unregister() call coming from media_device_unregister() (in
> media-device.c). This may, unless I'm mistaken, to the following sequence:
> 
> process 1 process 2
> fd = open(/dev/media0)
> 
>   media_device_unregister()
>   (things are being cleaned up
>   here but the devnode isn't
>   unregistered yet)
>   ...
> ioctl(fd, ...)
> __media_ioctl()
> media_devnode_is_registered()
>   (returns true here)
>   ...
>   media_devnode_unregister()
>   ...
>   (driver releases the media device
>   memory)
> 
> media_device_ioctl()
>   (By this point
>   devnode->media_dev does not
>   point to allocated memory.
>   Bad things will happen here.)
> 
> 
> You could try to serialise the operations. I wonder how ugly that might
> be, and I'm not sure this would be a workable approach.

I don't believe this problem can be solved with serializing operations.
media_devnode_is_registered() relies on media devnode to be valid until
the corresponding close is done.

We have:

struct media_device embedding struct media_devnode and media_devnode
embeds cdev. Each one of these have their own refcounts. media_device
can be released even when the cdev is busy. When media_device is released,
media_devnode goes away. The only sure way to ensure media_devnode sticks
around is by dynamically allocating it. This decouples media_devnode life
time management from media_device management. Granted media_devnode is tied
to media_device, but by decoupling, we make the media_devnode_is_registered()
safe even when media_device is released.

The next step is coupling media_devnode cdev lifetime with media_devnode
lifetime, by setting devnode strucr device kobj as the cdev kobj parent.
Please see the following patch I sent out:

https://patchwork.linuxtv.org/patch/34201/

This patch ensures devnode isn't released until the last application
closes the device and the last cdev kobj parent is released.

> 
> Secondly, a dependency is created from media devnode (i.e. media devnode
> becomes aware of the media device). This is against the original design of
> the two, as the media devnode was intended for other kinds of device nodes
> as well and is more generic than media device. I'm not necessarily arguing
> we have to keep it this way (as media devnode is only used by media device),
> but if we're not keeping it then it'd make sense to unify the two to keep it
> clean.

Unless there is a string reason to not make media devnode aware of the media
device, this is a good direction. As such, our current design is not ideal.
media devnode isn't aware of the structure its life depends on. :)

> 
> 
> Have you thought about taking a reference to the said structs (by 

Re: [PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Sakari Ailus]

Hi Mauro,

On Sat, May 07, 2016 at 12:12:09PM -0300, Mauro Carvalho Chehab wrote:
> struct media_devnode is currently embedded at struct media_device.
> 
> While this works fine during normal usage, it leads to a race
> condition during devnode unregister. the problem is that drivers
> assume that, after calling media_device_unregister(), the struct
> that contains media_device can be freed. This is not true, as it
> can't be freed until userspace closes all opened /dev/media devnodes.
> 
> In other words, if the media devnode is still open, and media_device
> gets freed, any call to an ioctl will make the core to try to access
> struct media_device, with will cause an use-after-free and even GPF.
> 
> Fix this by dynamically allocating the struct media_devnode and only
> freeing it when it is safe.

A few general comments on the patch --- I agree we've had the problem from
the day one, but it's really started showing up recently. I agree with the
taken approach of separating the lifetimes of both media device and the
devnode. However, I don't think the patch as such is enough.

For one, there are some issue that remain. In particular, access to the data
structures (i.e. media_device and media_devnode) aren't serialised: the
IOCTL or a system call passes media-devnode framework asynchronously with a
possible unregister() call coming from media_device_unregister() (in
media-device.c). This may, unless I'm mistaken, to the following sequence:

process 1   process 2
fd = open(/dev/media0)

media_device_unregister()
(things are being cleaned up
here but the devnode isn't
unregistered yet)
...
ioctl(fd, ...)
__media_ioctl()
media_devnode_is_registered()
(returns true here)
...
media_devnode_unregister()
...
(driver releases the media device
memory)

media_device_ioctl()
(By this point
devnode->media_dev does not
point to allocated memory.
Bad things will happen here.)


You could try to serialise the operations. I wonder how ugly that might
be, and I'm not sure this would be a workable approach.

Secondly, a dependency is created from media devnode (i.e. media devnode
becomes aware of the media device). This is against the original design of
the two, as the media devnode was intended for other kinds of device nodes
as well and is more generic than media device. I'm not necessarily arguing
we have to keep it this way (as media devnode is only used by media device),
but if we're not keeping it then it'd make sense to unify the two to keep it
clean.


Have you thought about taking a reference to the said structs (by the means
of kref) where one is acquired?

That way, we should be able to rather easily keep around everything that's
needed until the last remaining user has gone away: opening a file handle
should get a reference to both media device and media devnode as well as
registering them (media_device_register() and media_devnode_register()).

> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/media-device.c   | 44 
> +++---
>  drivers/media/media-devnode.c  |  7 +-
>  drivers/media/usb/au0828/au0828-core.c |  4 ++--
>  drivers/media/usb/uvc/uvc_driver.c |  2 +-
>  include/media/media-device.h   |  5 +---
>  include/media/media-devnode.h  | 13 +-
>  6 files changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 47a99af5525e..8c520e064c34 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp, 
> unsigned int cmd,
>  unsigned long arg)
>  {
>   struct media_devnode *devnode = media_devnode_data(filp);
> - struct media_device *dev = to_media_device(devnode);
> + struct media_device *dev = devnode->media_dev;
>   long ret;
>  
>   mutex_lock(>graph_mutex);
> @@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp, 
> unsigned int cmd,
> unsigned long arg)
>  {
>   struct media_devnode *devnode = media_devnode_data(filp);
> - struct media_device *dev = to_media_device(devnode);
> + struct media_device *dev = devnode->media_dev;
>   long ret;
>  
>   switch (cmd) {
> @@ -531,7 +531,8 @@ static const struct media_file_operations 
> media_device_fops = {
>  static ssize_t show_model(struct device *cd,
> 

Re: [PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Shuah Khan]

On 05/09/2016 09:03 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 9 May 2016 14:40:02 +0300
> Laurent Pinchart  escreveu:
> 
>> Hi Mauro,
>>
>> Thank you for the patch.
>>
>> On Saturday 07 May 2016 12:12:09 Mauro Carvalho Chehab wrote:
>>> struct media_devnode is currently embedded at struct media_device.  
>>
>> s/at/in/
>>
>>> While this works fine during normal usage, it leads to a race
>>> condition during devnode unregister.  
>>
>> Strictly speaking the race condition isn't cause by embedding struct 
>> media_devnode inside struct media_device. The race condition is unavoidable 
>> as 
>> we have two asynchronous operations (unregistration and userspace access) 
>> that 
>> affect the same structures. This isn't a problem as such, this kind of race 
>> conditions is handled in the kernel through release callbacks to implement 
>> proper lifetime management of data structures. The problem here is that the 
>> release callbacks are not propagated all the way up to the drivers.
> 
> We should not mix the description of the problem with its solution.
> 
> See more below.
>>
>>> the problem is that drivers  
>>
>> s/the/The/
>>
>>> assume that, after calling media_device_unregister(), the struct
>>> that contains media_device can be freed. This is not true, as it
>>> can't be freed until userspace closes all opened /dev/media devnodes.  
>>
>> Not all the open devnodes, just the one related to the struct media_devnode 
>> instance.
> 
> Sure. That's what I meant to say.
> 
>>
>>> In other words, if the media devnode is still open, and media_device
>>> gets freed, any call to an ioctl will make the core to try to access
>>> struct media_device, with will cause an use-after-free and even GPF.
>>>
>>> Fix this by dynamically allocating the struct media_devnode and only
>>> freeing it when it is safe.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab 
>>> ---
>>>  drivers/media/media-device.c   | 44 ---
>>>  drivers/media/media-devnode.c  |  7 +-
>>>  drivers/media/usb/au0828/au0828-core.c |  4 ++--
>>>  drivers/media/usb/uvc/uvc_driver.c |  2 +-
>>>  include/media/media-device.h   |  5 +---
>>>  include/media/media-devnode.h  | 13 +-
>>>  6 files changed, 52 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index 47a99af5525e..8c520e064c34 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp,
>>> unsigned int cmd, unsigned long arg)
>>>  {
>>> struct media_devnode *devnode = media_devnode_data(filp);
>>> -   struct media_device *dev = to_media_device(devnode);
>>> +   struct media_device *dev = devnode->media_dev;
>>> long ret;
>>>
>>> mutex_lock(>graph_mutex);
>>> @@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp,
>>> unsigned int cmd, unsigned long arg)
>>>  {
>>> struct media_devnode *devnode = media_devnode_data(filp);
>>> -   struct media_device *dev = to_media_device(devnode);
>>> +   struct media_device *dev = devnode->media_dev;
>>> long ret;
>>>
>>> switch (cmd) {
>>> @@ -531,7 +531,8 @@ static const struct media_file_operations
>>> media_device_fops = { static ssize_t show_model(struct device *cd,
>>>   struct device_attribute *attr, char *buf)
>>>  {
>>> -   struct media_device *mdev = to_media_device(to_media_devnode(cd));
>>> +   struct media_devnode *devnode = to_media_devnode(cd);
>>> +   struct media_device *mdev = devnode->media_dev;
>>>
>>> return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
>>>  }
>>> @@ -704,23 +705,34 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
>>>  int __must_check __media_device_register(struct media_device *mdev,
>>>  struct module *owner)
>>>  {
>>> +   struct media_devnode *devnode;
>>> int ret;
>>>
>>> +   devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
>>> +   if (!devnode)
>>> +   return -ENOMEM;
>>> +
>>> /* Register the device node. */
>>> -   mdev->devnode.fops = _device_fops;
>>> -   mdev->devnode.parent = mdev->dev;
>>> -   mdev->devnode.release = media_device_release;
>>> +   mdev->devnode = devnode;
>>> +   devnode->fops = _device_fops;
>>> +   devnode->parent = mdev->dev;
>>> +   devnode->release = media_device_release;
>>>
>>> /* Set version 0 to indicate user-space that the graph is static */
>>> mdev->topology_version = 0;
>>>
>>> -   ret = media_devnode_register(>devnode, owner);
>>> -   if (ret < 0)
>>> +   ret = media_devnode_register(mdev, devnode, owner);
>>> +   if (ret < 0) {
>>> +   mdev->devnode = NULL;
>>> +   kfree(devnode);
>>> return ret;
>>> +   }
>>>
>>> -   ret = device_create_file(>devnode.dev, _attr_model);
>>> +   ret = device_create_file(>dev, _attr_model);

Re: [PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Mauro Carvalho Chehab]

Em Mon, 9 May 2016 14:40:02 +0300
Laurent Pinchart  escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Saturday 07 May 2016 12:12:09 Mauro Carvalho Chehab wrote:
> > struct media_devnode is currently embedded at struct media_device.  
> 
> s/at/in/
> 
> > While this works fine during normal usage, it leads to a race
> > condition during devnode unregister.  
> 
> Strictly speaking the race condition isn't cause by embedding struct 
> media_devnode inside struct media_device. The race condition is unavoidable 
> as 
> we have two asynchronous operations (unregistration and userspace access) 
> that 
> affect the same structures. This isn't a problem as such, this kind of race 
> conditions is handled in the kernel through release callbacks to implement 
> proper lifetime management of data structures. The problem here is that the 
> release callbacks are not propagated all the way up to the drivers.

We should not mix the description of the problem with its solution.

See more below.
> 
> > the problem is that drivers  
> 
> s/the/The/
> 
> > assume that, after calling media_device_unregister(), the struct
> > that contains media_device can be freed. This is not true, as it
> > can't be freed until userspace closes all opened /dev/media devnodes.  
> 
> Not all the open devnodes, just the one related to the struct media_devnode 
> instance.

Sure. That's what I meant to say.

> 
> > In other words, if the media devnode is still open, and media_device
> > gets freed, any call to an ioctl will make the core to try to access
> > struct media_device, with will cause an use-after-free and even GPF.
> > 
> > Fix this by dynamically allocating the struct media_devnode and only
> > freeing it when it is safe.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  drivers/media/media-device.c   | 44 ---
> >  drivers/media/media-devnode.c  |  7 +-
> >  drivers/media/usb/au0828/au0828-core.c |  4 ++--
> >  drivers/media/usb/uvc/uvc_driver.c |  2 +-
> >  include/media/media-device.h   |  5 +---
> >  include/media/media-devnode.h  | 13 +-
> >  6 files changed, 52 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 47a99af5525e..8c520e064c34 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp,
> > unsigned int cmd, unsigned long arg)
> >  {
> > struct media_devnode *devnode = media_devnode_data(filp);
> > -   struct media_device *dev = to_media_device(devnode);
> > +   struct media_device *dev = devnode->media_dev;
> > long ret;
> > 
> > mutex_lock(>graph_mutex);
> > @@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp,
> > unsigned int cmd, unsigned long arg)
> >  {
> > struct media_devnode *devnode = media_devnode_data(filp);
> > -   struct media_device *dev = to_media_device(devnode);
> > +   struct media_device *dev = devnode->media_dev;
> > long ret;
> > 
> > switch (cmd) {
> > @@ -531,7 +531,8 @@ static const struct media_file_operations
> > media_device_fops = { static ssize_t show_model(struct device *cd,
> >   struct device_attribute *attr, char *buf)
> >  {
> > -   struct media_device *mdev = to_media_device(to_media_devnode(cd));
> > +   struct media_devnode *devnode = to_media_devnode(cd);
> > +   struct media_device *mdev = devnode->media_dev;
> > 
> > return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
> >  }
> > @@ -704,23 +705,34 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
> >  int __must_check __media_device_register(struct media_device *mdev,
> >  struct module *owner)
> >  {
> > +   struct media_devnode *devnode;
> > int ret;
> > 
> > +   devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
> > +   if (!devnode)
> > +   return -ENOMEM;
> > +
> > /* Register the device node. */
> > -   mdev->devnode.fops = _device_fops;
> > -   mdev->devnode.parent = mdev->dev;
> > -   mdev->devnode.release = media_device_release;
> > +   mdev->devnode = devnode;
> > +   devnode->fops = _device_fops;
> > +   devnode->parent = mdev->dev;
> > +   devnode->release = media_device_release;
> > 
> > /* Set version 0 to indicate user-space that the graph is static */
> > mdev->topology_version = 0;
> > 
> > -   ret = media_devnode_register(>devnode, owner);
> > -   if (ret < 0)
> > +   ret = media_devnode_register(mdev, devnode, owner);
> > +   if (ret < 0) {
> > +   mdev->devnode = NULL;
> > +   kfree(devnode);
> > return ret;
> > +   }
> > 
> > -   ret = device_create_file(>devnode.dev, _attr_model);
> > +   ret = device_create_file(>dev, _attr_model);
> > if (ret < 0) {
> > -   media_devnode_unregister(>devnode);
> > +  

Re: [PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Laurent Pinchart]

Hi Mauro,

Thank you for the patch.

On Saturday 07 May 2016 12:12:09 Mauro Carvalho Chehab wrote:
> struct media_devnode is currently embedded at struct media_device.

s/at/in/

> While this works fine during normal usage, it leads to a race
> condition during devnode unregister.

Strictly speaking the race condition isn't cause by embedding struct 
media_devnode inside struct media_device. The race condition is unavoidable as 
we have two asynchronous operations (unregistration and userspace access) that 
affect the same structures. This isn't a problem as such, this kind of race 
conditions is handled in the kernel through release callbacks to implement 
proper lifetime management of data structures. The problem here is that the 
release callbacks are not propagated all the way up to the drivers.

> the problem is that drivers

s/the/The/

> assume that, after calling media_device_unregister(), the struct
> that contains media_device can be freed. This is not true, as it
> can't be freed until userspace closes all opened /dev/media devnodes.

Not all the open devnodes, just the one related to the struct media_devnode 
instance.

> In other words, if the media devnode is still open, and media_device
> gets freed, any call to an ioctl will make the core to try to access
> struct media_device, with will cause an use-after-free and even GPF.
> 
> Fix this by dynamically allocating the struct media_devnode and only
> freeing it when it is safe.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/media-device.c   | 44 ---
>  drivers/media/media-devnode.c  |  7 +-
>  drivers/media/usb/au0828/au0828-core.c |  4 ++--
>  drivers/media/usb/uvc/uvc_driver.c |  2 +-
>  include/media/media-device.h   |  5 +---
>  include/media/media-devnode.h  | 13 +-
>  6 files changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 47a99af5525e..8c520e064c34 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp,
> unsigned int cmd, unsigned long arg)
>  {
>   struct media_devnode *devnode = media_devnode_data(filp);
> - struct media_device *dev = to_media_device(devnode);
> + struct media_device *dev = devnode->media_dev;
>   long ret;
> 
>   mutex_lock(>graph_mutex);
> @@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp,
> unsigned int cmd, unsigned long arg)
>  {
>   struct media_devnode *devnode = media_devnode_data(filp);
> - struct media_device *dev = to_media_device(devnode);
> + struct media_device *dev = devnode->media_dev;
>   long ret;
> 
>   switch (cmd) {
> @@ -531,7 +531,8 @@ static const struct media_file_operations
> media_device_fops = { static ssize_t show_model(struct device *cd,
> struct device_attribute *attr, char *buf)
>  {
> - struct media_device *mdev = to_media_device(to_media_devnode(cd));
> + struct media_devnode *devnode = to_media_devnode(cd);
> + struct media_device *mdev = devnode->media_dev;
> 
>   return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
>  }
> @@ -704,23 +705,34 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
>  int __must_check __media_device_register(struct media_device *mdev,
>struct module *owner)
>  {
> + struct media_devnode *devnode;
>   int ret;
> 
> + devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
> + if (!devnode)
> + return -ENOMEM;
> +
>   /* Register the device node. */
> - mdev->devnode.fops = _device_fops;
> - mdev->devnode.parent = mdev->dev;
> - mdev->devnode.release = media_device_release;
> + mdev->devnode = devnode;
> + devnode->fops = _device_fops;
> + devnode->parent = mdev->dev;
> + devnode->release = media_device_release;
> 
>   /* Set version 0 to indicate user-space that the graph is static */
>   mdev->topology_version = 0;
> 
> - ret = media_devnode_register(>devnode, owner);
> - if (ret < 0)
> + ret = media_devnode_register(mdev, devnode, owner);
> + if (ret < 0) {
> + mdev->devnode = NULL;
> + kfree(devnode);
>   return ret;
> + }
> 
> - ret = device_create_file(>devnode.dev, _attr_model);
> + ret = device_create_file(>dev, _attr_model);
>   if (ret < 0) {
> - media_devnode_unregister(>devnode);
> + mdev->devnode = NULL;
> + media_devnode_unregister(devnode);
> + kfree(devnode);
>   return ret;
>   }
> 
> @@ -771,7 +783,7 @@ void media_device_unregister(struct media_device *mdev)
>   mutex_lock(>graph_mutex);
> 
>   /* Check if mdev was ever registered at all */
> - if (!media_devnode_is_registered(>devnode)) {

[PATCH 2/2] [media] media-device: dynamically allocate struct media_devnode

[Mauro Carvalho Chehab]

struct media_devnode is currently embedded at struct media_device.

While this works fine during normal usage, it leads to a race
condition during devnode unregister. the problem is that drivers
assume that, after calling media_device_unregister(), the struct
that contains media_device can be freed. This is not true, as it
can't be freed until userspace closes all opened /dev/media devnodes.

In other words, if the media devnode is still open, and media_device
gets freed, any call to an ioctl will make the core to try to access
struct media_device, with will cause an use-after-free and even GPF.

Fix this by dynamically allocating the struct media_devnode and only
freeing it when it is safe.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/media-device.c   | 44 +++---
 drivers/media/media-devnode.c  |  7 +-
 drivers/media/usb/au0828/au0828-core.c |  4 ++--
 drivers/media/usb/uvc/uvc_driver.c |  2 +-
 include/media/media-device.h   |  5 +---
 include/media/media-devnode.h  | 13 +-
 6 files changed, 52 insertions(+), 23 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 47a99af5525e..8c520e064c34 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -423,7 +423,7 @@ static long media_device_ioctl(struct file *filp, unsigned 
int cmd,
   unsigned long arg)
 {
struct media_devnode *devnode = media_devnode_data(filp);
-   struct media_device *dev = to_media_device(devnode);
+   struct media_device *dev = devnode->media_dev;
long ret;
 
mutex_lock(>graph_mutex);
@@ -495,7 +495,7 @@ static long media_device_compat_ioctl(struct file *filp, 
unsigned int cmd,
  unsigned long arg)
 {
struct media_devnode *devnode = media_devnode_data(filp);
-   struct media_device *dev = to_media_device(devnode);
+   struct media_device *dev = devnode->media_dev;
long ret;
 
switch (cmd) {
@@ -531,7 +531,8 @@ static const struct media_file_operations media_device_fops 
= {
 static ssize_t show_model(struct device *cd,
  struct device_attribute *attr, char *buf)
 {
-   struct media_device *mdev = to_media_device(to_media_devnode(cd));
+   struct media_devnode *devnode = to_media_devnode(cd);
+   struct media_device *mdev = devnode->media_dev;
 
return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
 }
@@ -704,23 +705,34 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
 int __must_check __media_device_register(struct media_device *mdev,
 struct module *owner)
 {
+   struct media_devnode *devnode;
int ret;
 
+   devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
+   if (!devnode)
+   return -ENOMEM;
+
/* Register the device node. */
-   mdev->devnode.fops = _device_fops;
-   mdev->devnode.parent = mdev->dev;
-   mdev->devnode.release = media_device_release;
+   mdev->devnode = devnode;
+   devnode->fops = _device_fops;
+   devnode->parent = mdev->dev;
+   devnode->release = media_device_release;
 
/* Set version 0 to indicate user-space that the graph is static */
mdev->topology_version = 0;
 
-   ret = media_devnode_register(>devnode, owner);
-   if (ret < 0)
+   ret = media_devnode_register(mdev, devnode, owner);
+   if (ret < 0) {
+   mdev->devnode = NULL;
+   kfree(devnode);
return ret;
+   }
 
-   ret = device_create_file(>devnode.dev, _attr_model);
+   ret = device_create_file(>dev, _attr_model);
if (ret < 0) {
-   media_devnode_unregister(>devnode);
+   mdev->devnode = NULL;
+   media_devnode_unregister(devnode);
+   kfree(devnode);
return ret;
}
 
@@ -771,7 +783,7 @@ void media_device_unregister(struct media_device *mdev)
mutex_lock(>graph_mutex);
 
/* Check if mdev was ever registered at all */
-   if (!media_devnode_is_registered(>devnode)) {
+   if (!media_devnode_is_registered(mdev->devnode)) {
mutex_unlock(>graph_mutex);
return;
}
@@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev)
 
mutex_unlock(>graph_mutex);
 
-   device_remove_file(>devnode.dev, _attr_model);
-   dev_dbg(mdev->dev, "Media device unregistering\n");
-   media_devnode_unregister(>devnode);
+   dev_dbg(mdev->dev, "Media device unregistered\n");
+
+   /* Check if mdev devnode was registered */
+   if (media_devnode_is_registered(mdev->devnode)) {
+   device_remove_file(>devnode->dev, _attr_model);
+   media_devnode_unregister(mdev->devnode);
+   }
 }