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@xxxxxxxxxxxxxxx>
> > ---
> > 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
