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

[sashiko-bot@kernel.org]

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