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.
