On 03/23/2022, Jeff Johnson wrote:
> On 3/22/2022 2:58 PM, William McVicker wrote:
> > On 03/22/2022, Jeff Johnson wrote:
> > > On 3/21/2022 1:07 PM, Johannes Berg wrote:
> > > [..snip..]
> > >
> > > > > 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?
> > >
> > > On this point I just want to remind that prior to the locking change that a
> > > driver would specify on a per-vendor command basis whether or not it wanted
> > > the rtnl_lock to be held via NL80211_FLAG_NEED_RTNL. I'm guessing for the
> > > command in question the driver did not set this flag since the driver wanted
> > > to explicitly take the lock itself, otherwise it would have deadlocked on
> > > itself with the 5.10 kernel.
> > >
> > > /jeff
> >
> > On the 5.10 kernel, the core kernel sets NL80211_FLAG_NEED_RTNL as part of
> > the internal_flags for NL80211_CMD_VENDOR:
> >
> > net/wireless/nl80211.c:
> > {
> > .cmd = NL80211_CMD_VENDOR,
> > .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > .doit = nl80211_vendor_cmd,
> > .dumpit = nl80211_vendor_cmd_dump,
> > .flags = GENL_UNS_ADMIN_PERM,
> > .internal_flags = NL80211_FLAG_NEED_WIPHY |
> > NL80211_FLAG_NEED_RTNL |
> > NL80211_FLAG_CLEAR_SKB,
> > },
> >
> > So the 5.10 version of this driver doesn't need to directly call rtnl_lock()
> > within the vendor command doit() functions since pre_doit() handles the RTNL
> > locking.
> >
> > It would be nice if nl80211_vendor_cmd() could support taking the RTNL lock if
> > requested via the vendor flags. That would require moving the wiphy lock to
> > nl80211_vendor_cmds() so that it could take the RTNL and wiphy lock in the
> > correct order. Is that something you'd be open to Johannes?
> >
> > --Will
>
> Thanks for correcting my understanding. I concur that it would be useful for
> vendor commands to be able to specify that a given command needs the RTNL
> lock to be held.
>
>
Hi Johannes,
I found that we can hit this same ABBA deadlock within the nl80211 code
before ever even calling into the vendor doit() function. The issue I found
is caused by the way we unlock the RTNL mutex. Here is the call flow that
leads to the deadlock:
Thread 1 Thread 2
nl80211_pre_doit():
rtnl_lock()
wiphy_lock() nl80211_pre_doit():
rtnl_lock() // blocked by Thread 1
rtnl_unlock():
netdev_run_todo():
__rtnl_unlock()
<got RTNL lock>
wiphy_lock() // blocked by Thread 1
rtnl_lock(); // DEADLOCK
doit()
nl80211_post_doit():
wiphy_unlock();
Basically, unlocking the RTNL within netdev_run_todo() gives another thread
that is waiting for the RTNL in nl80211_pre_doit() a chance to grab the
RTNL lock leading to the deadlock. I found that there are multiple
instances where rtnl_lock() is called within netdev_run_todo(): a couple of
times inside netdev_wait_allrefs() and directly by netdev_run_todo().
Since I'm not really familiar with all the RNTL locking requirements, I was
hoping you could take a look at netdev_run_todo() to see if it's possible
to refactor it to avoid this deadlock. If not, then I don't think we can
call rtnl_unlock() while still holding the wiphy mutex.
Thanks,
Will
