Re: [PATCH] media: videobuf2: forbid create_bufs/remove_bufs when legacy fileio is active

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 20.10.2025 10:30, Benjamin Gaignard wrote:
> Le 20/10/2025 à 10:21, Marek Szyprowski a écrit :
>> On 20.10.2025 09:48, Benjamin Gaignard wrote:
>>> Le 20/10/2025 à 09:39, Hans Verkuil a écrit :
>>>> On 20/10/2025 09:34, Marek Szyprowski wrote:
>>>>> On 20.10.2025 09:11, Benjamin Gaignard wrote:
>>>>>> Le 16/10/2025 à 13:11, Marek Szyprowski a écrit :
>>>>>>> create_bufs and remove_bufs ioctl calls manipulate queue internal
>>>>>>> buffer
>>>>>>> list, potentially overwriting some pointers used by the legacy 
>>>>>>> fileio
>>>>>>> access mode. Simply forbid those calls when fileio is active to
>>>>>>> protect
>>>>>>> internal queue state between subsequent read/write calls.
>>>>>> Hi Marek,
>>>>>>
>>>>>> I may be wrong but using fileio API and create/remove API at the 
>>>>>> same
>>>>>> time
>>>>>> sound incorrect from application point of view, right ? If that not
>>>>>> the
>>>>>> case maybe we should also add a test in v4l2-compliance.
>>>>> Definitely that's incorrect and v4l2-core must forbid such calls. The
>>>>> standard reqbufs/qbuf/dqbuf API is also forbidden. Extending
>>>>> v4l2-compliance tools is probably a good idea.
>>>> Yes, please! A patch is welcome.
>>>>
>>>>    I also wonder if its a
>>>>> good time to add a kernel option to completely disable legacy fileio
>>>>> access mode, as it is not really needed for most of the systems
>>>>> nowadays.
>>>> No, that will break applications. Using read() is very common (and
>>>> convenient!)
>>>> for MPEG encoders such as the cx18 driver.
>>>>
>>>> The fileio code is not blocking any new development, it's just there
>>>> for those
>>>> drivers were it makes sense.
>>>>
>>>> Regards,
>>>>
>>>>      Hans
>>> I wonder if this patch in useful because when calling
>>> vb2_ioctl_create_bufs()
>>> it already check in vb2_verify_memory_type() if fileio is used or not.
>> Frankly speaking the original report I got was about mixing fileio with
>> vb2_ioctl_remove_bufs and that case is indeed not protected.
>>
>> While analyzing that I've inspected a symmetrical ioctl
>> (vb2_ioctl_create_bufs), but it looks I've I missed that a check is in
>> vb2_verify_memory_type(). I will remove it in v2 then.
>
> To keep vb2_ioctl_remove_bufs() symmetrical to vb2_ioctl_create_bufs()
> we should do in vb2_ioctl_remove_bufs() something like :
> res = vb2_verify_memory_type(vdev->queue, vdev->queue->memory, d->type);
> instead of vdev->queue->type != d->type.
>
> This way we test fileio too.


Right, that will fit best. I've sent a v2 with such change. Btw, the 
vb2_verify_memory_type() name is a bit misleading in this context. Maybe 
it should be renamed to something like vb2_is_queue_compatible()?


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland