Hi Aloka,
>
> Is rcu_read_lock is not allowed around dev_close() because it might
> block or *ANY* lock?
Well, I guess it's more nuanced than that.
rcu_read_lock() is not allowed because it enters an atomic context.
Similarly not allowed would be spinlocks, or local_bh_disable, or any
similar thing that makes the context atomic.
>From a locking perspective, normal mutexes *would* be allowed.
However, you'd have to be extremely careful to not allow recursion,
since dev_close() will call back into cfg80211/mac80211.
> 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)
This is probably already problematic.
> (2) ieee80211_del_iface() happens inside mutex_lock(&rdev->wiphy.mtx).
As you discovered, that's the same lock.
> Should these be unlocked temporarily too before calling dev_close()?
I don't think temporarily dropping locks is ever a good idea, it makes
it really hard to reason about the code.
But we already do this for AP-VLAN interfaces, so not sure why this is
so different?
>
> 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()
What guarantees you don't lose the device after the unlock()? I think
you'd risk list corruption here this way...
Look at the other instance of dev_close() here - as long as you can
guarantee there won't be recursion (and you do, because non-transmitting
interfaces don't have other non-transmitting below them, though they may
actually have AP-VLANs!), we should be fine just doing it like that
code?
johannes
