Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 22 Mar 2022 00:15:23 +0000
From:   William McVicker <willmcvicker@google.com>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org,
        Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [BUG] deadlock in nl80211_vendor_cmd
Message-ID: <YjkVG1ofyUg2IJP0@google.com>
References: <0000000000009e9b7105da6d1779@google.com>
 <99eda6d1dad3ff49435b74e539488091642b10a8.camel@sipsolutions.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <99eda6d1dad3ff49435b74e539488091642b10a8.camel@sipsolutions.net>
Precedence: bulk

On 03/21/2022, Johannes Berg wrote:
> Hi,
> 
> 
> > Basically, my wlan driver uses the wiphy_vendor_command ops to handle
> > a number of vendor specific operations.
> > 
> 
> I guess it's an out-of-tree driver, since I (hope I) fixed all the
> issues in the code here ... :)
> 
> > One of them in particular deletes
> > a cfg80211 interface.
> 
> There's quite normal API for that, why would you do that?!

Yeah, unfortunately this is the code I was given.

> 
> > The deadlock happens when thread 1 tries to take the
> > RTNL lock before calling cfg80211_unregister_device() while thread 2 is
> > inside nl80211_pre_doit(), holding the RTNL lock, and waiting on
> > wiphy_lock().
> > 
> > Here is the call flow:
> > 
> > Thread 1:                         Thread 2:
> > 
> > nl80211_pre_doit():
> >   -> rtnl_lock()
> >                                       nl80211_pre_doit():
> >                                        -> rtnl_lock()
> >                                        -> <blocked by Thread 1>
> >   -> wiphy_lock()
> >   -> rtnl_unlock()
> >   -> <unblock Thread 1>
> > exit nl80211_pre_doit()
> >                                        <Thread 2 got the RTNL lock>
> >                                        -> wiphy_lock()
> >                                        -> <blocked by Thread 1>
> > nl80211_doit()
> >   -> nl80211_vendor_cmd()
> >       -> rtnl_lock() <DEADLOCK>
> 
> Yeah, I guess the way we invoke vendor commands now w/o RTNL held means
> you cannot safely acquire RTNL in them.
> 
> I mean, the whole above thing basically collapses down to
> 
>  Thread 1                           Thread 2
>   wiphy_lock(); // nl80211
>                                      rtnl_lock();
>                                      wiphy_lock();
>   rtnl_lock();  // your driver
> 
> The correct order to _acquire_ these is rtnl -> wiphy, and we do it that
> way around everywhere (else).
> 
> > I'm not an networking expert. So my main question is if I'm allowed to take
> > the RTNL lock inside the nl80211_vendor_cmd callbacks?
> 
> Evidently, you're not. It's interesting though, it used to be that we
> called these with the RTNL held, now we don't, and the driver you're
> using somehow "got fixed" to take it, but whoever fixed it didn't take
> into account that this is not possible?

So in my quest to upgrade the Pixel 6 kernel from 5.10 to 5.15, I noticed that
I was hitting several ASSERT_RTNL() warnings during wlan testing. When I dug
into those asserts, I found commit a05829a7222e ("cfg80211: avoid holding the
RTNL when calling the driver") was causing these issues. So I went about adding
the necessary locks in the driver which led me to find this ABBA deadlock
scenario.

> 
> > I hope that helps explain the issue. Let me know if you need any more
> > details.
> 
> It does, but I don't think there's any way to fix it. You just
> fundamentally cannot acquire the RTNL in a vendor command operation
> since that introduced the ABBA deadlock you observed.
> 
> Since it's an out-of-tree driver that's about as much as I can help.
> 
> johannes

Yeah, I understand.  Thanks for the response!

--Will