lkml.org 
[lkml]   [2023]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] xhci: Do not create endpoint debugfs while holding the bandwidth mutex
On 1.6.2023 19.05, Ricardo Ribalda wrote:
> Hi Mathias
>
> On Thu, 1 Jun 2023 at 16:13, Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>>
>> Do you still have the lockdep output showing the deadlock?
>
> [ 459.731142] ======================================================
> [ 459.731150] WARNING: possible circular locking dependency detected
> [ 459.731161] 5.4.169-lockdep-17434-g505c8a10e6fe #1 Not tainted
> [ 459.731168] ------------------------------------------------------
> [ 459.731176] syz-executor.3/15308 is trying to acquire lock:
> [ 459.731184] ffffff80c63e0ee0 (&queue->mutex){+.+.}, at:
> uvc_queue_mmap+0x30/0xa0 [uvcvideo]
> [ 459.731226]
> but task is already holding lock:
> [ 459.731232] ffffff80a748eea8 (&mm->mmap_sem){++++}, at:
> vm_mmap_pgoff+0x10c/0x1f4
> [ 459.731255]
> which lock already depends on the new lock.
>
...
> [ 459.732148] Chain exists of:
> &queue->mutex --> &sb->s_type->i_mutex_key#4 --> &mm->mmap_sem
>
> [ 459.732165] Possible unsafe locking scenario:
>
> [ 459.732172] CPU0 CPU1
> [ 459.732178] ---- ----
> [ 459.732184] lock(&mm->mmap_sem);
> [ 459.732193] lock(&sb->s_type->i_mutex_key#4);
> [ 459.732204] lock(&mm->mmap_sem);
> [ 459.732212] lock(&queue->mutex);
> [ 459.732221]
> *** DEADLOCK ***
>
>>
>> I'm not sure how calling xhci_debugfs_create_endpoint() from
>> xhci_add_endpoint() instead of xhci_check_bandwidth() helps.
>>
>> Both are called with hcd->bandwidth_mutex held:
>>
>> usb_set_interface()
>> mutex_lock(hcd->bandwidth_mutex);
>> usb_hcd_alloc_bandwidth()
>> hcd->driver->add_endpoint() -> xhci_add_endpoint()
>> hcd->driver->check_bandwidth() -> xhci_check_bandwidth()
>> mutex_unlock(hcd->bandwidth_mutex);
>
> Yep, I guess I was lucky not to be able to repro again :)
>
> The locks involved are:
>
> hcd->bandwidth_mutex
> mm->mmap_sem
> [uvc] queue->mutex
>

Ok, took a look at this.
I don't think the bandwidth mutex matters that much.

To my understanding this is caused by the following lock chains:

ucv_queue_mmap()
mmap_sem --> queue->mutex

uvc_ioctl_streamon() calling usb_set_interface() calling debugfs_create_dir()
queue->mutex --> i_mutex_key

Some debugfs error case:
i_mutex_key --> mmap_sem

So we could end up with this deadlock:
CPU0 CPU1 CPU2
mmap_sem queue->mutex i_mutex_key

waiting for waiting for waiting for
queue->mutex i_mutex_key mmap_sem

I have no idea if this can be triggered in real life.

Looks like that requires a some specific debugfs error
to trigger at the same time we are creating a debugfs directory

Thanks
Mathias





\
 
 \ /
  Last update: 2023-06-12 16:34    [W:0.071 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site