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;
Message-ID: <dc556455-51a2-06e8-8ec5-b807c2901b7e@quicinc.com>
Date:   Wed, 23 Mar 2022 09:06:30 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [BUG] deadlock in nl80211_vendor_cmd
Content-Language: en-US
To:     William McVicker <willmcvicker@google.com>
CC:     Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>
References: <0000000000009e9b7105da6d1779@google.com>
 <99eda6d1dad3ff49435b74e539488091642b10a8.camel@sipsolutions.net>
 <5d5cf050-7de0-7bad-2407-276970222635@quicinc.com>
 <YjpGlRvcg72zNo8s@google.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <YjpGlRvcg72zNo8s@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.