Re: [PATCH v5] media: siano: fix URB work teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional dereference of `dev->coredev` in `smsusb_stop_streaming()` during early initialization failure paths causes a kernel panic.
--

commit d1f743fe5305e88770fc795f5d8412345cb04702
Author: Rohith Matam <rohithmatham@gmail.com>

media: siano: fix URB work teardown

This commit reinitializes URB work items before scheduling to avoid
workqueue warnings on teardown. It also fixes related lifetime bugs by
adding an abortable buffer wait and ensuring any URB submitted by an
already-running worker is completed before freeing resources.

> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index 0fdc2e0950b77..565543dbd4cb1 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c

[ ... ]

> @@ -168,20 +179,33 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev,
>  		smsusb_onresponse,
>  		surb
>  	);
> -	surb->urb->transfer_flags |= URB_FREE_BUFFER;
> +	rc = usb_submit_urb(surb->urb, GFP_ATOMIC);
> +	if (rc) {
> +		smscore_putbuffer(dev->coredev, surb->cb);
> +		surb->cb = NULL;
> +	}
>  
> -	return usb_submit_urb(surb->urb, GFP_ATOMIC);
> +	return rc;
>  }
>  
>  static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>  {
>  	int i;
>  
> -	for (i = 0; i < MAX_URBS; i++) {
> +	WRITE_ONCE(dev->streaming, false);
> +	WRITE_ONCE(dev->stopping, true);
> +	wake_up(&dev->coredev->buffer_mng_waitq);

[Severity: High]
Can this unconditionally dereference dev->coredev when it is NULL?

If smsusb_init_device() fails its early endpoint checks, it bails out by
calling smsusb_term_device():

drivers/media/usb/siano/smsusb.c:smsusb_init_device() {
...
	if (!dev->in_ep || !dev->out_ep || align < 0) {  /* Missing endpoints? */
		smsusb_term_device(intf);
		return -ENODEV;
	}
...
}

Because smsusb_term_device() unconditionally calls smsusb_stop_streaming(),
and dev->coredev hasn't been initialized yet during this early error path,
could this new wake_up() call trigger a NULL pointer dereference?

> +
> +	for (i = 0; i < MAX_URBS; i++)
>  		usb_kill_urb(dev->surbs[i].urb);
> -		if (dev->surbs[i].wq.func)
> -			cancel_work_sync(&dev->surbs[i].wq);
>  

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601160447.59917-1-rohithmatham@gmail.com?part=1