On 13/03/2023 17:53, Sakari Ailus wrote:
> Hi Hans,
>
> On Mon, Mar 13, 2023 at 03:39:25PM +0100, Hans Verkuil wrote:
>> On 13/03/2023 15:02, Sakari Ailus wrote:
>>> 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.
>>
>> Why?
>>
>> I have a filehandle open on /dev/mediaX. Now I unbind the device. That will
>> unregister the media device, which will cause all file ops on the filehandle
>> to return immediately, except for close().
>>
>> And close() just frees the devnode from what I can see.
>>
>> There is a race if the device is unbound while in an ioctl, then all bets are
>> off without proper life-time management.
>>
>> If it crashes after an unbind in the close() call, then something else is
>> wrong, it shouldn't do that.
>>
>> What happens if you do 'sleep 20 </dev/mediaX', then unbind the device?
>>
>> I feel that I am missing something here.
>
> Actually these seems to be a bug in the 25th patch --- testing the devnode
> registration needs to take place before checking for fops.
>
> After fixing that, the difference of issuing read(2) after unregistering
> the device is between (probably) crashing and graciously failing. The
> underlying problem is that without the 25th patch there pretty much is a
> guarantee devnode has been released by the time it is accessed.
Ah, it's a result of patch 06/26. Now devnode is embedded in struct media_device,
which in turn is embedded in a device's state structure. And when that is freed,
the devnode is also freed.
This is wrong IMHO: either struct media_devnode or struct media_device has to
be dynamically allocated. Embedding devices in a larger state structure causes
exactly these types of problems.
In this case I would just keep dynamically allocating the devnode. What is the reason
for reverting that patch? The commit log doesn't say.
Regards,
Hans
