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