Re: [PATCH v2] media: uvcvideo: Fix deadlock if uvc_status_stop is called from async_ctrl.work

[Hans de Goede <hansg@kernel.org>]

Hi,

On 24-Mar-26 18:38, Sean Anderson wrote:
> On 3/23/26 19:52, Laurent Pinchart wrote:
>> Hi Sean,
>>
>> Thank you for the patch.
>>
>> On Mon, Mar 16, 2026 at 11:58:22AM -0400, Sean Anderson wrote:
>>> If a UVC camera has an asynchronous control, uvc_status_stop may be
>>> called from async_ctrl.work:
>>>
>>> uvc_ctrl_status_event_work()
>>>     uvc_ctrl_status_event()
>>>         uvc_ctrl_clear_handle()
>>> 	    uvc_pm_put()
>>> 	        uvc_status_put()
>>> 		    uvc_status_stop()
>>> 		        cancel_work_sync()
>>>
>>> This will cause a deadlock, since cancel_work_sync will wait for
>>> uvc_ctrl_status_event_work to complete before returning.
>>>
>>> Fix this by returning early from uvc_status_stop if we are currently in
>>> the work function. flush_status now remains false until uvc_status_start
>>> is called again, ensuring that uvc_ctrl_status_event_work won't resubmit
>>> the URB.
>>>
>>> Fixes: a32d9c41bdb8 ("media: uvcvideo: Make power management granular")
>>> Closes: https://lore.kernel.org/all/6733bdfb-3e88-479f-8956-ab09c04c433e@linux.dev/
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>>
>>> Changes in v2:
>>> - Update comments with review feedback
>>> - Use flush_work instead of cancel_work_sync since the work should never
>>>   be rescheduled.
>>>
>>>  drivers/media/usb/uvc/uvc_status.c | 27 ++++++++++++++++++---------
>>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
>>> index 231cfee8e7c2c..ea45b11642e59 100644
>>> --- a/drivers/media/usb/uvc/uvc_status.c
>>> +++ b/drivers/media/usb/uvc/uvc_status.c
>>> @@ -316,6 +316,15 @@ static int uvc_status_start(struct uvc_device *dev, gfp_t flags)
>>>  	if (!dev->int_urb)
>>>  		return 0;
>>>  
>>> +	/*
>>> +	 * If the work called uvc_status_stop it may still be running. Wait for
>>> +	 * it to finish before we submit the urb.
>>> +	 */
>>> +	flush_work(&dev->async_ctrl.work);
>>
>> I assume the only way this can happen is for uvc_status_start() to run
>> after the uvc_status_put() call in
>>
>> uvc_ctrl_status_event_work()
>>     uvc_ctrl_status_event()
>>         uvc_ctrl_clear_handle()
>> 	    uvc_pm_put()
>> 	        uvc_status_put()
>>
>> returns. Is that right ? If so I'd like to capture that better in the
>> comment, as I have a feeling we'll need to revisit this code at some
>> point.
>>
>> 	/*
>> 	 * If the previous uvc_status_stop() call was from the async work, the
>> 	 * work may still be running. Wait for it to finish before we submit the
>> 	 * urb.
>> 	 */
>>
>> And if that's not correct, there's something I don't get and a better
>> comment is even more important :-)
>>
>> If the above change is fine, I can update the comment when applying, and
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Fine by me

Thank you everyone.

I've merged this in my local uvc/for-next branch with Laurent's updated
comment and with Laurent's and Ricardo's Reviewed-by added.

I'll push this out to gitlab for CI later today.

Regards,

Hans




> 
>> This being said, I wonder if we could have a simpler implementation if
>> we always stopped the work and URB when the last userspace user goes
>> away, instead of keeping the device active until we get the asynchronous
>> control notification.
>>
>>> +
>>> +	/* Clear the flush status if we were previously stopped. */
>>> +	smp_store_release(&dev->flush_status, false);
>>> +
>>>  	return usb_submit_urb(dev->int_urb, flags);
>>>  }
>>>  
>>> @@ -336,6 +345,15 @@ static void uvc_status_stop(struct uvc_device *dev)
>>>  	 */
>>>  	smp_store_release(&dev->flush_status, true);
>>>  
>>> +	/*
>>> +	 * If we are called from the event work function, the URB is guaranteed
>>> +	 * to not be in flight as it has completed and has not been resubmitted.
>>> +	 * There's no need to cancel the work (which would deadlock), or to kill
>>> +	 * the URB.
>>> +	 */
>>> +	if (current_work() == &w->work)
>>> +		return;
>>> +
>>>  	/*
>>>  	 * Cancel any pending asynchronous work. If any status event was queued,
>>>  	 * process it synchronously.
>>> @@ -354,15 +372,6 @@ static void uvc_status_stop(struct uvc_device *dev)
>>>  	 */
>>>  	if (cancel_work_sync(&w->work))
>>>  		uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
>>> -
>>> -	/*
>>> -	 * From this point, there are no events on the queue and the status URB
>>> -	 * is dead. No events will be queued until uvc_status_start() is called.
>>> -	 * The barrier is needed to make sure that flush_status is visible to
>>> -	 * uvc_ctrl_status_event_work() when uvc_status_start() will be called
>>> -	 * again.
>>> -	 */
>>> -	smp_store_release(&dev->flush_status, false);
>>>  }
>>>  
>>>  int uvc_status_resume(struct uvc_device *dev)
>>
>