* [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active [not found] <CGME20251023113101eucas1p2c227985b0198d888564cab00aeb94f01@eucas1p2.samsung.com> @ 2025-10-23 11:30 ` Marek Szyprowski 2025-10-23 11:36 ` Hans Verkuil 2025-10-23 11:44 ` Marek Szyprowski 0 siblings, 2 replies; 5+ messages in thread From: Marek Szyprowski @ 2025-10-23 11:30 UTC (permalink / raw) To: linux-media, linux-kernel Cc: Marek Szyprowski, Tomasz Figa, Mauro Carvalho Chehab, Guennadi Liakhovetski, Benjamin Gaignard, Hans Verkuil, stable, Shuangpeng Bai vb2_ioctl_remove_bufs() call manipulates queue internal buffer list, potentially overwriting some pointers used by the legacy fileio access mode. Add a vb2_verify_memory_type() check symmetrical to vb2_ioctl_create_bufs() to forbid that ioctl when fileio is active to protect internal queue state between subsequent read/write calls. CC: stable@vger.kernel.org Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl") Reported-by: Shuangpeng Bai<SJB7183@psu.edu> Suggested-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/media/common/videobuf2/videobuf2-v4l2.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index d911021c1bb0..a8a5b42a42d0 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -1000,13 +1000,15 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv, struct v4l2_remove_buffers *d) { struct video_device *vdev = video_devdata(file); - - if (vdev->queue->type != d->type) - return -EINVAL; + int res; if (d->count == 0) return 0; + res = vb2_verify_memory_type(vdev->queue, vdev->queue->memory, d->type); + if (res) + return res; + if (vb2_queue_is_busy(vdev->queue, file)) return -EBUSY; -- 2.34.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active 2025-10-23 11:30 ` [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active Marek Szyprowski @ 2025-10-23 11:36 ` Hans Verkuil 2025-10-23 11:55 ` Marek Szyprowski 2025-10-23 11:44 ` Marek Szyprowski 1 sibling, 1 reply; 5+ messages in thread From: Hans Verkuil @ 2025-10-23 11:36 UTC (permalink / raw) To: Marek Szyprowski, linux-media, linux-kernel Cc: Tomasz Figa, Mauro Carvalho Chehab, Guennadi Liakhovetski, Benjamin Gaignard, Hans Verkuil, stable, Shuangpeng Bai On 23/10/2025 13:30, Marek Szyprowski wrote: > vb2_ioctl_remove_bufs() call manipulates queue internal buffer list, > potentially overwriting some pointers used by the legacy fileio access > mode. Add a vb2_verify_memory_type() check symmetrical to > vb2_ioctl_create_bufs() to forbid that ioctl when fileio is active to > protect internal queue state between subsequent read/write calls. > > CC: stable@vger.kernel.org > Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl") > Reported-by: Shuangpeng Bai<SJB7183@psu.edu> > Suggested-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/media/common/videobuf2/videobuf2-v4l2.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index d911021c1bb0..a8a5b42a42d0 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -1000,13 +1000,15 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv, > struct v4l2_remove_buffers *d) > { > struct video_device *vdev = video_devdata(file); > - > - if (vdev->queue->type != d->type) > - return -EINVAL; > + int res; > > if (d->count == 0) > return 0; Ah, no. This should still check d->type. So: if (d->count == 0) return d->type == vdev->queue->type ? 0 : -EINVAL; Regards, Hans > > + res = vb2_verify_memory_type(vdev->queue, vdev->queue->memory, d->type); > + if (res) > + return res; > + > if (vb2_queue_is_busy(vdev->queue, file)) > return -EBUSY; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active 2025-10-23 11:36 ` Hans Verkuil @ 2025-10-23 11:55 ` Marek Szyprowski 2025-10-23 12:25 ` Hans Verkuil 0 siblings, 1 reply; 5+ messages in thread From: Marek Szyprowski @ 2025-10-23 11:55 UTC (permalink / raw) To: Hans Verkuil, linux-media, linux-kernel Cc: Tomasz Figa, Mauro Carvalho Chehab, Guennadi Liakhovetski, Benjamin Gaignard, Hans Verkuil, stable, Shuangpeng Bai On 23.10.2025 13:36, Hans Verkuil wrote: > On 23/10/2025 13:30, Marek Szyprowski wrote: >> vb2_ioctl_remove_bufs() call manipulates queue internal buffer list, >> potentially overwriting some pointers used by the legacy fileio access >> mode. Add a vb2_verify_memory_type() check symmetrical to >> vb2_ioctl_create_bufs() to forbid that ioctl when fileio is active to >> protect internal queue state between subsequent read/write calls. >> >> CC: stable@vger.kernel.org >> Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl") >> Reported-by: Shuangpeng Bai<SJB7183@psu.edu> >> Suggested-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/media/common/videobuf2/videobuf2-v4l2.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index d911021c1bb0..a8a5b42a42d0 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> @@ -1000,13 +1000,15 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv, >> struct v4l2_remove_buffers *d) >> { >> struct video_device *vdev = video_devdata(file); >> - >> - if (vdev->queue->type != d->type) >> - return -EINVAL; >> + int res; >> >> if (d->count == 0) >> return 0; > Ah, no. This should still check d->type. So: > > if (d->count == 0) > return d->type == vdev->queue->type ? 0 : -EINVAL; Then frankly speaking lets get back to v1 limited to vb2_ioctl_remove_bufs(), as using vb2_verify_memory_type() in this context only makes things harder to understand: 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 @@ -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); > Regards, > > Hans > >> >> + res = vb2_verify_memory_type(vdev->queue, vdev->queue->memory, d->type); >> + if (res) >> + return res; >> + >> if (vb2_queue_is_busy(vdev->queue, file)) >> return -EBUSY; >> Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active 2025-10-23 11:55 ` Marek Szyprowski @ 2025-10-23 12:25 ` Hans Verkuil 0 siblings, 0 replies; 5+ messages in thread From: Hans Verkuil @ 2025-10-23 12:25 UTC (permalink / raw) To: Marek Szyprowski, linux-media, linux-kernel Cc: Tomasz Figa, Mauro Carvalho Chehab, Guennadi Liakhovetski, Benjamin Gaignard, Hans Verkuil, stable, Shuangpeng Bai On 23/10/2025 13:55, Marek Szyprowski wrote: > On 23.10.2025 13:36, Hans Verkuil wrote: >> On 23/10/2025 13:30, Marek Szyprowski wrote: >>> vb2_ioctl_remove_bufs() call manipulates queue internal buffer list, >>> potentially overwriting some pointers used by the legacy fileio access >>> mode. Add a vb2_verify_memory_type() check symmetrical to >>> vb2_ioctl_create_bufs() to forbid that ioctl when fileio is active to >>> protect internal queue state between subsequent read/write calls. >>> >>> CC: stable@vger.kernel.org >>> Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl") >>> Reported-by: Shuangpeng Bai<SJB7183@psu.edu> >>> Suggested-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> drivers/media/common/videobuf2/videobuf2-v4l2.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c >>> index d911021c1bb0..a8a5b42a42d0 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c >>> @@ -1000,13 +1000,15 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv, >>> struct v4l2_remove_buffers *d) >>> { >>> struct video_device *vdev = video_devdata(file); >>> - >>> - if (vdev->queue->type != d->type) >>> - return -EINVAL; >>> + int res; >>> >>> if (d->count == 0) >>> return 0; >> Ah, no. This should still check d->type. So: >> >> if (d->count == 0) >> return d->type == vdev->queue->type ? 0 : -EINVAL; > > Then frankly speaking lets get back to v1 limited to > vb2_ioctl_remove_bufs(), as using vb2_verify_memory_type() in this > context only makes things harder to understand: > > 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 > > @@ -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); I agree. Can you post a v4? Regards, Hans > > >> Regards, >> >> Hans >> >>> >>> + res = vb2_verify_memory_type(vdev->queue, vdev->queue->memory, d->type); >>> + if (res) >>> + return res; >>> + >>> if (vb2_queue_is_busy(vdev->queue, file)) >>> return -EBUSY; >>> > Best regards ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active 2025-10-23 11:30 ` [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active Marek Szyprowski 2025-10-23 11:36 ` Hans Verkuil @ 2025-10-23 11:44 ` Marek Szyprowski 1 sibling, 0 replies; 5+ messages in thread From: Marek Szyprowski @ 2025-10-23 11:44 UTC (permalink / raw) To: linux-media, linux-kernel Cc: Tomasz Figa, Mauro Carvalho Chehab, Guennadi Liakhovetski, Benjamin Gaignard, Hans Verkuil, stable, Shuangpeng Bai On 23.10.2025 13:30, Marek Szyprowski wrote: > vb2_ioctl_remove_bufs() call manipulates queue internal buffer list, > potentially overwriting some pointers used by the legacy fileio access > mode. Add a vb2_verify_memory_type() check symmetrical to > vb2_ioctl_create_bufs() to forbid that ioctl when fileio is active to > protect internal queue state between subsequent read/write calls. > > CC: stable@vger.kernel.org > Fixes: a3293a85381e ("media: v4l2: Add REMOVE_BUFS ioctl") > Reported-by: Shuangpeng Bai<SJB7183@psu.edu> > Suggested-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- History: v3: - moved vb2_verify_memory_type() check after (d->count == 0) check to pass v4l2 compliance v2: https://lore.kernel.org/all/20251020160121.1985354-1-m.szyprowski@samsung.com/ - dropped a change to vb2_ioctl_create_bufs(), as it is already handled by the vb2_verify_memory_type() call - replaced queue->type check in vb2_ioctl_remove_bufs() by a call to vb2_verify_memory_type() which covers all cases v1: https://lore.kernel.org/all/20251016111154.993949-1-m.szyprowski@samsung.com/ > drivers/media/common/videobuf2/videobuf2-v4l2.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index d911021c1bb0..a8a5b42a42d0 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -1000,13 +1000,15 @@ int vb2_ioctl_remove_bufs(struct file *file, void *priv, > struct v4l2_remove_buffers *d) > { > struct video_device *vdev = video_devdata(file); > - > - if (vdev->queue->type != d->type) > - return -EINVAL; > + int res; > > if (d->count == 0) > return 0; > > + res = vb2_verify_memory_type(vdev->queue, vdev->queue->memory, d->type); > + if (res) > + return res; > + > if (vb2_queue_is_busy(vdev->queue, file)) > return -EBUSY; > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-10-23 12:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20251023113101eucas1p2c227985b0198d888564cab00aeb94f01@eucas1p2.samsung.com>
2025-10-23 11:30 ` [PATCH v3] media: videobuf2: forbid remove_bufs when legacy fileio is active Marek Szyprowski
2025-10-23 11:36 ` Hans Verkuil
2025-10-23 11:55 ` Marek Szyprowski
2025-10-23 12:25 ` Hans Verkuil
2025-10-23 11:44 ` Marek Szyprowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®