Re: [BUG] deadlock in nl80211_vendor_cmd

[William McVicker <willmcvicker@google.com>]

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