Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: alokad=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Mon, 19 Apr 2021 15:42:51 -0700
From:   Aloka Dixit <alokad@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org, John Crispin <john@phrozen.org>,
        alokad=codeaurora.org@codeaurora.org
Subject: Re: [PATCH v9 2/4] mac80211: add multiple bssid support to interface
 handling
In-Reply-To: <865a59c2dd3a07c4ee88716f51759e88@codeaurora.org>
References: <20210310182604.8858-1-alokad@codeaurora.org>
 <20210310182604.8858-3-alokad@codeaurora.org>
 (sfid-20210310_192729_241525_2DF37B20)
 <7f6f0a8c151746e8bb44ad50daf75259a0fac829.camel@sipsolutions.net>
 <494efc64a803693324dee5b7a03cfda0@codeaurora.org>
 <9ce462d7c0dc707259d8bb50ec27a189ec89ef61.camel@sipsolutions.net>
 <865a59c2dd3a07c4ee88716f51759e88@codeaurora.org>
Message-ID: <543bf0ddae17e1649b8008021bfde6f2@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2021-04-19 15:35, Aloka Dixit wrote:
> On 2021-04-19 04:28, Johannes Berg wrote:
>> Hi,
>> 
>>> > > +	if (sdata && sdata->vif.type == NL80211_IFTYPE_AP) {
>>> > > +		if (sdata->vif.multiple_bssid.flags &
>>> > > IEEE80211_VIF_MBSS_TRANSMITTING) {
>>> > > +			struct ieee80211_sub_if_data *child;
>>> > > +
>>> > > +			rcu_read_lock();
>>> > > +			list_for_each_entry_rcu(child, &sdata->local->interfaces, list)
>>> > > +				if (child->vif.multiple_bssid.parent == &sdata->vif)
>>> > > +					dev_close(child->wdev.netdev);
>>> > > +			rcu_read_unlock();
>> 
>>> This was added for graceful shutdown of non-transmitting interfaces
>>> whenever the transmitting one is being brought down. 
>>> 
>> 
>> I know, I asked you to.
>> 
>>> But I see that
>>> dev_close() is happening twice now.
>>> 
>> 
>> That wouldn't be an issue.
>> 
>> The issue is that dev_close() needs to be able to sleep, and it even
>> contains a might_sleep(), so you can't do it under the RCU protection
>> you used here.
>> 
>>> Inclining towards removing this and just return error to application 
>>> if
>>> it tries to remove transmitting before all non-transmitting are 
>>> deleted.
>>> However, currently the "parent" pointer to indicate the transmitting
>>> interface is maintained in mac80211, nothing in cfg80211.
>> 
>> That seems kinda awkward, considering e.g. if hostapd crashes and then 
>> a
>> new instance has to clean up, it might not really have much knowledge 
>> of
>> the order in which it should be doing that.
>> 
>> I think it's better if you just fix the locking here?
>> 
>> johannes
> 
> Hi Johannes,
> Thanks for the response, I need more help.
> 
> Is rcu_read_lock is not allowed around dev_close() because it might
> block or *ANY* lock?
> Because both functions with new dev_close() themselves are called with
> locks held,
> (1) ieee80211_do_stop() happens inside 
> wiphy_lock(sdata->local->hw.wiphy)
> (2) ieee80211_del_iface() happens inside mutex_lock(&rdev->wiphy.mtx).
> Should these be unlocked temporarily too before calling dev_close()?
> Didn't find any instance of wiphy.mtx lock/unlock in mac80211.
> 

My bad, wiphy_lock() internally uses wiphy.mtx so mac80211 does have a 
way, then should it be unlocked temporarily before dev_close()?

> Also, in cfg.c, list_for_each_entry(sdata, &local->interfaces, list)
> is called with two different murexes: (1) local->iflist_mtx
> (2) local->mtx
> 
> Which is the correct one for this purpose among above two and 
> rcu_read_lock()?
> Once that decided, would following be sufficient -
>     lock()
>     list_for_each_entry(sdata, &local->interfaces, list) {
>         get_child_pointer
>         unlock()
>         dev_close()
>         lock()
>     }
>     unlock
> 
> Thanks.