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

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