Re: [PATCH v9 2/4] mac80211: add multiple bssid support to interface handling

[Aloka Dixit <alokad@codeaurora.org>]

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.

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.