diff mbox series

[3/6] media: pwc: Return queued buffers on start_streaming() failure

Message ID d0fdfb94b56871e757812bfb7aa58e83f7215903.1778518085.git.vebohr@gmail.com (mailing list archive)
State New
Headers
Series media: vb2: Return queued buffers from start_streaming() on error |

Commit Message

Valery Borovsky May 11, 2026, 5:12 p.m. UTC 
The vb2 framework hands buffers to the driver via buf_queue() before
calling start_streaming().  If start_streaming() returns an error
without first returning those buffers via vb2_buffer_done(),
vb2_start_streaming() fires WARN_ON(owned_by_drv_count) and the queued
buffers leak.

pwc's start_streaming() had two early returns that hit this trap:
-ENODEV when the USB device was already disconnected, and -ERESTARTSYS
when mutex_lock_interruptible() was interrupted by a signal.  Call the
existing pwc_cleanup_queued_bufs() helper with VB2_BUF_STATE_QUEUED
before returning (matching the state already used by the
pwc_isoc_init() error path in the same function).

This mirrors the uvcvideo fix in commit 4cf3b6fd54eb ("media: uvcvideo:
Return queued buffers on start_streaming() failure").

Fixes: ceede9fa8939 ("[media] pwc: Fix locking")
Cc: stable@vger.kernel.org
Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
 drivers/media/usb/pwc/pwc-if.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org May 12, 2026, 11:37 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete Buffer Cleanup on Error in `start_streaming()` causing a buffer leak
--

commit 39f02233e64ac5cb48aca4fd3aaf5db7ee99320b
Author: Valery Borovsky <vebohr@gmail.com>
Subject: media: pwc: Return queued buffers on start_streaming() failure

This commit addresses leaked buffers during start_streaming() failures
by ensuring queued buffers are returned to videobuf2 before returning an
error on the early exit paths.

Cc: stable@vger.kernel.org

> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -710,11 +710,15 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	struct pwc_device *pdev = vb2_get_drv_priv(vq);
>  	int r;
>  
> -	if (!pdev->udev)
> +	if (!pdev->udev) {
> +		pwc_cleanup_queued_bufs(pdev, VB2_BUF_STATE_QUEUED);

Does this cleanup catch buffers that might have been moved to pdev->fill_buf?

While this isn't a new regression introduced by this specific patch, it
looks like a buffer leak could still happen if pwc_isoc_init() fails later
in this function.

During pwc_isoc_init(), usb_submit_urb() is called with GFP_KERNEL and
can sleep. If a previously submitted URB completes successfully before the
loop finishes, its completion handler pwc_isoc_handler() can execute and
move a buffer from pdev->queued_bufs into pdev->fill_buf.

If a subsequent usb_submit_urb() fails, pwc_isoc_init() aborts and kills
the URBs. The error path in start_streaming() then executes
pwc_cleanup_queued_bufs(). Since pdev->fill_buf is no longer in the list,
it would be ignored by the cleanup routine and leaked, triggering the
same WARN_ON(owned_by_drv_count) trap.

Should pdev->fill_buf be explicitly checked and returned to videobuf2 on
failure, similar to how it is handled in stop_streaming()?

>  		return -ENODEV;
> +	}
diff mbox series

Patch

diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index c416e2fc5754..59b99ac8fcb6 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -710,11 +710,15 @@  static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct pwc_device *pdev = vb2_get_drv_priv(vq);
 	int r;
 
-	if (!pdev->udev)
+	if (!pdev->udev) {
+		pwc_cleanup_queued_bufs(pdev, VB2_BUF_STATE_QUEUED);
 		return -ENODEV;
+	}
 
-	if (mutex_lock_interruptible(&pdev->v4l2_lock))
+	if (mutex_lock_interruptible(&pdev->v4l2_lock)) {
+		pwc_cleanup_queued_bufs(pdev, VB2_BUF_STATE_QUEUED);
 		return -ERESTARTSYS;
+	}
 	/* Turn on camera and set LEDS on */
 	pwc_camera_power(pdev, 1);
 	pwc_set_leds(pdev, leds[0], leds[1]);