| Message ID | 1461726512-9828-1-git-send-email-shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shuah, Good work! I have a few notes below. Em Tue, 26 Apr 2016 21:08:32 -0600 Shuah Khan <shuahkh@osg.samsung.com> escreveu: > When driver unbind is run while media_ioctl is in progress, media_ioctl() > fails with use-after-free. This first use-after-free is followed by more > user-after-free errors in media_release(), kobject_put(), and cdev_put() > as driver unbind continues. This problem is found on uvcvideo, em28xx, and > au0828 drivers and fix has been tested on all three. > > This fix allocates media devnode and manages its lifetime separate from the > struct media_device. Adds kobject to the media_devnode structure and this > kobject is set as the cdev parent kobject. This allows cdev_add() to hold > a reference to it and release the reference in cdev_del() ensuring that the > media_devnode is not deallocated as long as the application has the cdev > open. > > The first error is below: > > [ 472.424302] ================================================================== > [ 472.424333] BUG: KASAN: use-after-free in media_ioctl+0xf0/0x130 [media] at addr ffff880027b72330 > [ 472.424341] Read of size 8 by task media_device_te/1794 > [ 472.424348] ============================================================================= > [ 472.424356] BUG kmalloc-4096 (Not tainted): kasan: bad access detected > [ 472.424361] ----------------------------------------------------------------------------- > [ 472.431973] CPU: 1 PID: 1794 Comm: media_device_te Tainted: G B 4.6.0-rc5 #2 > [ 472.431988] Hardware name: Hewlett-Packard HP ProBook 6475b/180F, BIOS 68TTU Ver. F.04 08/03/2012 > [ 472.431996] ffffea00009edc00 ffff88009ddffc78 ffffffff81aecac3 ffff8801fa403200 > [ 472.432016] ffff880027b72260 ffff88009ddffca8 ffffffff815359b2 ffff8801fa403200 > [ 472.432040] ffffea00009edc00 ffff880027b72260 ffffffffa0c9cc60 ffff88009ddffcd0 > [ 472.432059] Call Trace: > [ 472.432079] [<ffffffff81aecac3>] dump_stack+0x67/0x94 > [ 472.432092] [<ffffffff815359b2>] print_trailer+0x112/0x1a0 > [ 472.432108] [<ffffffff8153b5e4>] object_err+0x34/0x40 > [ 472.432125] [<ffffffff8153d9d4>] kasan_report_error+0x224/0x530 > [ 472.432148] [<ffffffffa0c934c0>] ? __media_device_get_topology+0x1850/0x1850 [media] > [ 472.432167] [<ffffffff8153de13>] __asan_report_load8_noabort+0x43/0x50 > [ 472.432190] [<ffffffffa0c94f60>] ? media_ioctl+0x120/0x130 [media] > [ 472.432209] [<ffffffffa0c94f60>] media_ioctl+0x120/0x130 [media] > [ 472.432229] [<ffffffff815a9e44>] do_vfs_ioctl+0x184/0xe80 > [ 472.432243] [<ffffffff815a9cc0>] ? ioctl_preallocate+0x1a0/0x1a0 > [ 472.432256] [<ffffffff8127b1f0>] ? __hrtimer_init+0x170/0x170 > [ 472.432272] [<ffffffff82846b01>] ? do_nanosleep+0x161/0x480 > [ 472.432298] [<ffffffff811451d0>] ? sigprocmask+0x290/0x290 > [ 472.432323] [<ffffffff815c7209>] ? __fget_light+0x139/0x200 > [ 472.432358] [<ffffffff815aabb9>] SyS_ioctl+0x79/0x90 > [ 472.432381] [<ffffffff82848aa5>] entry_SYSCALL_64_fastpath+0x18/0xa8 This patch is almost identical to my patch, as you took the same approach as I did: https://patchwork.linuxtv.org/patch/33577/ So, I compared both to identify the differences. What I noticed is that: 1) your patch is setting cdev->parent; in my case, I fixed on a separate patch. IMHO, this should be on a separate patch, as cdev is a separate bug. 2) On my patch, I also fixed the error conditions at __media_device_register(): currently, it has a few issues, and, after the patch, if an error occurs, mdev->devnode should be set to NULL; 3) you added a kref. I would prefer to see this on a separate patch too, as this is not related with moving from an embedded struct to a dynamically allocated one. One logical change per patch, please. There's also the point that you're using my patch, but removing my credits. In this specific case, as a developer, I don't mind much about that, as it is just one patch and didn't take much of my time to produce. Yet, maintainers should not allow stripping off credits, as this could cause troubles later. So, IMHO, the best would be to split this patch in 3 patches: - my patch (fixing the context changes); - cdev patch; - kref patch. As a bonus side, by breaking into that, it helps to identify what fixes are needed if we found similar issues at the other parts of the subsystems. If I remember well, I ended by having some cdev troubles with the V4L2 core on one of my stress test. So, this is something that we want to double check at RC, DVB and V4L parts that handle cdev, and eventually porting the changes to the core of those subsystems. PS.: I did just a code review. I intend to test this along the week. Regards, Mauro > > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/media/media-device.c | 32 ++++++++++++++++++++------------ > drivers/media/media-devnode.c | 23 +++++++++++++++++++++++ > drivers/media/usb/au0828/au0828-core.c | 4 ++-- > drivers/media/usb/uvc/uvc_driver.c | 2 +- > include/media/media-device.h | 7 ++----- > include/media/media-devnode.h | 8 +++++++- > 6 files changed, 55 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > index 6e43c95..78b0350 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -428,7 +428,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; > > switch (cmd) { > @@ -504,7 +504,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) { > @@ -546,7 +546,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); > } > @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct media_device *mdev, > { > int ret; > > + mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL); > + if (!mdev->devnode) > + return -ENOMEM; > + > /* Register the device node. */ > - mdev->devnode.fops = &media_device_fops; > - mdev->devnode.parent = mdev->dev; > - mdev->devnode.release = media_device_release; > + mdev->devnode->fops = &media_device_fops; > + mdev->devnode->parent = mdev->dev; > + mdev->devnode->media_dev = mdev; > + mdev->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(&mdev->devnode, owner); > + ret = media_devnode_register(mdev->devnode, owner); > if (ret < 0) > return ret; > > - ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); > + ret = device_create_file(&mdev->devnode->dev, &dev_attr_model); > if (ret < 0) { > - media_devnode_unregister(&mdev->devnode); > + media_devnode_unregister(mdev->devnode); > return ret; > } > > @@ -790,7 +796,7 @@ void media_device_unregister(struct media_device *mdev) > spin_lock(&mdev->lock); > > /* Check if mdev was ever registered at all */ > - if (!media_devnode_is_registered(&mdev->devnode)) { > + if (!media_devnode_is_registered(mdev->devnode)) { > spin_unlock(&mdev->lock); > return; > } > @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev) > > spin_unlock(&mdev->lock); > > - device_remove_file(&mdev->devnode.dev, &dev_attr_model); > - media_devnode_unregister(&mdev->devnode); > + device_remove_file(&mdev->devnode->dev, &dev_attr_model); > + media_devnode_unregister(mdev->devnode); > + /* kfree devnode is done via kobject_put() handler */ > + mdev->devnode = NULL; > > dev_dbg(mdev->dev, "Media device unregistered\n"); > } > diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c > index 29409f4..9af9ba1 100644 > --- a/drivers/media/media-devnode.c > +++ b/drivers/media/media-devnode.c > @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) > mutex_unlock(&media_devnode_lock); > return -ENXIO; > } > + > + kobject_get(&mdev->kobj); > + > /* and increase the device refcount */ > get_device(&mdev->dev); > mutex_unlock(&media_devnode_lock); > @@ -181,6 +184,7 @@ static int media_open(struct inode *inode, struct file *filp) > ret = mdev->fops->open(filp); > if (ret) { > put_device(&mdev->dev); > + kobject_put(&mdev->kobj); > filp->private_data = NULL; > return ret; > } > @@ -200,6 +204,7 @@ static int media_release(struct inode *inode, struct file *filp) > /* decrease the refcount unconditionally since the release() > return value is ignored. */ > put_device(&mdev->dev); > + kobject_put(&mdev->kobj); > filp->private_data = NULL; > return 0; > } > @@ -218,6 +223,19 @@ static const struct file_operations media_devnode_fops = { > .llseek = no_llseek, > }; > > +static void media_devnode_free(struct kobject *kobj) > +{ > + struct media_devnode *devnode = > + container_of(kobj, struct media_devnode, kobj); > + > + kfree(devnode); > + pr_info("%s: Media Devnode Deallocated\n", __func__); > +} > + > +static struct kobj_type media_devnode_ktype = { > + .release = media_devnode_free, > +}; > + > int __must_check media_devnode_register(struct media_devnode *mdev, > struct module *owner) > { > @@ -238,9 +256,12 @@ int __must_check media_devnode_register(struct media_devnode *mdev, > > mdev->minor = minor; > > + kobject_init(&mdev->kobj, &media_devnode_ktype); > + > /* Part 2: Initialize and register the character device */ > cdev_init(&mdev->cdev, &media_devnode_fops); > mdev->cdev.owner = owner; > + mdev->cdev.kobj.parent = &mdev->kobj; > > ret = cdev_add(&mdev->cdev, MKDEV(MAJOR(media_dev_t), mdev->minor), 1); > if (ret < 0) { > @@ -269,6 +290,7 @@ int __must_check media_devnode_register(struct media_devnode *mdev, > error: > cdev_del(&mdev->cdev); > clear_bit(mdev->minor, media_devnode_nums); > + kobject_put(&mdev->kobj); > return ret; > } > > @@ -282,6 +304,7 @@ void media_devnode_unregister(struct media_devnode *mdev) > clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); > mutex_unlock(&media_devnode_lock); > device_unregister(&mdev->dev); > + kobject_put(&mdev->kobj); > } > > /* > diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c > index cc22b32..8af9344 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -136,7 +136,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) > > #ifdef CONFIG_MEDIA_CONTROLLER > if (dev->media_dev && > - media_devnode_is_registered(&dev->media_dev->devnode)) { > + media_devnode_is_registered(dev->media_dev->devnode)) { > /* clear enable_source, disable_source */ > dev->media_dev->source_priv = NULL; > dev->media_dev->enable_source = NULL; > @@ -468,7 +468,7 @@ static int au0828_media_device_register(struct au0828_dev *dev, > if (!dev->media_dev) > return 0; > > - if (!media_devnode_is_registered(&dev->media_dev->devnode)) { > + if (!media_devnode_is_registered(dev->media_dev->devnode)) { > > /* register media device */ > ret = media_device_register(dev->media_dev); > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 451e84e9..302e284 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1674,7 +1674,7 @@ static void uvc_delete(struct uvc_device *dev) > if (dev->vdev.dev) > v4l2_device_unregister(&dev->vdev); > #ifdef CONFIG_MEDIA_CONTROLLER > - if (media_devnode_is_registered(&dev->mdev.devnode)) > + if (media_devnode_is_registered(dev->mdev.devnode)) > media_device_unregister(&dev->mdev); > media_device_cleanup(&dev->mdev); > #endif > diff --git a/include/media/media-device.h b/include/media/media-device.h > index df74cfa..65394f3 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -283,7 +283,7 @@ struct media_entity_notify { > /** > * struct media_device - Media device > * @dev: Parent device > - * @devnode: Media device node > + * @devnode: Media device node pointer > * @driver_name: Optional device driver name. If not set, calls to > * %MEDIA_IOC_DEVICE_INFO will return dev->driver->name. > * This is needed for USB drivers for example, as otherwise > @@ -348,7 +348,7 @@ struct media_entity_notify { > struct media_device { > /* dev->driver_data points to this struct. */ > struct device *dev; > - struct media_devnode devnode; > + struct media_devnode *devnode; > > char model[32]; > char driver_name[32]; > @@ -396,9 +396,6 @@ struct usb_device; > #define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0 > #define MEDIA_DEV_NOTIFY_POST_LINK_CH 1 > > -/* media_devnode to media_device */ > -#define to_media_device(node) container_of(node, struct media_device, devnode) > - > /** > * media_entity_enum_init - Initialise an entity enumeration > * > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > index fe42f08..ba4bdaa 100644 > --- a/include/media/media-devnode.h > +++ b/include/media/media-devnode.h > @@ -70,7 +70,9 @@ struct media_file_operations { > * @fops: pointer to struct &media_file_operations with media device ops > * @dev: struct device pointer for the media controller device > * @cdev: struct cdev pointer character device > + * @kobj: struct kobject > * @parent: parent device > + * @media_dev: media device > * @minor: device node minor number > * @flags: flags, combination of the MEDIA_FLAG_* constants > * @release: release callback called at the end of media_devnode_release() > @@ -87,7 +89,9 @@ struct media_devnode { > /* sysfs */ > struct device dev; /* media device */ > struct cdev cdev; /* character device */ > + struct kobject kobj; /* set as cdev parent kobj */ > struct device *parent; /* device parent */ > + struct media_device *media_dev; /* media device for the devnode */ > > /* device info */ > int minor; > @@ -149,7 +153,9 @@ static inline struct media_devnode *media_devnode_data(struct file *filp) > */ > static inline int media_devnode_is_registered(struct media_devnode *mdev) > { > - return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); > + if (mdev) > + return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); > + return false; > } > > #endif /* _MEDIA_DEVNODE_H */
Hi Mauro, On 04/27/2016 03:55 AM, Mauro Carvalho Chehab wrote: > Hi Shuah, > > Good work! I have a few notes below. > > Em Tue, 26 Apr 2016 21:08:32 -0600 > Shuah Khan <shuahkh@osg.samsung.com> escreveu: > >> When driver unbind is run while media_ioctl is in progress, media_ioctl() >> fails with use-after-free. This first use-after-free is followed by more >> user-after-free errors in media_release(), kobject_put(), and cdev_put() >> as driver unbind continues. This problem is found on uvcvideo, em28xx, and >> au0828 drivers and fix has been tested on all three. >> >> This fix allocates media devnode and manages its lifetime separate from the >> struct media_device. Adds kobject to the media_devnode structure and this >> kobject is set as the cdev parent kobject. This allows cdev_add() to hold >> a reference to it and release the reference in cdev_del() ensuring that the >> media_devnode is not deallocated as long as the application has the cdev >> open. >> >> The first error is below: >> >> [ 472.424302] ================================================================== >> [ 472.424333] BUG: KASAN: use-after-free in media_ioctl+0xf0/0x130 [media] at addr ffff880027b72330 >> [ 472.424341] Read of size 8 by task media_device_te/1794 >> [ 472.424348] ============================================================================= >> [ 472.424356] BUG kmalloc-4096 (Not tainted): kasan: bad access detected >> [ 472.424361] ----------------------------------------------------------------------------- >> [ 472.431973] CPU: 1 PID: 1794 Comm: media_device_te Tainted: G B 4.6.0-rc5 #2 >> [ 472.431988] Hardware name: Hewlett-Packard HP ProBook 6475b/180F, BIOS 68TTU Ver. F.04 08/03/2012 >> [ 472.431996] ffffea00009edc00 ffff88009ddffc78 ffffffff81aecac3 ffff8801fa403200 >> [ 472.432016] ffff880027b72260 ffff88009ddffca8 ffffffff815359b2 ffff8801fa403200 >> [ 472.432040] ffffea00009edc00 ffff880027b72260 ffffffffa0c9cc60 ffff88009ddffcd0 >> [ 472.432059] Call Trace: >> [ 472.432079] [<ffffffff81aecac3>] dump_stack+0x67/0x94 >> [ 472.432092] [<ffffffff815359b2>] print_trailer+0x112/0x1a0 >> [ 472.432108] [<ffffffff8153b5e4>] object_err+0x34/0x40 >> [ 472.432125] [<ffffffff8153d9d4>] kasan_report_error+0x224/0x530 >> [ 472.432148] [<ffffffffa0c934c0>] ? __media_device_get_topology+0x1850/0x1850 [media] >> [ 472.432167] [<ffffffff8153de13>] __asan_report_load8_noabort+0x43/0x50 >> [ 472.432190] [<ffffffffa0c94f60>] ? media_ioctl+0x120/0x130 [media] >> [ 472.432209] [<ffffffffa0c94f60>] media_ioctl+0x120/0x130 [media] >> [ 472.432229] [<ffffffff815a9e44>] do_vfs_ioctl+0x184/0xe80 >> [ 472.432243] [<ffffffff815a9cc0>] ? ioctl_preallocate+0x1a0/0x1a0 >> [ 472.432256] [<ffffffff8127b1f0>] ? __hrtimer_init+0x170/0x170 >> [ 472.432272] [<ffffffff82846b01>] ? do_nanosleep+0x161/0x480 >> [ 472.432298] [<ffffffff811451d0>] ? sigprocmask+0x290/0x290 >> [ 472.432323] [<ffffffff815c7209>] ? __fget_light+0x139/0x200 >> [ 472.432358] [<ffffffff815aabb9>] SyS_ioctl+0x79/0x90 >> [ 472.432381] [<ffffffff82848aa5>] entry_SYSCALL_64_fastpath+0x18/0xa8 > > This patch is almost identical to my patch, as you took the same > approach as I did: > https://patchwork.linuxtv.org/patch/33577/ > > So, I compared both to identify the differences. What I noticed is > that: > > 1) your patch is setting cdev->parent; in my case, I fixed on a > separate patch. > > IMHO, this should be on a separate patch, as cdev is a separate bug. > > 2) On my patch, I also fixed the error conditions at > __media_device_register(): currently, it has a few issues, and, > after the patch, if an error occurs, mdev->devnode should be > set to NULL; > > 3) you added a kref. I would prefer to see this on a separate patch > too, as this is not related with moving from an embedded struct > to a dynamically allocated one. One logical change per patch, > please. > > There's also the point that you're using my patch, but removing > my credits. In this specific case, as a developer, I don't mind > much about that, as it is just one patch and didn't take much > of my time to produce. Yet, maintainers should not allow stripping > off credits, as this could cause troubles later. I don't do things like using another developer's work and strip credits. I didn't user your patch. I started from scratch to solve the problem. There is a good reason for that. For one thing, your patch came out when we were dealing with lots of problems with having au0828 and snd-us-audio in the mix. I also wanted to start from a clean slate and not use any code that was done while we were debugging two driver problems. As such, there a flood of patches from you at that time, and I didn't know which ones are applicable for a single driver case, and which ones aren't. So I just went the route of clean slate. That said, I am fine with you want to not take my patch and use yours. > > So, IMHO, the best would be to split this patch in 3 patches: > > - my patch (fixing the context changes); I will leave it up to you to fix the context changes if any. I can apply you patch as is and then add the cdev and kref patch. > - cdev patch; > - kref patch. > > As a bonus side, by breaking into that, it helps to identify what > fixes are needed if we found similar issues at the other parts of > the subsystems. No problem breaking the it into 3 patches. I think the order should be kref and the a patch to set cdev kobj parent. Is that what you had in mind? > > If I remember well, I ended by having some cdev troubles with the > V4L2 core on one of my stress test. So, this is something that > we want to double check at RC, DVB and V4L parts that handle > cdev, and eventually porting the changes to the core of those > subsystems. Is that when you were playing with allocating cdev as opposed to setting parent. btw. just setting parent isn't enough. Kobject is necessary as it can then invoke the kobject put handler from cdev-core. > > PS.: I did just a code review. I intend to test this along the > week. Please let me know if your patch is in good shape for me to use it. thanks, -- Shuah > > >> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >> --- >> drivers/media/media-device.c | 32 ++++++++++++++++++++------------ >> drivers/media/media-devnode.c | 23 +++++++++++++++++++++++ >> drivers/media/usb/au0828/au0828-core.c | 4 ++-- >> drivers/media/usb/uvc/uvc_driver.c | 2 +- >> include/media/media-device.h | 7 ++----- >> include/media/media-devnode.h | 8 +++++++- >> 6 files changed, 55 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c >> index 6e43c95..78b0350 100644 >> --- a/drivers/media/media-device.c >> +++ b/drivers/media/media-device.c >> @@ -428,7 +428,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; >> >> switch (cmd) { >> @@ -504,7 +504,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) { >> @@ -546,7 +546,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); >> } >> @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct media_device *mdev, >> { >> int ret; >> >> + mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL); >> + if (!mdev->devnode) >> + return -ENOMEM; >> + >> /* Register the device node. */ >> - mdev->devnode.fops = &media_device_fops; >> - mdev->devnode.parent = mdev->dev; >> - mdev->devnode.release = media_device_release; >> + mdev->devnode->fops = &media_device_fops; >> + mdev->devnode->parent = mdev->dev; >> + mdev->devnode->media_dev = mdev; >> + mdev->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(&mdev->devnode, owner); >> + ret = media_devnode_register(mdev->devnode, owner); >> if (ret < 0) >> return ret; >> >> - ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); >> + ret = device_create_file(&mdev->devnode->dev, &dev_attr_model); >> if (ret < 0) { >> - media_devnode_unregister(&mdev->devnode); >> + media_devnode_unregister(mdev->devnode); >> return ret; >> } >> >> @@ -790,7 +796,7 @@ void media_device_unregister(struct media_device *mdev) >> spin_lock(&mdev->lock); >> >> /* Check if mdev was ever registered at all */ >> - if (!media_devnode_is_registered(&mdev->devnode)) { >> + if (!media_devnode_is_registered(mdev->devnode)) { >> spin_unlock(&mdev->lock); >> return; >> } >> @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev) >> >> spin_unlock(&mdev->lock); >> >> - device_remove_file(&mdev->devnode.dev, &dev_attr_model); >> - media_devnode_unregister(&mdev->devnode); >> + device_remove_file(&mdev->devnode->dev, &dev_attr_model); >> + media_devnode_unregister(mdev->devnode); >> + /* kfree devnode is done via kobject_put() handler */ >> + mdev->devnode = NULL; >> >> dev_dbg(mdev->dev, "Media device unregistered\n"); >> } >> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c >> index 29409f4..9af9ba1 100644 >> --- a/drivers/media/media-devnode.c >> +++ b/drivers/media/media-devnode.c >> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) >> mutex_unlock(&media_devnode_lock); >> return -ENXIO; >> } >> + >> + kobject_get(&mdev->kobj); >> + >> /* and increase the device refcount */ >> get_device(&mdev->dev); >> mutex_unlock(&media_devnode_lock); >> @@ -181,6 +184,7 @@ static int media_open(struct inode *inode, struct file *filp) >> ret = mdev->fops->open(filp); >> if (ret) { >> put_device(&mdev->dev); >> + kobject_put(&mdev->kobj); >> filp->private_data = NULL; >> return ret; >> } >> @@ -200,6 +204,7 @@ static int media_release(struct inode *inode, struct file *filp) >> /* decrease the refcount unconditionally since the release() >> return value is ignored. */ >> put_device(&mdev->dev); >> + kobject_put(&mdev->kobj); >> filp->private_data = NULL; >> return 0; >> } >> @@ -218,6 +223,19 @@ static const struct file_operations media_devnode_fops = { >> .llseek = no_llseek, >> }; >> >> +static void media_devnode_free(struct kobject *kobj) >> +{ >> + struct media_devnode *devnode = >> + container_of(kobj, struct media_devnode, kobj); >> + >> + kfree(devnode); >> + pr_info("%s: Media Devnode Deallocated\n", __func__); >> +} >> + >> +static struct kobj_type media_devnode_ktype = { >> + .release = media_devnode_free, >> +}; >> + >> int __must_check media_devnode_register(struct media_devnode *mdev, >> struct module *owner) >> { >> @@ -238,9 +256,12 @@ int __must_check media_devnode_register(struct media_devnode *mdev, >> >> mdev->minor = minor; >> >> + kobject_init(&mdev->kobj, &media_devnode_ktype); >> + >> /* Part 2: Initialize and register the character device */ >> cdev_init(&mdev->cdev, &media_devnode_fops); >> mdev->cdev.owner = owner; >> + mdev->cdev.kobj.parent = &mdev->kobj; >> >> ret = cdev_add(&mdev->cdev, MKDEV(MAJOR(media_dev_t), mdev->minor), 1); >> if (ret < 0) { >> @@ -269,6 +290,7 @@ int __must_check media_devnode_register(struct media_devnode *mdev, >> error: >> cdev_del(&mdev->cdev); >> clear_bit(mdev->minor, media_devnode_nums); >> + kobject_put(&mdev->kobj); >> return ret; >> } >> >> @@ -282,6 +304,7 @@ void media_devnode_unregister(struct media_devnode *mdev) >> clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); >> mutex_unlock(&media_devnode_lock); >> device_unregister(&mdev->dev); >> + kobject_put(&mdev->kobj); >> } >> >> /* >> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c >> index cc22b32..8af9344 100644 >> --- a/drivers/media/usb/au0828/au0828-core.c >> +++ b/drivers/media/usb/au0828/au0828-core.c >> @@ -136,7 +136,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) >> >> #ifdef CONFIG_MEDIA_CONTROLLER >> if (dev->media_dev && >> - media_devnode_is_registered(&dev->media_dev->devnode)) { >> + media_devnode_is_registered(dev->media_dev->devnode)) { >> /* clear enable_source, disable_source */ >> dev->media_dev->source_priv = NULL; >> dev->media_dev->enable_source = NULL; >> @@ -468,7 +468,7 @@ static int au0828_media_device_register(struct au0828_dev *dev, >> if (!dev->media_dev) >> return 0; >> >> - if (!media_devnode_is_registered(&dev->media_dev->devnode)) { >> + if (!media_devnode_is_registered(dev->media_dev->devnode)) { >> >> /* register media device */ >> ret = media_device_register(dev->media_dev); >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c >> index 451e84e9..302e284 100644 >> --- a/drivers/media/usb/uvc/uvc_driver.c >> +++ b/drivers/media/usb/uvc/uvc_driver.c >> @@ -1674,7 +1674,7 @@ static void uvc_delete(struct uvc_device *dev) >> if (dev->vdev.dev) >> v4l2_device_unregister(&dev->vdev); >> #ifdef CONFIG_MEDIA_CONTROLLER >> - if (media_devnode_is_registered(&dev->mdev.devnode)) >> + if (media_devnode_is_registered(dev->mdev.devnode)) >> media_device_unregister(&dev->mdev); >> media_device_cleanup(&dev->mdev); >> #endif >> diff --git a/include/media/media-device.h b/include/media/media-device.h >> index df74cfa..65394f3 100644 >> --- a/include/media/media-device.h >> +++ b/include/media/media-device.h >> @@ -283,7 +283,7 @@ struct media_entity_notify { >> /** >> * struct media_device - Media device >> * @dev: Parent device >> - * @devnode: Media device node >> + * @devnode: Media device node pointer >> * @driver_name: Optional device driver name. If not set, calls to >> * %MEDIA_IOC_DEVICE_INFO will return dev->driver->name. >> * This is needed for USB drivers for example, as otherwise >> @@ -348,7 +348,7 @@ struct media_entity_notify { >> struct media_device { >> /* dev->driver_data points to this struct. */ >> struct device *dev; >> - struct media_devnode devnode; >> + struct media_devnode *devnode; >> >> char model[32]; >> char driver_name[32]; >> @@ -396,9 +396,6 @@ struct usb_device; >> #define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0 >> #define MEDIA_DEV_NOTIFY_POST_LINK_CH 1 >> >> -/* media_devnode to media_device */ >> -#define to_media_device(node) container_of(node, struct media_device, devnode) >> - >> /** >> * media_entity_enum_init - Initialise an entity enumeration >> * >> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h >> index fe42f08..ba4bdaa 100644 >> --- a/include/media/media-devnode.h >> +++ b/include/media/media-devnode.h >> @@ -70,7 +70,9 @@ struct media_file_operations { >> * @fops: pointer to struct &media_file_operations with media device ops >> * @dev: struct device pointer for the media controller device >> * @cdev: struct cdev pointer character device >> + * @kobj: struct kobject >> * @parent: parent device >> + * @media_dev: media device >> * @minor: device node minor number >> * @flags: flags, combination of the MEDIA_FLAG_* constants >> * @release: release callback called at the end of media_devnode_release() >> @@ -87,7 +89,9 @@ struct media_devnode { >> /* sysfs */ >> struct device dev; /* media device */ >> struct cdev cdev; /* character device */ >> + struct kobject kobj; /* set as cdev parent kobj */ >> struct device *parent; /* device parent */ >> + struct media_device *media_dev; /* media device for the devnode */ >> >> /* device info */ >> int minor; >> @@ -149,7 +153,9 @@ static inline struct media_devnode *media_devnode_data(struct file *filp) >> */ >> static inline int media_devnode_is_registered(struct media_devnode *mdev) >> { >> - return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); >> + if (mdev) >> + return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); >> + return false; >> } >> >> #endif /* _MEDIA_DEVNODE_H */ > > -- 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
Looks mostly good, a few comments. On 04/27/2016 05:08 AM, Shuah Khan wrote: [...] > @@ -428,7 +428,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); Can we keep the helper macro, means we don't need to touch this code. > + struct media_device *dev = devnode->media_dev; You need a lock to protect this from running concurrently with media_device_unregister() otherwise the struct might be freed while still in use. > long ret; > > switch (cmd) { [...] > @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct media_device *mdev, > { > int ret; > > + mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL); sizeof(*mdev->devnode) is preferred kernel style, > + if (!mdev->devnode) > + return -ENOMEM; > + > /* Register the device node. */ > - mdev->devnode.fops = &media_device_fops; > - mdev->devnode.parent = mdev->dev; > - mdev->devnode.release = media_device_release; > + mdev->devnode->fops = &media_device_fops; > + mdev->devnode->parent = mdev->dev; > + mdev->devnode->media_dev = mdev; > + mdev->devnode->release = media_device_release; This should no longer be necessary. Just drop the release callback altogether. > > /* Set version 0 to indicate user-space that the graph is static */ > mdev->topology_version = 0; > [...] > @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev) > > spin_unlock(&mdev->lock); > > - device_remove_file(&mdev->devnode.dev, &dev_attr_model); > - media_devnode_unregister(&mdev->devnode); > + device_remove_file(&mdev->devnode->dev, &dev_attr_model); > + media_devnode_unregister(mdev->devnode); > + /* kfree devnode is done via kobject_put() handler */ > + mdev->devnode = NULL; mdev->devnode->media_dev needs to be set to NULL. > > dev_dbg(mdev->dev, "Media device unregistered\n"); > } > diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c > index 29409f4..9af9ba1 100644 > --- a/drivers/media/media-devnode.c > +++ b/drivers/media/media-devnode.c > @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) > mutex_unlock(&media_devnode_lock); > return -ENXIO; > } > + > + kobject_get(&mdev->kobj); This is not necessary, and if it was it would be prone to race condition as the last reference could be dropped before this line. But assigning the cdev parent makes sure that we always have a reference to the object while the open() callback is running. > + > /* and increase the device refcount */ > get_device(&mdev->dev); > mutex_unlock(&media_devnode_lock); > /* [...] > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > index fe42f08..ba4bdaa 100644 > --- a/include/media/media-devnode.h > +++ b/include/media/media-devnode.h > @@ -70,7 +70,9 @@ struct media_file_operations { > * @fops: pointer to struct &media_file_operations with media device ops > * @dev: struct device pointer for the media controller device > * @cdev: struct cdev pointer character device > + * @kobj: struct kobject > * @parent: parent device > + * @media_dev: media device > * @minor: device node minor number > * @flags: flags, combination of the MEDIA_FLAG_* constants > * @release: release callback called at the end of media_devnode_release() > @@ -87,7 +89,9 @@ struct media_devnode { > /* sysfs */ > struct device dev; /* media device */ > struct cdev cdev; /* character device */ > + struct kobject kobj; /* set as cdev parent kobj */ You don't need a extra kobj. Just use the struct dev kobj. > struct device *parent; /* device parent */ > + struct media_device *media_dev; /* media device for the devnode */ > > /* device info */ > int minor; -- 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
On 04/27/2016 10:43 AM, Lars-Peter Clausen wrote: > Looks mostly good, a few comments. > > On 04/27/2016 05:08 AM, Shuah Khan wrote: > [...] >> @@ -428,7 +428,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); > > Can we keep the helper macro, means we don't need to touch this code. Yeah. I have been thinking about that as well. It avoids changes and abstracts it. > >> + struct media_device *dev = devnode->media_dev; > > You need a lock to protect this from running concurrently with > media_device_unregister() otherwise the struct might be freed while still in > use. > Right. This needs to be protected. >> long ret; >> >> switch (cmd) { > [...] >> @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct media_device *mdev, >> { >> int ret; >> >> + mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL); > > sizeof(*mdev->devnode) is preferred kernel style, Yeah. Force of habit, I keep forgetting it. > >> + if (!mdev->devnode) >> + return -ENOMEM; >> + >> /* Register the device node. */ >> - mdev->devnode.fops = &media_device_fops; >> - mdev->devnode.parent = mdev->dev; >> - mdev->devnode.release = media_device_release; >> + mdev->devnode->fops = &media_device_fops; >> + mdev->devnode->parent = mdev->dev; >> + mdev->devnode->media_dev = mdev; >> + mdev->devnode->release = media_device_release; > > This should no longer be necessary. Just drop the release callback altogether. It does nothing at the moment. I believe the intent is for this routine to invoke any driver hooks if any at media_device level. It gets called from media_devnode_release() which is the media_devnode->dev.release. I will look into if it can be removed. > >> >> /* Set version 0 to indicate user-space that the graph is static */ >> mdev->topology_version = 0; >> > [...] >> @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev) >> >> spin_unlock(&mdev->lock); >> >> - device_remove_file(&mdev->devnode.dev, &dev_attr_model); >> - media_devnode_unregister(&mdev->devnode); >> + device_remove_file(&mdev->devnode->dev, &dev_attr_model); >> + media_devnode_unregister(mdev->devnode); >> + /* kfree devnode is done via kobject_put() handler */ >> + mdev->devnode = NULL; > > mdev->devnode->media_dev needs to be set to NULL. Yes. Thanks for catching it. > >> >> dev_dbg(mdev->dev, "Media device unregistered\n"); >> } >> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c >> index 29409f4..9af9ba1 100644 >> --- a/drivers/media/media-devnode.c >> +++ b/drivers/media/media-devnode.c >> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) >> mutex_unlock(&media_devnode_lock); >> return -ENXIO; >> } >> + >> + kobject_get(&mdev->kobj); > > This is not necessary, and if it was it would be prone to race condition as > the last reference could be dropped before this line. But assigning the cdev > parent makes sure that we always have a reference to the object while the > open() callback is running. I don't see cdev parent kobj get in cdev_get() which does kobject_get() on cdev->kobj. Is that enough to get the reference? cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is the reason why I added a get here and put in media_release(). I can remove the get and put and test. Looks like I am not checking kobject_get() return value which isn't good? > >> + >> /* and increase the device refcount */ >> get_device(&mdev->dev); >> mutex_unlock(&media_devnode_lock); >> /* > [...] >> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h >> index fe42f08..ba4bdaa 100644 >> --- a/include/media/media-devnode.h >> +++ b/include/media/media-devnode.h >> @@ -70,7 +70,9 @@ struct media_file_operations { >> * @fops: pointer to struct &media_file_operations with media device ops >> * @dev: struct device pointer for the media controller device >> * @cdev: struct cdev pointer character device >> + * @kobj: struct kobject >> * @parent: parent device >> + * @media_dev: media device >> * @minor: device node minor number >> * @flags: flags, combination of the MEDIA_FLAG_* constants >> * @release: release callback called at the end of media_devnode_release() >> @@ -87,7 +89,9 @@ struct media_devnode { >> /* sysfs */ >> struct device dev; /* media device */ >> struct cdev cdev; /* character device */ >> + struct kobject kobj; /* set as cdev parent kobj */ > > You don't need a extra kobj. Just use the struct dev kobj. Yeah I can use that as long as I can override the default release function with media_devnode_free(). media_devnode should stick around until the last app closes /dev/mediaX even if the media_device is no longer registered. i.e media_ioctl should be able to check if devnode is registered or not. I think I am missing something and don't understand how struct dev kobj can be used. > >> struct device *parent; /* device parent */ >> + struct media_device *media_dev; /* media device for the devnode */ >> >> /* device info */ >> int minor; > > -- > 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 > -- 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
On 04/27/2016 11:56 PM, Shuah Khan wrote: >>> dev_dbg(mdev->dev, "Media device unregistered\n"); >>> } >>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c >>> index 29409f4..9af9ba1 100644 >>> --- a/drivers/media/media-devnode.c >>> +++ b/drivers/media/media-devnode.c >>> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) >>> mutex_unlock(&media_devnode_lock); >>> return -ENXIO; >>> } >>> + >>> + kobject_get(&mdev->kobj); >> >> This is not necessary, and if it was it would be prone to race condition as >> the last reference could be dropped before this line. But assigning the cdev >> parent makes sure that we always have a reference to the object while the >> open() callback is running. > > I don't see cdev parent kobj get in cdev_get() which does kobject_get() > on cdev->kobj. Is that enough to get the reference? > > cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is > the reason why I added a get here and put in media_release(). > The cdev takes the parent reference when created and only drops it once it is released. So as long as the cdev exists there is a reference to the parent. While cdev_del() puts one reference to the cdev there is also one reference for each open file. So as long as there is a open file there is a reference to the parent as well. > I can remove the get and put and test. Looks like I am not checking > kobject_get() return value which isn't good? kobject_get() can't fail. > >> >>> + >>> /* and increase the device refcount */ >>> get_device(&mdev->dev); >>> mutex_unlock(&media_devnode_lock); >>> /* >> [...] >>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h >>> index fe42f08..ba4bdaa 100644 >>> --- a/include/media/media-devnode.h >>> +++ b/include/media/media-devnode.h >>> @@ -70,7 +70,9 @@ struct media_file_operations { >>> * @fops: pointer to struct &media_file_operations with media device ops >>> * @dev: struct device pointer for the media controller device >>> * @cdev: struct cdev pointer character device >>> + * @kobj: struct kobject >>> * @parent: parent device >>> + * @media_dev: media device >>> * @minor: device node minor number >>> * @flags: flags, combination of the MEDIA_FLAG_* constants >>> * @release: release callback called at the end of media_devnode_release() >>> @@ -87,7 +89,9 @@ struct media_devnode { >>> /* sysfs */ >>> struct device dev; /* media device */ >>> struct cdev cdev; /* character device */ >>> + struct kobject kobj; /* set as cdev parent kobj */ >> >> You don't need a extra kobj. Just use the struct dev kobj. > > Yeah I can use that as long as I can override the default release > function with media_devnode_free(). media_devnode should stick around > until the last app closes /dev/mediaX even if the media_device is no > longer registered. i.e media_ioctl should be able to check if devnode > is registered or not. I think I am missing something and don't understand > how struct dev kobj can be used. The struct dev that is embedded into th media_devnode as the same live time as the media_devnode itself. In addition to that struct device is a reference counted object. This means a structure that embeds struct device must not be freed until the last reference is dropped. What you do here is introduce a independent reference counting mechanism for the same structure. Which means if there is a reference to struct device, but not to the new kobj you end up with a use-after-free again. The solution is to only use one reference counting mechanism (the struct device) and intialize the cdev kobj parent to the device kobj and whenever you did kobj_{get,put}() replace that with {get,put}_device(). And in the device release callback free the struct media_devnode. -- 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
Em Wed, 27 Apr 2016 15:56:33 -0600 Shuah Khan <shuahkh@osg.samsung.com> escreveu: > On 04/27/2016 10:43 AM, Lars-Peter Clausen wrote: > > Looks mostly good, a few comments. > > > > On 04/27/2016 05:08 AM, Shuah Khan wrote: > > [...] > >> @@ -428,7 +428,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); > > > > Can we keep the helper macro, means we don't need to touch this code. > > Yeah. I have been thinking about that as well. It avoids changes > and abstracts it. I don't like the idea of keeping the macro. It is used only on two cases: 1) inside the core; 2) on two drivers that check if the media device is registered. On the first case, if something changes, we want to be aware about that. On the second case, IMHO, the best would be to have a macro that would take struct media_device as argument, keeping media_devnode hidden from drivers. > > > > >> + struct media_device *dev = devnode->media_dev; > > > > You need a lock to protect this from running concurrently with > > media_device_unregister() otherwise the struct might be freed while still in > > use. > > > > Right. This needs to be protected. > > >> long ret; > >> > >> switch (cmd) { > > [...] > >> @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct media_device *mdev, > >> { > >> int ret; > >> > >> + mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL); > > > > sizeof(*mdev->devnode) is preferred kernel style, > > Yeah. Force of habit, I keep forgetting it. > > > > >> + if (!mdev->devnode) > >> + return -ENOMEM; > >> + > >> /* Register the device node. */ > >> - mdev->devnode.fops = &media_device_fops; > >> - mdev->devnode.parent = mdev->dev; > >> - mdev->devnode.release = media_device_release; > >> + mdev->devnode->fops = &media_device_fops; > >> + mdev->devnode->parent = mdev->dev; > >> + mdev->devnode->media_dev = mdev; > >> + mdev->devnode->release = media_device_release; > > > > This should no longer be necessary. Just drop the release callback altogether. > > It does nothing at the moment. I believe the intent is for this routine > to invoke any driver hooks if any at media_device level. It gets called > from media_devnode_release() which is the media_devnode->dev.release. > I will look into if it can be removed. Right now, media_device_release callback is not used, except to print that the device got removed, if debug enabled. Yet, this is a separate change. Better to send such as a separate patch. > > > > >> > >> /* Set version 0 to indicate user-space that the graph is static */ > >> mdev->topology_version = 0; > >> > > [...] > >> @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev) > >> > >> spin_unlock(&mdev->lock); > >> > >> - device_remove_file(&mdev->devnode.dev, &dev_attr_model); > >> - media_devnode_unregister(&mdev->devnode); > >> + device_remove_file(&mdev->devnode->dev, &dev_attr_model); > >> + media_devnode_unregister(mdev->devnode); > >> + /* kfree devnode is done via kobject_put() handler */ > >> + mdev->devnode = NULL; > > > > mdev->devnode->media_dev needs to be set to NULL. > > Yes. Thanks for catching it. > > > > >> > >> dev_dbg(mdev->dev, "Media device unregistered\n"); > >> } > >> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c > >> index 29409f4..9af9ba1 100644 > >> --- a/drivers/media/media-devnode.c > >> +++ b/drivers/media/media-devnode.c > >> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) > >> mutex_unlock(&media_devnode_lock); > >> return -ENXIO; > >> } > >> + > >> + kobject_get(&mdev->kobj); > > > > This is not necessary, and if it was it would be prone to race condition as > > the last reference could be dropped before this line. But assigning the cdev > > parent makes sure that we always have a reference to the object while the > > open() callback is running. > > I don't see cdev parent kobj get in cdev_get() which does kobject_get() > on cdev->kobj. Is that enough to get the reference? > > cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is > the reason why I added a get here and put in media_release(). > > I can remove the get and put and test. Looks like I am not checking > kobject_get() return value which isn't good? > > > > >> + > >> /* and increase the device refcount */ > >> get_device(&mdev->dev); > >> mutex_unlock(&media_devnode_lock); > >> /* > > [...] > >> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > >> index fe42f08..ba4bdaa 100644 > >> --- a/include/media/media-devnode.h > >> +++ b/include/media/media-devnode.h > >> @@ -70,7 +70,9 @@ struct media_file_operations { > >> * @fops: pointer to struct &media_file_operations with media device ops > >> * @dev: struct device pointer for the media controller device > >> * @cdev: struct cdev pointer character device > >> + * @kobj: struct kobject > >> * @parent: parent device > >> + * @media_dev: media device > >> * @minor: device node minor number > >> * @flags: flags, combination of the MEDIA_FLAG_* constants > >> * @release: release callback called at the end of media_devnode_release() > >> @@ -87,7 +89,9 @@ struct media_devnode { > >> /* sysfs */ > >> struct device dev; /* media device */ > >> struct cdev cdev; /* character device */ > >> + struct kobject kobj; /* set as cdev parent kobj */ > > > > You don't need a extra kobj. Just use the struct dev kobj. > > Yeah I can use that as long as I can override the default release > function with media_devnode_free(). media_devnode should stick around > until the last app closes /dev/mediaX even if the media_device is no > longer registered. i.e media_ioctl should be able to check if devnode > is registered or not. I think I am missing something and don't understand > how struct dev kobj can be used. > > > > >> struct device *parent; /* device parent */ > >> + struct media_device *media_dev; /* media device for the devnode */ > >> > >> /* device info */ > >> int minor; > > > > -- > > 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 > > >
Em Wed, 27 Apr 2016 07:51:08 -0600 Shuah Khan <shuahkh@osg.samsung.com> escreveu: > > - cdev patch; > > - kref patch. > > > > As a bonus side, by breaking into that, it helps to identify what > > fixes are needed if we found similar issues at the other parts of > > the subsystems. > > No problem breaking the it into 3 patches. I think the order should > be kref and the a patch to set cdev kobj parent. Is that what you > had in mind? Works for me. > > > > > If I remember well, I ended by having some cdev troubles with the > > V4L2 core on one of my stress test. So, this is something that > > we want to double check at RC, DVB and V4L parts that handle > > cdev, and eventually porting the changes to the core of those > > subsystems. > > Is that when you were playing with allocating cdev as opposed to > setting parent. btw. just setting parent isn't enough. Kobject > is necessary as it can then invoke the kobject put handler from > cdev-core. V4L2 has kref, as far as I remember, but not sure if it is doing the right thing. As I pointed on the previous e-mail, I'm getting some cdev issues that seem to be related to V4L2, like this: [ 1002.856150] general protection fault: 0000 [#5] SMP KASAN [ 1002.859995] Modules linked in: ir_lirc_codec lirc_dev au8522_dig xc5000 tuner au8522_decoder au8522_common au0828 videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core tveeprom dvb_core rc_core v4l2_common videodev media cpufreq_powersave cpufreq_conservative cpufreq_userspace cpufreq_stats parport_pc ppdev lp parport snd_hda_codec_hdmi intel_rapl x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm iTCO_wdt iTCO_vendor_support irqbypass crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i915 snd_hda_codec_realtek snd_hda_codec_generic sha256_ssse3 sha256_generic hmac drbg btusb btrtl btbcm aesni_intel evdev snd_hda_intel aes_x86_64 lrw btintel i2c_algo_bit snd_hda_codec gf128mul bluetooth glue_helper drm_kms_helper snd_hwdep ablk_helper cryptd snd_hda_core drm serio_raw [ 1002.870406] rfkill sg snd_pcm pcspkr snd_timer mei_me snd mei lpc_ich i2c_i801 mfd_core soundcore battery dw_dmac video dw_dmac_core i2c_designware_platform i2c_designware_core acpi_pad button tpm_tis tpm ext4 crc16 jbd2 mbcache dm_mod sd_mod ahci libahci psmouse libata e1000e ehci_pci xhci_pci ptp scsi_mod ehci_hcd pps_core xhci_hcd fan thermal sdhci_acpi sdhci mmc_core i2c_hid hid [ 1002.874971] CPU: 2 PID: 3640 Comm: v4l_id Tainted: G D 4.6.0-rc2+ #68 [ 1002.877246] Hardware name: /NUC5i7RYB, BIOS RYBDWi35.86A.0350.2015.0812.1722 08/12/2015 [ 1002.879650] task: ffff88009c273000 ti: ffff880035c70000 task.ti: ffff880035c70000 [ 1002.882891] RIP: 0010:[<ffffffff81d77a61>] [<ffffffff81d77a61>] usb_ifnum_to_if+0x31/0x270 [ 1002.886337] RSP: 0018:ffff880035c778d0 EFLAGS: 00010282 [ 1002.889950] RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 1ffff1001364fc5e [ 1002.892009] RDX: 000000000000009e RSI: 0000000000000000 RDI: 00000000000004f0 [ 1002.893996] RBP: ffff880035c77908 R08: 0000000000000001 R09: 0000000000000001 [ 1002.895895] R10: ffff8803a239abf0 R11: 0000000000000000 R12: ffffffffa12be0c0 [ 1002.898465] R13: ffff88009b27eb3c R14: ffff88009b27c080 R15: ffffffffa12a89b0 [ 1002.902048] FS: 00007f0312a7e700(0000) GS:ffff8803c6900000(0000) knlGS:0000000000000000 [ 1002.905584] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1002.908676] CR2: 0000000001c84db0 CR3: 00000003a1cd8000 CR4: 00000000003406e0 [ 1002.910528] Stack: [ 1002.912378] ffff8803bf048000 ffff88009b27e2f0 ffff88009b27c000 ffffffffa12be0c0 [ 1002.915578] ffff88009b27eb3c ffff88009b27c080 ffffffffa12a89b0 ffff880035c77940 [ 1002.919242] ffffffffa12a4a45 ffff880035c77940 ffff88009b27c000 0000000000000000 [ 1002.921590] Call Trace: [ 1002.923451] [<ffffffffa12a89b0>] ? au0828_v4l2_close+0x5a0/0x5a0 [au0828] [ 1002.925339] [<ffffffffa12a4a45>] au0828_analog_stream_enable+0x85/0x330 [au0828] [ 1002.927234] [<ffffffffa12a8b11>] au0828_v4l2_open+0x161/0x350 [au0828] [ 1002.930247] [<ffffffffa12a89b0>] ? au0828_v4l2_close+0x5a0/0x5a0 [au0828] [ 1002.932695] [<ffffffffa1169561>] v4l2_open+0x1d1/0x350 [videodev] [ 1002.934556] [<ffffffff815cc071>] chrdev_open+0x1f1/0x580 [ 1002.936446] [<ffffffff815cbe80>] ? cdev_put+0x50/0x50 [ 1002.938301] [<ffffffff815b98a7>] do_dentry_open+0x5d7/0xac0 [ 1002.941460] [<ffffffff815cbe80>] ? cdev_put+0x50/0x50 [ 1002.943705] [<ffffffff815bc05b>] vfs_open+0x16b/0x1e0 [ 1002.945529] [<ffffffff815e1c0b>] ? may_open+0x14b/0x260 [ 1002.947334] [<ffffffff815eb3f7>] path_openat+0x4f7/0x3a00 [ 1002.949259] [<ffffffff8156cc95>] ? alloc_debug_processing+0x75/0x1c0 [ 1002.951185] [<ffffffff815eaf00>] ? vfs_create+0x390/0x390 [ 1002.953004] [<ffffffff811ad88e>] ? __kernel_text_address+0x7e/0xa0 [ 1002.954820] [<ffffffff8109154f>] ? print_context_stack+0x7f/0xf0 [ 1002.956646] [<ffffffff8124b110>] ? debug_check_no_locks_freed+0x290/0x290 [ 1002.959007] [<ffffffff815b105b>] ? create_object+0x3eb/0x940 [ 1002.962537] [<ffffffff8124a5f6>] ? trace_hardirqs_on_caller+0x16/0x590 [ 1002.966090] [<ffffffff815f1cd5>] do_filp_open+0x175/0x230 [ 1002.969603] [<ffffffff815f1b60>] ? user_path_mountpoint_at+0x40/0x40 [ 1002.973088] [<ffffffff822d8567>] ? _raw_spin_unlock+0x27/0x40 [ 1002.974921] [<ffffffff81615b1a>] ? __alloc_fd+0x19a/0x4b0 [ 1002.976759] [<ffffffff8156d653>] ? kmem_cache_alloc+0x233/0x300 [ 1002.978808] [<ffffffff815bc615>] do_sys_open+0x195/0x340 [ 1002.982167] [<ffffffff8123eb5f>] ? up_read+0x1f/0x40 [ 1002.983986] [<ffffffff815bc480>] ? filp_open+0x60/0x60 [ 1002.985810] [<ffffffff81242681>] ? trace_hardirqs_off_caller+0x21/0x290 [ 1002.987630] [<ffffffff8100401b>] ? trace_hardirqs_on_thunk+0x1b/0x1d [ 1002.989456] [<ffffffff815bc7de>] SyS_open+0x1e/0x20 [ 1002.991271] [<ffffffff822d8dc0>] entry_SYSCALL_64_fastpath+0x23/0xc1 [ 1002.993950] [<ffffffff81242681>] ? trace_hardirqs_off_caller+0x21/0x290 [ 1002.997492] Code: 48 b8 00 00 00 00 00 fc ff df 48 89 e5 41 57 41 56 41 55 41 54 53 48 89 fb 48 81 c7 f0 04 00 00 48 89 fa 48 c1 ea 03 48 83 ec 10 <80> 3c 02 00 0f 85 c6 01 00 00 4c 8b bb f0 04 00 00 4d 85 ff 0f [ 1003.000679] RIP [<ffffffff81d77a61>] usb_ifnum_to_if+0x31/0x270 [ 1003.002620] RSP <ffff880035c778d0> [ 1003.019559] ---[ end trace 9127ab975e0f4107 ]---
On 04/28/2016 01:19 AM, Lars-Peter Clausen wrote: > On 04/27/2016 11:56 PM, Shuah Khan wrote: >>>> dev_dbg(mdev->dev, "Media device unregistered\n"); >>>> } >>>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c >>>> index 29409f4..9af9ba1 100644 >>>> --- a/drivers/media/media-devnode.c >>>> +++ b/drivers/media/media-devnode.c >>>> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) >>>> mutex_unlock(&media_devnode_lock); >>>> return -ENXIO; >>>> } >>>> + >>>> + kobject_get(&mdev->kobj); >>> >>> This is not necessary, and if it was it would be prone to race condition as >>> the last reference could be dropped before this line. But assigning the cdev >>> parent makes sure that we always have a reference to the object while the >>> open() callback is running. >> >> I don't see cdev parent kobj get in cdev_get() which does kobject_get() >> on cdev->kobj. Is that enough to get the reference? >> >> cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is >> the reason why I added a get here and put in media_release(). >> > > The cdev takes the parent reference when created and only drops it once it > is released. So as long as the cdev exists there is a reference to the > parent. While cdev_del() puts one reference to the cdev there is also one > reference for each open file. So as long as there is a open file there is a > reference to the parent as well. > >> I can remove the get and put and test. Looks like I am not checking >> kobject_get() return value which isn't good? > > kobject_get() can't fail. Yes looks that way, yet there are so many places in lib/kobject.c that check for kobject_get() returning NULL. :) > >> >>> >>>> + >>>> /* and increase the device refcount */ >>>> get_device(&mdev->dev); >>>> mutex_unlock(&media_devnode_lock); >>>> /* >>> [...] >>>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h >>>> index fe42f08..ba4bdaa 100644 >>>> --- a/include/media/media-devnode.h >>>> +++ b/include/media/media-devnode.h >>>> @@ -70,7 +70,9 @@ struct media_file_operations { >>>> * @fops: pointer to struct &media_file_operations with media device ops >>>> * @dev: struct device pointer for the media controller device >>>> * @cdev: struct cdev pointer character device >>>> + * @kobj: struct kobject >>>> * @parent: parent device >>>> + * @media_dev: media device >>>> * @minor: device node minor number >>>> * @flags: flags, combination of the MEDIA_FLAG_* constants >>>> * @release: release callback called at the end of media_devnode_release() >>>> @@ -87,7 +89,9 @@ struct media_devnode { >>>> /* sysfs */ >>>> struct device dev; /* media device */ >>>> struct cdev cdev; /* character device */ >>>> + struct kobject kobj; /* set as cdev parent kobj */ >>> >>> You don't need a extra kobj. Just use the struct dev kobj. >> >> Yeah I can use that as long as I can override the default release >> function with media_devnode_free(). media_devnode should stick around >> until the last app closes /dev/mediaX even if the media_device is no >> longer registered. i.e media_ioctl should be able to check if devnode >> is registered or not. I think I am missing something and don't understand >> how struct dev kobj can be used. > > The struct dev that is embedded into th media_devnode as the same live time > as the media_devnode itself. In addition to that struct device is a > reference counted object. This means a structure that embeds struct device > must not be freed until the last reference is dropped. > > What you do here is introduce a independent reference counting mechanism for > the same structure. Which means if there is a reference to struct device, > but not to the new kobj you end up with a use-after-free again. > > The solution is to only use one reference counting mechanism (the struct > device) and intialize the cdev kobj parent to the device kobj and whenever > you did kobj_{get,put}() replace that with {get,put}_device(). And in the > device release callback free the struct media_devnode. > Okay. It is all well and good that I can use the kobj in devnode->dev, however, devnode->dev doesn't get initialized in device_register(&devnode->dev) and cdev_add() is the one that does kobject_get() on the cdev parent kobj. I am seeing: [ 45.724866] au0828: Registered device AU0828 [Hauppauge HVR950Q] [ 45.724961] ------------[ cut here ]------------ [ 45.724975] WARNING: CPU: 1 PID: 312 at lib/kobject.c:597 kobject_get+0x8f/0xf0 [ 45.724982] kobject: '(null)' (ffff8801f6166530): is not initialized, yet kobject_get() is being called. warnings as soon as drivers do cdev_add(). This will be a problem at cdev_del() time, since cdev_del() is done after device_unregister(&devnode->dev) and device_del() deletes the dev->kobj In other words, devnode->dev lifetime is going to be within device_register(&devnode->dev) and device_unregister(&devnode->dev) and that won't work as cdev_add() happens before device_register(&devnode->dev) and cdev_del() after device_unregister(&devnode->dev) cdev_add() device_register(&devnode->dev) dev.kobj initialized device_unregister(&devnode->dev) dev.kobj deleted cdev_del() I will go back to adding kobject to devnode. thanks, -- Shuah -- 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
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 6e43c95..78b0350 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -428,7 +428,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; switch (cmd) { @@ -504,7 +504,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) { @@ -546,7 +546,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); } @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct media_device *mdev, { int ret; + mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL); + if (!mdev->devnode) + return -ENOMEM; + /* Register the device node. */ - mdev->devnode.fops = &media_device_fops; - mdev->devnode.parent = mdev->dev; - mdev->devnode.release = media_device_release; + mdev->devnode->fops = &media_device_fops; + mdev->devnode->parent = mdev->dev; + mdev->devnode->media_dev = mdev; + mdev->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(&mdev->devnode, owner); + ret = media_devnode_register(mdev->devnode, owner); if (ret < 0) return ret; - ret = device_create_file(&mdev->devnode.dev, &dev_attr_model); + ret = device_create_file(&mdev->devnode->dev, &dev_attr_model); if (ret < 0) { - media_devnode_unregister(&mdev->devnode); + media_devnode_unregister(mdev->devnode); return ret; } @@ -790,7 +796,7 @@ void media_device_unregister(struct media_device *mdev) spin_lock(&mdev->lock); /* Check if mdev was ever registered at all */ - if (!media_devnode_is_registered(&mdev->devnode)) { + if (!media_devnode_is_registered(mdev->devnode)) { spin_unlock(&mdev->lock); return; } @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev) spin_unlock(&mdev->lock); - device_remove_file(&mdev->devnode.dev, &dev_attr_model); - media_devnode_unregister(&mdev->devnode); + device_remove_file(&mdev->devnode->dev, &dev_attr_model); + media_devnode_unregister(mdev->devnode); + /* kfree devnode is done via kobject_put() handler */ + mdev->devnode = NULL; dev_dbg(mdev->dev, "Media device unregistered\n"); } diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c index 29409f4..9af9ba1 100644 --- a/drivers/media/media-devnode.c +++ b/drivers/media/media-devnode.c @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file *filp) mutex_unlock(&media_devnode_lock); return -ENXIO; } + + kobject_get(&mdev->kobj); + /* and increase the device refcount */ get_device(&mdev->dev); mutex_unlock(&media_devnode_lock); @@ -181,6 +184,7 @@ static int media_open(struct inode *inode, struct file *filp) ret = mdev->fops->open(filp); if (ret) { put_device(&mdev->dev); + kobject_put(&mdev->kobj); filp->private_data = NULL; return ret; } @@ -200,6 +204,7 @@ static int media_release(struct inode *inode, struct file *filp) /* decrease the refcount unconditionally since the release() return value is ignored. */ put_device(&mdev->dev); + kobject_put(&mdev->kobj); filp->private_data = NULL; return 0; } @@ -218,6 +223,19 @@ static const struct file_operations media_devnode_fops = { .llseek = no_llseek, }; +static void media_devnode_free(struct kobject *kobj) +{ + struct media_devnode *devnode = + container_of(kobj, struct media_devnode, kobj); + + kfree(devnode); + pr_info("%s: Media Devnode Deallocated\n", __func__); +} + +static struct kobj_type media_devnode_ktype = { + .release = media_devnode_free, +}; + int __must_check media_devnode_register(struct media_devnode *mdev, struct module *owner) { @@ -238,9 +256,12 @@ int __must_check media_devnode_register(struct media_devnode *mdev, mdev->minor = minor; + kobject_init(&mdev->kobj, &media_devnode_ktype); + /* Part 2: Initialize and register the character device */ cdev_init(&mdev->cdev, &media_devnode_fops); mdev->cdev.owner = owner; + mdev->cdev.kobj.parent = &mdev->kobj; ret = cdev_add(&mdev->cdev, MKDEV(MAJOR(media_dev_t), mdev->minor), 1); if (ret < 0) { @@ -269,6 +290,7 @@ int __must_check media_devnode_register(struct media_devnode *mdev, error: cdev_del(&mdev->cdev); clear_bit(mdev->minor, media_devnode_nums); + kobject_put(&mdev->kobj); return ret; } @@ -282,6 +304,7 @@ void media_devnode_unregister(struct media_devnode *mdev) clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); mutex_unlock(&media_devnode_lock); device_unregister(&mdev->dev); + kobject_put(&mdev->kobj); } /* diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index cc22b32..8af9344 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -136,7 +136,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) #ifdef CONFIG_MEDIA_CONTROLLER if (dev->media_dev && - media_devnode_is_registered(&dev->media_dev->devnode)) { + media_devnode_is_registered(dev->media_dev->devnode)) { /* clear enable_source, disable_source */ dev->media_dev->source_priv = NULL; dev->media_dev->enable_source = NULL; @@ -468,7 +468,7 @@ static int au0828_media_device_register(struct au0828_dev *dev, if (!dev->media_dev) return 0; - if (!media_devnode_is_registered(&dev->media_dev->devnode)) { + if (!media_devnode_is_registered(dev->media_dev->devnode)) { /* register media device */ ret = media_device_register(dev->media_dev); diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 451e84e9..302e284 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1674,7 +1674,7 @@ static void uvc_delete(struct uvc_device *dev) if (dev->vdev.dev) v4l2_device_unregister(&dev->vdev); #ifdef CONFIG_MEDIA_CONTROLLER - if (media_devnode_is_registered(&dev->mdev.devnode)) + if (media_devnode_is_registered(dev->mdev.devnode)) media_device_unregister(&dev->mdev); media_device_cleanup(&dev->mdev); #endif diff --git a/include/media/media-device.h b/include/media/media-device.h index df74cfa..65394f3 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -283,7 +283,7 @@ struct media_entity_notify { /** * struct media_device - Media device * @dev: Parent device - * @devnode: Media device node + * @devnode: Media device node pointer * @driver_name: Optional device driver name. If not set, calls to * %MEDIA_IOC_DEVICE_INFO will return dev->driver->name. * This is needed for USB drivers for example, as otherwise @@ -348,7 +348,7 @@ struct media_entity_notify { struct media_device { /* dev->driver_data points to this struct. */ struct device *dev; - struct media_devnode devnode; + struct media_devnode *devnode; char model[32]; char driver_name[32]; @@ -396,9 +396,6 @@ struct usb_device; #define MEDIA_DEV_NOTIFY_PRE_LINK_CH 0 #define MEDIA_DEV_NOTIFY_POST_LINK_CH 1 -/* media_devnode to media_device */ -#define to_media_device(node) container_of(node, struct media_device, devnode) - /** * media_entity_enum_init - Initialise an entity enumeration * diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h index fe42f08..ba4bdaa 100644 --- a/include/media/media-devnode.h +++ b/include/media/media-devnode.h @@ -70,7 +70,9 @@ struct media_file_operations { * @fops: pointer to struct &media_file_operations with media device ops * @dev: struct device pointer for the media controller device * @cdev: struct cdev pointer character device + * @kobj: struct kobject * @parent: parent device + * @media_dev: media device * @minor: device node minor number * @flags: flags, combination of the MEDIA_FLAG_* constants * @release: release callback called at the end of media_devnode_release() @@ -87,7 +89,9 @@ struct media_devnode { /* sysfs */ struct device dev; /* media device */ struct cdev cdev; /* character device */ + struct kobject kobj; /* set as cdev parent kobj */ struct device *parent; /* device parent */ + struct media_device *media_dev; /* media device for the devnode */ /* device info */ int minor; @@ -149,7 +153,9 @@ static inline struct media_devnode *media_devnode_data(struct file *filp) */ static inline int media_devnode_is_registered(struct media_devnode *mdev) { - return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); + if (mdev) + return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags); + return false; } #endif /* _MEDIA_DEVNODE_H */
When driver unbind is run while media_ioctl is in progress, media_ioctl() fails with use-after-free. This first use-after-free is followed by more user-after-free errors in media_release(), kobject_put(), and cdev_put() as driver unbind continues. This problem is found on uvcvideo, em28xx, and au0828 drivers and fix has been tested on all three. This fix allocates media devnode and manages its lifetime separate from the struct media_device. Adds kobject to the media_devnode structure and this kobject is set as the cdev parent kobject. This allows cdev_add() to hold a reference to it and release the reference in cdev_del() ensuring that the media_devnode is not deallocated as long as the application has the cdev open. The first error is below: [ 472.424302] ================================================================== [ 472.424333] BUG: KASAN: use-after-free in media_ioctl+0xf0/0x130 [media] at addr ffff880027b72330 [ 472.424341] Read of size 8 by task media_device_te/1794 [ 472.424348] ============================================================================= [ 472.424356] BUG kmalloc-4096 (Not tainted): kasan: bad access detected [ 472.424361] ----------------------------------------------------------------------------- [ 472.431973] CPU: 1 PID: 1794 Comm: media_device_te Tainted: G B 4.6.0-rc5 #2 [ 472.431988] Hardware name: Hewlett-Packard HP ProBook 6475b/180F, BIOS 68TTU Ver. F.04 08/03/2012 [ 472.431996] ffffea00009edc00 ffff88009ddffc78 ffffffff81aecac3 ffff8801fa403200 [ 472.432016] ffff880027b72260 ffff88009ddffca8 ffffffff815359b2 ffff8801fa403200 [ 472.432040] ffffea00009edc00 ffff880027b72260 ffffffffa0c9cc60 ffff88009ddffcd0 [ 472.432059] Call Trace: [ 472.432079] [<ffffffff81aecac3>] dump_stack+0x67/0x94 [ 472.432092] [<ffffffff815359b2>] print_trailer+0x112/0x1a0 [ 472.432108] [<ffffffff8153b5e4>] object_err+0x34/0x40 [ 472.432125] [<ffffffff8153d9d4>] kasan_report_error+0x224/0x530 [ 472.432148] [<ffffffffa0c934c0>] ? __media_device_get_topology+0x1850/0x1850 [media] [ 472.432167] [<ffffffff8153de13>] __asan_report_load8_noabort+0x43/0x50 [ 472.432190] [<ffffffffa0c94f60>] ? media_ioctl+0x120/0x130 [media] [ 472.432209] [<ffffffffa0c94f60>] media_ioctl+0x120/0x130 [media] [ 472.432229] [<ffffffff815a9e44>] do_vfs_ioctl+0x184/0xe80 [ 472.432243] [<ffffffff815a9cc0>] ? ioctl_preallocate+0x1a0/0x1a0 [ 472.432256] [<ffffffff8127b1f0>] ? __hrtimer_init+0x170/0x170 [ 472.432272] [<ffffffff82846b01>] ? do_nanosleep+0x161/0x480 [ 472.432298] [<ffffffff811451d0>] ? sigprocmask+0x290/0x290 [ 472.432323] [<ffffffff815c7209>] ? __fget_light+0x139/0x200 [ 472.432358] [<ffffffff815aabb9>] SyS_ioctl+0x79/0x90 [ 472.432381] [<ffffffff82848aa5>] entry_SYSCALL_64_fastpath+0x18/0xa8 Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/media/media-device.c | 32 ++++++++++++++++++++------------ drivers/media/media-devnode.c | 23 +++++++++++++++++++++++ drivers/media/usb/au0828/au0828-core.c | 4 ++-- drivers/media/usb/uvc/uvc_driver.c | 2 +- include/media/media-device.h | 7 ++----- include/media/media-devnode.h | 8 +++++++- 6 files changed, 55 insertions(+), 21 deletions(-)