| Message ID | 20240610100530.1107771-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Media device lifetime management | expand |
On 10/06/2024 12:05, Sakari Ailus wrote: > 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. Hmm, this too is due to a revert patch (03/26) removing something that needs to be reinstated. Wouldn't it be better to fold this into 04/26, with a comment in the commit log of that commit? The problem with this commit log is that it suggests that it fixes a bug, when really it just corrects something introduced by a revert. Regards, Hans > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/mc/mc-devnode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 2e33c2007f08..707593d127a7 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -252,9 +252,9 @@ 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); > + put_device(&devnode->dev); > > return ret; > }
On 10/06/2024 12:05, Sakari Ailus wrote: > The video device depends on the existence of its media device --- if there > is one. Acquire a reference to it. > > Note that when the media device release callback is used, then the V4L2 > device release callback is ignored and a warning is issued if both are > set. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > --- > drivers/media/v4l2-core/v4l2-dev.c | 53 ++++++++++++++++++++---------- > 1 file changed, 35 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c > index be2ba7ca5de2..4bf4398fd2fe 100644 > --- a/drivers/media/v4l2-core/v4l2-dev.c > +++ b/drivers/media/v4l2-core/v4l2-dev.c > @@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd) > { > struct video_device *vdev = to_video_device(cd); > struct v4l2_device *v4l2_dev = vdev->v4l2_dev; > + bool v4l2_dev_call_release = v4l2_dev->release; > +#ifdef CONFIG_MEDIA_CONTROLLER > + struct media_device *mdev = v4l2_dev->mdev; > + bool mdev_has_release = mdev && mdev->ops && mdev->ops->release; > +#endif > > mutex_lock(&videodev_lock); > if (WARN_ON(video_devices[vdev->minor] != vdev)) { > @@ -198,8 +203,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) > @@ -207,23 +212,28 @@ static void v4l2_device_release(struct device *cd) > } > #endif > > - /* Do not call v4l2_device_put if there is no release callback set. > - * Drivers that have no v4l2_device release callback might free the > - * v4l2_dev instance in the video_device release callback below, so we > - * must perform this check here. > - * > - * TODO: In the long run all drivers that use v4l2_device should use the > - * v4l2_device release callback. This check will then be unnecessary. > - */ > - if (v4l2_dev->release == NULL) > - v4l2_dev = NULL; > - > /* Release video_device and perform other > cleanups as needed. */ > vdev->release(vdev); > > - /* Decrease v4l2_device refcount */ > - if (v4l2_dev) > +#ifdef CONFIG_MEDIA_CONTROLLER > + if (mdev) > + media_device_put(mdev); > + > + /* > + * Generally both struct media_device and struct v4l2_device are > + * embedded in the same driver's context struct so having a release > + * callback in both is a bug. > + */ > + if (WARN_ON(v4l2_dev_call_release && mdev_has_release)) > + v4l2_dev_call_release = false; > +#endif > + > + /* > + * Decrease v4l2_device refcount, but only if the media device doesn't > + * have a release callback. > + */ > + if (v4l2_dev_call_release) > v4l2_device_put(v4l2_dev); > } > > @@ -795,11 +805,17 @@ static int video_register_media_controller(struct video_device *vdev) > u32 intf_type; > int ret; > > - /* Memory-to-memory devices are more complex and use > - * their own function to register its mc entities. > + if (!vdev->v4l2_dev->mdev) > + return 0; > + > + /* > + * Memory-to-memory devices are more complex and use their own function > + * to register its mc entities. > */ > - if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) > + if (vdev->vfl_dir == VFL_DIR_M2M) { > + media_device_get(vdev->v4l2_dev->mdev); > return 0; > + } > > vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE; > vdev->entity.function = MEDIA_ENT_F_UNKNOWN; > @@ -878,6 +894,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; > }
On 10/06/2024 12:05, Sakari Ailus wrote: > 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> That should be: Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/mc/mc-device.c | 54 ++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > index dd4d589a6701..f1f3addf7932 100644 > --- a/drivers/media/mc/mc-device.c > +++ b/drivers/media/mc/mc-device.c > @@ -673,6 +673,33 @@ void media_device_unregister_entity(struct media_entity *entity) > } > EXPORT_SYMBOL_GPL(media_device_unregister_entity); > > +void 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); > +} > +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); > @@ -740,33 +767,6 @@ int __must_check __media_device_register(struct media_device *mdev, > } > EXPORT_SYMBOL_GPL(__media_device_register); > > -void 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); > -} > -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;
On 10/06/2024 12:05, Sakari Ailus wrote: > The media device and associated resources may be released only when its > memory is no longer used. Warn about drivers not doing this, but instead > releasing the resources at driver unbind time. I think this should be folded in the previous patch. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/mc/mc-device.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > index 8cdd0d46e865..51836faa6d1a 100644 > --- a/drivers/media/mc/mc-device.c > +++ b/drivers/media/mc/mc-device.c > @@ -822,6 +822,9 @@ int __must_check __media_device_register(struct media_device *mdev, > ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL); > if (!ref) > return -ENOMEM; > + > + dev_warn(mdev->dev, > + "Set mdev release op to safely release resources!\n"); I think this needs a comment as well. Basically stating the same as the commit log message. Regards, Hans > } > > /* Register the device node. */
On 10/06/2024 12:05, Sakari Ailus wrote: > A media devnode may be registered only once. Enforce this by setting the > minor to -1 in init. > > Registration initialises the character device and sets up the device name. > These should take place only once during the lifetime of the media device. This has nothing to do with patch 23/26, right? I would move this to before that patch. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > --- > drivers/media/mc/mc-devnode.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 8cb4e0eec17f..758971f310c3 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -240,6 +240,7 @@ void media_devnode_init(struct media_devnode *devnode) > { > device_initialize(&devnode->dev); > devnode->dev.release = media_devnode_release; > + devnode->minor = -1; > } > > int __must_check media_devnode_register(struct media_devnode *devnode, > @@ -251,6 +252,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > int minor; > int ret; > > + if (devnode->minor != -1) > + return -EINVAL; > + > /* Part 1: Find a free minor number */ > mutex_lock(&media_devnode_lock); > minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES);
On 10/06/2024 12:05, 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. > > Some patches are temporarily reverted in order to make reworks easier, > then applied later on. Others are not re-applied: this is a change of > direction, not development over those patches. It would be possible to > squash the reverts into others on the expense of readability, so the > reverts are maintained for that reason. > > 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. I've also tested the vimc driver. You need to convert at least one m2m test driver (vicodec and ideally also vim2m). M2M device have their own media controller setup, so it is good to have at least one converted. Regards, Hans > > Questions and comments are welcome. > > since v3: > > - Rework commit message of patch re-converting to cdev_device_add(). It's > no longer the same patch. > > - Convert drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c > and drivers/media/test-drivers/visl/visl-core.c to changed > media_devnode_is_registered() argument. > > - Properly check for NULL Media device on a V4L2 device in > video_register_media_controller(). > > - Don't acquire Media device graph mutex in media_device_unregister() for > checking whether the media device is registered. > > - Remove extra call to v4l2_async_nf_cleanup() in > drivers/media/platform/ti/omap3isp/isp.c. > > since v2: > > - Switch to spin_{,un}lock_irq() in fw_list_lock use. > > - Clarify documentatino regarding unregistering and releasing the media > device. > > - Release minor number at unregister time (vs. release time). This > effectively caused a few patches to be dropped and one more to be added > (mc: Clear minor number reservation at unregistration time). > > - Added a comment in ipu3-cio2 driver to clarify destroying mutexes in > cio2_queue_exit(). > > - In ipu3-cio2 driver: > > - Clean up the notifier in probe. > > - Unregister the sub-devices at driver unbind time. > > - Remove queue initialisation error handling. The queues are > released in cio2_queues_exit() later in any case. > > - Clean up V4L2 device teardown as suggested by Hans. > > - Rewrap added text in video_register_media_controller(). > > - Make media_device_{get,put}() functions (they were macros) for better > type checking. > > - Improve kerneldoc for media_device_cleanup(). > > - Document release function in media_file_operations. > > - Correct kerneldoc documentation for struct media_devnode. > > - Fix media_devnode_is_registered() usage in sound/usb/media.c that was > missed in v2. > > - Drop old git tags from revert² patches. > > - Drop revert and re-applification of "media: uvcvideo: Refactor teardown > of uvc on USB disconnect", instead take this account into other patches. > > - Drop patch "ipu3-cio2: Call v4l2_device_unregister() earlier". > > - Remove patch "ipu3-cio2: Request IRQ earlier" from this set, it'll be > merged separately. > > since v1: > > - Align subject prefixes with current media tree practices. > > - Make release changes to the vimc driver (last patch of the set). This > was actually easy as vimc already centralised resource release to struct > v4l2_device, so it was just moved to the media device. > > - Move cdev field to struct media_devnode_compat_ref and add dev field to > the struct, these are needed during device release. This now includes > also the character device which is accessed by __fput(). I've now tested > ipu3-cio2 and vimc with KASAN. As a by-product the kref in struct > media_devnode_compat_ref becomes redundant and is removed. Both devices > are registered in case of best effort memory safety support and used for > refcounting. > > - Drop omap3isp driver patch moving away from devm_request_irq(). > > - Add a patch to warn of drivers not releasing media device safely (i.e. > relying on the best effort memory safety mechanism without refcounting). > > - Add a patch to document how the best effort memory release safety helper > works. > > - Add a note on releasing driver's context with the media device, not the > V4L2 device, in MC documentation. > > - Check media device is registered before accessing its fops in > media_read(), media_write(), media_ioctl and media_compat_ioctl(). > > - Document best effort media device lifetime management (new patch). > > - Use media_devnode_free_minor() in unallocating device node minor number > in media_devnode_register(). > > - Continue to rely on devm_register_irq() in ipu3-cio2 driver but register > the IRQ later on (compared to v1). > > - Drop the patch to move away from devm_request_irq() in omap3isp. > > - Fix putting references to media device and V4L2 device in > v4l2_device_release(). > > - Add missing media_device_get() (in v1) for M2M devices in > video_register_media_controller(). > > - Unconditionally set the media devnode release function in > media_device_init(). There's no harm doing so and the caller of > media_device_init() may set the ops after calling the function. > > > Laurent Pinchart (1): > media: mc: Add per-file-handle data support > > Sakari Ailus (25): > 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" > media: mc, cec: Make use of cdev_device_add() again > Revert "[media] media-device: dynamically allocate struct > media_devnode" > media: mc: Drop nop release callback > media: mc: Drop media_dev description from struct media_devnode > media: mc: Do not call cdev_device_del() if cdev_device_add() fails > media: mc: Delete character device early > media: mc: Clear minor number reservation at unregistration time > media: mc: Split initialising and adding media devnode > media: mc: Shuffle functions around > media: mc: Initialise media devnode in media_device_init() > media: mc: Refcount the media device > media: v4l: Acquire a reference to the media device for every video > device > media: mc: Postpone graph object removal until free > media: omap3isp: Release the isp device struct by media device > callback > media: ipu3-cio2: Release the cio2 device context by media device > callback > media: vimc: Release resources on media device release > media: Documentation: Document how Media device resources are released > media: mc: Maintain a list of open file handles in a media device > media: mc: Implement best effort media device removal safety sans > refcount > media: mc: Warn about drivers not releasing media device safely > media: mc: Enforce one-time registration > media: Documentation: Document media device memory safety helper > > Documentation/driver-api/media/mc-core.rst | 18 +- > drivers/media/cec/core/cec-core.c | 2 +- > drivers/media/mc/mc-device.c | 258 +++++++++++------- > drivers/media/mc/mc-devnode.c | 208 +++++++++----- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 72 +++-- > .../vcodec/decoder/mtk_vcodec_dec_drv.c | 2 +- > drivers/media/platform/ti/omap3isp/isp.c | 25 +- > drivers/media/test-drivers/vimc/vimc-core.c | 15 +- > drivers/media/test-drivers/visl/visl-core.c | 2 +- > drivers/media/usb/au0828/au0828-core.c | 4 +- > drivers/media/usb/uvc/uvc_driver.c | 2 +- > drivers/media/v4l2-core/v4l2-dev.c | 67 +++-- > drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- > include/media/media-device.h | 53 +++- > include/media/media-devnode.h | 136 ++++++--- > include/media/media-fh.h | 32 +++ > sound/usb/media.c | 8 +- > 17 files changed, 623 insertions(+), 283 deletions(-) > create mode 100644 include/media/media-fh.h >
Hi Hans, On Mon, Jun 17, 2024 at 11:13:57AM +0200, Hans Verkuil wrote: > On 10/06/2024 12:05, Sakari Ailus wrote: > > 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. > > Hmm, this too is due to a revert patch (03/26) removing something that needs > to be reinstated. > > Wouldn't it be better to fold this into 04/26, with a comment in the commit > log of that commit? I agree, I'll fold it there. > > The problem with this commit log is that it suggests that it fixes a bug, > when really it just corrects something introduced by a revert.
Hi Hans, On Mon, Jun 17, 2024 at 12:40:31PM +0200, Hans Verkuil wrote: > On 10/06/2024 12:05, Sakari Ailus wrote: > > The media device and associated resources may be released only when its > > memory is no longer used. Warn about drivers not doing this, but instead > > releasing the resources at driver unbind time. > > I think this should be folded in the previous patch. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/mc/mc-device.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > > index 8cdd0d46e865..51836faa6d1a 100644 > > --- a/drivers/media/mc/mc-device.c > > +++ b/drivers/media/mc/mc-device.c > > @@ -822,6 +822,9 @@ int __must_check __media_device_register(struct media_device *mdev, > > ref = kzalloc(sizeof(*mdev->devnode.ref), GFP_KERNEL); > > if (!ref) > > return -ENOMEM; > > + > > + dev_warn(mdev->dev, > > + "Set mdev release op to safely release resources!\n"); > > I think this needs a comment as well. Basically stating the same as the > commit log message. I'll address these for v5.
Hi Hans, On Mon, Jun 17, 2024 at 11:41:10AM +0200, Hans Verkuil wrote: > On 10/06/2024 12:05, Sakari Ailus wrote: > > 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> > > That should be: > > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> To be addressed for v5, the same for 16th patch.
Hi Hans, On Mon, Jun 17, 2024 at 12:42:02PM +0200, Hans Verkuil wrote: > On 10/06/2024 12:05, Sakari Ailus wrote: > > A media devnode may be registered only once. Enforce this by setting the > > minor to -1 in init. > > > > Registration initialises the character device and sets up the device name. > > These should take place only once during the lifetime of the media device. > > This has nothing to do with patch 23/26, right? > > I would move this to before that patch. Sounds good. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Thanks!
Hi Hans, On Mon, Jun 17, 2024 at 01:55:53PM +0200, Hans Verkuil wrote: > On 10/06/2024 12:05, 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. > > > > Some patches are temporarily reverted in order to make reworks easier, > > then applied later on. Others are not re-applied: this is a change of > > direction, not development over those patches. It would be possible to > > squash the reverts into others on the expense of readability, so the > > reverts are maintained for that reason. > > > > 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. I've also tested the vimc driver. > > You need to convert at least one m2m test driver (vicodec and ideally also > vim2m). M2M device have their own media controller setup, so it is good to > have at least one converted. My earlier objection to convert vim2m to managed media device lifetime was that vim2m was obviously intended to be compiled without MC but it seems since 016baa59bf9f6 CONFIG_MEDIA_CONTROLLER is selected for the driver. Thus the related #ifdefs can be removed, too. I'll add two patches for this, one that can be merged independently for removing the #ifdefs.
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. Some patches are temporarily reverted in order to make reworks easier, then applied later on. Others are not re-applied: this is a change of direction, not development over those patches. It would be possible to squash the reverts into others on the expense of readability, so the reverts are maintained for that reason. 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. I've also tested the vimc driver. Questions and comments are welcome. since v3: - Rework commit message of patch re-converting to cdev_device_add(). It's no longer the same patch. - Convert drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c and drivers/media/test-drivers/visl/visl-core.c to changed media_devnode_is_registered() argument. - Properly check for NULL Media device on a V4L2 device in video_register_media_controller(). - Don't acquire Media device graph mutex in media_device_unregister() for checking whether the media device is registered. - Remove extra call to v4l2_async_nf_cleanup() in drivers/media/platform/ti/omap3isp/isp.c. since v2: - Switch to spin_{,un}lock_irq() in fw_list_lock use. - Clarify documentatino regarding unregistering and releasing the media device. - Release minor number at unregister time (vs. release time). This effectively caused a few patches to be dropped and one more to be added (mc: Clear minor number reservation at unregistration time). - Added a comment in ipu3-cio2 driver to clarify destroying mutexes in cio2_queue_exit(). - In ipu3-cio2 driver: - Clean up the notifier in probe. - Unregister the sub-devices at driver unbind time. - Remove queue initialisation error handling. The queues are released in cio2_queues_exit() later in any case. - Clean up V4L2 device teardown as suggested by Hans. - Rewrap added text in video_register_media_controller(). - Make media_device_{get,put}() functions (they were macros) for better type checking. - Improve kerneldoc for media_device_cleanup(). - Document release function in media_file_operations. - Correct kerneldoc documentation for struct media_devnode. - Fix media_devnode_is_registered() usage in sound/usb/media.c that was missed in v2. - Drop old git tags from revert² patches. - Drop revert and re-applification of "media: uvcvideo: Refactor teardown of uvc on USB disconnect", instead take this account into other patches. - Drop patch "ipu3-cio2: Call v4l2_device_unregister() earlier". - Remove patch "ipu3-cio2: Request IRQ earlier" from this set, it'll be merged separately. since v1: - Align subject prefixes with current media tree practices. - Make release changes to the vimc driver (last patch of the set). This was actually easy as vimc already centralised resource release to struct v4l2_device, so it was just moved to the media device. - Move cdev field to struct media_devnode_compat_ref and add dev field to the struct, these are needed during device release. This now includes also the character device which is accessed by __fput(). I've now tested ipu3-cio2 and vimc with KASAN. As a by-product the kref in struct media_devnode_compat_ref becomes redundant and is removed. Both devices are registered in case of best effort memory safety support and used for refcounting. - Drop omap3isp driver patch moving away from devm_request_irq(). - Add a patch to warn of drivers not releasing media device safely (i.e. relying on the best effort memory safety mechanism without refcounting). - Add a patch to document how the best effort memory release safety helper works. - Add a note on releasing driver's context with the media device, not the V4L2 device, in MC documentation. - Check media device is registered before accessing its fops in media_read(), media_write(), media_ioctl and media_compat_ioctl(). - Document best effort media device lifetime management (new patch). - Use media_devnode_free_minor() in unallocating device node minor number in media_devnode_register(). - Continue to rely on devm_register_irq() in ipu3-cio2 driver but register the IRQ later on (compared to v1). - Drop the patch to move away from devm_request_irq() in omap3isp. - Fix putting references to media device and V4L2 device in v4l2_device_release(). - Add missing media_device_get() (in v1) for M2M devices in video_register_media_controller(). - Unconditionally set the media devnode release function in media_device_init(). There's no harm doing so and the caller of media_device_init() may set the ops after calling the function. Laurent Pinchart (1): media: mc: Add per-file-handle data support Sakari Ailus (25): 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" media: mc, cec: Make use of cdev_device_add() again Revert "[media] media-device: dynamically allocate struct media_devnode" media: mc: Drop nop release callback media: mc: Drop media_dev description from struct media_devnode media: mc: Do not call cdev_device_del() if cdev_device_add() fails media: mc: Delete character device early media: mc: Clear minor number reservation at unregistration time media: mc: Split initialising and adding media devnode media: mc: Shuffle functions around media: mc: Initialise media devnode in media_device_init() media: mc: Refcount the media device media: v4l: Acquire a reference to the media device for every video device media: mc: Postpone graph object removal until free media: omap3isp: Release the isp device struct by media device callback media: ipu3-cio2: Release the cio2 device context by media device callback media: vimc: Release resources on media device release media: Documentation: Document how Media device resources are released media: mc: Maintain a list of open file handles in a media device media: mc: Implement best effort media device removal safety sans refcount media: mc: Warn about drivers not releasing media device safely media: mc: Enforce one-time registration media: Documentation: Document media device memory safety helper Documentation/driver-api/media/mc-core.rst | 18 +- drivers/media/cec/core/cec-core.c | 2 +- drivers/media/mc/mc-device.c | 258 +++++++++++------- drivers/media/mc/mc-devnode.c | 208 +++++++++----- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 72 +++-- .../vcodec/decoder/mtk_vcodec_dec_drv.c | 2 +- drivers/media/platform/ti/omap3isp/isp.c | 25 +- drivers/media/test-drivers/vimc/vimc-core.c | 15 +- drivers/media/test-drivers/visl/visl-core.c | 2 +- drivers/media/usb/au0828/au0828-core.c | 4 +- drivers/media/usb/uvc/uvc_driver.c | 2 +- drivers/media/v4l2-core/v4l2-dev.c | 67 +++-- drivers/staging/media/sunxi/cedrus/cedrus.c | 2 +- include/media/media-device.h | 53 +++- include/media/media-devnode.h | 136 ++++++--- include/media/media-fh.h | 32 +++ sound/usb/media.c | 8 +- 17 files changed, 623 insertions(+), 283 deletions(-) create mode 100644 include/media/media-fh.h