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

[Aloka Dixit <alokad@codeaurora.org>]

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.