[PATCH 0/6] media: vb2: Return queued buffers from start_streaming() on error

[PATCH 0/6] media: vb2: Return queued buffers from start_streaming() on error

[Valery Borovsky]

The vb2 framework hands buffer ownership 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 != 0) and the
queued buffers leak.

This was previously fixed for uvcvideo in commit 4cf3b6fd54eb ("media:
uvcvideo: Return queued buffers on start_streaming() failure").  The
same pattern exists in six sibling drivers that still return an errno
early (USB disconnect, signal-interrupted mutex, or media-graph /
format lookup failure) without draining their internal queued-buffer
list.  Each patch reuses or extends the helper / cleanup label that
the driver's other error paths already use.

  airspy.c          - moves mutex_lock above the !s->udev check and
                      jumps to the existing err_clear_bit label, which
                      already drains s->queued_bufs.
  msi2500.c         - rebuilds start_streaming into a goto chain that
                      drains the queue on every failure (including
                      previously-masked set_usb_adc / ctrl_msg paths);
                      parameterizes msi2500_cleanup_queued_bufs() with
                      vb2_buffer_state so the start_streaming path
                      passes _QUEUED.  Rolls back isoc_init via
                      msi2500_isoc_cleanup() on ctrl_msg failure.
  pwc-if.c          - calls pwc_cleanup_queued_bufs(.., _QUEUED) before
                      each early return; state matches the existing
                      pwc_isoc_init() error-path in the same function.
  rtl2832_sdr.c     - calls rtl2832_sdr_cleanup_queued_bufs() before
                      each early return and at the err label.  Adds an
                      explicit success return so the cleanup at err no
                      longer runs on the success path.  Parameterizes
                      the helper with vb2_buffer_state.
  dcmipp-bytecap.c  - replaces the bare -EINVAL return for failed
                      subdev lookup with a goto err_buffer_done.
  sun4i_dma.c       - replaces the bare -EINVAL return for failed
                      csi-format lookup with a goto err_clear_dma_queue.

These sites were located by a Coccinelle pattern that matches
`return -ERRNO` inside a start_streaming(struct vb2_queue *, unsigned
int) callback without a preceding vb2_buffer_done() on the same path.

Build-tested with drivers/media/ allmodconfig.  Not runtime-tested on
any of the six devices.  Maintainers with hardware are kindly asked to
verify nothing regresses on their device.

Valery Borovsky (6):
  media: airspy: Return queued buffers on start_streaming() failure
  media: msi2500: Return queued buffers on start_streaming() failure
  media: pwc: Return queued buffers on start_streaming() failure
  media: rtl2832_sdr: Return queued buffers on start_streaming() failure
  media: stm32-dcmipp: Return queued buffers on start_streaming()
    failure
  media: sun4i-csi: Return queued buffers on start_streaming() failure

 drivers/media/dvb-frontends/rtl2832_sdr.c     | 19 ++++++++---
 .../st/stm32/stm32-dcmipp/dcmipp-bytecap.c    |  6 ++--
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      |  6 ++--
 drivers/media/usb/airspy/airspy.c             |  8 +++--
 drivers/media/usb/msi2500/msi2500.c           | 32 ++++++++++++++-----
 drivers/media/usb/pwc/pwc-if.c                |  8 +++--
 6 files changed, 57 insertions(+), 22 deletions(-)

-- 
2.51.0

[PATCH 1/6] media: airspy: Return queued buffers on start_streaming() failure

[Valery Borovsky]

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.

airspy_start_streaming() returned -ENODEV early when the USB device had
been disconnected (s->udev == NULL) without returning any buffers that
buf_queue() had already accepted.  Take v4l2_lock first and jump to the
existing err_clear_bit label, which already drains s->queued_bufs via
vb2_buffer_done(..., VB2_BUF_STATE_QUEUED) before unlocking.

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

Fixes: 634fe5033951 ("[media] airspy: AirSpy SDR driver")
Cc: stable@vger.kernel.org
Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
 drivers/media/usb/airspy/airspy.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/airspy/airspy.c b/drivers/media/usb/airspy/airspy.c
index 8f6b721ba107..57edb42463e8 100644
--- a/drivers/media/usb/airspy/airspy.c
+++ b/drivers/media/usb/airspy/airspy.c
@@ -522,11 +522,13 @@ static int airspy_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	dev_dbg(s->dev, "\n");
 
-	if (!s->udev)
-		return -ENODEV;
-
 	mutex_lock(&s->v4l2_lock);
 
+	if (!s->udev) {
+		ret = -ENODEV;
+		goto err_clear_bit;
+	}
+
 	s->sequence = 0;
 
 	set_bit(POWER_ON, &s->flags);
-- 
2.51.0

Re: [PATCH 1/6] media: airspy: Return queued buffers on start_streaming() failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Disconnect during active streaming will inevitably cause a NULL pointer dereference in stop_streaming().
--

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

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(), the queued buffers
leak. This patch returns them on failure, including the device disconnect
case where start_streaming() previously exited early.

> diff --git a/drivers/media/usb/airspy/airspy.c b/drivers/media/usb/airspy/airspy.c
> index 8f6b721ba107c..57edb42463e81 100644
> --- a/drivers/media/usb/airspy/airspy.c
> +++ b/drivers/media/usb/airspy/airspy.c
> @@ -522,11 +522,13 @@ static int airspy_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	dev_dbg(s->dev, "\n");
>  
> -	if (!s->udev)
> -		return -ENODEV;
> -
>  	mutex_lock(&s->v4l2_lock);
>  
> +	if (!s->udev) {
> +		ret = -ENODEV;
> +		goto err_clear_bit;
> +	}
> +

This isn't a regression introduced by this patch, but since we are handling
the disconnected state here, what happens if the physical USB device is
unplugged while streaming is already active?

If airspy_disconnect() runs and sets s->udev = NULL, and later the user
process closes the file descriptor triggering stream cleanup:

vb2_core_streamoff()
  airspy_stop_streaming()
    airspy_ctrl_msg(s, CMD_RECEIVER_MODE, 0, 0, NULL, 0)
      usb_sndctrlpipe(s->udev, 0)
        __create_pipe(dev, endpoint)

Could this unconditionally dereference the NULL s->udev pointer (accessing
dev->devnum) and cause a kernel oops?

Additionally, airspy_free_stream_bufs() is called immediately after:

airspy_stop_streaming()
  ...
  airspy_free_stream_bufs(s)
    usb_free_coherent(s->udev, ...)

Would passing the NULL s->udev pointer here also be problematic during
unplug?

Should there be a similar check for !s->udev in airspy_stop_streaming()
before attempting hardware communication and resource cleanup?

>  	s->sequence = 0;
>  
>  	set_bit(POWER_ON, &s->flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778518085.git.vebohr@gmail.com?part=1

[PATCH 2/6] media: msi2500: Return queued buffers on start_streaming() failure

[Valery Borovsky]

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.

msi2500_start_streaming() had five error paths that all hit this trap
and were further tangled by ret-overwriting between calls:

  - -ENODEV when the USB device was already disconnected
  - -ERESTARTSYS when mutex_lock_interruptible() was interrupted
  - msi2500_set_usb_adc() failure: ret was silently overwritten by
    the next call (msi2500_isoc_init), so the error was lost entirely
  - msi2500_isoc_init() failure: cleanup_queued_bufs was called, but
    the function then fell through to msi2500_ctrl_msg() and again
    masked the original error by overwriting ret
  - msi2500_ctrl_msg(CMD_START_STREAMING) failure: no cleanup at all,
    leaving isoc URBs submitted with no way for the driver to consume
    them

Consolidate the error paths into a small goto chain.  Every failure
now stops the function, drains the queued-buffer list, and returns
the real error code.  The ctrl_msg failure path also rolls back the
preceding msi2500_isoc_init() via msi2500_isoc_cleanup() before
unlocking and draining.

The cleanup helper takes a vb2_buffer_state argument so that the
start_streaming error paths can pass VB2_BUF_STATE_QUEUED (as
expected by userspace on start_streaming failure) while stop_streaming
keeps its existing VB2_BUF_STATE_ERROR semantics.

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

Fixes: 977e444f59ad ("[media] Mirics MSi3101 SDR Dongle driver")
Cc: stable@vger.kernel.org
Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
 drivers/media/usb/msi2500/msi2500.c | 32 +++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/msi2500/msi2500.c b/drivers/media/usb/msi2500/msi2500.c
index 1ff98956b680..0614087c3c3c 100644
--- a/drivers/media/usb/msi2500/msi2500.c
+++ b/drivers/media/usb/msi2500/msi2500.c
@@ -541,7 +541,8 @@ static int msi2500_isoc_init(struct msi2500_dev *dev)
 }
 
 /* Must be called with vb_queue_lock hold */
-static void msi2500_cleanup_queued_bufs(struct msi2500_dev *dev)
+static void msi2500_cleanup_queued_bufs(struct msi2500_dev *dev,
+					enum vb2_buffer_state state)
 {
 	unsigned long flags;
 
@@ -554,7 +555,7 @@ static void msi2500_cleanup_queued_bufs(struct msi2500_dev *dev)
 		buf = list_entry(dev->queued_bufs.next,
 				 struct msi2500_frame_buf, list);
 		list_del(&buf->list);
-		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+		vb2_buffer_done(&buf->vb.vb2_buf, state);
 	}
 	spin_unlock_irqrestore(&dev->queued_bufs_lock, flags);
 }
@@ -830,25 +831,40 @@ static int msi2500_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	dev_dbg(dev->dev, "\n");
 
-	if (!dev->udev)
-		return -ENODEV;
+	if (!dev->udev) {
+		ret = -ENODEV;
+		goto err_cleanup;
+	}
 
-	if (mutex_lock_interruptible(&dev->v4l2_lock))
-		return -ERESTARTSYS;
+	if (mutex_lock_interruptible(&dev->v4l2_lock)) {
+		ret = -ERESTARTSYS;
+		goto err_cleanup;
+	}
 
 	/* wake-up tuner */
 	v4l2_subdev_call(dev->v4l2_subdev, core, s_power, 1);
 
 	ret = msi2500_set_usb_adc(dev);
+	if (ret)
+		goto err_unlock_cleanup;
 
 	ret = msi2500_isoc_init(dev);
 	if (ret)
-		msi2500_cleanup_queued_bufs(dev);
+		goto err_unlock_cleanup;
 
 	ret = msi2500_ctrl_msg(dev, CMD_START_STREAMING, 0);
+	if (ret)
+		goto err_isoc_cleanup;
 
 	mutex_unlock(&dev->v4l2_lock);
+	return 0;
 
+err_isoc_cleanup:
+	msi2500_isoc_cleanup(dev);
+err_unlock_cleanup:
+	mutex_unlock(&dev->v4l2_lock);
+err_cleanup:
+	msi2500_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
 	return ret;
 }
 
@@ -863,7 +879,7 @@ static void msi2500_stop_streaming(struct vb2_queue *vq)
 	if (dev->udev)
 		msi2500_isoc_cleanup(dev);
 
-	msi2500_cleanup_queued_bufs(dev);
+	msi2500_cleanup_queued_bufs(dev, VB2_BUF_STATE_ERROR);
 
 	/* according to tests, at least 700us delay is required  */
 	msleep(20);
-- 
2.51.0

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

[Valery Borovsky]

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(-)

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]);
-- 
2.51.0

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

[sashiko-bot]

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;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778518085.git.vebohr@gmail.com?part=3

[PATCH 4/6] media: rtl2832_sdr: Return queued buffers on start_streaming() failure

[Valery Borovsky]

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.

rtl2832_sdr_start_streaming() had multiple error paths that hit this
trap: two direct early returns (-ENODEV, -ERESTARTSYS), plus six
`goto err` paths covering subdev s_power, tuner setup, ADC setup,
stream-buffer allocation, urb allocation, and urb submission failures.
None of them returned the queued buffers.

The original function had no distinct success exit and fell straight
through into the err label, which previously only did mutex_unlock and
"return ret".  Adding queued-buffer cleanup at err must therefore be
paired with an explicit success return; otherwise every successful
start would also drain the buffer queue and kill streaming.  Add that
success return, then add rtl2832_sdr_cleanup_queued_bufs() at the err
label and before each early return.

The cleanup helper takes a vb2_buffer_state argument so that the
start_streaming error paths can pass VB2_BUF_STATE_QUEUED (as
expected by userspace on start_streaming failure) while stop_streaming
keeps its existing VB2_BUF_STATE_ERROR semantics.

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

The err label still does not roll back power_ctrl(), frontend_ctrl(),
the POWER_ON flag, or stream/URB allocations that may have happened
before the failing step.  Those are pre-existing leaks of a different
class and are not addressed here.

Fixes: 771138920eaf ("[media] rtl2832_sdr: Realtek RTL2832 SDR driver module")
Cc: stable@vger.kernel.org
Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
 drivers/media/dvb-frontends/rtl2832_sdr.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
index 422d1a7b5456..c564485e3bbb 100644
--- a/drivers/media/dvb-frontends/rtl2832_sdr.c
+++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
@@ -399,7 +399,8 @@ static int rtl2832_sdr_alloc_urbs(struct rtl2832_sdr_dev *dev)
 }
 
 /* Must be called with vb_queue_lock hold */
-static void rtl2832_sdr_cleanup_queued_bufs(struct rtl2832_sdr_dev *dev)
+static void rtl2832_sdr_cleanup_queued_bufs(struct rtl2832_sdr_dev *dev,
+					    enum vb2_buffer_state state)
 {
 	struct platform_device *pdev = dev->pdev;
 	unsigned long flags;
@@ -413,7 +414,7 @@ static void rtl2832_sdr_cleanup_queued_bufs(struct rtl2832_sdr_dev *dev)
 		buf = list_entry(dev->queued_bufs.next,
 				struct rtl2832_sdr_frame_buf, list);
 		list_del(&buf->list);
-		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+		vb2_buffer_done(&buf->vb.vb2_buf, state);
 	}
 	spin_unlock_irqrestore(&dev->queued_bufs_lock, flags);
 }
@@ -855,11 +856,15 @@ static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	dev_dbg(&pdev->dev, "\n");
 
-	if (!dev->udev)
+	if (!dev->udev) {
+		rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
 		return -ENODEV;
+	}
 
-	if (mutex_lock_interruptible(&dev->v4l2_lock))
+	if (mutex_lock_interruptible(&dev->v4l2_lock)) {
+		rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
 		return -ERESTARTSYS;
+	}
 
 	if (d->props->power_ctrl)
 		d->props->power_ctrl(d, 1);
@@ -900,7 +905,11 @@ static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (ret)
 		goto err;
 
+	mutex_unlock(&dev->v4l2_lock);
+	return 0;
+
 err:
+	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
 	mutex_unlock(&dev->v4l2_lock);
 
 	return ret;
@@ -920,7 +929,7 @@ static void rtl2832_sdr_stop_streaming(struct vb2_queue *vq)
 	rtl2832_sdr_kill_urbs(dev);
 	rtl2832_sdr_free_urbs(dev);
 	rtl2832_sdr_free_stream_bufs(dev);
-	rtl2832_sdr_cleanup_queued_bufs(dev);
+	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_ERROR);
 	rtl2832_sdr_unset_adc(dev);
 
 	/* sleep tuner */
-- 
2.51.0

Re: [PATCH 4/6] media: rtl2832_sdr: Return queued buffers on start_streaming() failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unbounded memory leak and Out-Of-Bounds read/crash due to unrolled allocations on start_streaming failure.
- [High] Silent DMA memory leak on device disconnect due to cleared `udev` pointer.
--

commit b46c390ec17dc3b72b2306b1cffbddd15df567b7
Author: Valery Borovsky <vebohr@gmail.com>

media: rtl2832_sdr: Return queued buffers on start_streaming() failure

This commit fixes a memory leak of queued vb2 buffers when
rtl2832_sdr_start_streaming() returns an error. It adds explicit buffer
cleanup using a new helper function on the error paths.

> diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
> index 422d1a7b5456e..c564485e3bbb5 100644
> --- a/drivers/media/dvb-frontends/rtl2832_sdr.c
> +++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
> @@ -855,11 +856,15 @@ static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	dev_dbg(&pdev->dev, "\n");
>  
> -	if (!dev->udev)
> +	if (!dev->udev) {
> +		rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
>  		return -ENODEV;
> +	}

[ ... ]

> @@ -900,7 +905,11 @@ static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret)
>  		goto err;
>  
> +	mutex_unlock(&dev->v4l2_lock);
> +	return 0;
> +
>  err:
> +	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
>  	mutex_unlock(&dev->v4l2_lock);
>  
>  	return ret;

This isn't a bug introduced by this patch, but if rtl2832_sdr_submit_urbs()
fails and jumps to this error path, do we leak the newly allocated dma stream
buffers and urbs?

If userspace repeatedly attempts to start the stream via the streamon ioctl,
rtl2832_sdr_alloc_stream_bufs() unconditionally resets dev->buf_num = 0 and
overwrites dev->buf_list[], permanently leaking the dma memory.

Furthermore, rtl2832_sdr_alloc_urbs() does not reset dev->urbs_initialized;
it blindly increments it on each successful urb allocation. On a second
attempt, dev->urbs_initialized could exceed the array bounds. Later, when
rtl2832_sdr_free_urbs() is called, could it iterate from
dev->urbs_initialized - 1 down to 0, reading out-of-bounds past the
dev->urb_list[] array and passing garbage pointers to usb_free_urb(),
potentially causing a panic?

[ ... ]

> @@ -920,7 +929,7 @@ static void rtl2832_sdr_stop_streaming(struct vb2_queue *vq)
>  	rtl2832_sdr_kill_urbs(dev);
>  	rtl2832_sdr_free_urbs(dev);
>  	rtl2832_sdr_free_stream_bufs(dev);

This isn't a bug introduced by this patch, but if dev->udev is cleared to
null by the disconnect handler during device removal, does this cause a
silent dma memory leak?

When the user-space application eventually closes its file descriptor after a
device unplug, it triggers the vb2 release paths and calls
rtl2832_sdr_stop_streaming() which calls rtl2832_sdr_free_stream_bufs().

Because dev->udev is already null, usb_free_coherent(dev->udev, ...) will
immediately abort without freeing or unmapping the memory. Does this
guarantee that all dma stream buffers are permanently leaked every time the
sdr dongle is unplugged while streaming?

> -	rtl2832_sdr_cleanup_queued_bufs(dev);
> +	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_ERROR);
>  	rtl2832_sdr_unset_adc(dev);
>  
>  	/* sleep tuner */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778518085.git.vebohr@gmail.com?part=4

[PATCH 5/6] media: stm32-dcmipp: Return queued buffers on start_streaming() failure

[Valery Borovsky]

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.

dcmipp_bytecap_start_streaming() returned -EINVAL when the source
subdevice could not be resolved from the media graph, before
pm_runtime_resume_and_get() and media_pipeline_start() had been called.
The remaining error paths already converge on the err_buffer_done
label, which calls dcmipp_bytecap_all_buffers_done(...,
VB2_BUF_STATE_QUEUED).  Jump to that label directly: the intermediate
err_pm_put / err_media_pipeline_stop labels are skipped, which is
correct because nothing they would undo has happened yet.

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

Fixes: 28e0f3772296 ("media: stm32-dcmipp: STM32 DCMIPP camera interface driver")
Cc: stable@vger.kernel.org
Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
 .../media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c   | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
index a42f43d19f9e..f0e809458489 100644
--- a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
+++ b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
@@ -401,8 +401,10 @@ static int dcmipp_bytecap_start_streaming(struct vb2_queue *vq,
 	 */
 	if (!vcap->s_subdev) {
 		pad = media_pad_remote_pad_first(&vcap->vdev.entity.pads[0]);
-		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
-			return -EINVAL;
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity)) {
+			ret = -EINVAL;
+			goto err_buffer_done;
+		}
 		vcap->s_subdev = media_entity_to_v4l2_subdev(pad->entity);
 		vcap->s_subdev_pad_nb = pad->index;
 	}
-- 
2.51.0

[PATCH 6/6] media: sun4i-csi: Return queued buffers on start_streaming() failure

[Valery Borovsky]

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.

sun4i_csi_start_streaming() returned -EINVAL when no matching CSI
format could be found, before any setup (scratch buffer allocation,
pipeline start) had been performed.  The remaining error paths already
converge on the err_clear_dma_queue label, which calls
return_all_buffers(..., VB2_BUF_STATE_QUEUED) under csi->qlock.  Jump
to that label directly: the intermediate err_disable_device /
err_disable_pipeline / err_free_scratch_buffer labels are skipped,
which is correct because nothing they would undo has happened yet.

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

Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
index e911c7f7acc5..4781db21c205 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
@@ -234,8 +234,10 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
 	int ret;
 
 	csi_fmt = sun4i_csi_find_format(&csi->fmt.pixelformat, NULL);
-	if (!csi_fmt)
-		return -EINVAL;
+	if (!csi_fmt) {
+		ret = -EINVAL;
+		goto err_clear_dma_queue;
+	}
 
 	dev_dbg(csi->dev, "Starting capture\n");
 
-- 
2.51.0