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

From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com
Subject: Re: [PATCH 25/26] media: Implement best effort media device removal safety sans refcounting
Date: Fri, 3 Mar 2023 13:08:26 +0200	[thread overview]
Message-ID: <ZAHVKu3OPyd7iLhH@kekkonen.localdomain> (raw)
In-Reply-To: <768335ad-d5ba-d0a7-828a-80c0ec44e41b@xs4all.nl>

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