Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230531-xhci-deadlock-v1-1-57780bff5124@chromium.org>
 <14d94fa1-1499-de1f-c924-9b823a606580@linux.intel.com> <CANiDSCuTYRUfW8tLbPDq3dE+F7Wno5oc4C9qESMmTpaNyW-54Q@mail.gmail.com>
 <e153b3e8-6c0a-a142-c357-eb295eecdece@linux.intel.com>
In-Reply-To: <e153b3e8-6c0a-a142-c357-eb295eecdece@linux.intel.com>
From:   Ricardo Ribalda <ribalda@chromium.org>
Date:   Sat, 17 Jun 2023 14:09:46 +0200
Message-ID: <CANiDSCt2YYOM7DPv4Qjghv5Y2TPMbWTccM7jubcaNmU4Qd_G6g@mail.gmail.com>
Subject: Re: [PATCH] xhci: Do not create endpoint debugfs while holding the
 bandwidth mutex
To:     Mathias Nyman <mathias.nyman@linux.intel.com>
Cc:     Mathias Nyman <mathias.nyman@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Stephen Boyd <swboyd@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Mathias

Thanks for looking into this.  Relooking into the bug queue->mutex was
only bothering due to a downstream patch (that we are working on
upstreaming), so this should not affect upstream.

Sorry for  the noise :S

On Mon, 12 Jun 2023 at 16:28, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> 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
>