diff mbox series

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

Message ID 649601988189f031670215cb35add5e80439559d.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.

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

Comments

sashiko-bot@kernel.org May 12, 2026, 10:17 p.m. UTC | #1 
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);
diff mbox series

Patch

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