[PATCH 0/2] Prepare for cdev fixes

[PATCH 0/2] Prepare for cdev fixes

[Mauro Carvalho Chehab]

Those two patches are needed by Shuah's patch that fix use-after free troubles: 
	https://patchwork.linuxtv.org/patch/34201/

Those two patches were already sent  back on March, 23 but specially the second
patch  would need more review.

So, resend it, in order to get some acks. My plan is to test them together with Shuah's
patch on this Monday, and apply them as soon as possible, for the Kernel 4.7 merge
window. Those patches should be c/c to stable, in order to fix for older Kernels.

Mauro Carvalho Chehab (2):
  [media] media-devnode: fix namespace mess
  [media] media-device: dynamically allocate struct media_devnode

 drivers/media/media-device.c           |  44 +++++++++----
 drivers/media/media-devnode.c          | 115 +++++++++++++++++----------------
 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          |  27 +++++---
 6 files changed, 113 insertions(+), 84 deletions(-)

-- 
2.5.5

[PATCH 1/2] [media] media-devnode: fix namespace mess

[Mauro Carvalho Chehab]

Along all media controller code, "mdev" is used to represent
a pointer to struct media_device, and "devnode" for a pointer
to struct media_devnode.

However, inside media-devnode.[ch], "mdev" is used to represent
a pointer to struct media_devnode.

This is very confusing and may lead to development errors.

So, let's change all occurrences at media-devnode.[ch] to
also use "devnode" for such pointers.

This patch doesn't make any functional changes.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/media-devnode.c | 110 +++++++++++++++++++++---------------------
 include/media/media-devnode.h |  16 +++---
 2 files changed, 63 insertions(+), 63 deletions(-)

diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index b66dc9d0766b..7481c9610945 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -59,21 +59,21 @@ static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
 /* Called when the last user of the media device exits. */
 static void media_devnode_release(struct device *cd)
 {
-	struct media_devnode *mdev = to_media_devnode(cd);
+	struct media_devnode *devnode = to_media_devnode(cd);
 
 	mutex_lock(&media_devnode_lock);
 
 	/* Delete the cdev on this minor as well */
-	cdev_del(&mdev->cdev);
+	cdev_del(&devnode->cdev);
 
 	/* Mark device node number as free */
-	clear_bit(mdev->minor, media_devnode_nums);
+	clear_bit(devnode->minor, media_devnode_nums);
 
 	mutex_unlock(&media_devnode_lock);
 
 	/* Release media_devnode and perform other cleanups as needed. */
-	if (mdev->release)
-		mdev->release(mdev);
+	if (devnode->release)
+		devnode->release(devnode);
 }
 
 static struct bus_type media_bus_type = {
@@ -83,37 +83,37 @@ static struct bus_type media_bus_type = {
 static ssize_t media_read(struct file *filp, char __user *buf,
 		size_t sz, loff_t *off)
 {
-	struct media_devnode *mdev = media_devnode_data(filp);
+	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (!mdev->fops->read)
+	if (!devnode->fops->read)
 		return -EINVAL;
-	if (!media_devnode_is_registered(mdev))
+	if (!media_devnode_is_registered(devnode))
 		return -EIO;
-	return mdev->fops->read(filp, buf, sz, off);
+	return devnode->fops->read(filp, buf, sz, off);
 }
 
 static ssize_t media_write(struct file *filp, const char __user *buf,
 		size_t sz, loff_t *off)
 {
-	struct media_devnode *mdev = media_devnode_data(filp);
+	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (!mdev->fops->write)
+	if (!devnode->fops->write)
 		return -EINVAL;
-	if (!media_devnode_is_registered(mdev))
+	if (!media_devnode_is_registered(devnode))
 		return -EIO;
-	return mdev->fops->write(filp, buf, sz, off);
+	return devnode->fops->write(filp, buf, sz, off);
 }
 
 static unsigned int media_poll(struct file *filp,
 			       struct poll_table_struct *poll)
 {
-	struct media_devnode *mdev = media_devnode_data(filp);
+	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (!media_devnode_is_registered(mdev))
+	if (!media_devnode_is_registered(devnode))
 		return POLLERR | POLLHUP;
-	if (!mdev->fops->poll)
+	if (!devnode->fops->poll)
 		return DEFAULT_POLLMASK;
-	return mdev->fops->poll(filp, poll);
+	return devnode->fops->poll(filp, poll);
 }
 
 static long
@@ -121,12 +121,12 @@ __media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
 	      long (*ioctl_func)(struct file *filp, unsigned int cmd,
 				 unsigned long arg))
 {
-	struct media_devnode *mdev = media_devnode_data(filp);
+	struct media_devnode *devnode = media_devnode_data(filp);
 
 	if (!ioctl_func)
 		return -ENOTTY;
 
-	if (!media_devnode_is_registered(mdev))
+	if (!media_devnode_is_registered(devnode))
 		return -EIO;
 
 	return ioctl_func(filp, cmd, arg);
@@ -134,9 +134,9 @@ __media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
 
 static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
-	struct media_devnode *mdev = media_devnode_data(filp);
+	struct media_devnode *devnode = media_devnode_data(filp);
 
-	return __media_ioctl(filp, cmd, arg, mdev->fops->ioctl);
+	return __media_ioctl(filp, cmd, arg, devnode->fops->ioctl);
 }
 
 #ifdef CONFIG_COMPAT
@@ -144,9 +144,9 @@ static long media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 static long media_compat_ioctl(struct file *filp, unsigned int cmd,
 			       unsigned long arg)
 {
-	struct media_devnode *mdev = media_devnode_data(filp);
+	struct media_devnode *devnode = media_devnode_data(filp);
 
-	return __media_ioctl(filp, cmd, arg, mdev->fops->compat_ioctl);
+	return __media_ioctl(filp, cmd, arg, devnode->fops->compat_ioctl);
 }
 
 #endif /* CONFIG_COMPAT */
@@ -154,7 +154,7 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
 /* Override for the open function */
 static int media_open(struct inode *inode, struct file *filp)
 {
-	struct media_devnode *mdev;
+	struct media_devnode *devnode;
 	int ret;
 
 	/* Check if the media device is available. This needs to be done with
@@ -164,23 +164,23 @@ static int media_open(struct inode *inode, struct file *filp)
 	 * a crash.
 	 */
 	mutex_lock(&media_devnode_lock);
-	mdev = container_of(inode->i_cdev, struct media_devnode, cdev);
+	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. */
-	if (!media_devnode_is_registered(mdev)) {
+	if (!media_devnode_is_registered(devnode)) {
 		mutex_unlock(&media_devnode_lock);
 		return -ENXIO;
 	}
 	/* and increase the device refcount */
-	get_device(&mdev->dev);
+	get_device(&devnode->dev);
 	mutex_unlock(&media_devnode_lock);
 
-	filp->private_data = mdev;
+	filp->private_data = devnode;
 
-	if (mdev->fops->open) {
-		ret = mdev->fops->open(filp);
+	if (devnode->fops->open) {
+		ret = devnode->fops->open(filp);
 		if (ret) {
-			put_device(&mdev->dev);
+			put_device(&devnode->dev);
 			filp->private_data = NULL;
 			return ret;
 		}
@@ -192,16 +192,16 @@ static int media_open(struct inode *inode, struct file *filp)
 /* Override for the release function */
 static int media_release(struct inode *inode, struct file *filp)
 {
-	struct media_devnode *mdev = media_devnode_data(filp);
+	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (mdev->fops->release)
-		mdev->fops->release(filp);
+	if (devnode->fops->release)
+		devnode->fops->release(filp);
 
 	filp->private_data = NULL;
 
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
-	put_device(&mdev->dev);
+	put_device(&devnode->dev);
 	return 0;
 }
 
@@ -219,7 +219,7 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
-int __must_check media_devnode_register(struct media_devnode *mdev,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
 	int minor;
@@ -237,55 +237,55 @@ int __must_check media_devnode_register(struct media_devnode *mdev,
 	set_bit(minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
 
-	mdev->minor = minor;
+	devnode->minor = minor;
 
 	/* Part 2: Initialize and register the character device */
-	cdev_init(&mdev->cdev, &media_devnode_fops);
-	mdev->cdev.owner = owner;
+	cdev_init(&devnode->cdev, &media_devnode_fops);
+	devnode->cdev.owner = owner;
 
-	ret = cdev_add(&mdev->cdev, MKDEV(MAJOR(media_dev_t), mdev->minor), 1);
+	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), devnode->minor), 1);
 	if (ret < 0) {
 		pr_err("%s: cdev_add failed\n", __func__);
 		goto error;
 	}
 
 	/* Part 3: Register the media device */
-	mdev->dev.bus = &media_bus_type;
-	mdev->dev.devt = MKDEV(MAJOR(media_dev_t), mdev->minor);
-	mdev->dev.release = media_devnode_release;
-	if (mdev->parent)
-		mdev->dev.parent = mdev->parent;
-	dev_set_name(&mdev->dev, "media%d", mdev->minor);
-	ret = device_register(&mdev->dev);
+	devnode->dev.bus = &media_bus_type;
+	devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
+	devnode->dev.release = media_devnode_release;
+	if (devnode->parent)
+		devnode->dev.parent = devnode->parent;
+	dev_set_name(&devnode->dev, "media%d", devnode->minor);
+	ret = device_register(&devnode->dev);
 	if (ret < 0) {
 		pr_err("%s: device_register failed\n", __func__);
 		goto error;
 	}
 
 	/* Part 4: Activate this minor. The char device can now be used. */
-	set_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
+	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 
 	return 0;
 
 error:
 	mutex_lock(&media_devnode_lock);
-	cdev_del(&mdev->cdev);
-	clear_bit(mdev->minor, media_devnode_nums);
+	cdev_del(&devnode->cdev);
+	clear_bit(devnode->minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
 
 	return ret;
 }
 
-void media_devnode_unregister(struct media_devnode *mdev)
+void media_devnode_unregister(struct media_devnode *devnode)
 {
-	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(mdev))
+	/* Check if devnode was ever registered at all */
+	if (!media_devnode_is_registered(devnode))
 		return;
 
 	mutex_lock(&media_devnode_lock);
-	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
+	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
-	device_unregister(&mdev->dev);
+	device_unregister(&devnode->dev);
 }
 
 /*
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index fe42f08e72bd..e1d5af077adb 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -94,7 +94,7 @@ struct media_devnode {
 	unsigned long flags;		/* Use bitops to access flags */
 
 	/* callbacks */
-	void (*release)(struct media_devnode *mdev);
+	void (*release)(struct media_devnode *devnode);
 };
 
 /* dev to media_devnode */
@@ -103,7 +103,7 @@ struct media_devnode {
 /**
  * media_devnode_register - register a media device node
  *
- * @mdev: media device node structure we want to register
+ * @devnode: media device node structure we want to register
  * @owner: should be filled with %THIS_MODULE
  *
  * The registration code assigns minor numbers and registers the new device node
@@ -116,12 +116,12 @@ struct media_devnode {
  * the media_devnode structure is *not* called, so the caller is responsible for
  * freeing any data.
  */
-int __must_check media_devnode_register(struct media_devnode *mdev,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
 
 /**
  * media_devnode_unregister - unregister a media device node
- * @mdev: the device node to unregister
+ * @devnode: the device node to unregister
  *
  * This unregisters the passed device. Future open calls will be met with
  * errors.
@@ -129,7 +129,7 @@ int __must_check media_devnode_register(struct media_devnode *mdev,
  * This function can safely be called if the device node has never been
  * registered or has already been unregistered.
  */
-void media_devnode_unregister(struct media_devnode *mdev);
+void media_devnode_unregister(struct media_devnode *devnode);
 
 /**
  * media_devnode_data - returns a pointer to the &media_devnode
@@ -145,11 +145,11 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
  * media_devnode_is_registered - returns true if &media_devnode is registered;
  *	false otherwise.
  *
- * @mdev: pointer to struct &media_devnode.
+ * @devnode: pointer to struct &media_devnode.
  */
-static inline int media_devnode_is_registered(struct media_devnode *mdev)
+static inline int media_devnode_is_registered(struct media_devnode *devnode)
 {
-	return test_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
+	return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 }
 
 #endif /* _MEDIA_DEVNODE_H */
-- 
2.5.5

[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 <mchehab@osg.samsung.com>
---
 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(&dev->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 = &media_device_fops;
-	mdev->devnode.parent = mdev->dev;
-	mdev->devnode.release = media_device_release;
+	mdev->devnode = devnode;
+	devnode->fops = &media_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(&mdev->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(&mdev->devnode.dev, &dev_attr_model);
+	ret = device_create_file(&devnode->dev, &dev_attr_model);
 	if (ret < 0) {
-		media_devnode_unregister(&mdev->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(&mdev->graph_mutex);
 
 	/* Check if mdev was ever registered at all */
-	if (!media_devnode_is_registered(&mdev->devnode)) {
+	if (!media_devnode_is_registered(mdev->devnode)) {
 		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
@@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
-	dev_dbg(mdev->dev, "Media device unregistering\n");
-	media_devnode_unregister(&mdev->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(&mdev->devnode->dev, &dev_attr_model);
+		media_devnode_unregister(mdev->devnode);
+	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index 7481c9610945..ecdc02d6ed83 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -44,6 +44,7 @@
 #include <linux/uaccess.h>
 
 #include <media/media-devnode.h>
+#include <media/media-device.h>
 
 #define MEDIA_NUM_DEVICES	256
 #define MEDIA_NAME		"media"
@@ -74,6 +75,8 @@ static void media_devnode_release(struct device *cd)
 	/* Release media_devnode and perform other cleanups as needed. */
 	if (devnode->release)
 		devnode->release(devnode);
+
+	kfree(devnode);
 }
 
 static struct bus_type media_bus_type = {
@@ -219,7 +222,8 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
-int __must_check media_devnode_register(struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_device *mdev,
+					struct media_devnode *devnode,
 					struct module *owner)
 {
 	int minor;
@@ -238,6 +242,7 @@ int __must_check media_devnode_register(struct media_devnode *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);
diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 321ea5cf1329..bf53553d2624 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	struct media_device *mdev = dev->media_dev;
 	struct media_entity_notify *notify, *nextp;
 
-	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
+	if (!mdev || !media_devnode_is_registered(mdev->devnode))
 		return;
 
 	/* Remove au0828 entity_notify callbacks */
@@ -482,7 +482,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 451e84e962e2..302e284a95eb 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 a9b33c47310d..f743ae2210ee 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -347,7 +347,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];
@@ -393,9 +393,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 e1d5af077adb..5bb3b0e86d73 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -33,6 +33,8 @@
 #include <linux/device.h>
 #include <linux/cdev.h>
 
+struct media_device;
+
 /*
  * Flag to mark the media_devnode struct as registered. Drivers must not touch
  * this flag directly, it will be set and cleared by media_devnode_register and
@@ -81,6 +83,8 @@ struct media_file_operations {
  * before registering the node.
  */
 struct media_devnode {
+	struct media_device *media_dev;
+
 	/* device ops */
 	const struct media_file_operations *fops;
 
@@ -103,6 +107,7 @@ struct media_devnode {
 /**
  * media_devnode_register - register a media device node
  *
+ * @media_dev: struct media_device we want to register a device node
  * @devnode: media device node structure we want to register
  * @owner: should be filled with %THIS_MODULE
  *
@@ -116,7 +121,8 @@ struct media_devnode {
  * the media_devnode structure is *not* called, so the caller is responsible for
  * freeing any data.
  */
-int __must_check media_devnode_register(struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_device *mdev,
+					struct media_devnode *devnode,
 					struct module *owner);
 
 /**
@@ -146,9 +152,14 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
  *	false otherwise.
  *
  * @devnode: pointer to struct &media_devnode.
+ *
+ * Note: If mdev is NULL, it also returns false.
  */
 static inline int media_devnode_is_registered(struct media_devnode *devnode)
 {
+	if (!devnode)
+		return false;
+
 	return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 }
 
-- 
2.5.5

Re: [PATCH 0/2] Prepare for cdev fixes

[Mauro Carvalho Chehab]

Em Sat, 7 May 2016 12:12:07 -0300
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Those two patches are needed by Shuah's patch that fix use-after free troubles: 
> 	https://patchwork.linuxtv.org/patch/34201/
> 
> Those two patches were already sent  back on March, 23 but specially the second
> patch  would need more review.
> 
> So, resend it, in order to get some acks. My plan is to test them together with Shuah's
> patch on this Monday, and apply them as soon as possible, for the Kernel 4.7 merge
> window. Those patches should be c/c to stable, in order to fix for older Kernels.
> 
> Mauro Carvalho Chehab (2):
>   [media] media-devnode: fix namespace mess
>   [media] media-device: dynamically allocate struct media_devnode

In order to make easier for everyone to test, I applied the three
patches (Shuah's one, plus the two above) on this branch:
	https://git.linuxtv.org//mchehab/experimental.git/log/?h=media_cdev_fix

Shuah,

Please notice that some rebase was needed, as there were some other
patches fixing other stuff related with the MC that got applied
earlier.


> 
>  drivers/media/media-device.c           |  44 +++++++++----
>  drivers/media/media-devnode.c          | 115 +++++++++++++++++----------------
>  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          |  27 +++++---
>  6 files changed, 113 insertions(+), 84 deletions(-)
> 


-- 
Thanks,
Mauro

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 <mchehab@osg.samsung.com>
> ---
>  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(&dev->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 = &media_device_fops;
> -	mdev->devnode.parent = mdev->dev;
> -	mdev->devnode.release = media_device_release;
> +	mdev->devnode = devnode;
> +	devnode->fops = &media_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(&mdev->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(&mdev->devnode.dev, &dev_attr_model);
> +	ret = device_create_file(&devnode->dev, &dev_attr_model);
>  	if (ret < 0) {
> -		media_devnode_unregister(&mdev->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(&mdev->graph_mutex);
> 
>  	/* Check if mdev was ever registered at all */
> -	if (!media_devnode_is_registered(&mdev->devnode)) {
> +	if (!media_devnode_is_registered(mdev->devnode)) {
>  		mutex_unlock(&mdev->graph_mutex);
>  		return;
>  	}
> @@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev)
> 
>  	mutex_unlock(&mdev->graph_mutex);
> 
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> -	dev_dbg(mdev->dev, "Media device unregistering\n");
> -	media_devnode_unregister(&mdev->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(&mdev->devnode->dev, &dev_attr_model);
> +		media_devnode_unregister(mdev->devnode);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister);
> 
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index 7481c9610945..ecdc02d6ed83 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -44,6 +44,7 @@
>  #include <linux/uaccess.h>
> 
>  #include <media/media-devnode.h>
> +#include <media/media-device.h>
> 
>  #define MEDIA_NUM_DEVICES	256
>  #define MEDIA_NAME		"media"
> @@ -74,6 +75,8 @@ static void media_devnode_release(struct device *cd)
>  	/* Release media_devnode and perform other cleanups as needed. */
>  	if (devnode->release)
>  		devnode->release(devnode);
> +
> +	kfree(devnode);
>  }
> 
>  static struct bus_type media_bus_type = {
> @@ -219,7 +222,8 @@ static const struct file_operations media_devnode_fops =
> { .llseek = no_llseek,
>  };
> 
> -int __must_check media_devnode_register(struct media_devnode *devnode,
> +int __must_check media_devnode_register(struct media_device *mdev,
> +					struct media_devnode *devnode,
>  					struct module *owner)
>  {
>  	int minor;
> @@ -238,6 +242,7 @@ int __must_check media_devnode_register(struct
> media_devnode *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);
> diff --git a/drivers/media/usb/au0828/au0828-core.c
> b/drivers/media/usb/au0828/au0828-core.c index 321ea5cf1329..bf53553d2624
> 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct
> au0828_dev *dev) struct media_device *mdev = dev->media_dev;
>  	struct media_entity_notify *notify, *nextp;
> 
> -	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
> +	if (!mdev || !media_devnode_is_registered(mdev->devnode))
>  		return;
> 
>  	/* Remove au0828 entity_notify callbacks */
> @@ -482,7 +482,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 451e84e962e2..302e284a95eb
> 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 a9b33c47310d..f743ae2210ee 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -347,7 +347,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];
> @@ -393,9 +393,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 e1d5af077adb..5bb3b0e86d73 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -33,6 +33,8 @@
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> 
> +struct media_device;
> +
>  /*
>   * Flag to mark the media_devnode struct as registered. Drivers must not
> touch
> * this flag directly, it will be set and cleared by media_devnode_register
> and
> @@ -81,6 +83,8 @@ struct media_file_operations {
>   * before registering the node.
>   */
>  struct media_devnode {
> +	struct media_device *media_dev;
> +

The rationale behind struct media_devnode was to decouple devnode handling 
from media device handling. The initial implementation reused struct 
media_devnode in struct video_device, to share devnode handling code between 
media device and video device. This was rejected, but struct media_devnode was 
still kept separate instead of merged into struct media_device (I don't 
remember why though).

There are two possible fixes for this problem:

1. Handling structure lifetime in drivers with propagation of the release 
callback from media device to drivers. This is the most common strategy used 
in the kernel, and we implement it for video devices. Some drivers handle that 
properly (the best example that comes to my mind, albeit a bit self-centered, 
is the uvcvideo driver) but most don't. It took me a while to handle the race 
condition properly in uvcvideo, the implementation is certainly not trivial.

2. Handling structure lifetime in the core through dynamic allocation of 
structures. This is easier on the driver side, but requires drivers to stop 
embedding data structures.

I believe we all want to give the second option a try, but I don't think 
media_devnode is the structure that needs to be dynamically allocated. 
media_devnode has a release callback that we propagate to the media_device 
implementation, with a currently empty implementation. That's where the 
problem is, we need to make media_device dynamically allocated, and refcount 
it using the release callback.

Another variant of the second option would be to merge the media_device and 
media_devnode structures, as media_devnode is only used by media_device. I 
still believe that media_devnode could be useful in video_device, but I'm 
willing to consider merging the two structures.

>  	/* device ops */
>  	const struct media_file_operations *fops;
> 
> @@ -103,6 +107,7 @@ struct media_devnode {
>  /**
>   * media_devnode_register - register a media device node
>   *
> + * @media_dev: struct media_device we want to register a device node
>   * @devnode: media device node structure we want to register
>   * @owner: should be filled with %THIS_MODULE
>   *
> @@ -116,7 +121,8 @@ struct media_devnode {
>   * the media_devnode structure is *not* called, so the caller is
> responsible for * freeing any data.
>   */
> -int __must_check media_devnode_register(struct media_devnode *devnode,
> +int __must_check media_devnode_register(struct media_device *mdev,
> +					struct media_devnode *devnode,
>  					struct module *owner);
> 
>  /**
> @@ -146,9 +152,14 @@ static inline struct media_devnode
> *media_devnode_data(struct file *filp) *	false otherwise.
>   *
>   * @devnode: pointer to struct &media_devnode.
> + *
> + * Note: If mdev is NULL, it also returns false.
>   */
>  static inline int media_devnode_is_registered(struct media_devnode
> *devnode) {
> +	if (!devnode)
> +		return false;
> +
>  	return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>  }

-- 
Regards,

Laurent Pinchart

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 <laurent.pinchart@ideasonboard.com> 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 <mchehab@osg.samsung.com>
> > ---
> >  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(&dev->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 = &media_device_fops;
> > -	mdev->devnode.parent = mdev->dev;
> > -	mdev->devnode.release = media_device_release;
> > +	mdev->devnode = devnode;
> > +	devnode->fops = &media_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(&mdev->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(&mdev->devnode.dev, &dev_attr_model);
> > +	ret = device_create_file(&devnode->dev, &dev_attr_model);
> >  	if (ret < 0) {
> > -		media_devnode_unregister(&mdev->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(&mdev->graph_mutex);
> > 
> >  	/* Check if mdev was ever registered at all */
> > -	if (!media_devnode_is_registered(&mdev->devnode)) {
> > +	if (!media_devnode_is_registered(mdev->devnode)) {
> >  		mutex_unlock(&mdev->graph_mutex);
> >  		return;
> >  	}
> > @@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev)
> > 
> >  	mutex_unlock(&mdev->graph_mutex);
> > 
> > -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> > -	dev_dbg(mdev->dev, "Media device unregistering\n");
> > -	media_devnode_unregister(&mdev->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(&mdev->devnode->dev, &dev_attr_model);
> > +		media_devnode_unregister(mdev->devnode);
> > +	}
> >  }
> >  EXPORT_SYMBOL_GPL(media_device_unregister);
> > 
> > diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> > index 7481c9610945..ecdc02d6ed83 100644
> > --- a/drivers/media/media-devnode.c
> > +++ b/drivers/media/media-devnode.c
> > @@ -44,6 +44,7 @@
> >  #include <linux/uaccess.h>
> > 
> >  #include <media/media-devnode.h>
> > +#include <media/media-device.h>
> > 
> >  #define MEDIA_NUM_DEVICES	256
> >  #define MEDIA_NAME		"media"
> > @@ -74,6 +75,8 @@ static void media_devnode_release(struct device *cd)
> >  	/* Release media_devnode and perform other cleanups as needed. */
> >  	if (devnode->release)
> >  		devnode->release(devnode);
> > +
> > +	kfree(devnode);
> >  }
> > 
> >  static struct bus_type media_bus_type = {
> > @@ -219,7 +222,8 @@ static const struct file_operations media_devnode_fops =
> > { .llseek = no_llseek,
> >  };
> > 
> > -int __must_check media_devnode_register(struct media_devnode *devnode,
> > +int __must_check media_devnode_register(struct media_device *mdev,
> > +					struct media_devnode *devnode,
> >  					struct module *owner)
> >  {
> >  	int minor;
> > @@ -238,6 +242,7 @@ int __must_check media_devnode_register(struct
> > media_devnode *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);
> > diff --git a/drivers/media/usb/au0828/au0828-core.c
> > b/drivers/media/usb/au0828/au0828-core.c index 321ea5cf1329..bf53553d2624
> > 100644
> > --- a/drivers/media/usb/au0828/au0828-core.c
> > +++ b/drivers/media/usb/au0828/au0828-core.c
> > @@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct
> > au0828_dev *dev) struct media_device *mdev = dev->media_dev;
> >  	struct media_entity_notify *notify, *nextp;
> > 
> > -	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
> > +	if (!mdev || !media_devnode_is_registered(mdev->devnode))
> >  		return;
> > 
> >  	/* Remove au0828 entity_notify callbacks */
> > @@ -482,7 +482,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 451e84e962e2..302e284a95eb
> > 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 a9b33c47310d..f743ae2210ee 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -347,7 +347,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];
> > @@ -393,9 +393,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 e1d5af077adb..5bb3b0e86d73 100644
> > --- a/include/media/media-devnode.h
> > +++ b/include/media/media-devnode.h
> > @@ -33,6 +33,8 @@
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> > 
> > +struct media_device;
> > +
> >  /*
> >   * Flag to mark the media_devnode struct as registered. Drivers must not
> > touch
> > * this flag directly, it will be set and cleared by media_devnode_register
> > and
> > @@ -81,6 +83,8 @@ struct media_file_operations {
> >   * before registering the node.
> >   */
> >  struct media_devnode {
> > +	struct media_device *media_dev;
> > +  
> 
> The rationale behind struct media_devnode was to decouple devnode handling 
> from media device handling. The initial implementation reused struct 
> media_devnode in struct video_device, to share devnode handling code between 
> media device and video device. This was rejected, but struct media_devnode was 
> still kept separate instead of merged into struct media_device (I don't 
> remember why though).
> 
> There are two possible fixes for this problem:
> 
> 1. Handling structure lifetime in drivers with propagation of the release 
> callback from media device to drivers. This is the most common strategy used 
> in the kernel, and we implement it for video devices. Some drivers handle that 
> properly (the best example that comes to my mind, albeit a bit self-centered, 
> is the uvcvideo driver) but most don't. It took me a while to handle the race 
> condition properly in uvcvideo, the implementation is certainly not trivial.

I think you're actually referring just to the V4L2 part of uvcvideo, as the
media controller part of uvcvideo is also broken, causing use after free
if the MC is in use while the device going unbind.

The problem with release callbacks and kref is that it has one limit:
it has to have *just one* kref per struct (embedded or not). It means that
a struct that has both V4L2 and MC devnode data on it cannot use this
strategy.

Also, as you mentioned, even for you that wrote the uvcvideo driver,
it took you a while to get it right, for the V4L2 part.

So, a change like that can be disruptive at drivers level, as it may
require additional changes at the drivers data structures.

As the patches fixing cdev and use after free need to be backported
to stable Kernels (as all kernels since the addition of MC to
uvcvideo are broken), the aim is to have something that would require
minimal changes at the drivers.

> 2. Handling structure lifetime in the core through dynamic allocation of 
> structures. This is easier on the driver side, but requires drivers to stop 
> embedding data structures.

It only needs to de-embed the struct that embeds cdev.

> I believe we all want to give the second option a try, but I don't think 
> media_devnode is the structure that needs to be dynamically allocated. 
> media_devnode has a release callback that we propagate to the media_device 
> implementation, with a currently empty implementation.

Having the release callback empty for so long time it is a strong
indication that there's something wrong with that, and it can't be used
as-is.

I played with the release callback in order to solve the problems caused
by Shuah's devres patches. It worked there, but only because those
patches were dynamically allocating media_device too. Again, the changes
on existing drivers are disruptive, and would likely require extra
hard work to backport to stable. Among other changes, it required to
remove the media_device_cleanup() callback from the drivers, as, after the
change, such cleanup should be done by the release callback.

> That's where the 
> problem is, we need to make media_device dynamically allocated, and refcount 
> it using the release callback.
> 
> Another variant of the second option would be to merge the media_device and 
> media_devnode structures, as media_devnode is only used by media_device. I 
> still believe that media_devnode could be useful in video_device, but I'm 
> willing to consider merging the two structures.

Those variants could be tried *after* we fix the bug in a way that it
can be backported to -stable, but for now we need a solution that it is
not disruptive and can easily be backported to old Kernels.

Regards,
Mauro

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 <laurent.pinchart@ideasonboard.com> 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 <mchehab@osg.samsung.com>
>>> ---
>>>  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(&dev->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 = &media_device_fops;
>>> -	mdev->devnode.parent = mdev->dev;
>>> -	mdev->devnode.release = media_device_release;
>>> +	mdev->devnode = devnode;
>>> +	devnode->fops = &media_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(&mdev->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(&mdev->devnode.dev, &dev_attr_model);
>>> +	ret = device_create_file(&devnode->dev, &dev_attr_model);
>>>  	if (ret < 0) {
>>> -		media_devnode_unregister(&mdev->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(&mdev->graph_mutex);
>>>
>>>  	/* Check if mdev was ever registered at all */
>>> -	if (!media_devnode_is_registered(&mdev->devnode)) {
>>> +	if (!media_devnode_is_registered(mdev->devnode)) {
>>>  		mutex_unlock(&mdev->graph_mutex);
>>>  		return;
>>>  	}
>>> @@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev)
>>>
>>>  	mutex_unlock(&mdev->graph_mutex);
>>>
>>> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
>>> -	dev_dbg(mdev->dev, "Media device unregistering\n");
>>> -	media_devnode_unregister(&mdev->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(&mdev->devnode->dev, &dev_attr_model);
>>> +		media_devnode_unregister(mdev->devnode);
>>> +	}
>>>  }
>>>  EXPORT_SYMBOL_GPL(media_device_unregister);
>>>
>>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
>>> index 7481c9610945..ecdc02d6ed83 100644
>>> --- a/drivers/media/media-devnode.c
>>> +++ b/drivers/media/media-devnode.c
>>> @@ -44,6 +44,7 @@
>>>  #include <linux/uaccess.h>
>>>
>>>  #include <media/media-devnode.h>
>>> +#include <media/media-device.h>
>>>
>>>  #define MEDIA_NUM_DEVICES	256
>>>  #define MEDIA_NAME		"media"
>>> @@ -74,6 +75,8 @@ static void media_devnode_release(struct device *cd)
>>>  	/* Release media_devnode and perform other cleanups as needed. */
>>>  	if (devnode->release)
>>>  		devnode->release(devnode);
>>> +
>>> +	kfree(devnode);
>>>  }
>>>
>>>  static struct bus_type media_bus_type = {
>>> @@ -219,7 +222,8 @@ static const struct file_operations media_devnode_fops =
>>> { .llseek = no_llseek,
>>>  };
>>>
>>> -int __must_check media_devnode_register(struct media_devnode *devnode,
>>> +int __must_check media_devnode_register(struct media_device *mdev,
>>> +					struct media_devnode *devnode,
>>>  					struct module *owner)
>>>  {
>>>  	int minor;
>>> @@ -238,6 +242,7 @@ int __must_check media_devnode_register(struct
>>> media_devnode *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);
>>> diff --git a/drivers/media/usb/au0828/au0828-core.c
>>> b/drivers/media/usb/au0828/au0828-core.c index 321ea5cf1329..bf53553d2624
>>> 100644
>>> --- a/drivers/media/usb/au0828/au0828-core.c
>>> +++ b/drivers/media/usb/au0828/au0828-core.c
>>> @@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct
>>> au0828_dev *dev) struct media_device *mdev = dev->media_dev;
>>>  	struct media_entity_notify *notify, *nextp;
>>>
>>> -	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
>>> +	if (!mdev || !media_devnode_is_registered(mdev->devnode))
>>>  		return;
>>>
>>>  	/* Remove au0828 entity_notify callbacks */
>>> @@ -482,7 +482,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 451e84e962e2..302e284a95eb
>>> 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 a9b33c47310d..f743ae2210ee 100644
>>> --- a/include/media/media-device.h
>>> +++ b/include/media/media-device.h
>>> @@ -347,7 +347,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];
>>> @@ -393,9 +393,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 e1d5af077adb..5bb3b0e86d73 100644
>>> --- a/include/media/media-devnode.h
>>> +++ b/include/media/media-devnode.h
>>> @@ -33,6 +33,8 @@
>>>  #include <linux/device.h>
>>>  #include <linux/cdev.h>
>>>
>>> +struct media_device;
>>> +
>>>  /*
>>>   * Flag to mark the media_devnode struct as registered. Drivers must not
>>> touch
>>> * this flag directly, it will be set and cleared by media_devnode_register
>>> and
>>> @@ -81,6 +83,8 @@ struct media_file_operations {
>>>   * before registering the node.
>>>   */
>>>  struct media_devnode {
>>> +	struct media_device *media_dev;
>>> +  
>>
>> The rationale behind struct media_devnode was to decouple devnode handling 
>> from media device handling. The initial implementation reused struct 
>> media_devnode in struct video_device, to share devnode handling code between 
>> media device and video device. This was rejected, but struct media_devnode was 
>> still kept separate instead of merged into struct media_device (I don't 
>> remember why though).
>>
>> There are two possible fixes for this problem:
>>
>> 1. Handling structure lifetime in drivers with propagation of the release 
>> callback from media device to drivers. This is the most common strategy used 
>> in the kernel, and we implement it for video devices. Some drivers handle that 
>> properly (the best example that comes to my mind, albeit a bit self-centered, 
>> is the uvcvideo driver) but most don't. It took me a while to handle the race 
>> condition properly in uvcvideo, the implementation is certainly not trivial.
> 
> I think you're actually referring just to the V4L2 part of uvcvideo, as the
> media controller part of uvcvideo is also broken, causing use after free
> if the MC is in use while the device going unbind.
> 
> The problem with release callbacks and kref is that it has one limit:
> it has to have *just one* kref per struct (embedded or not). It means that
> a struct that has both V4L2 and MC devnode data on it cannot use this
> strategy.
> 
> Also, as you mentioned, even for you that wrote the uvcvideo driver,
> it took you a while to get it right, for the V4L2 part.
> 
> So, a change like that can be disruptive at drivers level, as it may
> require additional changes at the drivers data structures.
> 
> As the patches fixing cdev and use after free need to be backported
> to stable Kernels (as all kernels since the addition of MC to
> uvcvideo are broken), the aim is to have something that would require
> minimal changes at the drivers.
> 
>> 2. Handling structure lifetime in the core through dynamic allocation of 
>> structures. This is easier on the driver side, but requires drivers to stop 
>> embedding data structures.
> 
> It only needs to de-embed the struct that embeds cdev.
> 
>> I believe we all want to give the second option a try, but I don't think 
>> media_devnode is the structure that needs to be dynamically allocated. 
>> media_devnode has a release callback that we propagate to the media_device 
>> implementation, with a currently empty implementation.

devnode embeds cdev and it has the struct device in it. devonode
worked very well as a dynamic allocation as its struct device kobj
can be used a the cdev parent kobj. This allows using one kobj for
reference counting which is necessary to avoid muddying waters with
multiple release functions in the mix.

This fix is simple and can be back ported to stable releases easily.
In addition, it doesn't require changes to existing drivers that want
to continue to embed media_device. I think media_device should not be
embedded for other reasons such as when multiple drivers are in the mix,
and to avoid use-after-free on the usb parent device.

This will also simplify media_device global list implementation which
I am testing now on top of this patch set and my cdev fix.

thanks,
-- Shuah

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 <mchehab@osg.samsung.com>
> ---
>  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(&dev->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 = &media_device_fops;
> -	mdev->devnode.parent = mdev->dev;
> -	mdev->devnode.release = media_device_release;
> +	mdev->devnode = devnode;
> +	devnode->fops = &media_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(&mdev->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(&mdev->devnode.dev, &dev_attr_model);
> +	ret = device_create_file(&devnode->dev, &dev_attr_model);
>  	if (ret < 0) {
> -		media_devnode_unregister(&mdev->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(&mdev->graph_mutex);
>  
>  	/* Check if mdev was ever registered at all */
> -	if (!media_devnode_is_registered(&mdev->devnode)) {
> +	if (!media_devnode_is_registered(mdev->devnode)) {
>  		mutex_unlock(&mdev->graph_mutex);
>  		return;
>  	}
> @@ -794,9 +806,13 @@ void media_device_unregister(struct media_device *mdev)
>  
>  	mutex_unlock(&mdev->graph_mutex);
>  
> -	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> -	dev_dbg(mdev->dev, "Media device unregistering\n");
> -	media_devnode_unregister(&mdev->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(&mdev->devnode->dev, &dev_attr_model);
> +		media_devnode_unregister(mdev->devnode);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index 7481c9610945..ecdc02d6ed83 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -44,6 +44,7 @@
>  #include <linux/uaccess.h>
>  
>  #include <media/media-devnode.h>
> +#include <media/media-device.h>
>  
>  #define MEDIA_NUM_DEVICES	256
>  #define MEDIA_NAME		"media"
> @@ -74,6 +75,8 @@ static void media_devnode_release(struct device *cd)
>  	/* Release media_devnode and perform other cleanups as needed. */
>  	if (devnode->release)
>  		devnode->release(devnode);
> +
> +	kfree(devnode);
>  }
>  
>  static struct bus_type media_bus_type = {
> @@ -219,7 +222,8 @@ static const struct file_operations media_devnode_fops = {
>  	.llseek = no_llseek,
>  };
>  
> -int __must_check media_devnode_register(struct media_devnode *devnode,
> +int __must_check media_devnode_register(struct media_device *mdev,
> +					struct media_devnode *devnode,
>  					struct module *owner)
>  {
>  	int minor;
> @@ -238,6 +242,7 @@ int __must_check media_devnode_register(struct media_devnode *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);
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index 321ea5cf1329..bf53553d2624 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -142,7 +142,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
>  	struct media_device *mdev = dev->media_dev;
>  	struct media_entity_notify *notify, *nextp;
>  
> -	if (!mdev || !media_devnode_is_registered(&mdev->devnode))
> +	if (!mdev || !media_devnode_is_registered(mdev->devnode))
>  		return;
>  
>  	/* Remove au0828 entity_notify callbacks */
> @@ -482,7 +482,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 451e84e962e2..302e284a95eb 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 a9b33c47310d..f743ae2210ee 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -347,7 +347,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];
> @@ -393,9 +393,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 e1d5af077adb..5bb3b0e86d73 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -33,6 +33,8 @@
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  
> +struct media_device;
> +
>  /*
>   * Flag to mark the media_devnode struct as registered. Drivers must not touch
>   * this flag directly, it will be set and cleared by media_devnode_register and
> @@ -81,6 +83,8 @@ struct media_file_operations {
>   * before registering the node.
>   */
>  struct media_devnode {
> +	struct media_device *media_dev;
> +
>  	/* device ops */
>  	const struct media_file_operations *fops;
>  
> @@ -103,6 +107,7 @@ struct media_devnode {
>  /**
>   * media_devnode_register - register a media device node
>   *
> + * @media_dev: struct media_device we want to register a device node
>   * @devnode: media device node structure we want to register
>   * @owner: should be filled with %THIS_MODULE
>   *
> @@ -116,7 +121,8 @@ struct media_devnode {
>   * the media_devnode structure is *not* called, so the caller is responsible for
>   * freeing any data.
>   */
> -int __must_check media_devnode_register(struct media_devnode *devnode,
> +int __must_check media_devnode_register(struct media_device *mdev,
> +					struct media_devnode *devnode,
>  					struct module *owner);
>  
>  /**
> @@ -146,9 +152,14 @@ static inline struct media_devnode *media_devnode_data(struct file *filp)
>   *	false otherwise.
>   *
>   * @devnode: pointer to struct &media_devnode.
> + *
> + * Note: If mdev is NULL, it also returns false.
>   */
>  static inline int media_devnode_is_registered(struct media_devnode *devnode)
>  {
> +	if (!devnode)
> +		return false;
> +
>  	return test_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>  }
>  

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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 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()).

Please see above. The patch I sent out does this exactly. Decoupling devnode
from media_device structure is necessary to avoid multiple concurrent lifetimes.

I do think, this is a good direction for us to go. I also have media device
allocator patch API out and reviewed which is based on these two patches and
it is simple and clean as I could rely on this problem being addressed with
these two patches.

thanks,
-- Shuah

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 <shuahkh@osg.samsung.com> 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

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 <shuahkh@osg.samsung.com> 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.ailus@iki.fi	XMPP: sailus@retiisi.org.uk