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

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

[Marek Szyprowski]

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.

CC: stable@vger.kernel.org
Fixes: 2d86401c2cbf ("[media] V4L: vb2: add support for buffers of different sizes on a single queue")
Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index d911021c1bb0..f4104d5971dd 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -751,6 +751,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
 	int ret = vb2_verify_memory_type(q, create->memory, f->type);
 	unsigned i;
 
+	if (vb2_fileio_is_active(q)) {
+		dprintk(q, 1, "file io in progress\n");
+		return -EBUSY;
+	}
+
 	create->index = vb2_get_num_buffers(q);
 	vb2_set_flags_and_caps(q, create->memory, &create->flags,
 			       &create->capabilities, &create->max_num_buffers);
@@ -1010,6 +1015,11 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv,
 	if (vb2_queue_is_busy(vdev->queue, file))
 		return -EBUSY;
 
+	if (vb2_fileio_is_active(vdev->queue)) {
+		dprintk(vdev->queue, 1, "file io in progress\n");
+		return -EBUSY;
+	}
+
 	return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
 }
 EXPORT_SYMBOL_GPL(vb2_ioctl_remove_bufs);
-- 
2.34.1

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

[Benjamin Gaignard]

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.

Regards,
Benjamin

>
> CC: stable@vger.kernel.org
> Fixes: 2d86401c2cbf ("[media] V4L: vb2: add support for buffers of different sizes on a single queue")
> Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/media/common/videobuf2/videobuf2-v4l2.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index d911021c1bb0..f4104d5971dd 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -751,6 +751,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>   	int ret = vb2_verify_memory_type(q, create->memory, f->type);
>   	unsigned i;
>   
> +	if (vb2_fileio_is_active(q)) {
> +		dprintk(q, 1, "file io in progress\n");
> +		return -EBUSY;
> +	}
> +
>   	create->index = vb2_get_num_buffers(q);
>   	vb2_set_flags_and_caps(q, create->memory, &create->flags,
>   			       &create->capabilities, &create->max_num_buffers);
> @@ -1010,6 +1015,11 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv,
>   	if (vb2_queue_is_busy(vdev->queue, file))
>   		return -EBUSY;
>   
> +	if (vb2_fileio_is_active(vdev->queue)) {
> +		dprintk(vdev->queue, 1, "file io in progress\n");
> +		return -EBUSY;
> +	}
> +
>   	return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
>   }
>   EXPORT_SYMBOL_GPL(vb2_ioctl_remove_bufs);

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

[Marek Szyprowski]

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

 > ...

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

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

[Hans Verkuil]

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

> 
>  > ...
> 
> Best regards

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

[Benjamin Gaignard]

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.

>>   > ...
>>
>> Best regards
>

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

[Marek Szyprowski]

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.

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

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

[Benjamin Gaignard]

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.


>
> Best regards

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

[Marek Szyprowski]

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

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

[Marek Szyprowski]

On 16.10.2025 13:11, Marek Szyprowski wrote:
> 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.
>
> CC: stable@vger.kernel.org
> Fixes: 2d86401c2cbf ("[media] V4L: vb2: add support for buffers of different sizes on a single queue")
> Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Just for completeness, as I forgot to add in the initial patch:

Reported-by: Shuangpeng Bai<SJB7183@psu.edu>


> ---
>   drivers/media/common/videobuf2/videobuf2-v4l2.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index d911021c1bb0..f4104d5971dd 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -751,6 +751,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>   	int ret = vb2_verify_memory_type(q, create->memory, f->type);
>   	unsigned i;
>   
> +	if (vb2_fileio_is_active(q)) {
> +		dprintk(q, 1, "file io in progress\n");
> +		return -EBUSY;
> +	}
> +
>   	create->index = vb2_get_num_buffers(q);
>   	vb2_set_flags_and_caps(q, create->memory, &create->flags,
>   			       &create->capabilities, &create->max_num_buffers);
> @@ -1010,6 +1015,11 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv,
>   	if (vb2_queue_is_busy(vdev->queue, file))
>   		return -EBUSY;
>   
> +	if (vb2_fileio_is_active(vdev->queue)) {
> +		dprintk(vdev->queue, 1, "file io in progress\n");
> +		return -EBUSY;
> +	}
> +
>   	return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
>   }
>   EXPORT_SYMBOL_GPL(vb2_ioctl_remove_bufs);

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

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

[Bai, Shuangpeng]

Hi Marek,

Thank you very much for fixing this bug and for your contribution.

Since my original report was not posted to a public mailing list, I would
appreciate it if you could add the following credit to this patch:

Reported-by: Shuangpeng Bai <SJB7183@psu.edu>

Thanks again!

Best regards,
Shuangpeng