Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 12 Jun 2023 17:30:05 +0300 | From | Mathias Nyman <> | Subject | Re: [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
| ![\](/images/icornerr.gif) |