[PATCH 00/26] Media device lifetime management

[PATCH 00/26] Media device lifetime management

[Sakari Ailus]

Hi folks,

This is a refresh of my 2016 RFC patchset to start addressing object
lifetime issues in Media controller. It further allows continuing work to
address lifetime management of media entities.

The underlying problem is described in detail in v4 of the previous RFC:
<URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>.
In brief, there is currently no connection between releasing media device
(and related) memory and IOCTL calls, meaning that there is a time window
during which released kernel memory can be accessed, and that access can be
triggered from the user space. The only reason why this is not a grave
security issue is that it is not triggerable by the user alone but requires
unbinding a device. That is still not an excuse for not fixing it.

This set differs from the earlier RFC to address the issue in the
following respects:

- Make changes for ipu3-cio2 driver, too.

- Continue to provide best effort attempt to keep the window between device
  removal and user space being able to access released memory as small as
  possible. This means the problem won't become worse for drivers for which
  Media device lifetime management has not been implemented.

The latter is achieved by adding a new object, Media devnode compat
reference, which is allocated, refcounted and eventually released by the
Media controller framework itself, and where the information on registration
and open filehandles is maintained. This is only done if the driver does not
manage the lifetime of the media device itself, i.e. its release operation
is NULL.

Due to this, Media device file handles will also be introduced by this
patchset. I thought the first user of this would be Media device events but
it seems we already need them here.

Both ipu3-cio2 and omap3isp drivers are relieved of devm_request_irq() use,
as device_release() releases the resources before calling the driver's
remove function. While further work will be required also on these drivers
to safely stop he hardware at unbind time, I don't see a reason not to merge
these patches now.

Some patches are temporarily reverted in order to make reworks easier, then
applied later on.

I've tested this on ipu3-cio2 with and without the refcounting patch (media:
ipu3-cio2: Release the cio2 device context by media device callback),
including failures in a few parts of the driver initialisation process in
the MC framework.

Questions and comments are welcome.


Daniel Axtens (1):
  media: uvcvideo: Refactor teardown of uvc on USB disconnect

Laurent Pinchart (1):
  media: Add per-file-handle data support

Logan Gunthorpe (1):
  media: utilize new cdev_device_add helper function

Sakari Ailus (23):
  Revert "[media] media: fix media devnode ioctl/syscall and unregister
    race"
  Revert "media: utilize new cdev_device_add helper function"
  Revert "[media] media: fix use-after-free in cdev_put() when app exits
    after driver unbind"
  Revert "media: uvcvideo: Refactor teardown of uvc on USB disconnect"
  Revert "[media] media-device: dynamically allocate struct
    media_devnode"
  media device: Drop nop release callback
  media: Do not call cdev_device_del() if cdev_device_add() fails
  media-device: Delete character device early
  media: Split initialising and adding media devnode
  media: Shuffle functions around
  media device: Initialise media devnode in media_device_init()
  media device: Refcount the media device
  v4l: Acquire a reference to the media device for every video device
  media-device: Postpone graph object removal until free
  omap3isp: Release the isp device struct by media device callback
  omap3isp: Don't use devm_request_irq()
  media: Add nop implementations of media_device_{init,cleanup}
  media: ipu3-cio2: Call v4l2_device_unregister() earlier
  media: ipu3-cio2: Don't use devm_request_irq()
  media: ipu3-cio2: Release the cio2 device context by media device
    callback
  media: Maintain a list of open file handles in a media device
  media: Implement best effort media device removal safety sans
    refcounting
  media: Document how Media device resources are released

 Documentation/driver-api/media/mc-core.rst    |  12 +-
 drivers/media/cec/core/cec-core.c             |   2 +-
 drivers/media/mc/mc-device.c                  | 279 +++++++++++-------
 drivers/media/mc/mc-devnode.c                 |  94 +++---
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  75 +++--
 drivers/media/platform/ti/omap3isp/isp.c      |  33 ++-
 drivers/media/usb/au0828/au0828-core.c        |   4 +-
 drivers/media/usb/uvc/uvc_driver.c            |   2 +-
 drivers/media/v4l2-core/v4l2-dev.c            |  13 +-
 drivers/staging/media/sunxi/cedrus/cedrus.c   |   2 +-
 include/media/media-device.h                  |  56 +++-
 include/media/media-devnode.h                 |  99 ++++---
 include/media/media-fh.h                      |  32 ++
 13 files changed, 476 insertions(+), 227 deletions(-)
 create mode 100644 include/media/media-fh.h

-- 
2.30.2

[PATCH 01/26] Revert "[media] media: fix media devnode ioctl/syscall and unregister race"

[Sakari Ailus]

This reverts commit 6f0dd24a084a ("[media] media: fix media devnode
ioctl/syscall and unregister race"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c  | 15 +++++++--------
 drivers/media/mc/mc-devnode.c |  8 +-------
 include/media/media-devnode.h | 16 ++--------------
 3 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 25020d58eb06..013d54e1a55a 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -745,7 +745,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 	if (ret < 0) {
 		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
-		media_devnode_unregister_prepare(devnode);
 		media_devnode_unregister(devnode);
 		return ret;
 	}
@@ -802,9 +801,6 @@ void media_device_unregister(struct media_device *mdev)
 		return;
 	}
 
-	/* Clear the devnode register bit to avoid races with media dev open */
-	media_devnode_unregister_prepare(mdev->devnode);
-
 	/* Remove all entities from the media device */
 	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
 		__media_device_unregister_entity(entity);
@@ -829,10 +825,13 @@ void media_device_unregister(struct media_device *mdev)
 
 	dev_dbg(mdev->dev, "Media device unregistered\n");
 
-	device_remove_file(&mdev->devnode->dev, &dev_attr_model);
-	media_devnode_unregister(mdev->devnode);
-	/* devnode free is handled in media_devnode_*() */
-	mdev->devnode = NULL;
+	/* 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);
+		/* devnode free is handled in media_devnode_*() */
+		mdev->devnode = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 680fbb3a9340..0ab33214d243 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -267,7 +267,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	return ret;
 }
 
-void media_devnode_unregister_prepare(struct media_devnode *devnode)
+void media_devnode_unregister(struct media_devnode *devnode)
 {
 	/* Check if devnode was ever registered at all */
 	if (!media_devnode_is_registered(devnode))
@@ -275,12 +275,6 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	mutex_unlock(&media_devnode_lock);
-}
-
-void media_devnode_unregister(struct media_devnode *devnode)
-{
-	mutex_lock(&media_devnode_lock);
 	/* Delete the cdev on this minor as well */
 	cdev_device_del(&devnode->cdev, &devnode->dev);
 	devnode->media_dev = NULL;
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index d27c1c646c28..46f0d3ae44d1 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -115,19 +115,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 					struct media_devnode *devnode,
 					struct module *owner);
 
-/**
- * media_devnode_unregister_prepare - clear the media device node register bit
- * @devnode: the device node to prepare for unregister
- *
- * This clears the passed device register bit. Future open calls will be met
- * with errors. Should be called before media_devnode_unregister() to avoid
- * races with unregister and device file open calls.
- *
- * This function can safely be called if the device node has never been
- * registered or has already been unregistered.
- */
-void media_devnode_unregister_prepare(struct media_devnode *devnode);
-
 /**
  * media_devnode_unregister - unregister a media device node
  * @devnode: the device node to unregister
@@ -135,7 +122,8 @@ void media_devnode_unregister_prepare(struct media_devnode *devnode);
  * This unregisters the passed device. Future open calls will be met with
  * errors.
  *
- * Should be called after media_devnode_unregister_prepare()
+ * 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 *devnode);
 
-- 
2.30.2

[PATCH 02/26] Revert "media: utilize new cdev_device_add helper function"

[Sakari Ailus]

This reverts commit 857313e51006ff51524579bcd8808b70f9a80812. This patch is
temporarily reverted for internal rework.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/cec/core/cec-core.c | 16 ++++++++++++----
 drivers/media/mc/mc-devnode.c     | 21 ++++++++++++++++-----
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c
index af358e901b5f..0eb54aceff13 100644
--- a/drivers/media/cec/core/cec-core.c
+++ b/drivers/media/cec/core/cec-core.c
@@ -137,19 +137,26 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
 
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &cec_devnode_fops);
+	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	devnode->cdev.owner = owner;
 	kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor);
 
 	devnode->registered = true;
-	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
-	if (ret) {
+	ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1);
+	if (ret < 0) {
+		pr_err("%s: cdev_add failed\n", __func__);
 		devnode->registered = false;
-		pr_err("%s: cdev_device_add failed\n", __func__);
 		goto clr_bit;
 	}
 
+	ret = device_add(&devnode->dev);
+	if (ret)
+		goto cdev_del;
+
 	return 0;
 
+cdev_del:
+	cdev_del(&devnode->cdev);
 clr_bit:
 	mutex_lock(&cec_devnode_lock);
 	clear_bit(devnode->minor, cec_devnode_nums);
@@ -193,7 +200,8 @@ static void cec_devnode_unregister(struct cec_adapter *adap)
 	__cec_s_log_addrs(adap, NULL, false);
 	mutex_unlock(&adap->lock);
 
-	cdev_device_del(&devnode->cdev, &devnode->dev);
+	device_del(&devnode->dev);
+	cdev_del(&devnode->cdev);
 	put_device(&devnode->dev);
 }
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 0ab33214d243..740573552e5d 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -240,23 +240,33 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
 	device_initialize(&devnode->dev);
 
-	/* Part 2: Initialize the character device */
+	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
+	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
-	/* Part 3: Add the media and char device */
-	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
+	ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t),
+					     devnode->minor), 1);
 	if (ret < 0) {
-		pr_err("%s: cdev_device_add failed\n", __func__);
+		pr_err("%s: cdev_add failed\n", __func__);
 		goto cdev_add_error;
 	}
 
+	/* Part 3: Add the media device */
+	ret = device_add(&devnode->dev);
+	if (ret < 0) {
+		pr_err("%s: device_add failed\n", __func__);
+		goto device_add_error;
+	}
+
 	/* Part 4: Activate this minor. The char device can now be used. */
 	set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 
 	return 0;
 
+device_add_error:
+	cdev_del(&devnode->cdev);
 cdev_add_error:
 	mutex_lock(&media_devnode_lock);
 	clear_bit(devnode->minor, media_devnode_nums);
@@ -276,9 +286,10 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	/* Delete the cdev on this minor as well */
-	cdev_device_del(&devnode->cdev, &devnode->dev);
+	cdev_del(&devnode->cdev);
 	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
+	device_del(&devnode->dev);
 
 	put_device(&devnode->dev);
 }
-- 
2.30.2

[PATCH 03/26] Revert "[media] media: fix use-after-free in cdev_put() when app exits after driver unbind"

[Sakari Ailus]

This reverts commit 5b28dde51d0c ("[media] media: fix use-after-free in
cdev_put() when app exits after driver unbind"). The commit was part of an
original patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c  |  6 ++---
 drivers/media/mc/mc-devnode.c | 48 ++++++++++++++---------------------
 2 files changed, 21 insertions(+), 33 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 013d54e1a55a..b6640e2c8a4c 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -736,16 +736,16 @@ int __must_check __media_device_register(struct media_device *mdev,
 
 	ret = media_devnode_register(mdev, devnode, owner);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
+		kfree(devnode);
 		return ret;
 	}
 
 	ret = device_create_file(&devnode->dev, &dev_attr_model);
 	if (ret < 0) {
-		/* devnode free is handled in media_devnode_*() */
 		mdev->devnode = NULL;
 		media_devnode_unregister(devnode);
+		kfree(devnode);
 		return ret;
 	}
 
@@ -829,8 +829,6 @@ void media_device_unregister(struct media_device *mdev)
 	if (media_devnode_is_registered(mdev->devnode)) {
 		device_remove_file(&mdev->devnode->dev, &dev_attr_model);
 		media_devnode_unregister(mdev->devnode);
-		/* devnode free is handled in media_devnode_*() */
-		mdev->devnode = NULL;
 	}
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 740573552e5d..1e1792c3ae3f 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -51,8 +51,13 @@ static void media_devnode_release(struct device *cd)
 	struct media_devnode *devnode = to_media_devnode(cd);
 
 	mutex_lock(&media_devnode_lock);
+
+	/* Delete the cdev on this minor as well */
+	cdev_del(&devnode->cdev);
+
 	/* Mark device node number as free */
 	clear_bit(devnode->minor, media_devnode_nums);
+
 	mutex_unlock(&media_devnode_lock);
 
 	/* Release media_devnode and perform other cleanups as needed. */
@@ -60,7 +65,6 @@ static void media_devnode_release(struct device *cd)
 		devnode->release(devnode);
 
 	kfree(devnode);
-	pr_debug("%s: Media Devnode Deallocated\n", __func__);
 }
 
 static struct bus_type media_bus_type = {
@@ -189,8 +193,6 @@ static int media_release(struct inode *inode, struct file *filp)
 	/* decrease the refcount unconditionally since the release()
 	   return value is ignored. */
 	put_device(&devnode->dev);
-
-	pr_debug("%s: Media Release\n", __func__);
 	return 0;
 }
 
@@ -221,7 +223,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	if (minor == MEDIA_NUM_DEVICES) {
 		mutex_unlock(&media_devnode_lock);
 		pr_err("could not get a free minor\n");
-		kfree(devnode);
 		return -ENFILE;
 	}
 
@@ -231,33 +232,29 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	devnode->minor = minor;
 	devnode->media_dev = mdev;
 
-	/* Part 1: Initialize dev now to use dev.kobj for cdev.kobj.parent */
-	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);
-	device_initialize(&devnode->dev);
-
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
-	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
 	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 cdev_add_error;
+		goto error;
 	}
 
-	/* Part 3: Add the media device */
-	ret = device_add(&devnode->dev);
+	/* Part 3: Register the media device */
+	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_add failed\n", __func__);
-		goto device_add_error;
+		pr_err("%s: device_register failed\n", __func__);
+		goto error;
 	}
 
 	/* Part 4: Activate this minor. The char device can now be used. */
@@ -265,15 +262,12 @@ int __must_check media_devnode_register(struct media_device *mdev,
 
 	return 0;
 
-device_add_error:
-	cdev_del(&devnode->cdev);
-cdev_add_error:
+error:
 	mutex_lock(&media_devnode_lock);
+	cdev_del(&devnode->cdev);
 	clear_bit(devnode->minor, media_devnode_nums);
-	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
 
-	put_device(&devnode->dev);
 	return ret;
 }
 
@@ -285,13 +279,9 @@ void media_devnode_unregister(struct media_devnode *devnode)
 
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
-	/* Delete the cdev on this minor as well */
-	cdev_del(&devnode->cdev);
-	devnode->media_dev = NULL;
 	mutex_unlock(&media_devnode_lock);
-	device_del(&devnode->dev);
 
-	put_device(&devnode->dev);
+	device_unregister(&devnode->dev);
 }
 
 /*
-- 
2.30.2

[PATCH 04/26] media: utilize new cdev_device_add helper function

[Sakari Ailus]

From: Logan Gunthorpe <logang@deltatee.com>

Replace the open coded registration of the cdev and dev with the
new device_add_cdev() helper. The helper replaces a common pattern by
taking the proper reference against the parent device and adding both
the cdev and the device.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/cec/core/cec-core.c | 16 ++++------------
 drivers/media/mc/mc-devnode.c     | 23 +++++++++--------------
 2 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c
index 0eb54aceff13..57023d020973 100644
--- a/drivers/media/cec/core/cec-core.c
+++ b/drivers/media/cec/core/cec-core.c
@@ -137,26 +137,19 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode,
 
 	/* Part 2: Initialize and register the character device */
 	cdev_init(&devnode->cdev, &cec_devnode_fops);
-	devnode->cdev.kobj.parent = &devnode->dev.kobj;
 	devnode->cdev.owner = owner;
 	kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor);
 
 	devnode->registered = true;
-	ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1);
-	if (ret < 0) {
-		pr_err("%s: cdev_add failed\n", __func__);
+	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
+	if (ret) {
+		pr_err("%s: cdev_device_add failed\n", __func__);
 		devnode->registered = false;
 		goto clr_bit;
 	}
 
-	ret = device_add(&devnode->dev);
-	if (ret)
-		goto cdev_del;
-
 	return 0;
 
-cdev_del:
-	cdev_del(&devnode->cdev);
 clr_bit:
 	mutex_lock(&cec_devnode_lock);
 	clear_bit(devnode->minor, cec_devnode_nums);
@@ -200,8 +193,7 @@ static void cec_devnode_unregister(struct cec_adapter *adap)
 	__cec_s_log_addrs(adap, NULL, false);
 	mutex_unlock(&adap->lock);
 
-	device_del(&devnode->dev);
-	cdev_del(&devnode->cdev);
+	cdev_device_del(&devnode->cdev, &devnode->dev);
 	put_device(&devnode->dev);
 }
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 1e1792c3ae3f..fabcd646679b 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -232,29 +232,24 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	devnode->minor = minor;
 	devnode->media_dev = mdev;
 
-	/* Part 2: Initialize and register the character device */
+	/* Part 2: Initialize the media and character devices */
 	cdev_init(&devnode->cdev, &media_devnode_fops);
 	devnode->cdev.owner = owner;
 	kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor);
 
-	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 */
 	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);
+	device_initialize(&devnode->dev);
+
+	/* Part 3: Add the media and character devices */
+	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
 	if (ret < 0) {
-		pr_err("%s: device_register failed\n", __func__);
-		goto error;
+		pr_err("%s: cdev_device_add failed\n", __func__);
+		goto cdev_add_error;
 	}
 
 	/* Part 4: Activate this minor. The char device can now be used. */
@@ -262,9 +257,9 @@ int __must_check media_devnode_register(struct media_device *mdev,
 
 	return 0;
 
-error:
+cdev_add_error:
 	mutex_lock(&media_devnode_lock);
-	cdev_del(&devnode->cdev);
+	cdev_device_del(&devnode->cdev, &devnode->dev);
 	clear_bit(devnode->minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
 
-- 
2.30.2

[PATCH 05/26] Revert "media: uvcvideo: Refactor teardown of uvc on USB disconnect"

[Sakari Ailus]

This reverts commit 10e1fdb95809ed21406f53b5b4f064673a1b9ceb.

Temporarily revert this patch to revert a dependent patch. The patch is
re-applied later, rebased on the revert.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 13 ++++---------
 drivers/media/usb/uvc/uvc_status.c | 12 ++++--------
 drivers/media/usb/uvc/uvcvideo.h   |  1 -
 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 11f3d716b5bf..d414b2221dae 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1834,7 +1834,11 @@ static void uvc_delete(struct kref *kref)
 	usb_put_intf(dev->intf);
 	usb_put_dev(dev->udev);
 
+	if (dev->vdev.dev)
+		v4l2_device_unregister(&dev->vdev);
 #ifdef CONFIG_MEDIA_CONTROLLER
+	if (media_devnode_is_registered(dev->mdev.devnode))
+		media_device_unregister(&dev->mdev);
 	media_device_cleanup(&dev->mdev);
 #endif
 
@@ -1891,15 +1895,6 @@ static void uvc_unregister_video(struct uvc_device *dev)
 
 		uvc_debugfs_cleanup_stream(stream);
 	}
-
-	uvc_status_unregister(dev);
-
-	if (dev->vdev.dev)
-		v4l2_device_unregister(&dev->vdev);
-#ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(dev->mdev.devnode))
-		media_device_unregister(&dev->mdev);
-#endif
 }
 
 int uvc_register_video_device(struct uvc_device *dev,
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index a78a88c710e2..015be0886801 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -73,7 +73,7 @@ static int uvc_input_init(struct uvc_device *dev)
 	return ret;
 }
 
-static void uvc_input_unregister(struct uvc_device *dev)
+static void uvc_input_cleanup(struct uvc_device *dev)
 {
 	if (dev->input)
 		input_unregister_device(dev->input);
@@ -90,7 +90,7 @@ static void uvc_input_report_key(struct uvc_device *dev, unsigned int code,
 
 #else
 #define uvc_input_init(dev)
-#define uvc_input_unregister(dev)
+#define uvc_input_cleanup(dev)
 #define uvc_input_report_key(dev, code, value)
 #endif /* CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV */
 
@@ -290,16 +290,12 @@ int uvc_status_init(struct uvc_device *dev)
 	return 0;
 }
 
-void uvc_status_unregister(struct uvc_device *dev)
-{
-	usb_kill_urb(dev->int_urb);
-	uvc_input_unregister(dev);
-}
-
 void uvc_status_cleanup(struct uvc_device *dev)
 {
+	usb_kill_urb(dev->int_urb);
 	usb_free_urb(dev->int_urb);
 	kfree(dev->status);
+	uvc_input_cleanup(dev);
 }
 
 int uvc_status_start(struct uvc_device *dev, gfp_t flags)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9a596c8d894a..80de6f8395c8 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -742,7 +742,6 @@ int uvc_register_video_device(struct uvc_device *dev,
 
 /* Status */
 int uvc_status_init(struct uvc_device *dev);
-void uvc_status_unregister(struct uvc_device *dev);
 void uvc_status_cleanup(struct uvc_device *dev);
 int uvc_status_start(struct uvc_device *dev, gfp_t flags);
 void uvc_status_stop(struct uvc_device *dev);
-- 
2.30.2

[PATCH 06/26] Revert "[media] media-device: dynamically allocate struct media_devnode"

[Sakari Ailus]

This reverts commit a087ce704b80 ("[media] media-device: dynamically
allocate struct media_devnode"). The commit was part of an original
patchset to avoid crashes when an unregistering device is in use.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c                | 44 +++++++--------------
 drivers/media/mc/mc-devnode.c               |  7 +---
 drivers/media/usb/au0828/au0828-core.c      |  4 +-
 drivers/media/usb/uvc/uvc_driver.c          |  2 +-
 drivers/staging/media/sunxi/cedrus/cedrus.c |  2 +-
 include/media/media-device.h                |  5 ++-
 include/media/media-devnode.h               | 15 +------
 7 files changed, 25 insertions(+), 54 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index b6640e2c8a4c..5c2e65717c19 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -439,7 +439,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 = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	const struct media_ioctl_info *info;
 	void __user *arg = (void __user *)__arg;
 	char __karg[256], *karg = __karg;
@@ -523,7 +523,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 = devnode->media_dev;
+	struct media_device *dev = to_media_device(devnode);
 	long ret;
 
 	switch (cmd) {
@@ -559,8 +559,7 @@ static const struct media_file_operations media_device_fops = {
 static ssize_t model_show(struct device *cd,
 			  struct device_attribute *attr, char *buf)
 {
-	struct media_devnode *devnode = to_media_devnode(cd);
-	struct media_device *mdev = devnode->media_dev;
+	struct media_device *mdev = to_media_device(to_media_devnode(cd));
 
 	return sprintf(buf, "%.*s\n", (int)sizeof(mdev->model), mdev->model);
 }
@@ -718,34 +717,23 @@ 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 = devnode;
-	devnode->fops = &media_device_fops;
-	devnode->parent = mdev->dev;
-	devnode->release = media_device_release;
+	mdev->devnode.fops = &media_device_fops;
+	mdev->devnode.parent = mdev->dev;
+	mdev->devnode.release = media_device_release;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
-	ret = media_devnode_register(mdev, devnode, owner);
-	if (ret < 0) {
-		mdev->devnode = NULL;
-		kfree(devnode);
+	ret = media_devnode_register(&mdev->devnode, owner);
+	if (ret < 0)
 		return ret;
-	}
 
-	ret = device_create_file(&devnode->dev, &dev_attr_model);
+	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
 	if (ret < 0) {
-		mdev->devnode = NULL;
-		media_devnode_unregister(devnode);
-		kfree(devnode);
+		media_devnode_unregister(&mdev->devnode);
 		return ret;
 	}
 
@@ -796,7 +784,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;
 	}
@@ -823,13 +811,9 @@ void media_device_unregister(struct media_device *mdev)
 
 	mutex_unlock(&mdev->graph_mutex);
 
-	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);
-	}
+	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
+	dev_dbg(mdev->dev, "Media device unregistering\n");
+	media_devnode_unregister(&mdev->devnode);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index fabcd646679b..ce93ab9be676 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -32,7 +32,6 @@
 #include <linux/uaccess.h>
 
 #include <media/media-devnode.h>
-#include <media/media-device.h>
 
 #define MEDIA_NUM_DEVICES	256
 #define MEDIA_NAME		"media"
@@ -63,8 +62,6 @@ 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 = {
@@ -210,8 +207,7 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
-int __must_check media_devnode_register(struct media_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
 	int minor;
@@ -230,7 +226,6 @@ int __must_check media_devnode_register(struct media_device *mdev,
 	mutex_unlock(&media_devnode_lock);
 
 	devnode->minor = minor;
-	devnode->media_dev = mdev;
 
 	/* Part 2: Initialize the media and character devices */
 	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 877e85a451cb..0876b267568d 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -128,7 +128,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 */
@@ -566,7 +566,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 d414b2221dae..e13b9e012e05 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1837,7 +1837,7 @@ static void uvc_delete(struct kref *kref)
 	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/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index a43d5ff66716..41d3c84becfe 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -547,7 +547,7 @@ static int cedrus_remove(struct platform_device *pdev)
 {
 	struct cedrus_dev *dev = platform_get_drvdata(pdev);
 
-	if (media_devnode_is_registered(dev->mdev.devnode)) {
+	if (media_devnode_is_registered(&dev->mdev.devnode)) {
 		media_device_unregister(&dev->mdev);
 		v4l2_m2m_unregister_media_controller(dev->m2m_dev);
 		media_device_cleanup(&dev->mdev);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 86716ee7cc6c..a33820075aa4 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -145,7 +145,7 @@ struct media_device_ops {
 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];
@@ -191,6 +191,9 @@ 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_device_init() - Initializes a media device element
  *
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 46f0d3ae44d1..1117d1dfd6bf 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -21,8 +21,6 @@
 #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
@@ -73,8 +71,6 @@ struct media_file_operations {
  * before registering the node.
  */
 struct media_devnode {
-	struct media_device *media_dev;
-
 	/* device ops */
 	const struct media_file_operations *fops;
 
@@ -97,8 +93,7 @@ struct media_devnode {
 /**
  * media_devnode_register - register a media device node
  *
- * @mdev: struct media_device we want to register a device node
- * @devnode: media device node structure we want to register
+ * @devnode: struct media_devnode we want to register a device node
  * @owner: should be filled with %THIS_MODULE
  *
  * The registration code assigns minor numbers and registers the new device node
@@ -111,8 +106,7 @@ 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_device *mdev,
-					struct media_devnode *devnode,
+int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
 
 /**
@@ -142,14 +136,9 @@ 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.30.2

[PATCH 07/26] media: uvcvideo: Refactor teardown of uvc on USB disconnect

[Sakari Ailus]

From: Daniel Axtens <dja@axtens.net>

Currently, disconnecting a USB webcam while it is in use prints out a
number of warnings, such as:

WARNING: CPU: 2 PID: 3118 at /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237 sysfs_remove_group+0x8b/0x90
sysfs group ffffffffa7cd0780 not found for kobject 'event13'

This has been noticed before. [0]

This is because of the order in which things are torn down.

If there are no streams active during a USB disconnect:

 - uvc_disconnect() is invoked via device_del() through the bus
   notifier mechanism.

 - this calls uvc_unregister_video().

 - uvc_unregister_video() unregisters the video device for each
   stream,

 - because there are no streams open, it calls uvc_delete()

 - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
   input device.

 - uvc_delete() calls media_device_unregister(), which cleans up the
   media device

 - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all
   return, and we end up back in device_del().

 - device_del() then cleans up the sysfs folder for the camera with
   dpm_sysfs_remove(). Because uvc_status_cleanup() and
   media_device_unregister() have already been called, this all works
   nicely.

If, on the other hand, there *are* streams active during a USB disconnect:

 - uvc_disconnect() is invoked

 - this calls uvc_unregister_video()

 - uvc_unregister_video() unregisters the video device for each
   stream,

 - uvc_unregister_video() and uvc_disconnect() return, and we end up
   back in device_del().

 - device_del() then cleans up the sysfs folder for the camera with
   dpm_sysfs_remove(). Because the status input device and the media
   device are children of the USB device, this also deletes their
   sysfs folders.

 - Sometime later, the final stream is closed, invoking uvc_release().

 - uvc_release() calls uvc_delete()

 - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
   input device. Because the sysfs directory has already been removed,
   this causes a WARNing.

 - uvc_delete() calls media_device_unregister(), which cleans up the
   media device. Because the sysfs directory has already been removed,
   this causes another WARNing.

To fix this, we need to make sure the devices are always unregistered
before the end of uvc_disconnect(). To this, move the unregistration
into the disconnect path:

 - split uvc_status_cleanup() into two parts, one on disconnect that
   unregisters and one on delete that frees.

 - move v4l2_device_unregister() and media_device_unregister() into
   the disconnect path.

[0]: https://lkml.org/lkml/2016/12/8/657

[Renamed uvc_input_cleanup() to uvc_input_unregister()]

Signed-off-by: Daniel Axtens <dja@axtens.net>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
[Sakari Ailus: Rebase on patch Revert "[media] media-device: dynamically allocate struct media_devnode"]
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++----
 drivers/media/usb/uvc/uvc_status.c | 12 ++++++++----
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index e13b9e012e05..5beefbb25fcc 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1834,11 +1834,7 @@ static void uvc_delete(struct kref *kref)
 	usb_put_intf(dev->intf);
 	usb_put_dev(dev->udev);
 
-	if (dev->vdev.dev)
-		v4l2_device_unregister(&dev->vdev);
 #ifdef CONFIG_MEDIA_CONTROLLER
-	if (media_devnode_is_registered(&dev->mdev.devnode))
-		media_device_unregister(&dev->mdev);
 	media_device_cleanup(&dev->mdev);
 #endif
 
@@ -1895,6 +1891,15 @@ static void uvc_unregister_video(struct uvc_device *dev)
 
 		uvc_debugfs_cleanup_stream(stream);
 	}
+
+	uvc_status_unregister(dev);
+
+	if (dev->vdev.dev)
+		v4l2_device_unregister(&dev->vdev);
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (media_devnode_is_registered(&dev->mdev.devnode))
+		media_device_unregister(&dev->mdev);
+#endif
 }
 
 int uvc_register_video_device(struct uvc_device *dev,
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 015be0886801..a78a88c710e2 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -73,7 +73,7 @@ static int uvc_input_init(struct uvc_device *dev)
 	return ret;
 }
 
-static void uvc_input_cleanup(struct uvc_device *dev)
+static void uvc_input_unregister(struct uvc_device *dev)
 {
 	if (dev->input)
 		input_unregister_device(dev->input);
@@ -90,7 +90,7 @@ static void uvc_input_report_key(struct uvc_device *dev, unsigned int code,
 
 #else
 #define uvc_input_init(dev)
-#define uvc_input_cleanup(dev)
+#define uvc_input_unregister(dev)
 #define uvc_input_report_key(dev, code, value)
 #endif /* CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV */
 
@@ -290,12 +290,16 @@ int uvc_status_init(struct uvc_device *dev)
 	return 0;
 }
 
-void uvc_status_cleanup(struct uvc_device *dev)
+void uvc_status_unregister(struct uvc_device *dev)
 {
 	usb_kill_urb(dev->int_urb);
+	uvc_input_unregister(dev);
+}
+
+void uvc_status_cleanup(struct uvc_device *dev)
+{
 	usb_free_urb(dev->int_urb);
 	kfree(dev->status);
-	uvc_input_cleanup(dev);
 }
 
 int uvc_status_start(struct uvc_device *dev, gfp_t flags)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 80de6f8395c8..9a596c8d894a 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -742,6 +742,7 @@ int uvc_register_video_device(struct uvc_device *dev,
 
 /* Status */
 int uvc_status_init(struct uvc_device *dev);
+void uvc_status_unregister(struct uvc_device *dev);
 void uvc_status_cleanup(struct uvc_device *dev);
 int uvc_status_start(struct uvc_device *dev, gfp_t flags);
 void uvc_status_stop(struct uvc_device *dev);
-- 
2.30.2

[PATCH 08/26] media device: Drop nop release callback

[Sakari Ailus]

The release callback is only used to print a debug message. Drop it. (It
will be re-introduced later in a different form.)

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 5c2e65717c19..64cc6921a8bc 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -570,11 +570,6 @@ static DEVICE_ATTR_RO(model);
  * Registration/unregistration
  */
 
-static void media_device_release(struct media_devnode *devnode)
-{
-	dev_dbg(devnode->parent, "Media device released\n");
-}
-
 static void __media_device_unregister_entity(struct media_entity *entity)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
@@ -722,7 +717,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Register the device node. */
 	mdev->devnode.fops = &media_device_fops;
 	mdev->devnode.parent = mdev->dev;
-	mdev->devnode.release = media_device_release;
 
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
-- 
2.30.2

[PATCH 09/26] media: Do not call cdev_device_del() if cdev_device_add() fails

[Sakari Ailus]

cdev_device_del() is the right function to remove a device when
cdev_device_add() succeeds. If it does not, however, put_device() needs to
be used instead. Fix this.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-devnode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index ce93ab9be676..7e22938dfd81 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -254,7 +254,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 
 cdev_add_error:
 	mutex_lock(&media_devnode_lock);
-	cdev_device_del(&devnode->cdev, &devnode->dev);
 	clear_bit(devnode->minor, media_devnode_nums);
 	mutex_unlock(&media_devnode_lock);
 
-- 
2.30.2

[PATCH 10/26] media-device: Delete character device early

[Sakari Ailus]

The parent of the character device related to the media devnode is the media
devnode. Thus the character device needs to be released before the media
devnode's release function. Move it to unregistering of the media devnode,
which mirrors adding the character device in conjunction with registering
the media devnode.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-devnode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 7e22938dfd81..8bc7450ac144 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -51,9 +51,6 @@ 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);
-
 	/* Mark device node number as free */
 	clear_bit(devnode->minor, media_devnode_nums);
 
@@ -270,6 +267,7 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
 
+	cdev_del(&devnode->cdev);
 	device_unregister(&devnode->dev);
 }
 
-- 
2.30.2

[PATCH 11/26] media: Split initialising and adding media devnode

[Sakari Ailus]

As registering a device node of an entity belonging to a media device
will require a reference to the struct device. Taking that reference is
only possible once the device has been initialised, which took place only
when it was registered. Split this in two, and initialise the device when
the media device is allocated.

Don't distribute the effects of these changes yet. Add media_device_get()
and media_device_put() first.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c  | 18 +++++++++++++-----
 drivers/media/mc/mc-devnode.c | 17 ++++++++++-------
 include/media/media-devnode.h | 19 ++++++++++++++-----
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 64cc6921a8bc..d63807e85f1c 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -721,19 +721,26 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
+	media_devnode_init(&mdev->devnode);
+
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		return ret;
+		goto out_put;
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
-	if (ret < 0) {
-		media_devnode_unregister(&mdev->devnode);
-		return ret;
-	}
+	if (ret < 0)
+		goto out_unregister;
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
+
+out_unregister:
+	media_devnode_unregister(&mdev->devnode);
+out_put:
+	put_device(&mdev->devnode.dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
@@ -808,6 +815,7 @@ void media_device_unregister(struct media_device *mdev)
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
+	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 8bc7450ac144..7b17419050fb 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -204,6 +204,11 @@ static const struct file_operations media_devnode_fops = {
 	.llseek = no_llseek,
 };
 
+void media_devnode_init(struct media_devnode *devnode)
+{
+	device_initialize(&devnode->dev);
+}
+
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner)
 {
@@ -235,7 +240,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
 	if (devnode->parent)
 		devnode->dev.parent = devnode->parent;
 	dev_set_name(&devnode->dev, "media%d", devnode->minor);
-	device_initialize(&devnode->dev);
 
 	/* Part 3: Add the media and character devices */
 	ret = cdev_device_add(&devnode->cdev, &devnode->dev);
@@ -267,14 +271,13 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
 
-	cdev_del(&devnode->cdev);
-	device_unregister(&devnode->dev);
+	cdev_device_del(&devnode->cdev, &devnode->dev);
 }
 
 /*
  *	Initialise media for linux
  */
-static int __init media_devnode_init(void)
+static int __init media_devnode_module_init(void)
 {
 	int ret;
 
@@ -296,14 +299,14 @@ static int __init media_devnode_init(void)
 	return 0;
 }
 
-static void __exit media_devnode_exit(void)
+static void __exit media_devnode_module_exit(void)
 {
 	bus_unregister(&media_bus_type);
 	unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
 }
 
-subsys_initcall(media_devnode_init);
-module_exit(media_devnode_exit)
+subsys_initcall(media_devnode_module_init);
+module_exit(media_devnode_module_exit)
 
 MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
 MODULE_DESCRIPTION("Device node registration for media drivers");
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 1117d1dfd6bf..6d46c658be21 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -90,6 +90,17 @@ struct media_devnode {
 /* dev to media_devnode */
 #define to_media_devnode(cd) container_of(cd, struct media_devnode, dev)
 
+/**
+ * media_devnode_init - initialise a media devnode
+ *
+ * @devnode: struct media_devnode we want to initialise
+ *
+ * Initialise a media devnode. Note that after initialising the media
+ * devnode is refcounted. Releasing references to it may be done using
+ * put_device().
+ */
+void media_devnode_init(struct media_devnode *devnode);
+
 /**
  * media_devnode_register - register a media device node
  *
@@ -100,11 +111,9 @@ struct media_devnode {
  * with the kernel. An error is returned if no free minor number can be found,
  * or if the registration of the device node fails.
  *
- * Zero is returned on success.
- *
- * Note that if the media_devnode_register call fails, the release() callback of
- * the media_devnode structure is *not* called, so the caller is responsible for
- * freeing any data.
+ * Zero is returned on success. Note that in case
+ * media_devnode_register() fails, the caller is responsible for
+ * releasing the reference to the device using put_device().
  */
 int __must_check media_devnode_register(struct media_devnode *devnode,
 					struct module *owner);
-- 
2.30.2

[PATCH 12/26] media: Shuffle functions around

[Sakari Ailus]

As the call paths of the functions in question will change, move them
around in anticipation of that. No other changes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/mc/mc-device.c | 56 ++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index d63807e85f1c..c90f79454988 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -677,6 +677,34 @@ void media_device_unregister_entity(struct media_entity *entity)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity);
 
+int __must_check media_device_register_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	list_add_tail(&nptr->list, &mdev->entity_notify);
+	mutex_unlock(&mdev->graph_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
+
+/*
+ * Note: Should be called with mdev->lock held.
+ */
+static void __media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	list_del(&nptr->list);
+}
+
+void media_device_unregister_entity_notify(struct media_device *mdev,
+					struct media_entity_notify *nptr)
+{
+	mutex_lock(&mdev->graph_mutex);
+	__media_device_unregister_entity_notify(mdev, nptr);
+	mutex_unlock(&mdev->graph_mutex);
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
+
 void media_device_init(struct media_device *mdev)
 {
 	INIT_LIST_HEAD(&mdev->entities);
@@ -744,34 +772,6 @@ int __must_check __media_device_register(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
-int __must_check media_device_register_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	list_add_tail(&nptr->list, &mdev->entity_notify);
-	mutex_unlock(&mdev->graph_mutex);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(media_device_register_entity_notify);
-
-/*
- * Note: Should be called with mdev->lock held.
- */
-static void __media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	list_del(&nptr->list);
-}
-
-void media_device_unregister_entity_notify(struct media_device *mdev,
-					struct media_entity_notify *nptr)
-{
-	mutex_lock(&mdev->graph_mutex);
-	__media_device_unregister_entity_notify(mdev, nptr);
-	mutex_unlock(&mdev->graph_mutex);
-}
-EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
-
 void media_device_unregister(struct media_device *mdev)
 {
 	struct media_entity *entity;
-- 
2.30.2

[PATCH 13/26] media device: Initialise media devnode in media_device_init()

[Sakari Ailus]

Call media_devnode_init() from media_device_init(). This has the effect of
creating a struct device for the media_devnode before it is registered,
making it possible to obtain a reference to it for e.g. video devices.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index c90f79454988..f4d880fcd977 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -716,8 +716,8 @@ void media_device_init(struct media_device *mdev)
 	mutex_init(&mdev->req_queue_mutex);
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
-
 	atomic_set(&mdev->request_id, 0);
+	media_devnode_init(&mdev->devnode);
 
 	if (!*mdev->bus_info)
 		media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
@@ -734,6 +734,7 @@ void media_device_cleanup(struct media_device *mdev)
 	media_graph_walk_cleanup(&mdev->pm_count_walk);
 	mutex_destroy(&mdev->graph_mutex);
 	mutex_destroy(&mdev->req_queue_mutex);
+	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
 
@@ -749,26 +750,19 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
-	media_devnode_init(&mdev->devnode);
-
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		goto out_put;
+		return ret;
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
-	if (ret < 0)
-		goto out_unregister;
+	if (ret < 0) {
+		media_devnode_unregister(&mdev->devnode);
+		return ret;
+	}
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
-
-out_unregister:
-	media_devnode_unregister(&mdev->devnode);
-out_put:
-	put_device(&mdev->devnode.dev);
-
-	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
@@ -815,7 +809,6 @@ void media_device_unregister(struct media_device *mdev)
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
-	put_device(&mdev->devnode.dev);
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
-- 
2.30.2

[PATCH 14/26] media device: Refcount the media device

[Sakari Ailus]

As the struct media_device embeds struct media_devnode, the lifetime of
that object must be that same than that of the media_device.

References are obtained by media_device_get() and released by
media_device_put(). In order to use refcounting, the driver must set the
release callback before calling media_device_init() on the media device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c | 43 +++++++++++++++++++++++++++++++-----
 include/media/media-device.h | 28 +++++++++++++++++++++++
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index f4d880fcd977..c13cbdfdbaab 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -705,6 +705,30 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
 }
 EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
 
+static void __media_device_release(struct media_device *mdev)
+{
+	dev_dbg(mdev->dev, "Media device released\n");
+
+	ida_destroy(&mdev->entity_internal_idx);
+	mdev->entity_internal_idx_max = 0;
+	media_graph_walk_cleanup(&mdev->pm_count_walk);
+	mutex_destroy(&mdev->graph_mutex);
+	mutex_destroy(&mdev->req_queue_mutex);
+}
+
+static void media_device_release(struct media_devnode *devnode)
+{
+	struct media_device *mdev = to_media_device(devnode);
+
+	__media_device_release(mdev);
+
+	if (mdev->ops && mdev->ops->release)
+		mdev->ops->release(mdev);
+	else
+		dev_warn(mdev->dev,
+			 "calling media_device_release but no release callback set!\n");
+}
+
 void media_device_init(struct media_device *mdev)
 {
 	INIT_LIST_HEAD(&mdev->entities);
@@ -717,6 +741,17 @@ void media_device_init(struct media_device *mdev)
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
 	atomic_set(&mdev->request_id, 0);
+
+	/*
+	 * Set the release callback to the media device if we have one
+	 * set. Otherwise, the caller is responsible for calling
+	 * media_device_cleanup() in order to release resources
+	 * related to the media device, i.e. the media device is not
+	 * refcounted. This is deprecated.
+	 */
+	if (mdev->ops && mdev->ops->release)
+		mdev->devnode.release = media_device_release;
+
 	media_devnode_init(&mdev->devnode);
 
 	if (!*mdev->bus_info)
@@ -729,12 +764,8 @@ EXPORT_SYMBOL_GPL(media_device_init);
 
 void media_device_cleanup(struct media_device *mdev)
 {
-	ida_destroy(&mdev->entity_internal_idx);
-	mdev->entity_internal_idx_max = 0;
-	media_graph_walk_cleanup(&mdev->pm_count_walk);
-	mutex_destroy(&mdev->graph_mutex);
-	mutex_destroy(&mdev->req_queue_mutex);
-	put_device(&mdev->devnode.dev);
+	__media_device_release(mdev);
+	media_device_put(mdev);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index a33820075aa4..7e8bca6756ba 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -62,6 +62,7 @@ struct media_entity_notify {
  *	       request (and thus the buffer) must be available to the driver.
  *	       And once a buffer is queued, then the driver can complete
  *	       or delete objects from the request before req_queue exits.
+ * @release: Release the resources of the media device.
  */
 struct media_device_ops {
 	int (*link_notify)(struct media_link *link, u32 flags,
@@ -70,6 +71,7 @@ struct media_device_ops {
 	void (*req_free)(struct media_request *req);
 	int (*req_validate)(struct media_request *req);
 	void (*req_queue)(struct media_request *req);
+	void (*release)(struct media_device *mdev);
 };
 
 /**
@@ -219,6 +221,30 @@ struct usb_device;
  */
 void media_device_init(struct media_device *mdev);
 
+/**
+ * media_device_get() - Get a reference to a media device
+ *
+ * @mdev: media device
+ */
+#define media_device_get(mdev)						\
+	do {								\
+		dev_dbg((mdev)->dev, "%s: get media device %s\n",	\
+			__func__, (mdev)->bus_info);			\
+		get_device(&(mdev)->devnode.dev);			\
+	} while (0)
+
+/**
+ * media_device_put() - Put a reference to a media device
+ *
+ * @mdev: media device
+ */
+#define media_device_put(mdev)						\
+	do {								\
+		dev_dbg((mdev)->dev, "%s: put media device %s\n",	\
+			__func__, (mdev)->bus_info);			\
+		put_device(&(mdev)->devnode.dev);			\
+	} while (0)
+
 /**
  * media_device_cleanup() - Cleanups a media device element
  *
@@ -432,6 +458,8 @@ void __media_device_usb_init(struct media_device *mdev,
 			     const char *driver_name);
 
 #else
+#define media_device_get(mdev) do { } while (0)
+#define media_device_put(mdev) do { } while (0)
 static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
-- 
2.30.2

[PATCH 15/26] v4l: Acquire a reference to the media device for every video device

[Sakari Ailus]

The video device depends on the existence of its media device --- if there
is one. Acquire a reference to it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 397d553177fa..6386948eefaa 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -176,6 +176,9 @@ static void v4l2_device_release(struct device *cd)
 {
 	struct video_device *vdev = to_video_device(cd);
 	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_device *mdev = v4l2_dev->mdev;
+#endif
 
 	mutex_lock(&videodev_lock);
 	if (WARN_ON(video_devices[vdev->minor] != vdev)) {
@@ -198,8 +201,8 @@ static void v4l2_device_release(struct device *cd)
 
 	mutex_unlock(&videodev_lock);
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) {
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (mdev && vdev->vfl_dir != VFL_DIR_M2M) {
 		/* Remove interfaces and interface links */
 		media_devnode_remove(vdev->intf_devnode);
 		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
@@ -225,6 +228,11 @@ static void v4l2_device_release(struct device *cd)
 	/* Decrease v4l2_device refcount */
 	if (v4l2_dev)
 		v4l2_device_put(v4l2_dev);
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (mdev)
+		media_device_put(mdev);
+#endif
 }
 
 static struct class video_class = {
@@ -872,6 +880,7 @@ static int video_register_media_controller(struct video_device *vdev)
 
 	/* FIXME: how to create the other interface links? */
 
+	media_device_get(vdev->v4l2_dev->mdev);
 #endif
 	return 0;
 }
-- 
2.30.2

[PATCH 16/26] media-device: Postpone graph object removal until free

[Sakari Ailus]

The media device itself will be unregistered based on it being unbound and
driver's remove callback being called. The graph objects themselves may
still be in use; rely on the media device release callback to release
them.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/mc/mc-device.c | 53 +++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index c13cbdfdbaab..00c44752b4a1 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -707,8 +707,33 @@ EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
 
 static void __media_device_release(struct media_device *mdev)
 {
+	struct media_entity *entity;
+	struct media_entity *next;
+	struct media_interface *intf, *tmp_intf;
+	struct media_entity_notify *notify, *nextp;
+
 	dev_dbg(mdev->dev, "Media device released\n");
 
+	/* Remove all entities from the media device */
+	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
+		__media_device_unregister_entity(entity);
+
+	/* Remove all entity_notify callbacks from the media device */
+	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
+		__media_device_unregister_entity_notify(mdev, notify);
+
+	/* Remove all interfaces from the media device */
+	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
+				 graph_obj.list) {
+		/*
+		 * Unlink the interface, but don't free it here; the
+		 * module which created it is responsible for freeing
+		 * it
+		 */
+		__media_remove_intf_links(intf);
+		media_gobj_destroy(&intf->graph_obj);
+	}
+
 	ida_destroy(&mdev->entity_internal_idx);
 	mdev->entity_internal_idx_max = 0;
 	media_graph_walk_cleanup(&mdev->pm_count_walk);
@@ -799,42 +824,14 @@ EXPORT_SYMBOL_GPL(__media_device_register);
 
 void media_device_unregister(struct media_device *mdev)
 {
-	struct media_entity *entity;
-	struct media_entity *next;
-	struct media_interface *intf, *tmp_intf;
-	struct media_entity_notify *notify, *nextp;
-
 	if (mdev == NULL)
 		return;
 
 	mutex_lock(&mdev->graph_mutex);
-
-	/* Check if mdev was ever registered at all */
 	if (!media_devnode_is_registered(&mdev->devnode)) {
 		mutex_unlock(&mdev->graph_mutex);
 		return;
 	}
-
-	/* Remove all entities from the media device */
-	list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
-		__media_device_unregister_entity(entity);
-
-	/* Remove all entity_notify callbacks from the media device */
-	list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)
-		__media_device_unregister_entity_notify(mdev, notify);
-
-	/* Remove all interfaces from the media device */
-	list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
-				 graph_obj.list) {
-		/*
-		 * Unlink the interface, but don't free it here; the
-		 * module which created it is responsible for freeing
-		 * it
-		 */
-		__media_remove_intf_links(intf);
-		media_gobj_destroy(&intf->graph_obj);
-	}
-
 	mutex_unlock(&mdev->graph_mutex);
 
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
-- 
2.30.2

[PATCH 17/26] omap3isp: Release the isp device struct by media device callback

[Sakari Ailus]

Use the media device release callback to release the isp device's data
structure. This approach has the benefit of not releasing memory which may
still be accessed through open file handles whilst the isp driver is being
unbound.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/ti/omap3isp/isp.c | 25 ++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c
index e7327e38482d..9665f1eb345e 100644
--- a/drivers/media/platform/ti/omap3isp/isp.c
+++ b/drivers/media/platform/ti/omap3isp/isp.c
@@ -649,8 +649,11 @@ static irqreturn_t isp_isr(int irq, void *_isp)
 	return IRQ_HANDLED;
 }
 
+static void isp_release(struct media_device *mdev);
+
 static const struct media_device_ops isp_media_ops = {
 	.link_notify = v4l2_pipeline_link_notify,
+	.release = isp_release,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1607,7 +1610,6 @@ static void isp_unregister_entities(struct isp_device *isp)
 	omap3isp_stat_unregister_entities(&isp->isp_hist);
 
 	v4l2_device_unregister(&isp->v4l2_dev);
-	media_device_cleanup(&isp->media_dev);
 }
 
 static int isp_link_entity(
@@ -1955,6 +1957,19 @@ static void isp_detach_iommu(struct isp_device *isp)
 #endif
 }
 
+static void isp_release(struct media_device *mdev)
+{
+	struct isp_device *isp =
+		container_of(mdev, struct isp_device, media_dev);
+
+	isp_cleanup_modules(isp);
+
+	media_entity_enum_cleanup(&isp->crashed);
+	v4l2_async_notifier_cleanup(&isp->notifier);
+
+	kfree(isp);
+}
+
 static int isp_attach_iommu(struct isp_device *isp)
 {
 #ifdef CONFIG_ARM_DMA_USE_IOMMU
@@ -2003,17 +2018,15 @@ static int isp_remove(struct platform_device *pdev)
 
 	v4l2_async_nf_unregister(&isp->notifier);
 	isp_unregister_entities(isp);
-	isp_cleanup_modules(isp);
+
 	isp_xclk_cleanup(isp);
 
 	__omap3isp_get(isp, false);
 	isp_detach_iommu(isp);
 	__omap3isp_put(isp, false);
 
-	media_entity_enum_cleanup(&isp->crashed);
-	v4l2_async_nf_cleanup(&isp->notifier);
-
-	kfree(isp);
+	/* May release isp immediately */
+	media_device_put(&isp->media_dev);
 
 	return 0;
 }
-- 
2.30.2

[PATCH 18/26] omap3isp: Don't use devm_request_irq()

[Sakari Ailus]

Use request_irq() instead of devm_request_irq(), as a handler set using
devm_request_irq() may still be called once the driver's remove() callback
has been called.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/ti/omap3isp/isp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c
index 9665f1eb345e..904a2c2141f9 100644
--- a/drivers/media/platform/ti/omap3isp/isp.c
+++ b/drivers/media/platform/ti/omap3isp/isp.c
@@ -2024,6 +2024,7 @@ static int isp_remove(struct platform_device *pdev)
 	__omap3isp_get(isp, false);
 	isp_detach_iommu(isp);
 	__omap3isp_put(isp, false);
+	free_irq(isp->irq_num, isp);
 
 	/* May release isp immediately */
 	media_device_put(&isp->media_dev);
@@ -2419,8 +2420,7 @@ static int isp_probe(struct platform_device *pdev)
 	}
 	isp->irq_num = ret;
 
-	if (devm_request_irq(isp->dev, isp->irq_num, isp_isr, IRQF_SHARED,
-			     "OMAP3 ISP", isp)) {
+	if (request_irq(isp->irq_num, isp_isr, IRQF_SHARED, "OMAP3 ISP", isp)) {
 		dev_err(isp->dev, "Unable to request IRQ\n");
 		ret = -EINVAL;
 		goto error_iommu;
@@ -2429,7 +2429,7 @@ static int isp_probe(struct platform_device *pdev)
 	/* Entities */
 	ret = isp_initialize_modules(isp);
 	if (ret < 0)
-		goto error_iommu;
+		goto error_irq;
 
 	ret = isp_register_entities(isp);
 	if (ret < 0)
@@ -2454,6 +2454,8 @@ static int isp_probe(struct platform_device *pdev)
 	isp_unregister_entities(isp);
 error_modules:
 	isp_cleanup_modules(isp);
+error_irq:
+	free_irq(isp->irq_num, isp);
 error_iommu:
 	isp_detach_iommu(isp);
 error_isp:
-- 
2.30.2

[PATCH 19/26] media: Add nop implementations of media_device_{init,cleanup}

[Sakari Ailus]

To support compliation with Media controller disabled, drivers were
required to conditionally call media_device_init and media_device_cleanup.
Add nop implementations of both so drivers don't need to care (or at least
care less).

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/media-device.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/media/media-device.h b/include/media/media-device.h
index 7e8bca6756ba..780440bbb39d 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -460,6 +460,11 @@ void __media_device_usb_init(struct media_device *mdev,
 #else
 #define media_device_get(mdev) do { } while (0)
 #define media_device_put(mdev) do { } while (0)
+
+static inline void media_device_init(struct media_device *mdev)
+{
+}
+
 static inline int media_device_register(struct media_device *mdev)
 {
 	return 0;
@@ -467,6 +472,11 @@ static inline int media_device_register(struct media_device *mdev)
 static inline void media_device_unregister(struct media_device *mdev)
 {
 }
+
+static inline void media_device_cleanup(struct media_device *mdev)
+{
+}
+
 static inline int media_device_register_entity(struct media_device *mdev,
 						struct media_entity *entity)
 {
-- 
2.30.2

[PATCH 20/26] media: ipu3-cio2: Call v4l2_device_unregister() earlier

[Sakari Ailus]

v4l2_device_unregister() unregisters V4L2 sub-device nodes among other
things. Call it before releasing memory and other resources.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 3b76a9d0383a..d1bfcfba112f 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1836,11 +1836,11 @@ static void cio2_pci_remove(struct pci_dev *pci_dev)
 	struct cio2_device *cio2 = pci_get_drvdata(pci_dev);
 
 	media_device_unregister(&cio2->media_dev);
+	v4l2_device_unregister(&cio2->v4l2_dev);
 	v4l2_async_nf_unregister(&cio2->notifier);
 	v4l2_async_nf_cleanup(&cio2->notifier);
 	cio2_queues_exit(cio2);
 	cio2_fbpt_exit_dummy(cio2);
-	v4l2_device_unregister(&cio2->v4l2_dev);
 	media_device_cleanup(&cio2->media_dev);
 	mutex_destroy(&cio2->lock);
 
-- 
2.30.2

[PATCH 21/26] media: ipu3-cio2: Don't use devm_request_irq()

[Sakari Ailus]

Use request_irq() instead of devm_request_irq(), as a handler set using
devm_request_irq() may still be called once the driver's remove() callback
has been called.

Also register the IRQ earlier on.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index d1bfcfba112f..9fdfb2a794db 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1773,6 +1773,12 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	if (r)
 		return r;
 
+	r = request_irq(pci_dev->irq, cio2_irq, IRQF_SHARED, CIO2_NAME, cio2);
+	if (r) {
+		dev_err(dev, "failed to request IRQ (%d)\n", r);
+		goto fail_mutex_destroy;
+	}
+
 	mutex_init(&cio2->lock);
 
 	cio2->media_dev.dev = dev;
@@ -1783,7 +1789,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	media_device_init(&cio2->media_dev);
 	r = media_device_register(&cio2->media_dev);
 	if (r < 0)
-		goto fail_mutex_destroy;
+		goto fail_free_irq;
 
 	cio2->v4l2_dev.mdev = &cio2->media_dev;
 	r = v4l2_device_register(dev, &cio2->v4l2_dev);
@@ -1803,13 +1809,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	if (r)
 		goto fail_clean_notifier;
 
-	r = devm_request_irq(dev, pci_dev->irq, cio2_irq, IRQF_SHARED,
-			     CIO2_NAME, cio2);
-	if (r) {
-		dev_err(dev, "failed to request IRQ (%d)\n", r);
-		goto fail_clean_notifier;
-	}
-
 	pm_runtime_put_noidle(dev);
 	pm_runtime_allow(dev);
 
@@ -1824,6 +1823,8 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 fail_media_device_unregister:
 	media_device_unregister(&cio2->media_dev);
 	media_device_cleanup(&cio2->media_dev);
+fail_free_irq:
+	free_irq(pci_dev->irq, cio2);
 fail_mutex_destroy:
 	mutex_destroy(&cio2->lock);
 	cio2_fbpt_exit_dummy(cio2);
@@ -1837,6 +1838,7 @@ static void cio2_pci_remove(struct pci_dev *pci_dev)
 
 	media_device_unregister(&cio2->media_dev);
 	v4l2_device_unregister(&cio2->v4l2_dev);
+	free_irq(pci_dev->irq, cio2);
 	v4l2_async_nf_unregister(&cio2->notifier);
 	v4l2_async_nf_cleanup(&cio2->notifier);
 	cio2_queues_exit(cio2);
-- 
2.30.2

[PATCH 22/26] media: ipu3-cio2: Release the cio2 device context by media device callback

[Sakari Ailus]

Use the media device release callback to release the cio2 device's data
structure. This approach has the benefit of not releasing memory which may
still be accessed through open file handles whilst the ipu3-cio2 driver is
being unbound.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 55 ++++++++++++++-----
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 9fdfb2a794db..78bef16b2152 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -236,9 +236,15 @@ static int cio2_fbpt_init(struct cio2_device *cio2, struct cio2_queue *q)
 	return 0;
 }
 
-static void cio2_fbpt_exit(struct cio2_queue *q, struct device *dev)
+static int cio2_fbpt_exit(struct cio2_queue *q, struct device *dev)
 {
+	if (!q->fbpt)
+		return -ENOENT;
+
 	dma_free_coherent(dev, CIO2_FBPT_SIZE, q->fbpt, q->fbpt_bus_addr);
+	q->fbpt = NULL;
+
+	return 0;
 }
 
 /**************** CSI2 hardware setup ****************/
@@ -1652,13 +1658,13 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q)
 
 static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q)
 {
-	vb2_video_unregister_device(&q->vdev);
 	media_entity_cleanup(&q->vdev.entity);
 	v4l2_device_unregister_subdev(&q->subdev);
 	media_entity_cleanup(&q->subdev.entity);
-	cio2_fbpt_exit(q, &cio2->pci_dev->dev);
-	mutex_destroy(&q->subdev_lock);
-	mutex_destroy(&q->lock);
+	if (!cio2_fbpt_exit(q, &cio2->pci_dev->dev)) {
+		mutex_destroy(&q->subdev_lock);
+		mutex_destroy(&q->lock);
+	}
 }
 
 static int cio2_queues_init(struct cio2_device *cio2)
@@ -1704,6 +1710,23 @@ static int cio2_check_fwnode_graph(struct fwnode_handle *fwnode)
 	return cio2_check_fwnode_graph(fwnode->secondary);
 }
 
+static void cio2_media_release(struct media_device *mdev)
+{
+	struct cio2_device *cio2 =
+		container_of(mdev, struct cio2_device, media_dev);
+
+	v4l2_async_nf_cleanup(&cio2->notifier);
+	cio2_queues_exit(cio2);
+	cio2_fbpt_exit_dummy(cio2);
+	mutex_destroy(&cio2->lock);
+
+	kfree(cio2);
+}
+
+static struct media_device_ops cio2_mdev_ops = {
+	.release = cio2_media_release,
+};
+
 /**************** PCI interface ****************/
 
 static int cio2_pci_probe(struct pci_dev *pci_dev,
@@ -1731,7 +1754,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 			return r;
 	}
 
-	cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL);
+	cio2 = kzalloc(sizeof(*cio2), GFP_KERNEL);
 	if (!cio2)
 		return -ENOMEM;
 	cio2->pci_dev = pci_dev;
@@ -1782,6 +1805,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	mutex_init(&cio2->lock);
 
 	cio2->media_dev.dev = dev;
+	cio2->media_dev.ops = &cio2_mdev_ops;
 	strscpy(cio2->media_dev.model, CIO2_DEVICE_NAME,
 		sizeof(cio2->media_dev.model));
 	cio2->media_dev.hw_revision = 0;
@@ -1816,15 +1840,18 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 
 fail_clean_notifier:
 	v4l2_async_nf_unregister(&cio2->notifier);
-	v4l2_async_nf_cleanup(&cio2->notifier);
-	cio2_queues_exit(cio2);
+
 fail_v4l2_device_unregister:
 	v4l2_device_unregister(&cio2->v4l2_dev);
+
 fail_media_device_unregister:
 	media_device_unregister(&cio2->media_dev);
-	media_device_cleanup(&cio2->media_dev);
+
 fail_free_irq:
 	free_irq(pci_dev->irq, cio2);
+	media_device_put(&cio2->media_dev);
+	return r;
+
 fail_mutex_destroy:
 	mutex_destroy(&cio2->lock);
 	cio2_fbpt_exit_dummy(cio2);
@@ -1835,19 +1862,19 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 static void cio2_pci_remove(struct pci_dev *pci_dev)
 {
 	struct cio2_device *cio2 = pci_get_drvdata(pci_dev);
+	unsigned int i;
 
 	media_device_unregister(&cio2->media_dev);
+	for (i = 0; i < CIO2_QUEUES; i++)
+		vb2_video_unregister_device(&cio2->queue[i].vdev);
 	v4l2_device_unregister(&cio2->v4l2_dev);
 	free_irq(pci_dev->irq, cio2);
 	v4l2_async_nf_unregister(&cio2->notifier);
-	v4l2_async_nf_cleanup(&cio2->notifier);
-	cio2_queues_exit(cio2);
-	cio2_fbpt_exit_dummy(cio2);
-	media_device_cleanup(&cio2->media_dev);
-	mutex_destroy(&cio2->lock);
 
 	pm_runtime_forbid(&pci_dev->dev);
 	pm_runtime_get_noresume(&pci_dev->dev);
+
+	media_device_put(&cio2->media_dev);
 }
 
 static int __maybe_unused cio2_runtime_suspend(struct device *dev)
-- 
2.30.2

[PATCH 23/26] media: Add per-file-handle data support

[Sakari Ailus]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The media devnode core associates devnodes with files by storing the
devnode pointer in the file structure private_data field. In order to
allow tracking of per-file-handle data introduce a new media devnode
file handle structure that stores the devnode pointer, and store a
pointer to that structure in the file private_data field.

Users of the media devnode code (the only existing user being
media_device) are responsible for managing their own subclass of the
media_devnode_fh structure.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Prepare struct media_device_fh to be used for maintaining file handle list
to avoid shuffling things here and there right after.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c  | 14 +++++++++++++-
 drivers/media/mc/mc-devnode.c | 20 +++++++++-----------
 include/media/media-device.h  |  7 +++++++
 include/media/media-devnode.h | 18 +++++++++++++++++-
 include/media/media-fh.h      | 32 ++++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 13 deletions(-)
 create mode 100644 include/media/media-fh.h

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 00c44752b4a1..0419a7deccc3 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -22,6 +22,7 @@
 #include <media/media-device.h>
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
+#include <media/media-fh.h>
 #include <media/media-request.h>
 
 #ifdef CONFIG_MEDIA_CONTROLLER
@@ -35,7 +36,6 @@
 #define MEDIA_ENT_SUBTYPE_MASK			0x0000ffff
 #define MEDIA_ENT_T_DEVNODE_UNKNOWN		(MEDIA_ENT_F_OLD_BASE | \
 						 MEDIA_ENT_SUBTYPE_MASK)
-
 /* -----------------------------------------------------------------------------
  * Userspace API
  */
@@ -47,11 +47,23 @@ static inline void __user *media_get_uptr(__u64 arg)
 
 static int media_device_open(struct file *filp)
 {
+	struct media_device_fh *fh;
+
+	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
+	if (!fh)
+		return -ENOMEM;
+
+	filp->private_data = &fh->fh;
+
 	return 0;
 }
 
 static int media_device_close(struct file *filp)
 {
+	struct media_device_fh *fh = media_device_fh(filp);
+
+	kfree(fh);
+
 	return 0;
 }
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 7b17419050fb..cb07b3a87bfb 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -140,6 +140,7 @@ static long media_compat_ioctl(struct file *filp, unsigned int cmd,
 static int media_open(struct inode *inode, struct file *filp)
 {
 	struct media_devnode *devnode;
+	struct media_devnode_fh *fh;
 	int ret;
 
 	/* Check if the media device is available. This needs to be done with
@@ -160,17 +161,15 @@ static int media_open(struct inode *inode, struct file *filp)
 	get_device(&devnode->dev);
 	mutex_unlock(&media_devnode_lock);
 
-	filp->private_data = devnode;
-
-	if (devnode->fops->open) {
-		ret = devnode->fops->open(filp);
-		if (ret) {
-			put_device(&devnode->dev);
-			filp->private_data = NULL;
-			return ret;
-		}
+	ret = devnode->fops->open(filp);
+	if (ret) {
+		put_device(&devnode->dev);
+		return ret;
 	}
 
+	fh = filp->private_data;
+	fh->devnode = devnode;
+
 	return 0;
 }
 
@@ -179,8 +178,7 @@ static int media_release(struct inode *inode, struct file *filp)
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (devnode->fops->release)
-		devnode->fops->release(filp);
+	devnode->fops->release(filp);
 
 	filp->private_data = NULL;
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 780440bbb39d..e363b4f3b01d 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -110,6 +110,10 @@ struct media_device_ops {
  *		     other operations that stop or start streaming.
  * @request_id: Used to generate unique request IDs
  *
+ * @fh_list:	List of file handles in the media device
+ *		(struct media_device_fh.mdev_list).
+ * @fh_list_lock: Serialise access to fh_list list.
+ *
  * This structure represents an abstract high-level media device. It allows easy
  * access to entities and provides basic media device-level support. The
  * structure can be allocated directly or embedded in a larger structure.
@@ -182,6 +186,9 @@ struct media_device {
 
 	struct mutex req_queue_mutex;
 	atomic_t request_id;
+
+	struct list_head fh_list;
+	spinlock_t fh_list_lock;
 };
 
 /* We don't need to include usb.h here */
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index 6d46c658be21..f22b3835702e 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -53,6 +53,20 @@ struct media_file_operations {
 	int (*release) (struct file *);
 };
 
+/**
+ * struct media_devnode_fh - Media device node file handle
+ * @devnode:	pointer to the media device node
+ *
+ * This structure serves as a base for per-file-handle data storage. Media
+ * device node users embed media_devnode_fh in their custom file handle data
+ * structures and store the media_devnode_fh in the file private_data in order
+ * to let the media device node core locate the media_devnode corresponding to a
+ * file handle.
+ */
+struct media_devnode_fh {
+	struct media_devnode *devnode;
+};
+
 /**
  * struct media_devnode - Media device node
  * @media_dev:	pointer to struct &media_device
@@ -137,7 +151,9 @@ void media_devnode_unregister(struct media_devnode *devnode);
  */
 static inline struct media_devnode *media_devnode_data(struct file *filp)
 {
-	return filp->private_data;
+	struct media_devnode_fh *fh = filp->private_data;
+
+	return fh->devnode;
 }
 
 /**
diff --git a/include/media/media-fh.h b/include/media/media-fh.h
new file mode 100644
index 000000000000..6f00744b81d6
--- /dev/null
+++ b/include/media/media-fh.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Media device file handle
+ *
+ * Copyright (C) 2019--2023 Intel Corporation
+ */
+
+#ifndef MEDIA_FH_H
+#define MEDIA_FH_H
+
+#include <linux/list.h>
+#include <linux/file.h>
+
+#include <media/media-devnode.h>
+
+/**
+ * struct media_device_fh - File handle specific information on MC
+ *
+ * @fh: The media device file handle
+ * @mdev_list: This file handle in media device's list of file handles
+ */
+struct media_device_fh {
+	struct media_devnode_fh fh;
+	struct list_head mdev_list;
+};
+
+static inline struct media_device_fh *media_device_fh(struct file *filp)
+{
+	return container_of(filp->private_data, struct media_device_fh, fh);
+}
+
+#endif /* MEDIA_FH_H */
-- 
2.30.2

[PATCH 24/26] media: Maintain a list of open file handles in a media device

[Sakari Ailus]

The list of file handles is needed to deliver media events as well as for
other purposes in the future.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c  | 23 ++++++++++++++++++++++-
 drivers/media/mc/mc-devnode.c |  2 +-
 include/media/media-devnode.h |  4 +++-
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 0419a7deccc3..3a1db5fdbba7 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -45,9 +45,11 @@ static inline void __user *media_get_uptr(__u64 arg)
 	return (void __user *)(uintptr_t)arg;
 }
 
-static int media_device_open(struct file *filp)
+static int media_device_open(struct media_devnode *devnode, struct file *filp)
 {
+	struct media_device *mdev = to_media_device(devnode);
 	struct media_device_fh *fh;
+	unsigned long flags;
 
 	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
 	if (!fh)
@@ -55,12 +57,23 @@ static int media_device_open(struct file *filp)
 
 	filp->private_data = &fh->fh;
 
+	spin_lock_irqsave(&mdev->fh_list_lock, flags);
+	list_add(&fh->mdev_list, &mdev->fh_list);
+	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
+
 	return 0;
 }
 
 static int media_device_close(struct file *filp)
 {
+	struct media_devnode *devnode = media_devnode_data(filp);
+	struct media_device *mdev = to_media_device(devnode);
 	struct media_device_fh *fh = media_device_fh(filp);
+	unsigned long flags;
+
+	spin_lock_irqsave(&mdev->fh_list_lock, flags);
+	list_del(&fh->mdev_list);
+	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
 
 	kfree(fh);
 
@@ -773,11 +786,13 @@ void media_device_init(struct media_device *mdev)
 	INIT_LIST_HEAD(&mdev->pads);
 	INIT_LIST_HEAD(&mdev->links);
 	INIT_LIST_HEAD(&mdev->entity_notify);
+	INIT_LIST_HEAD(&mdev->fh_list);
 
 	mutex_init(&mdev->req_queue_mutex);
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
 	atomic_set(&mdev->request_id, 0);
+	spin_lock_init(&mdev->fh_list_lock);
 
 	/*
 	 * Set the release callback to the media device if we have one
@@ -836,6 +851,8 @@ EXPORT_SYMBOL_GPL(__media_device_register);
 
 void media_device_unregister(struct media_device *mdev)
 {
+	unsigned long flags;
+
 	if (mdev == NULL)
 		return;
 
@@ -846,6 +863,10 @@ void media_device_unregister(struct media_device *mdev)
 	}
 	mutex_unlock(&mdev->graph_mutex);
 
+	spin_lock_irqsave(&mdev->fh_list_lock, flags);
+	list_del_init(&mdev->fh_list);
+	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
+
 	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
 	dev_dbg(mdev->dev, "Media device unregistering\n");
 	media_devnode_unregister(&mdev->devnode);
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index cb07b3a87bfb..760314dd22e1 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -161,7 +161,7 @@ static int media_open(struct inode *inode, struct file *filp)
 	get_device(&devnode->dev);
 	mutex_unlock(&media_devnode_lock);
 
-	ret = devnode->fops->open(filp);
+	ret = devnode->fops->open(devnode, filp);
 	if (ret) {
 		put_device(&devnode->dev);
 		return ret;
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index f22b3835702e..d21c13829072 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -21,6 +21,8 @@
 #include <linux/device.h>
 #include <linux/cdev.h>
 
+struct media_devnode;
+
 /*
  * 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
@@ -49,7 +51,7 @@ struct media_file_operations {
 	__poll_t (*poll) (struct file *, struct poll_table_struct *);
 	long (*ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
-	int (*open) (struct file *);
+	int (*open) (struct media_devnode *, struct file *);
 	int (*release) (struct file *);
 };
 
-- 
2.30.2

[PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Add a new helper data structure media_devnode_compat_ref, which is used to
prevent user space from calling IOCTLs or other system calls to the media
device that has been already unregistered.

The media device's memory may of course still be released during the call
but there is only so much that can be done to this without the driver
managing the lifetime of the resources it needs somehow.

This patch should be reverted once all drivers have been converted to manage
their resources' lifetime.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
 drivers/media/mc/mc-devnode.c | 21 ++++++++----
 include/media/media-devnode.h | 29 +++++++++++++++++
 3 files changed, 96 insertions(+), 14 deletions(-)

diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
index 3a1db5fdbba7..22fdaa6370ea 100644
--- a/drivers/media/mc/mc-device.c
+++ b/drivers/media/mc/mc-device.c
@@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
 	return (void __user *)(uintptr_t)arg;
 }
 
+static void compat_ref_release(struct kref *kref)
+{
+	struct media_devnode_compat_ref *ref =
+		container_of_const(kref, struct media_devnode_compat_ref, kref);
+
+	kfree(ref);
+}
+
 static int media_device_open(struct media_devnode *devnode, struct file *filp)
 {
 	struct media_device *mdev = to_media_device(devnode);
 	struct media_device_fh *fh;
 	unsigned long flags;
 
+	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
+			     !kref_get_unless_zero(&devnode->ref->kref)))
+		return -ENXIO;
+
 	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
-	if (!fh)
+	if (!fh) {
+		if (devnode->ref)
+			kref_put(&devnode->ref->kref, compat_ref_release);
 		return -ENOMEM;
+	}
 
-	filp->private_data = &fh->fh;
+	fh->fh.ref = devnode->ref;
 
+	filp->private_data = &fh->fh;
 	spin_lock_irqsave(&mdev->fh_list_lock, flags);
 	list_add(&fh->mdev_list, &mdev->fh_list);
 	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
@@ -71,9 +87,14 @@ static int media_device_close(struct file *filp)
 	struct media_device_fh *fh = media_device_fh(filp);
 	unsigned long flags;
 
-	spin_lock_irqsave(&mdev->fh_list_lock, flags);
-	list_del(&fh->mdev_list);
-	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
+	if (!fh->fh.ref || atomic_read(&fh->fh.ref->registered)) {
+		spin_lock_irqsave(&mdev->fh_list_lock, flags);
+		list_del(&fh->mdev_list);
+		spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
+	}
+
+	if (fh->fh.ref)
+		kref_put(&fh->fh.ref->kref, compat_ref_release);
 
 	kfree(fh);
 
@@ -816,6 +837,8 @@ EXPORT_SYMBOL_GPL(media_device_init);
 
 void media_device_cleanup(struct media_device *mdev)
 {
+	if (mdev->devnode.ref)
+		kref_put(&mdev->devnode.ref->kref, compat_ref_release);
 	__media_device_release(mdev);
 	media_device_put(mdev);
 }
@@ -824,6 +847,7 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
 int __must_check __media_device_register(struct media_device *mdev,
 					 struct module *owner)
 {
+	struct media_devnode_compat_ref *ref = NULL;
 	int ret;
 
 	/* Register the device node. */
@@ -833,19 +857,39 @@ int __must_check __media_device_register(struct media_device *mdev,
 	/* Set version 0 to indicate user-space that the graph is static */
 	mdev->topology_version = 0;
 
+	if (!mdev->ops || !mdev->ops->release) {
+		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
+		if (!ref)
+			return -ENOMEM;
+
+		kref_init(&ref->kref);
+		atomic_set(&ref->registered, 1);
+		mdev->devnode.ref = ref;
+	}
+
 	ret = media_devnode_register(&mdev->devnode, owner);
 	if (ret < 0)
-		return ret;
+		goto err_release_ref;
 
 	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
 	if (ret < 0) {
-		media_devnode_unregister(&mdev->devnode);
-		return ret;
+		goto err_devnode_unregister;
 	}
 
 	dev_dbg(mdev->dev, "Media device registered\n");
 
 	return 0;
+
+err_devnode_unregister:
+	media_devnode_unregister(&mdev->devnode);
+
+err_release_ref:
+	if (ref) {
+		kref_put(&ref->kref, compat_ref_release);
+		mdev->devnode.ref = NULL;
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
 
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
index 760314dd22e1..f2cb3617df02 100644
--- a/drivers/media/mc/mc-devnode.c
+++ b/drivers/media/mc/mc-devnode.c
@@ -65,6 +65,14 @@ static struct bus_type media_bus_type = {
 	.name = MEDIA_NAME,
 };
 
+static bool media_devnode_is_registered_compat(struct media_devnode_fh *fh)
+{
+	if (fh->ref)
+		return atomic_read(&fh->ref->registered);
+
+	return media_devnode_is_registered(fh->devnode);
+}
+
 static ssize_t media_read(struct file *filp, char __user *buf,
 		size_t sz, loff_t *off)
 {
@@ -72,7 +80,7 @@ static ssize_t media_read(struct file *filp, char __user *buf,
 
 	if (!devnode->fops->read)
 		return -EINVAL;
-	if (!media_devnode_is_registered(devnode))
+	if (!media_devnode_is_registered_compat(filp->private_data))
 		return -EIO;
 	return devnode->fops->read(filp, buf, sz, off);
 }
@@ -84,7 +92,7 @@ static ssize_t media_write(struct file *filp, const char __user *buf,
 
 	if (!devnode->fops->write)
 		return -EINVAL;
-	if (!media_devnode_is_registered(devnode))
+	if (!media_devnode_is_registered_compat(filp->private_data))
 		return -EIO;
 	return devnode->fops->write(filp, buf, sz, off);
 }
@@ -94,7 +102,7 @@ static __poll_t media_poll(struct file *filp,
 {
 	struct media_devnode *devnode = media_devnode_data(filp);
 
-	if (!media_devnode_is_registered(devnode))
+	if (!media_devnode_is_registered_compat(filp->private_data))
 		return EPOLLERR | EPOLLHUP;
 	if (!devnode->fops->poll)
 		return DEFAULT_POLLMASK;
@@ -106,12 +114,10 @@ __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 *devnode = media_devnode_data(filp);
-
 	if (!ioctl_func)
 		return -ENOTTY;
 
-	if (!media_devnode_is_registered(devnode))
+	if (!media_devnode_is_registered_compat(filp->private_data))
 		return -EIO;
 
 	return ioctl_func(filp, cmd, arg);
@@ -265,6 +271,9 @@ void media_devnode_unregister(struct media_devnode *devnode)
 	if (!media_devnode_is_registered(devnode))
 		return;
 
+	if (devnode->ref)
+		atomic_set(&devnode->ref->registered, 0);
+
 	mutex_lock(&media_devnode_lock);
 	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
 	mutex_unlock(&media_devnode_lock);
diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
index d21c13829072..9ea55c53e5cb 100644
--- a/include/media/media-devnode.h
+++ b/include/media/media-devnode.h
@@ -20,6 +20,7 @@
 #include <linux/fs.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/kref.h>
 
 struct media_devnode;
 
@@ -55,9 +56,31 @@ struct media_file_operations {
 	int (*release) (struct file *);
 };
 
+/**
+ * struct media_devnode_compat_ref - Workaround for drivers not managing media
+ *				     device lifetime
+ *
+ * The purpose if this struct is to support drivers that do not manage the
+ * lifetime of their respective media devices to avoid the worst effects of
+ * this, namely an IOCTL call on an open file handle to a device that has been
+ * unbound causing a kernel oops systematically. This is not a fix, the proper,
+ * reliable way to handle this is to manage the resources used by the
+ * driver. This struct and its use can be removed once all drivers have been
+ * converted.
+ *
+ * @kref: kref for the memory of this struct
+ * @registered: is this device registered?
+ */
+struct media_devnode_compat_ref {
+	struct kref kref;
+	atomic_t registered;
+};
+
 /**
  * struct media_devnode_fh - Media device node file handle
  * @devnode:	pointer to the media device node
+ * @ref:	media device compat ref, if the driver does not manage media
+ *		device lifetime
  *
  * This structure serves as a base for per-file-handle data storage. Media
  * device node users embed media_devnode_fh in their custom file handle data
@@ -67,6 +90,7 @@ struct media_file_operations {
  */
 struct media_devnode_fh {
 	struct media_devnode *devnode;
+	struct media_devnode_compat_ref *ref;
 };
 
 /**
@@ -80,6 +104,8 @@ struct media_devnode_fh {
  * @flags:	flags, combination of the ``MEDIA_FLAG_*`` constants
  * @release:	release callback called at the end of ``media_devnode_release()``
  *		routine at media-device.c.
+ * @ref:	reference for providing best effort system call safety in device
+ *		removal
  *
  * This structure represents a media-related device node.
  *
@@ -101,6 +127,9 @@ struct media_devnode {
 
 	/* callbacks */
 	void (*release)(struct media_devnode *devnode);
+
+	/* compat reference */
+	struct media_devnode_compat_ref *ref;
 };
 
 /* dev to media_devnode */
-- 
2.30.2

[PATCH 26/26] media: Document how Media device resources are released

[Sakari Ailus]

Document that after unregistering, Media device memory resources are
released by the release() callback rather than by calling
media_device_cleanup().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/driver-api/media/mc-core.rst | 12 ++++++++++--
 include/media/media-device.h               |  6 ++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/media/mc-core.rst b/Documentation/driver-api/media/mc-core.rst
index 400b8ca29367..b5bebef40e56 100644
--- a/Documentation/driver-api/media/mc-core.rst
+++ b/Documentation/driver-api/media/mc-core.rst
@@ -46,8 +46,16 @@ Drivers initialise media device instances by calling
 :c:func:`media_device_init()`. After initialising a media device instance, it is
 registered by calling :c:func:`__media_device_register()` via the macro
 ``media_device_register()`` and unregistered by calling
-:c:func:`media_device_unregister()`. An initialised media device must be
-eventually cleaned up by calling :c:func:`media_device_cleanup()`.
+:c:func:`media_device_unregister()`. The resources of an unregistered media
+device will be released by the ``release()`` callback of :c:type:`media_device`
+ops, which will be called when the last user of the media device has released it
+calling :c:func:`media_device_put()`.
+
+The ``release()`` callback is the way all the resources of the media device are
+released once :c:func:`media_device_init()` has been called. This is also
+relevant during device driver's probe function as the ``release()`` callback
+will also have to be able to safely release the resources related to a partially
+initialised media device.
 
 Note that it is not allowed to unregister a media device instance that was not
 previously registered, or clean up a media device instance that was not
diff --git a/include/media/media-device.h b/include/media/media-device.h
index e363b4f3b01d..ca9a4d7272c0 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -257,8 +257,10 @@ void media_device_init(struct media_device *mdev);
  *
  * @mdev:	pointer to struct &media_device
  *
- * This function that will destroy the graph_mutex that is
- * initialized in media_device_init().
+ * This function that will destroy the graph_mutex that is initialized in
+ * media_device_init(). Note that *only* drivers that do not manage releasing
+ * the memory of th media device itself call this function. This function is
+ * thus effectively DEPRECATED.
  */
 void media_device_cleanup(struct media_device *mdev);
 
-- 
2.30.2

Re: [PATCH 21/26] media: ipu3-cio2: Don't use devm_request_irq()

[Hans Verkuil]

On 01/02/2023 22:45, Sakari Ailus wrote:
> Use request_irq() instead of devm_request_irq(), as a handler set using
> devm_request_irq() may still be called once the driver's remove() callback
> has been called.
> 
> Also register the IRQ earlier on.

Why register it earlier? You do not explain the reason.

Also, does this patch (and also 18/26) belong in this patch series?
It seems more like a normal bug fix and not related to life-time management.

And isn't it the responsibility of the driver to ensure that the irqs are
masked in the remove() callback to prevent the irq from being called?

devm_request_irq() is used a lot in the kernel, so if this is a
common issue, then just fixing it in two drivers isn't going to make
much of a difference.

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> index d1bfcfba112f..9fdfb2a794db 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
> @@ -1773,6 +1773,12 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  	if (r)
>  		return r;
>  
> +	r = request_irq(pci_dev->irq, cio2_irq, IRQF_SHARED, CIO2_NAME, cio2);
> +	if (r) {
> +		dev_err(dev, "failed to request IRQ (%d)\n", r);
> +		goto fail_mutex_destroy;
> +	}
> +
>  	mutex_init(&cio2->lock);
>  
>  	cio2->media_dev.dev = dev;
> @@ -1783,7 +1789,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  	media_device_init(&cio2->media_dev);
>  	r = media_device_register(&cio2->media_dev);
>  	if (r < 0)
> -		goto fail_mutex_destroy;
> +		goto fail_free_irq;
>  
>  	cio2->v4l2_dev.mdev = &cio2->media_dev;
>  	r = v4l2_device_register(dev, &cio2->v4l2_dev);
> @@ -1803,13 +1809,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  	if (r)
>  		goto fail_clean_notifier;
>  
> -	r = devm_request_irq(dev, pci_dev->irq, cio2_irq, IRQF_SHARED,
> -			     CIO2_NAME, cio2);
> -	if (r) {
> -		dev_err(dev, "failed to request IRQ (%d)\n", r);
> -		goto fail_clean_notifier;
> -	}
> -
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_allow(dev);
>  
> @@ -1824,6 +1823,8 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>  fail_media_device_unregister:
>  	media_device_unregister(&cio2->media_dev);
>  	media_device_cleanup(&cio2->media_dev);
> +fail_free_irq:
> +	free_irq(pci_dev->irq, cio2);
>  fail_mutex_destroy:
>  	mutex_destroy(&cio2->lock);
>  	cio2_fbpt_exit_dummy(cio2);
> @@ -1837,6 +1838,7 @@ static void cio2_pci_remove(struct pci_dev *pci_dev)
>  
>  	media_device_unregister(&cio2->media_dev);
>  	v4l2_device_unregister(&cio2->v4l2_dev);
> +	free_irq(pci_dev->irq, cio2);
>  	v4l2_async_nf_unregister(&cio2->notifier);
>  	v4l2_async_nf_cleanup(&cio2->notifier);
>  	cio2_queues_exit(cio2);

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Hans Verkuil]

On 01/02/2023 22:45, Sakari Ailus wrote:
> Add a new helper data structure media_devnode_compat_ref, which is used to
> prevent user space from calling IOCTLs or other system calls to the media
> device that has been already unregistered.
> 
> The media device's memory may of course still be released during the call
> but there is only so much that can be done to this without the driver
> managing the lifetime of the resources it needs somehow.
> 
> This patch should be reverted once all drivers have been converted to manage
> their resources' lifetime.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
>  include/media/media-devnode.h | 29 +++++++++++++++++
>  3 files changed, 96 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index 3a1db5fdbba7..22fdaa6370ea 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
>  	return (void __user *)(uintptr_t)arg;
>  }
>  
> +static void compat_ref_release(struct kref *kref)
> +{
> +	struct media_devnode_compat_ref *ref =
> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> +
> +	kfree(ref);
> +}
> +
>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
>  {
>  	struct media_device *mdev = to_media_device(devnode);
>  	struct media_device_fh *fh;
>  	unsigned long flags;
>  
> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> +			     !kref_get_unless_zero(&devnode->ref->kref)))
> +		return -ENXIO;
> +

This seems pointless: if the media device is unregistered, then the device
node disappears and it can't be opened anymore.

I'm confused by this patch in general: when media_device_unregister() is called,
it is no longer possible to call ioctls and basically do anything except close
the open fh.

So what am I missing here? It all looks odd.

Regards,

	Hans

>  	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> -	if (!fh)
> +	if (!fh) {
> +		if (devnode->ref)
> +			kref_put(&devnode->ref->kref, compat_ref_release);
>  		return -ENOMEM;
> +	}
>  
> -	filp->private_data = &fh->fh;
> +	fh->fh.ref = devnode->ref;
>  
> +	filp->private_data = &fh->fh;
>  	spin_lock_irqsave(&mdev->fh_list_lock, flags);
>  	list_add(&fh->mdev_list, &mdev->fh_list);
>  	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
> @@ -71,9 +87,14 @@ static int media_device_close(struct file *filp)
>  	struct media_device_fh *fh = media_device_fh(filp);
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&mdev->fh_list_lock, flags);
> -	list_del(&fh->mdev_list);
> -	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
> +	if (!fh->fh.ref || atomic_read(&fh->fh.ref->registered)) {
> +		spin_lock_irqsave(&mdev->fh_list_lock, flags);
> +		list_del(&fh->mdev_list);
> +		spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
> +	}
> +
> +	if (fh->fh.ref)
> +		kref_put(&fh->fh.ref->kref, compat_ref_release);
>  
>  	kfree(fh);
>  
> @@ -816,6 +837,8 @@ EXPORT_SYMBOL_GPL(media_device_init);
>  
>  void media_device_cleanup(struct media_device *mdev)
>  {
> +	if (mdev->devnode.ref)
> +		kref_put(&mdev->devnode.ref->kref, compat_ref_release);
>  	__media_device_release(mdev);
>  	media_device_put(mdev);
>  }
> @@ -824,6 +847,7 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
>  int __must_check __media_device_register(struct media_device *mdev,
>  					 struct module *owner)
>  {
> +	struct media_devnode_compat_ref *ref = NULL;
>  	int ret;
>  
>  	/* Register the device node. */
> @@ -833,19 +857,39 @@ int __must_check __media_device_register(struct media_device *mdev,
>  	/* Set version 0 to indicate user-space that the graph is static */
>  	mdev->topology_version = 0;
>  
> +	if (!mdev->ops || !mdev->ops->release) {
> +		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
> +		if (!ref)
> +			return -ENOMEM;
> +
> +		kref_init(&ref->kref);
> +		atomic_set(&ref->registered, 1);
> +		mdev->devnode.ref = ref;
> +	}
> +
>  	ret = media_devnode_register(&mdev->devnode, owner);
>  	if (ret < 0)
> -		return ret;
> +		goto err_release_ref;
>  
>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>  	if (ret < 0) {
> -		media_devnode_unregister(&mdev->devnode);
> -		return ret;
> +		goto err_devnode_unregister;
>  	}
>  
>  	dev_dbg(mdev->dev, "Media device registered\n");
>  
>  	return 0;
> +
> +err_devnode_unregister:
> +	media_devnode_unregister(&mdev->devnode);
> +
> +err_release_ref:
> +	if (ref) {
> +		kref_put(&ref->kref, compat_ref_release);
> +		mdev->devnode.ref = NULL;
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__media_device_register);
>  
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 760314dd22e1..f2cb3617df02 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -65,6 +65,14 @@ static struct bus_type media_bus_type = {
>  	.name = MEDIA_NAME,
>  };
>  
> +static bool media_devnode_is_registered_compat(struct media_devnode_fh *fh)
> +{
> +	if (fh->ref)
> +		return atomic_read(&fh->ref->registered);
> +
> +	return media_devnode_is_registered(fh->devnode);
> +}
> +
>  static ssize_t media_read(struct file *filp, char __user *buf,
>  		size_t sz, loff_t *off)
>  {
> @@ -72,7 +80,7 @@ static ssize_t media_read(struct file *filp, char __user *buf,
>  
>  	if (!devnode->fops->read)
>  		return -EINVAL;
> -	if (!media_devnode_is_registered(devnode))
> +	if (!media_devnode_is_registered_compat(filp->private_data))
>  		return -EIO;
>  	return devnode->fops->read(filp, buf, sz, off);
>  }
> @@ -84,7 +92,7 @@ static ssize_t media_write(struct file *filp, const char __user *buf,
>  
>  	if (!devnode->fops->write)
>  		return -EINVAL;
> -	if (!media_devnode_is_registered(devnode))
> +	if (!media_devnode_is_registered_compat(filp->private_data))
>  		return -EIO;
>  	return devnode->fops->write(filp, buf, sz, off);
>  }
> @@ -94,7 +102,7 @@ static __poll_t media_poll(struct file *filp,
>  {
>  	struct media_devnode *devnode = media_devnode_data(filp);
>  
> -	if (!media_devnode_is_registered(devnode))
> +	if (!media_devnode_is_registered_compat(filp->private_data))
>  		return EPOLLERR | EPOLLHUP;
>  	if (!devnode->fops->poll)
>  		return DEFAULT_POLLMASK;
> @@ -106,12 +114,10 @@ __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 *devnode = media_devnode_data(filp);
> -
>  	if (!ioctl_func)
>  		return -ENOTTY;
>  
> -	if (!media_devnode_is_registered(devnode))
> +	if (!media_devnode_is_registered_compat(filp->private_data))
>  		return -EIO;
>  
>  	return ioctl_func(filp, cmd, arg);
> @@ -265,6 +271,9 @@ void media_devnode_unregister(struct media_devnode *devnode)
>  	if (!media_devnode_is_registered(devnode))
>  		return;
>  
> +	if (devnode->ref)
> +		atomic_set(&devnode->ref->registered, 0);
> +
>  	mutex_lock(&media_devnode_lock);
>  	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>  	mutex_unlock(&media_devnode_lock);
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index d21c13829072..9ea55c53e5cb 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -20,6 +20,7 @@
>  #include <linux/fs.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/kref.h>
>  
>  struct media_devnode;
>  
> @@ -55,9 +56,31 @@ struct media_file_operations {
>  	int (*release) (struct file *);
>  };
>  
> +/**
> + * struct media_devnode_compat_ref - Workaround for drivers not managing media
> + *				     device lifetime
> + *
> + * The purpose if this struct is to support drivers that do not manage the
> + * lifetime of their respective media devices to avoid the worst effects of
> + * this, namely an IOCTL call on an open file handle to a device that has been
> + * unbound causing a kernel oops systematically. This is not a fix, the proper,
> + * reliable way to handle this is to manage the resources used by the
> + * driver. This struct and its use can be removed once all drivers have been
> + * converted.
> + *
> + * @kref: kref for the memory of this struct
> + * @registered: is this device registered?
> + */
> +struct media_devnode_compat_ref {
> +	struct kref kref;
> +	atomic_t registered;
> +};
> +
>  /**
>   * struct media_devnode_fh - Media device node file handle
>   * @devnode:	pointer to the media device node
> + * @ref:	media device compat ref, if the driver does not manage media
> + *		device lifetime
>   *
>   * This structure serves as a base for per-file-handle data storage. Media
>   * device node users embed media_devnode_fh in their custom file handle data
> @@ -67,6 +90,7 @@ struct media_file_operations {
>   */
>  struct media_devnode_fh {
>  	struct media_devnode *devnode;
> +	struct media_devnode_compat_ref *ref;
>  };
>  
>  /**
> @@ -80,6 +104,8 @@ struct media_devnode_fh {
>   * @flags:	flags, combination of the ``MEDIA_FLAG_*`` constants
>   * @release:	release callback called at the end of ``media_devnode_release()``
>   *		routine at media-device.c.
> + * @ref:	reference for providing best effort system call safety in device
> + *		removal
>   *
>   * This structure represents a media-related device node.
>   *
> @@ -101,6 +127,9 @@ struct media_devnode {
>  
>  	/* callbacks */
>  	void (*release)(struct media_devnode *devnode);
> +
> +	/* compat reference */
> +	struct media_devnode_compat_ref *ref;
>  };
>  
>  /* dev to media_devnode */

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Hans Verkuil]

On 03/03/2023 09:39, Hans Verkuil wrote:
> On 01/02/2023 22:45, Sakari Ailus wrote:
>> Add a new helper data structure media_devnode_compat_ref, which is used to
>> prevent user space from calling IOCTLs or other system calls to the media
>> device that has been already unregistered.
>>
>> The media device's memory may of course still be released during the call
>> but there is only so much that can be done to this without the driver
>> managing the lifetime of the resources it needs somehow.
>>
>> This patch should be reverted once all drivers have been converted to manage
>> their resources' lifetime.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
>>  include/media/media-devnode.h | 29 +++++++++++++++++
>>  3 files changed, 96 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>> index 3a1db5fdbba7..22fdaa6370ea 100644
>> --- a/drivers/media/mc/mc-device.c
>> +++ b/drivers/media/mc/mc-device.c
>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
>>  	return (void __user *)(uintptr_t)arg;
>>  }
>>  
>> +static void compat_ref_release(struct kref *kref)
>> +{
>> +	struct media_devnode_compat_ref *ref =
>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
>> +
>> +	kfree(ref);
>> +}
>> +
>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
>>  {
>>  	struct media_device *mdev = to_media_device(devnode);
>>  	struct media_device_fh *fh;
>>  	unsigned long flags;
>>  
>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
>> +		return -ENXIO;
>> +
> 
> This seems pointless: if the media device is unregistered, then the device
> node disappears and it can't be opened anymore.
> 
> I'm confused by this patch in general: when media_device_unregister() is called,
> it is no longer possible to call ioctls and basically do anything except close
> the open fh.
> 
> So what am I missing here? It all looks odd.

I read up on this a bit more, and I think this patch is bogus: drivers not
converted to the release() callback will indeed just crash, but that's no
different than many existing drivers, media or otherwise, when you forcibly
unbind them. It's broken today, and since you have to be root to unbind, I
would say that we can just leave it as-is rather than introducing a rather
ugly workaround. I don't think it will help anyway, since most likely
such drivers will also fails if the application has a video device open
when the device is unbound.

The way to fix it is converting driver to use the new release() callback.

Where this becomes really important is for usb devices which, by their
nature, are hotpluggable. So uvc at least should be converted to this,
I think.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>  	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
>> -	if (!fh)
>> +	if (!fh) {
>> +		if (devnode->ref)
>> +			kref_put(&devnode->ref->kref, compat_ref_release);
>>  		return -ENOMEM;
>> +	}
>>  
>> -	filp->private_data = &fh->fh;
>> +	fh->fh.ref = devnode->ref;
>>  
>> +	filp->private_data = &fh->fh;
>>  	spin_lock_irqsave(&mdev->fh_list_lock, flags);
>>  	list_add(&fh->mdev_list, &mdev->fh_list);
>>  	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
>> @@ -71,9 +87,14 @@ static int media_device_close(struct file *filp)
>>  	struct media_device_fh *fh = media_device_fh(filp);
>>  	unsigned long flags;
>>  
>> -	spin_lock_irqsave(&mdev->fh_list_lock, flags);
>> -	list_del(&fh->mdev_list);
>> -	spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
>> +	if (!fh->fh.ref || atomic_read(&fh->fh.ref->registered)) {
>> +		spin_lock_irqsave(&mdev->fh_list_lock, flags);
>> +		list_del(&fh->mdev_list);
>> +		spin_unlock_irqrestore(&mdev->fh_list_lock, flags);
>> +	}
>> +
>> +	if (fh->fh.ref)
>> +		kref_put(&fh->fh.ref->kref, compat_ref_release);
>>  
>>  	kfree(fh);
>>  
>> @@ -816,6 +837,8 @@ EXPORT_SYMBOL_GPL(media_device_init);
>>  
>>  void media_device_cleanup(struct media_device *mdev)
>>  {
>> +	if (mdev->devnode.ref)
>> +		kref_put(&mdev->devnode.ref->kref, compat_ref_release);
>>  	__media_device_release(mdev);
>>  	media_device_put(mdev);
>>  }
>> @@ -824,6 +847,7 @@ EXPORT_SYMBOL_GPL(media_device_cleanup);
>>  int __must_check __media_device_register(struct media_device *mdev,
>>  					 struct module *owner)
>>  {
>> +	struct media_devnode_compat_ref *ref = NULL;
>>  	int ret;
>>  
>>  	/* Register the device node. */
>> @@ -833,19 +857,39 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  	/* Set version 0 to indicate user-space that the graph is static */
>>  	mdev->topology_version = 0;
>>  
>> +	if (!mdev->ops || !mdev->ops->release) {
>> +		ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL);
>> +		if (!ref)
>> +			return -ENOMEM;
>> +
>> +		kref_init(&ref->kref);
>> +		atomic_set(&ref->registered, 1);
>> +		mdev->devnode.ref = ref;
>> +	}
>> +
>>  	ret = media_devnode_register(&mdev->devnode, owner);
>>  	if (ret < 0)
>> -		return ret;
>> +		goto err_release_ref;
>>  
>>  	ret = device_create_file(&mdev->devnode.dev, &dev_attr_model);
>>  	if (ret < 0) {
>> -		media_devnode_unregister(&mdev->devnode);
>> -		return ret;
>> +		goto err_devnode_unregister;
>>  	}
>>  
>>  	dev_dbg(mdev->dev, "Media device registered\n");
>>  
>>  	return 0;
>> +
>> +err_devnode_unregister:
>> +	media_devnode_unregister(&mdev->devnode);
>> +
>> +err_release_ref:
>> +	if (ref) {
>> +		kref_put(&ref->kref, compat_ref_release);
>> +		mdev->devnode.ref = NULL;
>> +	}
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(__media_device_register);
>>  
>> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
>> index 760314dd22e1..f2cb3617df02 100644
>> --- a/drivers/media/mc/mc-devnode.c
>> +++ b/drivers/media/mc/mc-devnode.c
>> @@ -65,6 +65,14 @@ static struct bus_type media_bus_type = {
>>  	.name = MEDIA_NAME,
>>  };
>>  
>> +static bool media_devnode_is_registered_compat(struct media_devnode_fh *fh)
>> +{
>> +	if (fh->ref)
>> +		return atomic_read(&fh->ref->registered);
>> +
>> +	return media_devnode_is_registered(fh->devnode);
>> +}
>> +
>>  static ssize_t media_read(struct file *filp, char __user *buf,
>>  		size_t sz, loff_t *off)
>>  {
>> @@ -72,7 +80,7 @@ static ssize_t media_read(struct file *filp, char __user *buf,
>>  
>>  	if (!devnode->fops->read)
>>  		return -EINVAL;
>> -	if (!media_devnode_is_registered(devnode))
>> +	if (!media_devnode_is_registered_compat(filp->private_data))
>>  		return -EIO;
>>  	return devnode->fops->read(filp, buf, sz, off);
>>  }
>> @@ -84,7 +92,7 @@ static ssize_t media_write(struct file *filp, const char __user *buf,
>>  
>>  	if (!devnode->fops->write)
>>  		return -EINVAL;
>> -	if (!media_devnode_is_registered(devnode))
>> +	if (!media_devnode_is_registered_compat(filp->private_data))
>>  		return -EIO;
>>  	return devnode->fops->write(filp, buf, sz, off);
>>  }
>> @@ -94,7 +102,7 @@ static __poll_t media_poll(struct file *filp,
>>  {
>>  	struct media_devnode *devnode = media_devnode_data(filp);
>>  
>> -	if (!media_devnode_is_registered(devnode))
>> +	if (!media_devnode_is_registered_compat(filp->private_data))
>>  		return EPOLLERR | EPOLLHUP;
>>  	if (!devnode->fops->poll)
>>  		return DEFAULT_POLLMASK;
>> @@ -106,12 +114,10 @@ __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 *devnode = media_devnode_data(filp);
>> -
>>  	if (!ioctl_func)
>>  		return -ENOTTY;
>>  
>> -	if (!media_devnode_is_registered(devnode))
>> +	if (!media_devnode_is_registered_compat(filp->private_data))
>>  		return -EIO;
>>  
>>  	return ioctl_func(filp, cmd, arg);
>> @@ -265,6 +271,9 @@ void media_devnode_unregister(struct media_devnode *devnode)
>>  	if (!media_devnode_is_registered(devnode))
>>  		return;
>>  
>> +	if (devnode->ref)
>> +		atomic_set(&devnode->ref->registered, 0);
>> +
>>  	mutex_lock(&media_devnode_lock);
>>  	clear_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>>  	mutex_unlock(&media_devnode_lock);
>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>> index d21c13829072..9ea55c53e5cb 100644
>> --- a/include/media/media-devnode.h
>> +++ b/include/media/media-devnode.h
>> @@ -20,6 +20,7 @@
>>  #include <linux/fs.h>
>>  #include <linux/device.h>
>>  #include <linux/cdev.h>
>> +#include <linux/kref.h>
>>  
>>  struct media_devnode;
>>  
>> @@ -55,9 +56,31 @@ struct media_file_operations {
>>  	int (*release) (struct file *);
>>  };
>>  
>> +/**
>> + * struct media_devnode_compat_ref - Workaround for drivers not managing media
>> + *				     device lifetime
>> + *
>> + * The purpose if this struct is to support drivers that do not manage the
>> + * lifetime of their respective media devices to avoid the worst effects of
>> + * this, namely an IOCTL call on an open file handle to a device that has been
>> + * unbound causing a kernel oops systematically. This is not a fix, the proper,
>> + * reliable way to handle this is to manage the resources used by the
>> + * driver. This struct and its use can be removed once all drivers have been
>> + * converted.
>> + *
>> + * @kref: kref for the memory of this struct
>> + * @registered: is this device registered?
>> + */
>> +struct media_devnode_compat_ref {
>> +	struct kref kref;
>> +	atomic_t registered;
>> +};
>> +
>>  /**
>>   * struct media_devnode_fh - Media device node file handle
>>   * @devnode:	pointer to the media device node
>> + * @ref:	media device compat ref, if the driver does not manage media
>> + *		device lifetime
>>   *
>>   * This structure serves as a base for per-file-handle data storage. Media
>>   * device node users embed media_devnode_fh in their custom file handle data
>> @@ -67,6 +90,7 @@ struct media_file_operations {
>>   */
>>  struct media_devnode_fh {
>>  	struct media_devnode *devnode;
>> +	struct media_devnode_compat_ref *ref;
>>  };
>>  
>>  /**
>> @@ -80,6 +104,8 @@ struct media_devnode_fh {
>>   * @flags:	flags, combination of the ``MEDIA_FLAG_*`` constants
>>   * @release:	release callback called at the end of ``media_devnode_release()``
>>   *		routine at media-device.c.
>> + * @ref:	reference for providing best effort system call safety in device
>> + *		removal
>>   *
>>   * This structure represents a media-related device node.
>>   *
>> @@ -101,6 +127,9 @@ struct media_devnode {
>>  
>>  	/* callbacks */
>>  	void (*release)(struct media_devnode *devnode);
>> +
>> +	/* compat reference */
>> +	struct media_devnode_compat_ref *ref;
>>  };
>>  
>>  /* dev to media_devnode */
> 

Re: [PATCH 00/26] Media device lifetime management

[Hans Verkuil]

Hi Sakari,

On 01/02/2023 22:45, Sakari Ailus wrote:
> Hi folks,
> 
> This is a refresh of my 2016 RFC patchset to start addressing object
> lifetime issues in Media controller. It further allows continuing work to
> address lifetime management of media entities.
> 
> The underlying problem is described in detail in v4 of the previous RFC:
> <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>.
> In brief, there is currently no connection between releasing media device
> (and related) memory and IOCTL calls, meaning that there is a time window
> during which released kernel memory can be accessed, and that access can be
> triggered from the user space. The only reason why this is not a grave
> security issue is that it is not triggerable by the user alone but requires
> unbinding a device. That is still not an excuse for not fixing it.
> 
> This set differs from the earlier RFC to address the issue in the
> following respects:
> 
> - Make changes for ipu3-cio2 driver, too.
> 
> - Continue to provide best effort attempt to keep the window between device
>   removal and user space being able to access released memory as small as
>   possible. This means the problem won't become worse for drivers for which
>   Media device lifetime management has not been implemented.
> 
> The latter is achieved by adding a new object, Media devnode compat
> reference, which is allocated, refcounted and eventually released by the
> Media controller framework itself, and where the information on registration
> and open filehandles is maintained. This is only done if the driver does not
> manage the lifetime of the media device itself, i.e. its release operation
> is NULL.
> 
> Due to this, Media device file handles will also be introduced by this
> patchset. I thought the first user of this would be Media device events but
> it seems we already need them here.
> 
> Both ipu3-cio2 and omap3isp drivers are relieved of devm_request_irq() use,
> as device_release() releases the resources before calling the driver's
> remove function. While further work will be required also on these drivers
> to safely stop he hardware at unbind time, I don't see a reason not to merge
> these patches now.
> 
> Some patches are temporarily reverted in order to make reworks easier, then
> applied later on.
> 
> I've tested this on ipu3-cio2 with and without the refcounting patch (media:
> ipu3-cio2: Release the cio2 device context by media device callback),
> including failures in a few parts of the driver initialisation process in
> the MC framework.
> 
> Questions and comments are welcome.

Most of this series looks good.

You can add my:

Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

for patches 1-17, 19, 20 and 22.

As I mentioned in my review of patch 21, I don't think 18 and 21 belong to
this series.

I am also not keen on patch 25 (and thus 23 and 24 that it needs). Perhaps
take that out for now for more discussion later?

I would like to see some more drivers to be converted: specifically uvc
and the test drivers (vidtv, vim2m, vimc, visl, vivid). uvc because it is
widely used, the test drivers because they function as reference code.

Finally, I don't think this series addresses another life-cycle problem:
unbinding subdevices when they are still being used, either by userspace
or a bridge driver.

I have patches for that here:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=subdev-kref

I think these are pretty much independent from this patch series, but
I'll wait with posting them until this is merged.

Background: we have an fpga that implements subdevices and also
(depending on the configuration) complete v4l2 platform drivers.

When the fpga is unloaded when going to standby, subdevices and/or
platform drivers just disappear, and without correct life-time management
you get crashes. Basically exactly the same problem as you have with the
media device.

Regards,

	Hans

> 
> 
> Daniel Axtens (1):
>   media: uvcvideo: Refactor teardown of uvc on USB disconnect
> 
> Laurent Pinchart (1):
>   media: Add per-file-handle data support
> 
> Logan Gunthorpe (1):
>   media: utilize new cdev_device_add helper function
> 
> Sakari Ailus (23):
>   Revert "[media] media: fix media devnode ioctl/syscall and unregister
>     race"
>   Revert "media: utilize new cdev_device_add helper function"
>   Revert "[media] media: fix use-after-free in cdev_put() when app exits
>     after driver unbind"
>   Revert "media: uvcvideo: Refactor teardown of uvc on USB disconnect"
>   Revert "[media] media-device: dynamically allocate struct
>     media_devnode"
>   media device: Drop nop release callback
>   media: Do not call cdev_device_del() if cdev_device_add() fails
>   media-device: Delete character device early
>   media: Split initialising and adding media devnode
>   media: Shuffle functions around
>   media device: Initialise media devnode in media_device_init()
>   media device: Refcount the media device
>   v4l: Acquire a reference to the media device for every video device
>   media-device: Postpone graph object removal until free
>   omap3isp: Release the isp device struct by media device callback
>   omap3isp: Don't use devm_request_irq()
>   media: Add nop implementations of media_device_{init,cleanup}
>   media: ipu3-cio2: Call v4l2_device_unregister() earlier
>   media: ipu3-cio2: Don't use devm_request_irq()
>   media: ipu3-cio2: Release the cio2 device context by media device
>     callback
>   media: Maintain a list of open file handles in a media device
>   media: Implement best effort media device removal safety sans
>     refcounting
>   media: Document how Media device resources are released
> 
>  Documentation/driver-api/media/mc-core.rst    |  12 +-
>  drivers/media/cec/core/cec-core.c             |   2 +-
>  drivers/media/mc/mc-device.c                  | 279 +++++++++++-------
>  drivers/media/mc/mc-devnode.c                 |  94 +++---
>  drivers/media/pci/intel/ipu3/ipu3-cio2-main.c |  75 +++--
>  drivers/media/platform/ti/omap3isp/isp.c      |  33 ++-
>  drivers/media/usb/au0828/au0828-core.c        |   4 +-
>  drivers/media/usb/uvc/uvc_driver.c            |   2 +-
>  drivers/media/v4l2-core/v4l2-dev.c            |  13 +-
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |   2 +-
>  include/media/media-device.h                  |  56 +++-
>  include/media/media-devnode.h                 |  99 ++++---
>  include/media/media-fh.h                      |  32 ++
>  13 files changed, 476 insertions(+), 227 deletions(-)
>  create mode 100644 include/media/media-fh.h
> 

Re: [PATCH 21/26] media: ipu3-cio2: Don't use devm_request_irq()

[Sakari Ailus]

Hi Hans,

Thanks for the review!

On Fri, Mar 03, 2023 at 09:21:03AM +0100, Hans Verkuil wrote:
> On 01/02/2023 22:45, Sakari Ailus wrote:
> > Use request_irq() instead of devm_request_irq(), as a handler set using
> > devm_request_irq() may still be called once the driver's remove() callback
> > has been called.
> > 
> > Also register the IRQ earlier on.
> 
> Why register it earlier? You do not explain the reason.

The device nodes are created before the interrupt handler is registered. It
should happen in the other way around. I'll change tha patch description.

> 
> Also, does this patch (and also 18/26) belong in this patch series?
> It seems more like a normal bug fix and not related to life-time management.
> 
> And isn't it the responsibility of the driver to ensure that the irqs are
> masked in the remove() callback to prevent the irq from being called?
> 
> devm_request_irq() is used a lot in the kernel, so if this is a
> common issue, then just fixing it in two drivers isn't going to make
> much of a difference.

I came to think of this after sending the patch as well. It's memory that
is the problem, any hardware access needs to end before remove is called.

I'll drop the devm removal.

-- 
Kind regrads,

Sakari Ailus

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Hi Hans,

On Fri, Mar 03, 2023 at 09:39:46AM +0100, Hans Verkuil wrote:
> On 01/02/2023 22:45, Sakari Ailus wrote:
> > Add a new helper data structure media_devnode_compat_ref, which is used to
> > prevent user space from calling IOCTLs or other system calls to the media
> > device that has been already unregistered.
> > 
> > The media device's memory may of course still be released during the call
> > but there is only so much that can be done to this without the driver
> > managing the lifetime of the resources it needs somehow.
> > 
> > This patch should be reverted once all drivers have been converted to manage
> > their resources' lifetime.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
> >  drivers/media/mc/mc-devnode.c | 21 ++++++++----
> >  include/media/media-devnode.h | 29 +++++++++++++++++
> >  3 files changed, 96 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index 3a1db5fdbba7..22fdaa6370ea 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
> >  	return (void __user *)(uintptr_t)arg;
> >  }
> >  
> > +static void compat_ref_release(struct kref *kref)
> > +{
> > +	struct media_devnode_compat_ref *ref =
> > +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> > +
> > +	kfree(ref);
> > +}
> > +
> >  static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >  {
> >  	struct media_device *mdev = to_media_device(devnode);
> >  	struct media_device_fh *fh;
> >  	unsigned long flags;
> >  
> > +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> > +			     !kref_get_unless_zero(&devnode->ref->kref)))
> > +		return -ENXIO;
> > +
> 
> This seems pointless: if the media device is unregistered, then the device
> node disappears and it can't be opened anymore.
> 
> I'm confused by this patch in general: when media_device_unregister() is
> called, it is no longer possible to call ioctls and basically do anything
> except close the open fh.

That is true.

However for drivers that don't manage media device lifetime, the devnode
is released right at the time the driver's remove callback is called. This
means that for all the open file handles the private_data is pointing to
released memory.

With this patch, the devnode compat ref will remain as long as any file
handle is open, and the devnode registered status is maintained there.

This certainly is risky but it reduces the time of danger to a very small
moment. Just as it was before this patchset.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Hi Hans,

On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
> On 03/03/2023 09:39, Hans Verkuil wrote:
> > On 01/02/2023 22:45, Sakari Ailus wrote:
> >> Add a new helper data structure media_devnode_compat_ref, which is used to
> >> prevent user space from calling IOCTLs or other system calls to the media
> >> device that has been already unregistered.
> >>
> >> The media device's memory may of course still be released during the call
> >> but there is only so much that can be done to this without the driver
> >> managing the lifetime of the resources it needs somehow.
> >>
> >> This patch should be reverted once all drivers have been converted to manage
> >> their resources' lifetime.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
> >>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
> >>  include/media/media-devnode.h | 29 +++++++++++++++++
> >>  3 files changed, 96 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> >> index 3a1db5fdbba7..22fdaa6370ea 100644
> >> --- a/drivers/media/mc/mc-device.c
> >> +++ b/drivers/media/mc/mc-device.c
> >> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
> >>  	return (void __user *)(uintptr_t)arg;
> >>  }
> >>  
> >> +static void compat_ref_release(struct kref *kref)
> >> +{
> >> +	struct media_devnode_compat_ref *ref =
> >> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> >> +
> >> +	kfree(ref);
> >> +}
> >> +
> >>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >>  {
> >>  	struct media_device *mdev = to_media_device(devnode);
> >>  	struct media_device_fh *fh;
> >>  	unsigned long flags;
> >>  
> >> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> >> +			     !kref_get_unless_zero(&devnode->ref->kref)))
> >> +		return -ENXIO;
> >> +
> > 
> > This seems pointless: if the media device is unregistered, then the device
> > node disappears and it can't be opened anymore.
> > 
> > I'm confused by this patch in general: when media_device_unregister() is called,
> > it is no longer possible to call ioctls and basically do anything except close
> > the open fh.
> > 
> > So what am I missing here? It all looks odd.
> 
> I read up on this a bit more, and I think this patch is bogus: drivers not
> converted to the release() callback will indeed just crash, but that's no
> different than many existing drivers, media or otherwise, when you forcibly
> unbind them. It's broken today, and since you have to be root to unbind, I
> would say that we can just leave it as-is rather than introducing a rather
> ugly workaround. I don't think it will help anyway, since most likely
> such drivers will also fails if the application has a video device open
> when the device is unbound.

The main difference is whether accessing such a file handle will access
released memory always or whether that is possible only during a very brief
amount of time.

-- 
Sakari Ailus

Re: [PATCH 00/26] Media device lifetime management

[Sakari Ailus]

Hi Hans,

Many thanks for the review.

On Fri, Mar 03, 2023 at 10:07:38AM +0100, Hans Verkuil wrote:
> Hi Sakari,
> 
> On 01/02/2023 22:45, Sakari Ailus wrote:
> > Hi folks,
> > 
> > This is a refresh of my 2016 RFC patchset to start addressing object
> > lifetime issues in Media controller. It further allows continuing work to
> > address lifetime management of media entities.
> > 
> > The underlying problem is described in detail in v4 of the previous RFC:
> > <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>.
> > In brief, there is currently no connection between releasing media device
> > (and related) memory and IOCTL calls, meaning that there is a time window
> > during which released kernel memory can be accessed, and that access can be
> > triggered from the user space. The only reason why this is not a grave
> > security issue is that it is not triggerable by the user alone but requires
> > unbinding a device. That is still not an excuse for not fixing it.
> > 
> > This set differs from the earlier RFC to address the issue in the
> > following respects:
> > 
> > - Make changes for ipu3-cio2 driver, too.
> > 
> > - Continue to provide best effort attempt to keep the window between device
> >   removal and user space being able to access released memory as small as
> >   possible. This means the problem won't become worse for drivers for which
> >   Media device lifetime management has not been implemented.
> > 
> > The latter is achieved by adding a new object, Media devnode compat
> > reference, which is allocated, refcounted and eventually released by the
> > Media controller framework itself, and where the information on registration
> > and open filehandles is maintained. This is only done if the driver does not
> > manage the lifetime of the media device itself, i.e. its release operation
> > is NULL.
> > 
> > Due to this, Media device file handles will also be introduced by this
> > patchset. I thought the first user of this would be Media device events but
> > it seems we already need them here.
> > 
> > Both ipu3-cio2 and omap3isp drivers are relieved of devm_request_irq() use,
> > as device_release() releases the resources before calling the driver's
> > remove function. While further work will be required also on these drivers
> > to safely stop he hardware at unbind time, I don't see a reason not to merge
> > these patches now.
> > 
> > Some patches are temporarily reverted in order to make reworks easier, then
> > applied later on.
> > 
> > I've tested this on ipu3-cio2 with and without the refcounting patch (media:
> > ipu3-cio2: Release the cio2 device context by media device callback),
> > including failures in a few parts of the driver initialisation process in
> > the MC framework.
> > 
> > Questions and comments are welcome.
> 
> Most of this series looks good.
> 
> You can add my:
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> for patches 1-17, 19, 20 and 22.

Thank you!

> 
> As I mentioned in my review of patch 21, I don't think 18 and 21 belong to
> this series.

Yes, some patches I wrote as part of this should be merged earlier. I think
I'll just pick them to my master branch once we have rc1.

> 
> I am also not keen on patch 25 (and thus 23 and 24 that it needs). Perhaps
> take that out for now for more discussion later?
> 
> I would like to see some more drivers to be converted: specifically uvc
> and the test drivers (vidtv, vim2m, vimc, visl, vivid). uvc because it is
> widely used, the test drivers because they function as reference code.

Sounds reasonable. Uvc especially should benefit from this. The conversion
isn't even difficult at all, but still requires testing.

> 
> Finally, I don't think this series addresses another life-cycle problem:
> unbinding subdevices when they are still being used, either by userspace
> or a bridge driver.

That is correct. I wanted to address this for the media device first and
tackle other problems once we have these patches in.

> 
> I have patches for that here:
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=subdev-kref
> 
> I think these are pretty much independent from this patch series, but
> I'll wait with posting them until this is merged.

Interesting. I thought in practice addressing the problem for sub-devices
would require addressing media device lifetime management first. In
practice most drivers have one big allocation for everything and that can
be released only once all users are gone.

> 
> Background: we have an fpga that implements subdevices and also
> (depending on the configuration) complete v4l2 platform drivers.
> 
> When the fpga is unloaded when going to standby, subdevices and/or
> platform drivers just disappear, and without correct life-time management
> you get crashes. Basically exactly the same problem as you have with the
> media device.

Yes.

Have you posted the subdev-kref patches to the list yet?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 00/26] Media device lifetime management

[Hans Verkuil]

On 03/03/2023 12:23, Sakari Ailus wrote:
> Hi Hans,
> 
> Many thanks for the review.
> 
> On Fri, Mar 03, 2023 at 10:07:38AM +0100, Hans Verkuil wrote:
>> Hi Sakari,
>>
>> On 01/02/2023 22:45, Sakari Ailus wrote:
>>> Hi folks,
>>>
>>> This is a refresh of my 2016 RFC patchset to start addressing object
>>> lifetime issues in Media controller. It further allows continuing work to
>>> address lifetime management of media entities.
>>>
>>> The underlying problem is described in detail in v4 of the previous RFC:
>>> <URL:https://lore.kernel.org/linux-media/20161108135438.GO3217@valkosipuli.retiisi.org.uk/>.
>>> In brief, there is currently no connection between releasing media device
>>> (and related) memory and IOCTL calls, meaning that there is a time window
>>> during which released kernel memory can be accessed, and that access can be
>>> triggered from the user space. The only reason why this is not a grave
>>> security issue is that it is not triggerable by the user alone but requires
>>> unbinding a device. That is still not an excuse for not fixing it.
>>>
>>> This set differs from the earlier RFC to address the issue in the
>>> following respects:
>>>
>>> - Make changes for ipu3-cio2 driver, too.
>>>
>>> - Continue to provide best effort attempt to keep the window between device
>>>   removal and user space being able to access released memory as small as
>>>   possible. This means the problem won't become worse for drivers for which
>>>   Media device lifetime management has not been implemented.
>>>
>>> The latter is achieved by adding a new object, Media devnode compat
>>> reference, which is allocated, refcounted and eventually released by the
>>> Media controller framework itself, and where the information on registration
>>> and open filehandles is maintained. This is only done if the driver does not
>>> manage the lifetime of the media device itself, i.e. its release operation
>>> is NULL.
>>>
>>> Due to this, Media device file handles will also be introduced by this
>>> patchset. I thought the first user of this would be Media device events but
>>> it seems we already need them here.
>>>
>>> Both ipu3-cio2 and omap3isp drivers are relieved of devm_request_irq() use,
>>> as device_release() releases the resources before calling the driver's
>>> remove function. While further work will be required also on these drivers
>>> to safely stop he hardware at unbind time, I don't see a reason not to merge
>>> these patches now.
>>>
>>> Some patches are temporarily reverted in order to make reworks easier, then
>>> applied later on.
>>>
>>> I've tested this on ipu3-cio2 with and without the refcounting patch (media:
>>> ipu3-cio2: Release the cio2 device context by media device callback),
>>> including failures in a few parts of the driver initialisation process in
>>> the MC framework.
>>>
>>> Questions and comments are welcome.
>>
>> Most of this series looks good.
>>
>> You can add my:
>>
>> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> for patches 1-17, 19, 20 and 22.
> 
> Thank you!
> 
>>
>> As I mentioned in my review of patch 21, I don't think 18 and 21 belong to
>> this series.
> 
> Yes, some patches I wrote as part of this should be merged earlier. I think
> I'll just pick them to my master branch once we have rc1.
> 
>>
>> I am also not keen on patch 25 (and thus 23 and 24 that it needs). Perhaps
>> take that out for now for more discussion later?
>>
>> I would like to see some more drivers to be converted: specifically uvc
>> and the test drivers (vidtv, vim2m, vimc, visl, vivid). uvc because it is
>> widely used, the test drivers because they function as reference code.
> 
> Sounds reasonable. Uvc especially should benefit from this. The conversion
> isn't even difficult at all, but still requires testing.
> 
>>
>> Finally, I don't think this series addresses another life-cycle problem:
>> unbinding subdevices when they are still being used, either by userspace
>> or a bridge driver.
> 
> That is correct. I wanted to address this for the media device first and
> tackle other problems once we have these patches in.
> 
>>
>> I have patches for that here:
>>
>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=subdev-kref
>>
>> I think these are pretty much independent from this patch series, but
>> I'll wait with posting them until this is merged.
> 
> Interesting. I thought in practice addressing the problem for sub-devices
> would require addressing media device lifetime management first. In
> practice most drivers have one big allocation for everything and that can
> be released only once all users are gone.
> 
>>
>> Background: we have an fpga that implements subdevices and also
>> (depending on the configuration) complete v4l2 platform drivers.
>>
>> When the fpga is unloaded when going to standby, subdevices and/or
>> platform drivers just disappear, and without correct life-time management
>> you get crashes. Basically exactly the same problem as you have with the
>> media device.
> 
> Yes.
> 
> Have you posted the subdev-kref patches to the list yet?
> 

No, I don't believe I did. I've been sitting on them waiting for this series,
basically.

But we (Cisco) have been using these patches for some time now. But that's
on a really old kernel :-(

I also need to double-check vimc as well: I have a memory that I had to make
changes there, but I will have to retest it.

Regards,

	Hans

Re: [PATCH 00/26] Media device lifetime management

[Sakari Ailus]

Hi Hans,

On Fri, Mar 03, 2023 at 01:23:52PM +0200, Sakari Ailus wrote:
> > As I mentioned in my review of patch 21, I don't think 18 and 21 belong to
> > this series.
> 
> Yes, some patches I wrote as part of this should be merged earlier. I think
> I'll just pick them to my master branch once we have rc1.

I originally thought that you meant "media: Add nop implementations of
media_device_{init,cleanup}". The omap3isp devm_request_irq() patch can be
dropped and the ipu3-cio2 requivalent changed as I discussed in the other
e-mail.

-- 
Sakari Ailus

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Hans Verkuil]

On 03/03/2023 12:08, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
>> On 03/03/2023 09:39, Hans Verkuil wrote:
>>> On 01/02/2023 22:45, Sakari Ailus wrote:
>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
>>>> prevent user space from calling IOCTLs or other system calls to the media
>>>> device that has been already unregistered.
>>>>
>>>> The media device's memory may of course still be released during the call
>>>> but there is only so much that can be done to this without the driver
>>>> managing the lifetime of the resources it needs somehow.
>>>>
>>>> This patch should be reverted once all drivers have been converted to manage
>>>> their resources' lifetime.
>>>>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
>>>> --- a/drivers/media/mc/mc-device.c
>>>> +++ b/drivers/media/mc/mc-device.c
>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
>>>>  	return (void __user *)(uintptr_t)arg;
>>>>  }
>>>>  
>>>> +static void compat_ref_release(struct kref *kref)
>>>> +{
>>>> +	struct media_devnode_compat_ref *ref =
>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
>>>> +
>>>> +	kfree(ref);
>>>> +}
>>>> +
>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
>>>>  {
>>>>  	struct media_device *mdev = to_media_device(devnode);
>>>>  	struct media_device_fh *fh;
>>>>  	unsigned long flags;
>>>>  
>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
>>>> +		return -ENXIO;
>>>> +
>>>
>>> This seems pointless: if the media device is unregistered, then the device
>>> node disappears and it can't be opened anymore.
>>>
>>> I'm confused by this patch in general: when media_device_unregister() is called,
>>> it is no longer possible to call ioctls and basically do anything except close
>>> the open fh.
>>>
>>> So what am I missing here? It all looks odd.
>>
>> I read up on this a bit more, and I think this patch is bogus: drivers not
>> converted to the release() callback will indeed just crash, but that's no
>> different than many existing drivers, media or otherwise, when you forcibly
>> unbind them. It's broken today, and since you have to be root to unbind, I
>> would say that we can just leave it as-is rather than introducing a rather
>> ugly workaround. I don't think it will help anyway, since most likely
>> such drivers will also fails if the application has a video device open
>> when the device is unbound.
> 
> The main difference is whether accessing such a file handle will access
> released memory always or whether that is possible only during a very brief
> amount of time.
> 

I still don't like this. It was broken before, and it is broken now (perhaps a
bit less broken, but still...).

There is a right fix now, and drivers that are likely to be removed forcibly
should be converted. This patch just makes it more likely that such drivers
won't be converted since it is less likely to hit this, so people will just
think that this is 'good enough'. And it makes the code a lot uglier.

Note that if we still want this in, then it needs a lot more comments explaining
what is going on.

Regards,

	Hans

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Hi Hans,

On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
> On 03/03/2023 12:08, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
> >> On 03/03/2023 09:39, Hans Verkuil wrote:
> >>> On 01/02/2023 22:45, Sakari Ailus wrote:
> >>>> Add a new helper data structure media_devnode_compat_ref, which is used to
> >>>> prevent user space from calling IOCTLs or other system calls to the media
> >>>> device that has been already unregistered.
> >>>>
> >>>> The media device's memory may of course still be released during the call
> >>>> but there is only so much that can be done to this without the driver
> >>>> managing the lifetime of the resources it needs somehow.
> >>>>
> >>>> This patch should be reverted once all drivers have been converted to manage
> >>>> their resources' lifetime.
> >>>>
> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>> ---
> >>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
> >>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
> >>>>  include/media/media-devnode.h | 29 +++++++++++++++++
> >>>>  3 files changed, 96 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> >>>> index 3a1db5fdbba7..22fdaa6370ea 100644
> >>>> --- a/drivers/media/mc/mc-device.c
> >>>> +++ b/drivers/media/mc/mc-device.c
> >>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
> >>>>  	return (void __user *)(uintptr_t)arg;
> >>>>  }
> >>>>  
> >>>> +static void compat_ref_release(struct kref *kref)
> >>>> +{
> >>>> +	struct media_devnode_compat_ref *ref =
> >>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> >>>> +
> >>>> +	kfree(ref);
> >>>> +}
> >>>> +
> >>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >>>>  {
> >>>>  	struct media_device *mdev = to_media_device(devnode);
> >>>>  	struct media_device_fh *fh;
> >>>>  	unsigned long flags;
> >>>>  
> >>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> >>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
> >>>> +		return -ENXIO;
> >>>> +
> >>>
> >>> This seems pointless: if the media device is unregistered, then the device
> >>> node disappears and it can't be opened anymore.
> >>>
> >>> I'm confused by this patch in general: when media_device_unregister() is called,
> >>> it is no longer possible to call ioctls and basically do anything except close
> >>> the open fh.
> >>>
> >>> So what am I missing here? It all looks odd.
> >>
> >> I read up on this a bit more, and I think this patch is bogus: drivers not
> >> converted to the release() callback will indeed just crash, but that's no
> >> different than many existing drivers, media or otherwise, when you forcibly
> >> unbind them. It's broken today, and since you have to be root to unbind, I
> >> would say that we can just leave it as-is rather than introducing a rather
> >> ugly workaround. I don't think it will help anyway, since most likely
> >> such drivers will also fails if the application has a video device open
> >> when the device is unbound.
> > 
> > The main difference is whether accessing such a file handle will access
> > released memory always or whether that is possible only during a very brief
> > amount of time.
> > 
> 
> I still don't like this. It was broken before, and it is broken now (perhaps a
> bit less broken, but still...).
> 
> There is a right fix now, and drivers that are likely to be removed forcibly
> should be converted. This patch just makes it more likely that such drivers
> won't be converted since it is less likely to hit this, so people will just
> think that this is 'good enough'. And it makes the code a lot uglier.

I agree, although converting the drivers is easier said than done. Note
that also DVB is affected by this, not just V4L2. There are quite a few DVB
USB devices.

The behaviour before this set (since ~ 2017) is restored by the last few
patches, without these we're on pre-2017 behaviour which basically means
that all media IOCTLs on file handles the device of which is gone, will
always access released memory. That was quite bad, too.

Back then Mauro objected merging the set due to lack of DVB support, and as
far as I recall, also for not including best effort attempt to avoid freed
memory accesses.

I have experimental DVB support patches (from ~ 2017) that do not actually
work and I won't have time to fix them.

Cc Mauro.

> 
> Note that if we still want this in, then it needs a lot more comments explaining
> what is going on.

I'm fine with adding that.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Hans Verkuil]

On 13/03/2023 15:02, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
>> On 03/03/2023 12:08, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
>>>> On 03/03/2023 09:39, Hans Verkuil wrote:
>>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
>>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
>>>>>> prevent user space from calling IOCTLs or other system calls to the media
>>>>>> device that has been already unregistered.
>>>>>>
>>>>>> The media device's memory may of course still be released during the call
>>>>>> but there is only so much that can be done to this without the driver
>>>>>> managing the lifetime of the resources it needs somehow.
>>>>>>
>>>>>> This patch should be reverted once all drivers have been converted to manage
>>>>>> their resources' lifetime.
>>>>>>
>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
>>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
>>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
>>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
>>>>>> --- a/drivers/media/mc/mc-device.c
>>>>>> +++ b/drivers/media/mc/mc-device.c
>>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
>>>>>>  	return (void __user *)(uintptr_t)arg;
>>>>>>  }
>>>>>>  
>>>>>> +static void compat_ref_release(struct kref *kref)
>>>>>> +{
>>>>>> +	struct media_devnode_compat_ref *ref =
>>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
>>>>>> +
>>>>>> +	kfree(ref);
>>>>>> +}
>>>>>> +
>>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
>>>>>>  {
>>>>>>  	struct media_device *mdev = to_media_device(devnode);
>>>>>>  	struct media_device_fh *fh;
>>>>>>  	unsigned long flags;
>>>>>>  
>>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
>>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
>>>>>> +		return -ENXIO;
>>>>>> +
>>>>>
>>>>> This seems pointless: if the media device is unregistered, then the device
>>>>> node disappears and it can't be opened anymore.
>>>>>
>>>>> I'm confused by this patch in general: when media_device_unregister() is called,
>>>>> it is no longer possible to call ioctls and basically do anything except close
>>>>> the open fh.
>>>>>
>>>>> So what am I missing here? It all looks odd.
>>>>
>>>> I read up on this a bit more, and I think this patch is bogus: drivers not
>>>> converted to the release() callback will indeed just crash, but that's no
>>>> different than many existing drivers, media or otherwise, when you forcibly
>>>> unbind them. It's broken today, and since you have to be root to unbind, I
>>>> would say that we can just leave it as-is rather than introducing a rather
>>>> ugly workaround. I don't think it will help anyway, since most likely
>>>> such drivers will also fails if the application has a video device open
>>>> when the device is unbound.
>>>
>>> The main difference is whether accessing such a file handle will access
>>> released memory always or whether that is possible only during a very brief
>>> amount of time.
>>>
>>
>> I still don't like this. It was broken before, and it is broken now (perhaps a
>> bit less broken, but still...).
>>
>> There is a right fix now, and drivers that are likely to be removed forcibly
>> should be converted. This patch just makes it more likely that such drivers
>> won't be converted since it is less likely to hit this, so people will just
>> think that this is 'good enough'. And it makes the code a lot uglier.
> 
> I agree, although converting the drivers is easier said than done. Note
> that also DVB is affected by this, not just V4L2. There are quite a few DVB
> USB devices.
> 
> The behaviour before this set (since ~ 2017) is restored by the last few
> patches, without these we're on pre-2017 behaviour which basically means
> that all media IOCTLs on file handles the device of which is gone, will
> always access released memory. That was quite bad, too.

Why?

I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
unregister the media device, which will cause all file ops on the filehandle
to return immediately, except for close().

And close() just frees the devnode from what I can see.

There is a race if the device is unbound while in an ioctl, then all bets are
off without proper life-time management.

If it crashes after an unbind in the close() call, then something else is
wrong, it shouldn't do that.

What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?

I feel that I am missing something here.

Regards,

	Hans

> 
> Back then Mauro objected merging the set due to lack of DVB support, and as
> far as I recall, also for not including best effort attempt to avoid freed
> memory accesses.
> 
> I have experimental DVB support patches (from ~ 2017) that do not actually
> work and I won't have time to fix them.
> 
> Cc Mauro.
> 
>>
>> Note that if we still want this in, then it needs a lot more comments explaining
>> what is going on.
> 
> I'm fine with adding that.
> 

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Hi Hans,

On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
> On 13/03/2023 15:02, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
> >> On 03/03/2023 12:08, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
> >>>> On 03/03/2023 09:39, Hans Verkuil wrote:
> >>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
> >>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
> >>>>>> prevent user space from calling IOCTLs or other system calls to the media
> >>>>>> device that has been already unregistered.
> >>>>>>
> >>>>>> The media device's memory may of course still be released during the call
> >>>>>> but there is only so much that can be done to this without the driver
> >>>>>> managing the lifetime of the resources it needs somehow.
> >>>>>>
> >>>>>> This patch should be reverted once all drivers have been converted to manage
> >>>>>> their resources' lifetime.
> >>>>>>
> >>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>> ---
> >>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
> >>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
> >>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
> >>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> >>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
> >>>>>> --- a/drivers/media/mc/mc-device.c
> >>>>>> +++ b/drivers/media/mc/mc-device.c
> >>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
> >>>>>>  	return (void __user *)(uintptr_t)arg;
> >>>>>>  }
> >>>>>>  
> >>>>>> +static void compat_ref_release(struct kref *kref)
> >>>>>> +{
> >>>>>> +	struct media_devnode_compat_ref *ref =
> >>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> >>>>>> +
> >>>>>> +	kfree(ref);
> >>>>>> +}
> >>>>>> +
> >>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >>>>>>  {
> >>>>>>  	struct media_device *mdev = to_media_device(devnode);
> >>>>>>  	struct media_device_fh *fh;
> >>>>>>  	unsigned long flags;
> >>>>>>  
> >>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> >>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
> >>>>>> +		return -ENXIO;
> >>>>>> +
> >>>>>
> >>>>> This seems pointless: if the media device is unregistered, then the device
> >>>>> node disappears and it can't be opened anymore.
> >>>>>
> >>>>> I'm confused by this patch in general: when media_device_unregister() is called,
> >>>>> it is no longer possible to call ioctls and basically do anything except close
> >>>>> the open fh.
> >>>>>
> >>>>> So what am I missing here? It all looks odd.
> >>>>
> >>>> I read up on this a bit more, and I think this patch is bogus: drivers not
> >>>> converted to the release() callback will indeed just crash, but that's no
> >>>> different than many existing drivers, media or otherwise, when you forcibly
> >>>> unbind them. It's broken today, and since you have to be root to unbind, I
> >>>> would say that we can just leave it as-is rather than introducing a rather
> >>>> ugly workaround. I don't think it will help anyway, since most likely
> >>>> such drivers will also fails if the application has a video device open
> >>>> when the device is unbound.
> >>>
> >>> The main difference is whether accessing such a file handle will access
> >>> released memory always or whether that is possible only during a very brief
> >>> amount of time.
> >>>
> >>
> >> I still don't like this. It was broken before, and it is broken now (perhaps a
> >> bit less broken, but still...).
> >>
> >> There is a right fix now, and drivers that are likely to be removed forcibly
> >> should be converted. This patch just makes it more likely that such drivers
> >> won't be converted since it is less likely to hit this, so people will just
> >> think that this is 'good enough'. And it makes the code a lot uglier.
> > 
> > I agree, although converting the drivers is easier said than done. Note
> > that also DVB is affected by this, not just V4L2. There are quite a few DVB
> > USB devices.
> > 
> > The behaviour before this set (since ~ 2017) is restored by the last few
> > patches, without these we're on pre-2017 behaviour which basically means
> > that all media IOCTLs on file handles the device of which is gone, will
> > always access released memory. That was quite bad, too.
> 
> Why?
> 
> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
> unregister the media device, which will cause all file ops on the filehandle
> to return immediately, except for close().
> 
> And close() just frees the devnode from what I can see.
> 
> There is a race if the device is unbound while in an ioctl, then all bets are
> off without proper life-time management.
> 
> If it crashes after an unbind in the close() call, then something else is
> wrong, it shouldn't do that.
> 
> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
> 
> I feel that I am missing something here.

Actually these seems to be a bug in the 25th patch --- testing the devnode
registration needs to take place before checking for fops.

After fixing that, the difference of issuing read(2) after unregistering
the device is between (probably) crashing and graciously failing. The
underlying problem is that without the 25th patch there pretty much is a
guarantee devnode has been released by the time it is accessed.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Hans Verkuil]

On 13/03/2023 17:53, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
>> On 13/03/2023 15:02, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
>>>> On 03/03/2023 12:08, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
>>>>>> On 03/03/2023 09:39, Hans Verkuil wrote:
>>>>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
>>>>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
>>>>>>>> prevent user space from calling IOCTLs or other system calls to the media
>>>>>>>> device that has been already unregistered.
>>>>>>>>
>>>>>>>> The media device's memory may of course still be released during the call
>>>>>>>> but there is only so much that can be done to this without the driver
>>>>>>>> managing the lifetime of the resources it needs somehow.
>>>>>>>>
>>>>>>>> This patch should be reverted once all drivers have been converted to manage
>>>>>>>> their resources' lifetime.
>>>>>>>>
>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
>>>>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
>>>>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
>>>>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>>>>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
>>>>>>>> --- a/drivers/media/mc/mc-device.c
>>>>>>>> +++ b/drivers/media/mc/mc-device.c
>>>>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
>>>>>>>>  	return (void __user *)(uintptr_t)arg;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static void compat_ref_release(struct kref *kref)
>>>>>>>> +{
>>>>>>>> +	struct media_devnode_compat_ref *ref =
>>>>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
>>>>>>>> +
>>>>>>>> +	kfree(ref);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
>>>>>>>>  {
>>>>>>>>  	struct media_device *mdev = to_media_device(devnode);
>>>>>>>>  	struct media_device_fh *fh;
>>>>>>>>  	unsigned long flags;
>>>>>>>>  
>>>>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
>>>>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
>>>>>>>> +		return -ENXIO;
>>>>>>>> +
>>>>>>>
>>>>>>> This seems pointless: if the media device is unregistered, then the device
>>>>>>> node disappears and it can't be opened anymore.
>>>>>>>
>>>>>>> I'm confused by this patch in general: when media_device_unregister() is called,
>>>>>>> it is no longer possible to call ioctls and basically do anything except close
>>>>>>> the open fh.
>>>>>>>
>>>>>>> So what am I missing here? It all looks odd.
>>>>>>
>>>>>> I read up on this a bit more, and I think this patch is bogus: drivers not
>>>>>> converted to the release() callback will indeed just crash, but that's no
>>>>>> different than many existing drivers, media or otherwise, when you forcibly
>>>>>> unbind them. It's broken today, and since you have to be root to unbind, I
>>>>>> would say that we can just leave it as-is rather than introducing a rather
>>>>>> ugly workaround. I don't think it will help anyway, since most likely
>>>>>> such drivers will also fails if the application has a video device open
>>>>>> when the device is unbound.
>>>>>
>>>>> The main difference is whether accessing such a file handle will access
>>>>> released memory always or whether that is possible only during a very brief
>>>>> amount of time.
>>>>>
>>>>
>>>> I still don't like this. It was broken before, and it is broken now (perhaps a
>>>> bit less broken, but still...).
>>>>
>>>> There is a right fix now, and drivers that are likely to be removed forcibly
>>>> should be converted. This patch just makes it more likely that such drivers
>>>> won't be converted since it is less likely to hit this, so people will just
>>>> think that this is 'good enough'. And it makes the code a lot uglier.
>>>
>>> I agree, although converting the drivers is easier said than done. Note
>>> that also DVB is affected by this, not just V4L2. There are quite a few DVB
>>> USB devices.
>>>
>>> The behaviour before this set (since ~ 2017) is restored by the last few
>>> patches, without these we're on pre-2017 behaviour which basically means
>>> that all media IOCTLs on file handles the device of which is gone, will
>>> always access released memory. That was quite bad, too.
>>
>> Why?
>>
>> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
>> unregister the media device, which will cause all file ops on the filehandle
>> to return immediately, except for close().
>>
>> And close() just frees the devnode from what I can see.
>>
>> There is a race if the device is unbound while in an ioctl, then all bets are
>> off without proper life-time management.
>>
>> If it crashes after an unbind in the close() call, then something else is
>> wrong, it shouldn't do that.
>>
>> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
>>
>> I feel that I am missing something here.
> 
> Actually these seems to be a bug in the 25th patch --- testing the devnode
> registration needs to take place before checking for fops.
> 
> After fixing that, the difference of issuing read(2) after unregistering
> the device is between (probably) crashing and graciously failing. The
> underlying problem is that without the 25th patch there pretty much is a
> guarantee devnode has been released by the time it is accessed.

Ah, it's a result of patch 06/26. Now devnode is embedded in struct media_device,
which in turn is embedded in a device's state structure. And when that is freed,
the devnode is also freed.

This is wrong IMHO: either struct media_devnode or struct media_device has to
be dynamically allocated. Embedding devices in a larger state structure causes
exactly these types of problems.

In this case I would just keep dynamically allocating the devnode. What is the reason
for reverting that patch? The commit log doesn't say.

Regards,

	Hans

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Hi Hans,

On Tue, Mar 14, 2023 at 09:30:52AM +0100, Hans Verkuil wrote:
> On 13/03/2023 17:53, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
> >> On 13/03/2023 15:02, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
> >>>> On 03/03/2023 12:08, Sakari Ailus wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
> >>>>>> On 03/03/2023 09:39, Hans Verkuil wrote:
> >>>>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
> >>>>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
> >>>>>>>> prevent user space from calling IOCTLs or other system calls to the media
> >>>>>>>> device that has been already unregistered.
> >>>>>>>>
> >>>>>>>> The media device's memory may of course still be released during the call
> >>>>>>>> but there is only so much that can be done to this without the driver
> >>>>>>>> managing the lifetime of the resources it needs somehow.
> >>>>>>>>
> >>>>>>>> This patch should be reverted once all drivers have been converted to manage
> >>>>>>>> their resources' lifetime.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
> >>>>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
> >>>>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
> >>>>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> >>>>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
> >>>>>>>> --- a/drivers/media/mc/mc-device.c
> >>>>>>>> +++ b/drivers/media/mc/mc-device.c
> >>>>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
> >>>>>>>>  	return (void __user *)(uintptr_t)arg;
> >>>>>>>>  }
> >>>>>>>>  
> >>>>>>>> +static void compat_ref_release(struct kref *kref)
> >>>>>>>> +{
> >>>>>>>> +	struct media_devnode_compat_ref *ref =
> >>>>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> >>>>>>>> +
> >>>>>>>> +	kfree(ref);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >>>>>>>>  {
> >>>>>>>>  	struct media_device *mdev = to_media_device(devnode);
> >>>>>>>>  	struct media_device_fh *fh;
> >>>>>>>>  	unsigned long flags;
> >>>>>>>>  
> >>>>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> >>>>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
> >>>>>>>> +		return -ENXIO;
> >>>>>>>> +
> >>>>>>>
> >>>>>>> This seems pointless: if the media device is unregistered, then the device
> >>>>>>> node disappears and it can't be opened anymore.
> >>>>>>>
> >>>>>>> I'm confused by this patch in general: when media_device_unregister() is called,
> >>>>>>> it is no longer possible to call ioctls and basically do anything except close
> >>>>>>> the open fh.
> >>>>>>>
> >>>>>>> So what am I missing here? It all looks odd.
> >>>>>>
> >>>>>> I read up on this a bit more, and I think this patch is bogus: drivers not
> >>>>>> converted to the release() callback will indeed just crash, but that's no
> >>>>>> different than many existing drivers, media or otherwise, when you forcibly
> >>>>>> unbind them. It's broken today, and since you have to be root to unbind, I
> >>>>>> would say that we can just leave it as-is rather than introducing a rather
> >>>>>> ugly workaround. I don't think it will help anyway, since most likely
> >>>>>> such drivers will also fails if the application has a video device open
> >>>>>> when the device is unbound.
> >>>>>
> >>>>> The main difference is whether accessing such a file handle will access
> >>>>> released memory always or whether that is possible only during a very brief
> >>>>> amount of time.
> >>>>>
> >>>>
> >>>> I still don't like this. It was broken before, and it is broken now (perhaps a
> >>>> bit less broken, but still...).
> >>>>
> >>>> There is a right fix now, and drivers that are likely to be removed forcibly
> >>>> should be converted. This patch just makes it more likely that such drivers
> >>>> won't be converted since it is less likely to hit this, so people will just
> >>>> think that this is 'good enough'. And it makes the code a lot uglier.
> >>>
> >>> I agree, although converting the drivers is easier said than done. Note
> >>> that also DVB is affected by this, not just V4L2. There are quite a few DVB
> >>> USB devices.
> >>>
> >>> The behaviour before this set (since ~ 2017) is restored by the last few
> >>> patches, without these we're on pre-2017 behaviour which basically means
> >>> that all media IOCTLs on file handles the device of which is gone, will
> >>> always access released memory. That was quite bad, too.
> >>
> >> Why?
> >>
> >> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
> >> unregister the media device, which will cause all file ops on the filehandle
> >> to return immediately, except for close().
> >>
> >> And close() just frees the devnode from what I can see.
> >>
> >> There is a race if the device is unbound while in an ioctl, then all bets are
> >> off without proper life-time management.
> >>
> >> If it crashes after an unbind in the close() call, then something else is
> >> wrong, it shouldn't do that.
> >>
> >> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
> >>
> >> I feel that I am missing something here.
> > 
> > Actually these seems to be a bug in the 25th patch --- testing the devnode
> > registration needs to take place before checking for fops.
> > 
> > After fixing that, the difference of issuing read(2) after unregistering
> > the device is between (probably) crashing and graciously failing. The
> > underlying problem is that without the 25th patch there pretty much is a
> > guarantee devnode has been released by the time it is accessed.
> 
> Ah, it's a result of patch 06/26. Now devnode is embedded in struct media_device,
> which in turn is embedded in a device's state structure. And when that is freed,
> the devnode is also freed.
> 
> This is wrong IMHO: either struct media_devnode or struct media_device has to
> be dynamically allocated. Embedding devices in a larger state structure causes
> exactly these types of problems.
> 
> In this case I would just keep dynamically allocating the devnode. What is the reason
> for reverting that patch? The commit log doesn't say.

I'll add that to the cover page of the next version.

The struct media_device and media_devnode are different structs as it was
thought that V4L2 and other kernel subsystems would start using MC for what
did not materialise in the end, and therefore infrastructure was added to
enable that. We do not need that today, as we did not need it six years
ago. Thus there is no longer a reason to keep media_device and
media_devnode separate.

By separately allocating these, as was done back in 2017, you can reduce
the window of possible access of released memory but you cannot eliminate
it. For that you need refcounting so that an open file handle will maintain
the in-memory data structures to carry out its IOCTL-related functions.

So this set does not yet merge struct media_device and struct media_devnode
but makes it much easier to do as they are allocated together. It's just
about moving a little bit code around.

The end goal (and partially the result already) is a cleaner codebase
without object lifetime issues.

-- 
Regards,

Sakari Ailus

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Hans Verkuil]

On 14/03/2023 09:43, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Mar 14, 2023 at 09:30:52AM +0100, Hans Verkuil wrote:
>> On 13/03/2023 17:53, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
>>>> On 13/03/2023 15:02, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
>>>>>> On 03/03/2023 12:08, Sakari Ailus wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
>>>>>>>> On 03/03/2023 09:39, Hans Verkuil wrote:
>>>>>>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
>>>>>>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
>>>>>>>>>> prevent user space from calling IOCTLs or other system calls to the media
>>>>>>>>>> device that has been already unregistered.
>>>>>>>>>>
>>>>>>>>>> The media device's memory may of course still be released during the call
>>>>>>>>>> but there is only so much that can be done to this without the driver
>>>>>>>>>> managing the lifetime of the resources it needs somehow.
>>>>>>>>>>
>>>>>>>>>> This patch should be reverted once all drivers have been converted to manage
>>>>>>>>>> their resources' lifetime.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
>>>>>>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
>>>>>>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
>>>>>>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>>>>>>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
>>>>>>>>>> --- a/drivers/media/mc/mc-device.c
>>>>>>>>>> +++ b/drivers/media/mc/mc-device.c
>>>>>>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
>>>>>>>>>>  	return (void __user *)(uintptr_t)arg;
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>> +static void compat_ref_release(struct kref *kref)
>>>>>>>>>> +{
>>>>>>>>>> +	struct media_devnode_compat_ref *ref =
>>>>>>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
>>>>>>>>>> +
>>>>>>>>>> +	kfree(ref);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
>>>>>>>>>>  {
>>>>>>>>>>  	struct media_device *mdev = to_media_device(devnode);
>>>>>>>>>>  	struct media_device_fh *fh;
>>>>>>>>>>  	unsigned long flags;
>>>>>>>>>>  
>>>>>>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
>>>>>>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
>>>>>>>>>> +		return -ENXIO;
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> This seems pointless: if the media device is unregistered, then the device
>>>>>>>>> node disappears and it can't be opened anymore.
>>>>>>>>>
>>>>>>>>> I'm confused by this patch in general: when media_device_unregister() is called,
>>>>>>>>> it is no longer possible to call ioctls and basically do anything except close
>>>>>>>>> the open fh.
>>>>>>>>>
>>>>>>>>> So what am I missing here? It all looks odd.
>>>>>>>>
>>>>>>>> I read up on this a bit more, and I think this patch is bogus: drivers not
>>>>>>>> converted to the release() callback will indeed just crash, but that's no
>>>>>>>> different than many existing drivers, media or otherwise, when you forcibly
>>>>>>>> unbind them. It's broken today, and since you have to be root to unbind, I
>>>>>>>> would say that we can just leave it as-is rather than introducing a rather
>>>>>>>> ugly workaround. I don't think it will help anyway, since most likely
>>>>>>>> such drivers will also fails if the application has a video device open
>>>>>>>> when the device is unbound.
>>>>>>>
>>>>>>> The main difference is whether accessing such a file handle will access
>>>>>>> released memory always or whether that is possible only during a very brief
>>>>>>> amount of time.
>>>>>>>
>>>>>>
>>>>>> I still don't like this. It was broken before, and it is broken now (perhaps a
>>>>>> bit less broken, but still...).
>>>>>>
>>>>>> There is a right fix now, and drivers that are likely to be removed forcibly
>>>>>> should be converted. This patch just makes it more likely that such drivers
>>>>>> won't be converted since it is less likely to hit this, so people will just
>>>>>> think that this is 'good enough'. And it makes the code a lot uglier.
>>>>>
>>>>> I agree, although converting the drivers is easier said than done. Note
>>>>> that also DVB is affected by this, not just V4L2. There are quite a few DVB
>>>>> USB devices.
>>>>>
>>>>> The behaviour before this set (since ~ 2017) is restored by the last few
>>>>> patches, without these we're on pre-2017 behaviour which basically means
>>>>> that all media IOCTLs on file handles the device of which is gone, will
>>>>> always access released memory. That was quite bad, too.
>>>>
>>>> Why?
>>>>
>>>> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
>>>> unregister the media device, which will cause all file ops on the filehandle
>>>> to return immediately, except for close().
>>>>
>>>> And close() just frees the devnode from what I can see.
>>>>
>>>> There is a race if the device is unbound while in an ioctl, then all bets are
>>>> off without proper life-time management.
>>>>
>>>> If it crashes after an unbind in the close() call, then something else is
>>>> wrong, it shouldn't do that.
>>>>
>>>> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
>>>>
>>>> I feel that I am missing something here.
>>>
>>> Actually these seems to be a bug in the 25th patch --- testing the devnode
>>> registration needs to take place before checking for fops.
>>>
>>> After fixing that, the difference of issuing read(2) after unregistering
>>> the device is between (probably) crashing and graciously failing. The
>>> underlying problem is that without the 25th patch there pretty much is a
>>> guarantee devnode has been released by the time it is accessed.
>>
>> Ah, it's a result of patch 06/26. Now devnode is embedded in struct media_device,
>> which in turn is embedded in a device's state structure. And when that is freed,
>> the devnode is also freed.
>>
>> This is wrong IMHO: either struct media_devnode or struct media_device has to
>> be dynamically allocated. Embedding devices in a larger state structure causes
>> exactly these types of problems.
>>
>> In this case I would just keep dynamically allocating the devnode. What is the reason
>> for reverting that patch? The commit log doesn't say.
> 
> I'll add that to the cover page of the next version.
> 
> The struct media_device and media_devnode are different structs as it was
> thought that V4L2 and other kernel subsystems would start using MC for what
> did not materialise in the end, and therefore infrastructure was added to
> enable that. We do not need that today, as we did not need it six years
> ago. Thus there is no longer a reason to keep media_device and
> media_devnode separate.
> 
> By separately allocating these, as was done back in 2017, you can reduce
> the window of possible access of released memory but you cannot eliminate
> it. For that you need refcounting so that an open file handle will maintain
> the in-memory data structures to carry out its IOCTL-related functions.
> 
> So this set does not yet merge struct media_device and struct media_devnode
> but makes it much easier to do as they are allocated together. It's just
> about moving a little bit code around.
> 
> The end goal (and partially the result already) is a cleaner codebase
> without object lifetime issues.
> 

So you revert a patch to make it cleaner, then have to add a really ugly patch
back to fix what you broke?

It's not just 'moving code around', you break functionality (imperfect as it is),
and then have to fix it in another way.

I would just drop this revert patch. And when all drivers are converted to 'do the
right thing', then you can revert it.

Unless there is another reason for reverting it?

Regards,

	Hans

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Hi Hans,

On Tue, Mar 14, 2023 at 09:58:43AM +0100, Hans Verkuil wrote:
> On 14/03/2023 09:43, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Tue, Mar 14, 2023 at 09:30:52AM +0100, Hans Verkuil wrote:
> >> On 13/03/2023 17:53, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
> >>>> On 13/03/2023 15:02, Sakari Ailus wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
> >>>>>> On 03/03/2023 12:08, Sakari Ailus wrote:
> >>>>>>> Hi Hans,
> >>>>>>>
> >>>>>>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
> >>>>>>>> On 03/03/2023 09:39, Hans Verkuil wrote:
> >>>>>>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
> >>>>>>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
> >>>>>>>>>> prevent user space from calling IOCTLs or other system calls to the media
> >>>>>>>>>> device that has been already unregistered.
> >>>>>>>>>>
> >>>>>>>>>> The media device's memory may of course still be released during the call
> >>>>>>>>>> but there is only so much that can be done to this without the driver
> >>>>>>>>>> managing the lifetime of the resources it needs somehow.
> >>>>>>>>>>
> >>>>>>>>>> This patch should be reverted once all drivers have been converted to manage
> >>>>>>>>>> their resources' lifetime.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
> >>>>>>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
> >>>>>>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
> >>>>>>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> >>>>>>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
> >>>>>>>>>> --- a/drivers/media/mc/mc-device.c
> >>>>>>>>>> +++ b/drivers/media/mc/mc-device.c
> >>>>>>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
> >>>>>>>>>>  	return (void __user *)(uintptr_t)arg;
> >>>>>>>>>>  }
> >>>>>>>>>>  
> >>>>>>>>>> +static void compat_ref_release(struct kref *kref)
> >>>>>>>>>> +{
> >>>>>>>>>> +	struct media_devnode_compat_ref *ref =
> >>>>>>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> >>>>>>>>>> +
> >>>>>>>>>> +	kfree(ref);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >>>>>>>>>>  {
> >>>>>>>>>>  	struct media_device *mdev = to_media_device(devnode);
> >>>>>>>>>>  	struct media_device_fh *fh;
> >>>>>>>>>>  	unsigned long flags;
> >>>>>>>>>>  
> >>>>>>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> >>>>>>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
> >>>>>>>>>> +		return -ENXIO;
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> This seems pointless: if the media device is unregistered, then the device
> >>>>>>>>> node disappears and it can't be opened anymore.
> >>>>>>>>>
> >>>>>>>>> I'm confused by this patch in general: when media_device_unregister() is called,
> >>>>>>>>> it is no longer possible to call ioctls and basically do anything except close
> >>>>>>>>> the open fh.
> >>>>>>>>>
> >>>>>>>>> So what am I missing here? It all looks odd.
> >>>>>>>>
> >>>>>>>> I read up on this a bit more, and I think this patch is bogus: drivers not
> >>>>>>>> converted to the release() callback will indeed just crash, but that's no
> >>>>>>>> different than many existing drivers, media or otherwise, when you forcibly
> >>>>>>>> unbind them. It's broken today, and since you have to be root to unbind, I
> >>>>>>>> would say that we can just leave it as-is rather than introducing a rather
> >>>>>>>> ugly workaround. I don't think it will help anyway, since most likely
> >>>>>>>> such drivers will also fails if the application has a video device open
> >>>>>>>> when the device is unbound.
> >>>>>>>
> >>>>>>> The main difference is whether accessing such a file handle will access
> >>>>>>> released memory always or whether that is possible only during a very brief
> >>>>>>> amount of time.
> >>>>>>>
> >>>>>>
> >>>>>> I still don't like this. It was broken before, and it is broken now (perhaps a
> >>>>>> bit less broken, but still...).
> >>>>>>
> >>>>>> There is a right fix now, and drivers that are likely to be removed forcibly
> >>>>>> should be converted. This patch just makes it more likely that such drivers
> >>>>>> won't be converted since it is less likely to hit this, so people will just
> >>>>>> think that this is 'good enough'. And it makes the code a lot uglier.
> >>>>>
> >>>>> I agree, although converting the drivers is easier said than done. Note
> >>>>> that also DVB is affected by this, not just V4L2. There are quite a few DVB
> >>>>> USB devices.
> >>>>>
> >>>>> The behaviour before this set (since ~ 2017) is restored by the last few
> >>>>> patches, without these we're on pre-2017 behaviour which basically means
> >>>>> that all media IOCTLs on file handles the device of which is gone, will
> >>>>> always access released memory. That was quite bad, too.
> >>>>
> >>>> Why?
> >>>>
> >>>> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
> >>>> unregister the media device, which will cause all file ops on the filehandle
> >>>> to return immediately, except for close().
> >>>>
> >>>> And close() just frees the devnode from what I can see.
> >>>>
> >>>> There is a race if the device is unbound while in an ioctl, then all bets are
> >>>> off without proper life-time management.
> >>>>
> >>>> If it crashes after an unbind in the close() call, then something else is
> >>>> wrong, it shouldn't do that.
> >>>>
> >>>> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
> >>>>
> >>>> I feel that I am missing something here.
> >>>
> >>> Actually these seems to be a bug in the 25th patch --- testing the devnode
> >>> registration needs to take place before checking for fops.
> >>>
> >>> After fixing that, the difference of issuing read(2) after unregistering
> >>> the device is between (probably) crashing and graciously failing. The
> >>> underlying problem is that without the 25th patch there pretty much is a
> >>> guarantee devnode has been released by the time it is accessed.
> >>
> >> Ah, it's a result of patch 06/26. Now devnode is embedded in struct media_device,
> >> which in turn is embedded in a device's state structure. And when that is freed,
> >> the devnode is also freed.
> >>
> >> This is wrong IMHO: either struct media_devnode or struct media_device has to
> >> be dynamically allocated. Embedding devices in a larger state structure causes
> >> exactly these types of problems.
> >>
> >> In this case I would just keep dynamically allocating the devnode. What is the reason
> >> for reverting that patch? The commit log doesn't say.
> > 
> > I'll add that to the cover page of the next version.
> > 
> > The struct media_device and media_devnode are different structs as it was
> > thought that V4L2 and other kernel subsystems would start using MC for what
> > did not materialise in the end, and therefore infrastructure was added to
> > enable that. We do not need that today, as we did not need it six years
> > ago. Thus there is no longer a reason to keep media_device and
> > media_devnode separate.
> > 
> > By separately allocating these, as was done back in 2017, you can reduce
> > the window of possible access of released memory but you cannot eliminate
> > it. For that you need refcounting so that an open file handle will maintain
> > the in-memory data structures to carry out its IOCTL-related functions.
> > 
> > So this set does not yet merge struct media_device and struct media_devnode
> > but makes it much easier to do as they are allocated together. It's just
> > about moving a little bit code around.
> > 
> > The end goal (and partially the result already) is a cleaner codebase
> > without object lifetime issues.
> > 
> 
> So you revert a patch to make it cleaner, then have to add a really ugly
> patch back to fix what you broke?

It's all broken to begin with.

The alternative to this is either fix all drivers (a lot of work, largely
untestable) or considering MC terminally broken. I'd prefer the former as
we don't have an alternative for MC at the moment.

If you really dislike the new hack (I don't think it's worse than the old
hack, it's much more localised at easier to understand it's broken), we
could keep it a few kernel releases, move the unconverted drivers to
staging and remove them with the hack, again after a few releases.

> 
> It's not just 'moving code around', you break functionality (imperfect as
> it is), and then have to fix it in another way.

That is the intention. Repairing the state before this set without
reverting patches would make the intermediate patches very, very ugly and
difficult to review. Of course you could compare the result with the end
result of this patchset.

> 
> I would just drop this revert patch. And when all drivers are converted
> to 'do the right thing', then you can revert it.
> 
> Unless there is another reason for reverting it?

The reverts enable fixing the root problem, they are a pre-condition for
doing that. I'd prefer to enable writing drivers that are not broken (well,
at least this way). The 2017 fixes always were a dead end and we knew that
perfectly well.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Hans Verkuil]

Hi Sakari,

On 14/03/2023 11:59, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Mar 14, 2023 at 09:58:43AM +0100, Hans Verkuil wrote:
>> On 14/03/2023 09:43, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Tue, Mar 14, 2023 at 09:30:52AM +0100, Hans Verkuil wrote:
>>>> On 13/03/2023 17:53, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
>>>>>> On 13/03/2023 15:02, Sakari Ailus wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
>>>>>>>> On 03/03/2023 12:08, Sakari Ailus wrote:
>>>>>>>>> Hi Hans,
>>>>>>>>>
>>>>>>>>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
>>>>>>>>>> On 03/03/2023 09:39, Hans Verkuil wrote:
>>>>>>>>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
>>>>>>>>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
>>>>>>>>>>>> prevent user space from calling IOCTLs or other system calls to the media
>>>>>>>>>>>> device that has been already unregistered.
>>>>>>>>>>>>
>>>>>>>>>>>> The media device's memory may of course still be released during the call
>>>>>>>>>>>> but there is only so much that can be done to this without the driver
>>>>>>>>>>>> managing the lifetime of the resources it needs somehow.
>>>>>>>>>>>>
>>>>>>>>>>>> This patch should be reverted once all drivers have been converted to manage
>>>>>>>>>>>> their resources' lifetime.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
>>>>>>>>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
>>>>>>>>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
>>>>>>>>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
>>>>>>>>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
>>>>>>>>>>>> --- a/drivers/media/mc/mc-device.c
>>>>>>>>>>>> +++ b/drivers/media/mc/mc-device.c
>>>>>>>>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
>>>>>>>>>>>>  	return (void __user *)(uintptr_t)arg;
>>>>>>>>>>>>  }
>>>>>>>>>>>>  
>>>>>>>>>>>> +static void compat_ref_release(struct kref *kref)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +	struct media_devnode_compat_ref *ref =
>>>>>>>>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
>>>>>>>>>>>> +
>>>>>>>>>>>> +	kfree(ref);
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
>>>>>>>>>>>>  {
>>>>>>>>>>>>  	struct media_device *mdev = to_media_device(devnode);
>>>>>>>>>>>>  	struct media_device_fh *fh;
>>>>>>>>>>>>  	unsigned long flags;
>>>>>>>>>>>>  
>>>>>>>>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
>>>>>>>>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
>>>>>>>>>>>> +		return -ENXIO;
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> This seems pointless: if the media device is unregistered, then the device
>>>>>>>>>>> node disappears and it can't be opened anymore.
>>>>>>>>>>>
>>>>>>>>>>> I'm confused by this patch in general: when media_device_unregister() is called,
>>>>>>>>>>> it is no longer possible to call ioctls and basically do anything except close
>>>>>>>>>>> the open fh.
>>>>>>>>>>>
>>>>>>>>>>> So what am I missing here? It all looks odd.
>>>>>>>>>>
>>>>>>>>>> I read up on this a bit more, and I think this patch is bogus: drivers not
>>>>>>>>>> converted to the release() callback will indeed just crash, but that's no
>>>>>>>>>> different than many existing drivers, media or otherwise, when you forcibly
>>>>>>>>>> unbind them. It's broken today, and since you have to be root to unbind, I
>>>>>>>>>> would say that we can just leave it as-is rather than introducing a rather
>>>>>>>>>> ugly workaround. I don't think it will help anyway, since most likely
>>>>>>>>>> such drivers will also fails if the application has a video device open
>>>>>>>>>> when the device is unbound.
>>>>>>>>>
>>>>>>>>> The main difference is whether accessing such a file handle will access
>>>>>>>>> released memory always or whether that is possible only during a very brief
>>>>>>>>> amount of time.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I still don't like this. It was broken before, and it is broken now (perhaps a
>>>>>>>> bit less broken, but still...).
>>>>>>>>
>>>>>>>> There is a right fix now, and drivers that are likely to be removed forcibly
>>>>>>>> should be converted. This patch just makes it more likely that such drivers
>>>>>>>> won't be converted since it is less likely to hit this, so people will just
>>>>>>>> think that this is 'good enough'. And it makes the code a lot uglier.
>>>>>>>
>>>>>>> I agree, although converting the drivers is easier said than done. Note
>>>>>>> that also DVB is affected by this, not just V4L2. There are quite a few DVB
>>>>>>> USB devices.
>>>>>>>
>>>>>>> The behaviour before this set (since ~ 2017) is restored by the last few
>>>>>>> patches, without these we're on pre-2017 behaviour which basically means
>>>>>>> that all media IOCTLs on file handles the device of which is gone, will
>>>>>>> always access released memory. That was quite bad, too.
>>>>>>
>>>>>> Why?
>>>>>>
>>>>>> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
>>>>>> unregister the media device, which will cause all file ops on the filehandle
>>>>>> to return immediately, except for close().
>>>>>>
>>>>>> And close() just frees the devnode from what I can see.
>>>>>>
>>>>>> There is a race if the device is unbound while in an ioctl, then all bets are
>>>>>> off without proper life-time management.
>>>>>>
>>>>>> If it crashes after an unbind in the close() call, then something else is
>>>>>> wrong, it shouldn't do that.
>>>>>>
>>>>>> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
>>>>>>
>>>>>> I feel that I am missing something here.
>>>>>
>>>>> Actually these seems to be a bug in the 25th patch --- testing the devnode
>>>>> registration needs to take place before checking for fops.
>>>>>
>>>>> After fixing that, the difference of issuing read(2) after unregistering
>>>>> the device is between (probably) crashing and graciously failing. The
>>>>> underlying problem is that without the 25th patch there pretty much is a
>>>>> guarantee devnode has been released by the time it is accessed.
>>>>
>>>> Ah, it's a result of patch 06/26. Now devnode is embedded in struct media_device,
>>>> which in turn is embedded in a device's state structure. And when that is freed,
>>>> the devnode is also freed.
>>>>
>>>> This is wrong IMHO: either struct media_devnode or struct media_device has to
>>>> be dynamically allocated. Embedding devices in a larger state structure causes
>>>> exactly these types of problems.
>>>>
>>>> In this case I would just keep dynamically allocating the devnode. What is the reason
>>>> for reverting that patch? The commit log doesn't say.
>>>
>>> I'll add that to the cover page of the next version.
>>>
>>> The struct media_device and media_devnode are different structs as it was
>>> thought that V4L2 and other kernel subsystems would start using MC for what
>>> did not materialise in the end, and therefore infrastructure was added to
>>> enable that. We do not need that today, as we did not need it six years
>>> ago. Thus there is no longer a reason to keep media_device and
>>> media_devnode separate.
>>>
>>> By separately allocating these, as was done back in 2017, you can reduce
>>> the window of possible access of released memory but you cannot eliminate
>>> it. For that you need refcounting so that an open file handle will maintain
>>> the in-memory data structures to carry out its IOCTL-related functions.
>>>
>>> So this set does not yet merge struct media_device and struct media_devnode
>>> but makes it much easier to do as they are allocated together. It's just
>>> about moving a little bit code around.
>>>
>>> The end goal (and partially the result already) is a cleaner codebase
>>> without object lifetime issues.
>>>
>>
>> So you revert a patch to make it cleaner, then have to add a really ugly
>> patch back to fix what you broke?
> 
> It's all broken to begin with.
> 
> The alternative to this is either fix all drivers (a lot of work, largely
> untestable) or considering MC terminally broken. I'd prefer the former as
> we don't have an alternative for MC at the moment.
> 
> If you really dislike the new hack (I don't think it's worse than the old
> hack, it's much more localised at easier to understand it's broken), we
> could keep it a few kernel releases, move the unconverted drivers to
> staging and remove them with the hack, again after a few releases.
> 
>>
>> It's not just 'moving code around', you break functionality (imperfect as
>> it is), and then have to fix it in another way.
> 
> That is the intention. Repairing the state before this set without
> reverting patches would make the intermediate patches very, very ugly and
> difficult to review. Of course you could compare the result with the end
> result of this patchset.
> 
>>
>> I would just drop this revert patch. And when all drivers are converted
>> to 'do the right thing', then you can revert it.
>>
>> Unless there is another reason for reverting it?
> 
> The reverts enable fixing the root problem, they are a pre-condition for
> doing that. I'd prefer to enable writing drivers that are not broken (well,
> at least this way). The 2017 fixes always were a dead end and we knew that
> perfectly well.
> 

I won't block this patch, but I think it is really ugly. And I am afraid that
we might be stuck with this hack for a long time.

How many drivers would need to be converted for this hack to go away?

Note that this series needs to be rebased, I had a compile error in omap3isp,
visl and in a mediatek driver.

However, I also tested this series with a kernel that has CONFIG_DEBUG_KOBJECT_RELEASE
on, and if I run the test-media script in v4l-utils:

cd contrib/test
sudo ./test-media mc

then this crashes at the 'unbind vivid' stage:

[  292.900982] CPU: 2 PID: 108 Comm: kworker/2:3 Tainted: G    B D W          6.3.0-rc2-debug #29
[  292.900986] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
[  292.900990] Workqueue: events kobject_delayed_cleanup
[  292.900999] RIP: 0010:kobject_put+0x16/0x90
[  292.901004] Code: 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 48 85 ff 74 7c 55 53 48 89 fb 48 8d bf e8 00 00 00 e8 7a c2 88 fe <f6> 83 e8 00 00 00 01 74 2b 48 8d 6b 38 be 04 00 00
00 48 89 ef e8
[  292.901008] RSP: 0018:ffffc900016cfcf0 EFLAGS: 00010286
[  292.901012] RAX: 0000000000000000 RBX: ffff8881b1e200d0 RCX: ffffffff82cd91c6
[  292.901015] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff8881b1e201b8
[  292.901018] RBP: ffff88814f576400 R08: 0000000000000000 R09: ffffffff84879fd7
[  292.901020] R10: fffffbfff090f3fa R11: 0000000000000006 R12: ffff8881b1e22e98
[  292.901023] R13: ffff8881445853e8 R14: 0000000000000000 R15: ffff8881f6338200
[  292.901026] FS:  0000000000000000(0000) GS:ffff8881f6300000(0000) knlGS:0000000000000000
[  292.901037] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  292.901040] CR2: ffff8881b1e201b8 CR3: 0000000149c63000 CR4: 0000000000750ee0
[  292.901048] PKRU: 55555554
[  292.901051] Call Trace:
[  292.901054]  <TASK>
[  292.901056]  device_release+0x5a/0x100
[  292.901063]  kobject_delayed_cleanup+0x5e/0xa0
[  292.901066]  process_one_work+0x535/0xa50
[  292.901073]  ? __pfx_process_one_work+0x10/0x10
[  292.901077]  ? __pfx_do_raw_spin_lock+0x10/0x10
[  292.901082]  ? __list_add_valid+0x33/0xd0
[  292.901087]  worker_thread+0x8a/0x620
[  292.901091]  ? __kthread_parkme+0xd3/0xf0
[  292.901095]  ? __pfx_worker_thread+0x10/0x10
[  292.901099]  kthread+0x173/0x1b0
[  292.901102]  ? __pfx_kthread+0x10/0x10
[  292.901105]  ret_from_fork+0x2c/0x50
[  292.901110]  </TASK>
[  292.901112] Modules linked in: vivid v4l2_tpg videobuf2_dma_contig v4l2_dv_timings videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videodev videobuf2_common mc
[  292.901126] CR2: ffff8881b1e201b8
[  292.901130] ---[ end trace 0000000000000000 ]---
[  292.901133] RIP: 0010:__mutex_lock+0xdb/0xcc0
[  292.901138] Code: c0 e8 f9 4f 57 fe 2e 2e 2e 31 c0 48 c7 c7 60 d2 31 86 e8 98 07 84 fe 8b 05 d2 83 5f 03 85 c0 75 13 48 8d 7b 60 e8 b5 08 84 fe <48> 3b 5b 60 0f 85 e9 05 00 00 bf 01 00 00 00 e8 41
60 57 fe 48 8d
[  292.901141] RSP: 0018:ffffc90002727a40 EFLAGS: 00010282
[  292.901145] RAX: 0000000000000000 RBX: 0493011a00000f42 RCX: ffffffff82d24e9b
[  292.901148] RDX: 0000000000000000 RSI: 0000000000000008 RDI: 0493011a00000fa2
[  292.901151] RBP: ffffc90002727b90 R08: ffffffff81e74db3 R09: ffffffff84879fd7
[  292.901154] R10: ffffc90002727ba8 R11: 0000000000000000 R12: 0000000000000000
[  292.901157] R13: 0000000000000000 R14: ffff8881582b3078 R15: 1ffff920004e4f54
[  292.901160] FS:  0000000000000000(0000) GS:ffff8881f6300000(0000) knlGS:0000000000000000
[  292.901169] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  292.901172] CR2: ffff8881b1e201b8 CR3: 0000000149c63000 CR4: 0000000000750ee0
[  292.901185] PKRU: 55555554

The test-media regression test is really good at testing unbind scenarios for the virtual
drivers that we have. Together with the CONFIG_DEBUG_KOBJECT_RELEASE option it is a
good test to run.

Regards,

	Hans

Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting

[Sakari Ailus]

Hi Hans,

Thank you for returning to the topic.

On Fri, Mar 31, 2023 at 12:53:49PM +0200, Hans Verkuil wrote:
> Hi Sakari,
> 
> On 14/03/2023 11:59, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Tue, Mar 14, 2023 at 09:58:43AM +0100, Hans Verkuil wrote:
> >> On 14/03/2023 09:43, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Tue, Mar 14, 2023 at 09:30:52AM +0100, Hans Verkuil wrote:
> >>>> On 13/03/2023 17:53, Sakari Ailus wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
> >>>>>> On 13/03/2023 15:02, Sakari Ailus wrote:
> >>>>>>> Hi Hans,
> >>>>>>>
> >>>>>>> On Mon, Mar 13, 2023 at 02:46:27PM +0100, Hans Verkuil wrote:
> >>>>>>>> On 03/03/2023 12:08, Sakari Ailus wrote:
> >>>>>>>>> Hi Hans,
> >>>>>>>>>
> >>>>>>>>> On Fri, Mar 03, 2023 at 09:54:40AM +0100, Hans Verkuil wrote:
> >>>>>>>>>> On 03/03/2023 09:39, Hans Verkuil wrote:
> >>>>>>>>>>> On 01/02/2023 22:45, Sakari Ailus wrote:
> >>>>>>>>>>>> Add a new helper data structure media_devnode_compat_ref, which is used to
> >>>>>>>>>>>> prevent user space from calling IOCTLs or other system calls to the media
> >>>>>>>>>>>> device that has been already unregistered.
> >>>>>>>>>>>>
> >>>>>>>>>>>> The media device's memory may of course still be released during the call
> >>>>>>>>>>>> but there is only so much that can be done to this without the driver
> >>>>>>>>>>>> managing the lifetime of the resources it needs somehow.
> >>>>>>>>>>>>
> >>>>>>>>>>>> This patch should be reverted once all drivers have been converted to manage
> >>>>>>>>>>>> their resources' lifetime.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>  drivers/media/mc/mc-device.c  | 60 ++++++++++++++++++++++++++++++-----
> >>>>>>>>>>>>  drivers/media/mc/mc-devnode.c | 21 ++++++++----
> >>>>>>>>>>>>  include/media/media-devnode.h | 29 +++++++++++++++++
> >>>>>>>>>>>>  3 files changed, 96 insertions(+), 14 deletions(-)
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> >>>>>>>>>>>> index 3a1db5fdbba7..22fdaa6370ea 100644
> >>>>>>>>>>>> --- a/drivers/media/mc/mc-device.c
> >>>>>>>>>>>> +++ b/drivers/media/mc/mc-device.c
> >>>>>>>>>>>> @@ -45,18 +45,34 @@ static inline void __user *media_get_uptr(__u64 arg)
> >>>>>>>>>>>>  	return (void __user *)(uintptr_t)arg;
> >>>>>>>>>>>>  }
> >>>>>>>>>>>>  
> >>>>>>>>>>>> +static void compat_ref_release(struct kref *kref)
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> +	struct media_devnode_compat_ref *ref =
> >>>>>>>>>>>> +		container_of_const(kref, struct media_devnode_compat_ref, kref);
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +	kfree(ref);
> >>>>>>>>>>>> +}
> >>>>>>>>>>>> +
> >>>>>>>>>>>>  static int media_device_open(struct media_devnode *devnode, struct file *filp)
> >>>>>>>>>>>>  {
> >>>>>>>>>>>>  	struct media_device *mdev = to_media_device(devnode);
> >>>>>>>>>>>>  	struct media_device_fh *fh;
> >>>>>>>>>>>>  	unsigned long flags;
> >>>>>>>>>>>>  
> >>>>>>>>>>>> +	if (devnode->ref && (!atomic_read(&devnode->ref->registered) ||
> >>>>>>>>>>>> +			     !kref_get_unless_zero(&devnode->ref->kref)))
> >>>>>>>>>>>> +		return -ENXIO;
> >>>>>>>>>>>> +
> >>>>>>>>>>>
> >>>>>>>>>>> This seems pointless: if the media device is unregistered, then the device
> >>>>>>>>>>> node disappears and it can't be opened anymore.
> >>>>>>>>>>>
> >>>>>>>>>>> I'm confused by this patch in general: when media_device_unregister() is called,
> >>>>>>>>>>> it is no longer possible to call ioctls and basically do anything except close
> >>>>>>>>>>> the open fh.
> >>>>>>>>>>>
> >>>>>>>>>>> So what am I missing here? It all looks odd.
> >>>>>>>>>>
> >>>>>>>>>> I read up on this a bit more, and I think this patch is bogus: drivers not
> >>>>>>>>>> converted to the release() callback will indeed just crash, but that's no
> >>>>>>>>>> different than many existing drivers, media or otherwise, when you forcibly
> >>>>>>>>>> unbind them. It's broken today, and since you have to be root to unbind, I
> >>>>>>>>>> would say that we can just leave it as-is rather than introducing a rather
> >>>>>>>>>> ugly workaround. I don't think it will help anyway, since most likely
> >>>>>>>>>> such drivers will also fails if the application has a video device open
> >>>>>>>>>> when the device is unbound.
> >>>>>>>>>
> >>>>>>>>> The main difference is whether accessing such a file handle will access
> >>>>>>>>> released memory always or whether that is possible only during a very brief
> >>>>>>>>> amount of time.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I still don't like this. It was broken before, and it is broken now (perhaps a
> >>>>>>>> bit less broken, but still...).
> >>>>>>>>
> >>>>>>>> There is a right fix now, and drivers that are likely to be removed forcibly
> >>>>>>>> should be converted. This patch just makes it more likely that such drivers
> >>>>>>>> won't be converted since it is less likely to hit this, so people will just
> >>>>>>>> think that this is 'good enough'. And it makes the code a lot uglier.
> >>>>>>>
> >>>>>>> I agree, although converting the drivers is easier said than done. Note
> >>>>>>> that also DVB is affected by this, not just V4L2. There are quite a few DVB
> >>>>>>> USB devices.
> >>>>>>>
> >>>>>>> The behaviour before this set (since ~ 2017) is restored by the last few
> >>>>>>> patches, without these we're on pre-2017 behaviour which basically means
> >>>>>>> that all media IOCTLs on file handles the device of which is gone, will
> >>>>>>> always access released memory. That was quite bad, too.
> >>>>>>
> >>>>>> Why?
> >>>>>>
> >>>>>> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
> >>>>>> unregister the media device, which will cause all file ops on the filehandle
> >>>>>> to return immediately, except for close().
> >>>>>>
> >>>>>> And close() just frees the devnode from what I can see.
> >>>>>>
> >>>>>> There is a race if the device is unbound while in an ioctl, then all bets are
> >>>>>> off without proper life-time management.
> >>>>>>
> >>>>>> If it crashes after an unbind in the close() call, then something else is
> >>>>>> wrong, it shouldn't do that.
> >>>>>>
> >>>>>> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
> >>>>>>
> >>>>>> I feel that I am missing something here.
> >>>>>
> >>>>> Actually these seems to be a bug in the 25th patch --- testing the devnode
> >>>>> registration needs to take place before checking for fops.
> >>>>>
> >>>>> After fixing that, the difference of issuing read(2) after unregistering
> >>>>> the device is between (probably) crashing and graciously failing. The
> >>>>> underlying problem is that without the 25th patch there pretty much is a
> >>>>> guarantee devnode has been released by the time it is accessed.
> >>>>
> >>>> Ah, it's a result of patch 06/26. Now devnode is embedded in struct media_device,
> >>>> which in turn is embedded in a device's state structure. And when that is freed,
> >>>> the devnode is also freed.
> >>>>
> >>>> This is wrong IMHO: either struct media_devnode or struct media_device has to
> >>>> be dynamically allocated. Embedding devices in a larger state structure causes
> >>>> exactly these types of problems.
> >>>>
> >>>> In this case I would just keep dynamically allocating the devnode. What is the reason
> >>>> for reverting that patch? The commit log doesn't say.
> >>>
> >>> I'll add that to the cover page of the next version.
> >>>
> >>> The struct media_device and media_devnode are different structs as it was
> >>> thought that V4L2 and other kernel subsystems would start using MC for what
> >>> did not materialise in the end, and therefore infrastructure was added to
> >>> enable that. We do not need that today, as we did not need it six years
> >>> ago. Thus there is no longer a reason to keep media_device and
> >>> media_devnode separate.
> >>>
> >>> By separately allocating these, as was done back in 2017, you can reduce
> >>> the window of possible access of released memory but you cannot eliminate
> >>> it. For that you need refcounting so that an open file handle will maintain
> >>> the in-memory data structures to carry out its IOCTL-related functions.
> >>>
> >>> So this set does not yet merge struct media_device and struct media_devnode
> >>> but makes it much easier to do as they are allocated together. It's just
> >>> about moving a little bit code around.
> >>>
> >>> The end goal (and partially the result already) is a cleaner codebase
> >>> without object lifetime issues.
> >>>
> >>
> >> So you revert a patch to make it cleaner, then have to add a really ugly
> >> patch back to fix what you broke?
> > 
> > It's all broken to begin with.
> > 
> > The alternative to this is either fix all drivers (a lot of work, largely
> > untestable) or considering MC terminally broken. I'd prefer the former as
> > we don't have an alternative for MC at the moment.
> > 
> > If you really dislike the new hack (I don't think it's worse than the old
> > hack, it's much more localised at easier to understand it's broken), we
> > could keep it a few kernel releases, move the unconverted drivers to
> > staging and remove them with the hack, again after a few releases.
> > 
> >>
> >> It's not just 'moving code around', you break functionality (imperfect as
> >> it is), and then have to fix it in another way.
> > 
> > That is the intention. Repairing the state before this set without
> > reverting patches would make the intermediate patches very, very ugly and
> > difficult to review. Of course you could compare the result with the end
> > result of this patchset.
> > 
> >>
> >> I would just drop this revert patch. And when all drivers are converted
> >> to 'do the right thing', then you can revert it.
> >>
> >> Unless there is another reason for reverting it?
> > 
> > The reverts enable fixing the root problem, they are a pre-condition for
> > doing that. I'd prefer to enable writing drivers that are not broken (well,
> > at least this way). The 2017 fixes always were a dead end and we knew that
> > perfectly well.
> > 
> 
> I won't block this patch, but I think it is really ugly. And I am afraid that
> we might be stuck with this hack for a long time.

For some time, definitely yes. We should encourage driver developers to
convert the drivers. I can convert some, too, it's not very difficult. But
testing will be an issue, the changes aren't entirely trivial. Perhaps
something to do early in the cycle, right after rc1 a few drivers at a
time?

Do also note that such a hack is present without these patches, it's just
spread across a larger codebase and not very visible. I'd argue that a
localised and visible hack is better than that.

> 
> How many drivers would need to be converted for this hack to go away?

Closer to 40, including the DVB framework. Basically this includes anything
that registers a media device need to be changed.

> 
> Note that this series needs to be rebased, I had a compile error in omap3isp,
> visl and in a mediatek driver.
> 
> However, I also tested this series with a kernel that has CONFIG_DEBUG_KOBJECT_RELEASE
> on, and if I run the test-media script in v4l-utils:
> 
> cd contrib/test
> sudo ./test-media mc
> 
> then this crashes at the 'unbind vivid' stage:
> 
> [  292.900982] CPU: 2 PID: 108 Comm: kworker/2:3 Tainted: G    B D W          6.3.0-rc2-debug #29
> [  292.900986] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
> [  292.900990] Workqueue: events kobject_delayed_cleanup
> [  292.900999] RIP: 0010:kobject_put+0x16/0x90
> [  292.901004] Code: 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 48 85 ff 74 7c 55 53 48 89 fb 48 8d bf e8 00 00 00 e8 7a c2 88 fe <f6> 83 e8 00 00 00 01 74 2b 48 8d 6b 38 be 04 00 00
> 00 48 89 ef e8
> [  292.901008] RSP: 0018:ffffc900016cfcf0 EFLAGS: 00010286
> [  292.901012] RAX: 0000000000000000 RBX: ffff8881b1e200d0 RCX: ffffffff82cd91c6
> [  292.901015] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff8881b1e201b8
> [  292.901018] RBP: ffff88814f576400 R08: 0000000000000000 R09: ffffffff84879fd7
> [  292.901020] R10: fffffbfff090f3fa R11: 0000000000000006 R12: ffff8881b1e22e98
> [  292.901023] R13: ffff8881445853e8 R14: 0000000000000000 R15: ffff8881f6338200
> [  292.901026] FS:  0000000000000000(0000) GS:ffff8881f6300000(0000) knlGS:0000000000000000
> [  292.901037] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  292.901040] CR2: ffff8881b1e201b8 CR3: 0000000149c63000 CR4: 0000000000750ee0
> [  292.901048] PKRU: 55555554
> [  292.901051] Call Trace:
> [  292.901054]  <TASK>
> [  292.901056]  device_release+0x5a/0x100
> [  292.901063]  kobject_delayed_cleanup+0x5e/0xa0
> [  292.901066]  process_one_work+0x535/0xa50
> [  292.901073]  ? __pfx_process_one_work+0x10/0x10
> [  292.901077]  ? __pfx_do_raw_spin_lock+0x10/0x10
> [  292.901082]  ? __list_add_valid+0x33/0xd0
> [  292.901087]  worker_thread+0x8a/0x620
> [  292.901091]  ? __kthread_parkme+0xd3/0xf0
> [  292.901095]  ? __pfx_worker_thread+0x10/0x10
> [  292.901099]  kthread+0x173/0x1b0
> [  292.901102]  ? __pfx_kthread+0x10/0x10
> [  292.901105]  ret_from_fork+0x2c/0x50
> [  292.901110]  </TASK>
> [  292.901112] Modules linked in: vivid v4l2_tpg videobuf2_dma_contig v4l2_dv_timings videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videodev videobuf2_common mc
> [  292.901126] CR2: ffff8881b1e201b8
> [  292.901130] ---[ end trace 0000000000000000 ]---
> [  292.901133] RIP: 0010:__mutex_lock+0xdb/0xcc0
> [  292.901138] Code: c0 e8 f9 4f 57 fe 2e 2e 2e 31 c0 48 c7 c7 60 d2 31 86 e8 98 07 84 fe 8b 05 d2 83 5f 03 85 c0 75 13 48 8d 7b 60 e8 b5 08 84 fe <48> 3b 5b 60 0f 85 e9 05 00 00 bf 01 00 00 00 e8 41
> 60 57 fe 48 8d
> [  292.901141] RSP: 0018:ffffc90002727a40 EFLAGS: 00010282
> [  292.901145] RAX: 0000000000000000 RBX: 0493011a00000f42 RCX: ffffffff82d24e9b
> [  292.901148] RDX: 0000000000000000 RSI: 0000000000000008 RDI: 0493011a00000fa2
> [  292.901151] RBP: ffffc90002727b90 R08: ffffffff81e74db3 R09: ffffffff84879fd7
> [  292.901154] R10: ffffc90002727ba8 R11: 0000000000000000 R12: 0000000000000000
> [  292.901157] R13: 0000000000000000 R14: ffff8881582b3078 R15: 1ffff920004e4f54
> [  292.901160] FS:  0000000000000000(0000) GS:ffff8881f6300000(0000) knlGS:0000000000000000
> [  292.901169] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  292.901172] CR2: ffff8881b1e201b8 CR3: 0000000149c63000 CR4: 0000000000750ee0
> [  292.901185] PKRU: 55555554
> 
> The test-media regression test is really good at testing unbind scenarios for the virtual
> drivers that we have. Together with the CONFIG_DEBUG_KOBJECT_RELEASE option it is a
> good test to run.

Thanks for the hint. I'll look into this and address it for v2.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 21/26] media: ipu3-cio2: Don't use devm_request_irq()

[Sakari Ailus]

Hi Hans,

On Fri, Mar 03, 2023 at 12:58:52PM +0200, Sakari Ailus wrote:
> > devm_request_irq() is used a lot in the kernel, so if this is a
> > common issue, then just fixing it in two drivers isn't going to make
> > much of a difference.
> 
> I came to think of this after sending the patch as well. It's memory that
> is the problem, any hardware access needs to end before remove is called.
> 
> I'll drop the devm removal.

Actually, the reason why devm_request_irq() isn't a great idea is that if
an interrupt arrives between device's remove function and actually
releasing that IRQ, bad things will (again) happen. So it's only safe to
use devm_request_irq() if you can guarantee you're actually not going to
get an interrupt then. You generally shouldn't be getting one, but that's
not the same thing than being sure.

-- 
Sakari Ailus