On 03/24/2016 08:59 PM, Mauro Carvalho Chehab wrote: > Currently, cdev is embedded inside media_devnode. This causes > a problem with the fs core, as __fput() will try to release > its access by calling cdev_put(): > > [ 399.653545] BUG: KASAN: use-after-free in media_release+0xe1/0xf0 [media] at addr ffff88036a9ba4e0 > [ 399.653550] Read of size 8 by task mc_nextgen_test/19761 > [ 399.653554] page:ffffea000daa6e80 count:0 mapcount:0 mapping: (null) index:0xffff88036a9bad20 > [ 399.653559] flags: 0x2ffff8000000000() > [ 399.653562] page dumped because: kasan: bad access detected > [ 399.653567] CPU: 1 PID: 19761 Comm: mc_nextgen_test Tainted: G B 4.5.0+ #62 > [ 399.653570] Hardware name: /NUC5i7RYB, BIOS RYBDWi35.86A.0350.2015.0812.1722 08/12/2015 > [ 399.653574] ffff88036a9ba4e0 ffff8803c465fd10 ffffffff819447c1 ffff88036a9ba4e0 > [ 399.653582] ffff8803c465fda8 ffff8803c465fd98 ffffffff8156ef05 0000000800000001 > [ 399.653591] ffff8803c689fa10 0000000000000292 0000000041b58ab3 ffffffff82813e00 > [ 399.653599] Call Trace: > [ 399.653604] [<ffffffff819447c1>] dump_stack+0x85/0xc4 > [ 399.653609] [<ffffffff8156ef05>] kasan_report_error+0x525/0x550 > [ 399.653615] [<ffffffff81685d10>] ? __fsnotify_inode_delete+0x20/0x20 > [ 399.653620] [<ffffffff8124acd0>] ? debug_check_no_locks_freed+0x290/0x290 > [ 399.653626] [<ffffffff8156f063>] __asan_report_load8_noabort+0x43/0x50 > [ 399.653633] [<ffffffffa11f53b1>] ? media_release+0xe1/0xf0 [media] > [ 399.653640] [<ffffffffa11f53b1>] media_release+0xe1/0xf0 [media] > [ 399.653646] [<ffffffff815c2c4f>] __fput+0x20f/0x6d0 > [ 399.653651] [<ffffffff815c317e>] ____fput+0xe/0x10 > [ 399.653656] [<ffffffff811acde7>] task_work_run+0x137/0x200 > [ 399.653662] [<ffffffff81005d54>] exit_to_usermode_loop+0x154/0x180 > [ 399.653667] [<ffffffff8124a1b6>] ? trace_hardirqs_on_caller+0x16/0x590 > [ 399.653672] [<ffffffff810073a6>] syscall_return_slowpath+0x186/0x1c0 > [ 399.653678] [<ffffffff822e7a1c>] entry_SYSCALL_64_fastpath+0xbf/0xc1 > > There are two alternatives to solve it: we could either use a static > var for cdev or to dynamically allocate it. Let's choose the last one, > as this is the same solution at v4l2 core, from where this code seems > to have originated. For reference only: when I posted my CEC framework code it was based on what v4l2-dev.c did, and I got yelled at by Russell King. I took his advice and the new approach seems to work well without having to allocate cdev. The code is here: http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/100380/focus=100378 Search for cec_devnode_register. Russell's mail is here: http://www.spinics.net/lists/dri-devel/msg88417.html However, the struct cec_adapter itself does have to be allocated, otherwise you will get nasty lifetime issues. This seems to be a common theme: allocate the main struct (cec_adapter, video_device, rc_device), then register the character device as a separate step. On advantage of this is that a driver can allocate everything first and do the registration of the devices as the last step when it knows everything is consistent and initialized properly. We've been embedding video_device/media_device in top-level structs and that looks like it was a bad idea. Digging into this mess is time consuming, but I thought I should at least share this advice from Russell as an example. Regards, Hans > > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx> > --- > drivers/media/media-devnode.c | 39 ++++++++++++++++++++++++++------------- > include/media/media-devnode.h | 4 ++-- > 2 files changed, 28 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c > index db47063d8801..7f9a7e65df20 100644 > --- a/drivers/media/media-devnode.c > +++ b/drivers/media/media-devnode.c > @@ -56,6 +56,7 @@ static dev_t media_dev_t; > */ > static DEFINE_MUTEX(media_devnode_lock); > static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES); > +static struct media_devnode *media_minors[MEDIA_NUM_DEVICES]; > > /* Called when the last user of the media device exits. */ > static void media_devnode_release(struct device *cd) > @@ -65,7 +66,9 @@ static void media_devnode_release(struct device *cd) > mutex_lock(&media_devnode_lock); > > /* Delete the cdev on this minor as well */ > - cdev_del(&devnode->cdev); > + cdev_del(devnode->cdev); > + devnode->cdev = NULL; > + media_minors[devnode->minor] = NULL; > > /* Mark device node number as free */ > clear_bit(devnode->minor, media_devnode_nums); > @@ -167,9 +170,7 @@ static int media_open(struct inode *inode, struct file *filp) > * a crash. > */ > mutex_lock(&media_devnode_lock); > - devnode = container_of(inode->i_cdev, struct media_devnode, cdev); > - /* return ENXIO if the media device has been removed > - already or if it is not registered anymore. */ > + devnode = media_minors[iminor(inode)]; > if (!media_devnode_is_registered(devnode)) { > mutex_unlock(&media_devnode_lock); > return -ENXIO; > @@ -227,6 +228,7 @@ int __must_check media_devnode_register(struct media_device *mdev, > { > int minor; > int ret; > + dev_t devt; > > /* Part 1: Find a free minor number */ > mutex_lock(&media_devnode_lock); > @@ -238,28 +240,35 @@ int __must_check media_devnode_register(struct media_device *mdev, > } > > set_bit(minor, media_devnode_nums); > + media_minors[minor] = devnode; > mutex_unlock(&media_devnode_lock); > > - devnode->minor = minor; > - devnode->media_dev = mdev; > - > /* Part 2: Initialize and register the character device */ > - cdev_init(&devnode->cdev, &media_devnode_fops); > - devnode->cdev.owner = owner; > + devnode->cdev = cdev_alloc(); > + if (!devnode->cdev) { > + ret = -ENOMEM; > + goto error; > + } > > - ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1); > + cdev_init(devnode->cdev, &media_devnode_fops); > + devnode->cdev->owner = owner; > + > + devt = MKDEV(MAJOR(media_dev_t), minor); > + ret = cdev_add(devnode->cdev, devt, 1); > if (ret < 0) { > pr_err("%s: cdev_add failed\n", __func__); > goto error; > } > > /* Part 3: Register the media device */ > + devnode->minor = minor; > + devnode->media_dev = mdev; > devnode->dev.bus = &media_bus_type; > - devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor); > + devnode->dev.devt = devt; > devnode->dev.release = media_devnode_release; > if (devnode->parent) > devnode->dev.parent = devnode->parent; > - dev_set_name(&devnode->dev, "media%d", devnode->minor); > + dev_set_name(&devnode->dev, "media%d", minor); > ret = device_register(&devnode->dev); > if (ret < 0) { > pr_err("%s: device_register failed\n", __func__); > @@ -273,8 +282,12 @@ int __must_check media_devnode_register(struct media_device *mdev, > > error: > mutex_lock(&media_devnode_lock); > - cdev_del(&devnode->cdev); > + if (devnode->cdev) { > + cdev_del(devnode->cdev); > + devnode->cdev = NULL; > + } > clear_bit(devnode->minor, media_devnode_nums); > + media_minors[minor] = NULL; > mutex_unlock(&media_devnode_lock); > > return ret; > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > index cc2b3155593c..9fe627ca5ec9 100644 > --- a/include/media/media-devnode.h > +++ b/include/media/media-devnode.h > @@ -71,7 +71,7 @@ struct media_file_operations { > * struct media_devnode - Media device node > * @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 > + * @cdev: struct cdev pointer character device > * @parent: parent device > * @minor: device node minor number > * @flags: flags, combination of the MEDIA_FLAG_* constants > @@ -90,7 +90,7 @@ struct media_devnode { > > /* sysfs */ > struct device dev; /* media device */ > - struct cdev cdev; /* character device */ > + struct cdev *cdev; /* character device */ > struct device *parent; /* device parent */ > > /* device info */ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
