Re: [PATCH v2] media: usb: siano: fix URB work teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Calling `cancel_work_sync()` on zero-initialized `work_struct` triggers a kernel warning and potential oops on early initialization error paths.

Pre-existing issues:
- [High] Deadlock during USB disconnect due to mismatched `wait_event()` and `wake_up_interruptible()` in buffer management.
- [High] Ignored URB submission failure causes permanent resource loss and breaks the streaming pipeline.
--

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

media: usb: siano: fix URB work teardown

This patch reinitializes the URB work items once before URB allocation
and addresses teardown races that can trigger workqueue warnings. It also
adjusts the teardown sequence so that any URB submitted by an already-running
worker completes before buffers and the device are freed.

Closes: https://syzkaller.appspot.com/bug?extid=0d6ef2b7ceb6014d756c

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

[ ... ]

> @@ -72,7 +73,8 @@ static void do_submit_urb(struct work_struct *work)
>  	struct smsusb_urb_t *surb = container_of(work, struct smsusb_urb_t, wq);
>  	struct smsusb_device_t *dev = surb->dev;
>  
> -	smsusb_submit_urb(dev, surb);
> +	if (READ_ONCE(dev->streaming))
> +		smsusb_submit_urb(dev, surb);

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
smsusb_submit_urb() here cause permanent resource loss?

If transient failures occur (e.g., -ENOMEM under memory pressure), the URB
is not queued to the USB core, and its completion handler
(smsusb_onresponse) will never trigger.

Because the URB worker is only rescheduled from the completion handler, the
URB and its associated buffer are permanently dropped from the streaming
pipeline. If this happens to all URBs, the receiver silently stalls.

>  }

[ ... ]

> @@ -177,10 +177,12 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev)
>  {
>  	int i;
>  
> +	WRITE_ONCE(dev->streaming, false);
> +
>  	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);
> +		cancel_work_sync(&dev->surbs[i].wq);

[Severity: High]
Will this unconditional call to cancel_work_sync() execute on uninitialized
work items if smsusb_init_device() fails early?

If smsusb_init_device() fails during early initialization (e.g., due to
missing endpoints or smscore_register_device() returning an error), the
error path calls smsusb_term_device(), which invokes smsusb_stop_streaming().

Because this patch removed the safety check on wq.func and relocated the
INIT_WORK() loop further down in smsusb_init_device(), the work_struct
objects remain zero-initialized by kzalloc_obj().

Calling cancel_work_sync() unconditionally on a zero-initialized work_struct
modifies uninitialized fields and triggers a warning in __flush_work(),
leading to backtraces on device probe failure.

[Severity: High]
This is a pre-existing issue, but can this cancel_work_sync() hang forever
waiting for a deadlocked worker thread?

In drivers/media/common/siano/smscoreapi.c:smscore_getbuffer(), the code
uses wait_event() which puts the thread into TASK_UNINTERRUPTIBLE:

    wait_event(coredev->buffer_mng_waitq, (cb = get_entry(coredev)));

However, smscore_putbuffer() signals waiters using wake_up_interruptible(),
which only wakes TASK_INTERRUPTIBLE tasks:

    wake_up_interruptible(&coredev->buffer_mng_waitq);

This mismatch means any worker thread blocked in smscore_getbuffer() will
hang permanently. During a USB disconnect or teardown, cancel_work_sync()
will wait forever for the deadlocked worker, resulting in an unrecoverable
hang of the USB hub thread.

> +		usb_kill_urb(dev->surbs[i].urb);
>  
>  		if (dev->surbs[i].cb) {
>  			smscore_putbuffer(dev->coredev, dev->surbs[i].cb);

[ ... ]

> @@ -468,6 +472,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>  	/* initialize urbs */
>  	for (i = 0; i < MAX_URBS; i++) {
>  		dev->surbs[i].dev = dev;
> +		INIT_WORK(&dev->surbs[i].wq, do_submit_urb);
> +	}
> +
> +	for (i = 0; i < MAX_URBS; i++) {
>  		dev->surbs[i].urb = usb_alloc_urb(0, GFP_KERNEL);
>  		if (!dev->surbs[i].urb)
>  			goto err_unregister_device;

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