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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux