Hi, 

> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Monday, January 24, 2022 3:04 AM
> To: Pkshih <pkshih@realtek.com>
> Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org;
> johannes@sipsolutions.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou
> <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed Swierk <eswierk@gh.st>
> Subject: Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
> 
> Hi Ping-Ke,
> 
> On Fri, Jan 21, 2022 at 9:10 AM Pkshih <pkshih@realtek.com> wrote:
> [...]
> > >
> > > I do stressed test of connection and suspend, and it get stuck after about
> > > 4 hours but no useful messages. I will re-build my kernel and turn on lockdep debug
> > > to see if it can tell me what is wrong.
> First of all: thank you so much for testing this and investigating the deadlock!
> 
> > I found some deadlock:
> >
> > [ 4891.169653]        CPU0                    CPU1
> > [ 4891.169732]        ----                    ----
> > [ 4891.169799]   lock(&rtwdev->mutex);
> > [ 4891.169874]                                lock(&local->sta_mtx);
> > [ 4891.169948]                                lock(&rtwdev->mutex);
> > [ 4891.170050]   lock(&local->sta_mtx);
> >
> >
> > [ 4919.598630]        CPU0                    CPU1
> > [ 4919.598715]        ----                    ----
> > [ 4919.598779]   lock(&local->iflist_mtx);
> > [ 4919.598900]                                lock(&rtwdev->mutex);
> > [ 4919.598995]                                lock(&local->iflist_mtx);
> > [ 4919.599092]   lock(&rtwdev->mutex);
> This looks similar to the problem fixed by 5b0efb4d670c8b ("rtw88:
> avoid circular locking between local->iflist_mtx and rtwdev->mutex")
> which you have pointed out earlier.
> It seems to me that we should avoid using the mutex version of
> ieee80211_iterate_*() because it can lead to more of these issues. So
> from my point of view the general idea of the code from your attached
> patch looks good. That said, I'm still very new to mac80211/cfg80211
> so I'm also interested in other's opinions.
> 

The attached patch can work "mostly", because both callers of iterate() and
::remove_interface hold rtwdev->mutex. Theoretically, the exception is a caller
forks another work to iterate() between leaving ::remove_interface and mac80211
doesn't yet free the vif, but the work executes after mac80211 free the vif.
This will lead use-after-free, but I'm not sure if this scenario will happen.
I need time to dig this, or you can help to do this.

To avoid this, we can add a flag to struct rtw_vif, and set this flag
when ::remove_interface. Then, only collect vif without this flag into list
when we use iterate_actiom().

As well as ieee80211_sta can do similar fix.

> > So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that
> > use _atomic version to collect sta and vif, and use list_for_each() to iterate.
> > Reference code is attached, and I'm still thinking if we can have better method.
> With "better method" do you mean something like in patch #2 from this
> series (using unsigned int num_si and struct rtw_sta_info
> *si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a
> better way in general?
> 

I would like a straight method, for example, we can have another version of
ieee80211_iterate_xxx() and do things in iterator, like original, so we just
need to change the code slightly.

Initially, I have an idea we can hold driver lock, like rtwdev->mutex, in both
places where we use ieee80211_iterate_() and remove sta or vif. Hopefully,
this can ensure it's safe to run iterator without other locks. Then, we can
define another ieee80211_iterate_() version with a drv_lock argument, like

#define ieee80211_iterate_active_interfaces_drv_lock(hw, iter_flags, iterator, data, drv_lock) \
while (0) {	\
	lockdep_assert_wiphy(drv_lock); \
	ieee80211_iterate_active_interfaces_no_lock(hw, iter_flags, iterator, data); \
}

The driv_lock argument can avoid user forgetting to hold a lock, and we need
a helper of no_lock version:

void ieee80211_iterate_active_interfaces_no_lock(
	struct ieee80211_hw *hw, u32 iter_flags,
	void (*iterator)(void *data, u8 *mac,
			 struct ieee80211_vif *vif),
	void *data)
{
	struct ieee80211_local *local = hw_to_local(hw);

	__iterate_interfaces(local, iter_flags | IEEE80211_IFACE_ITER_ACTIVE,
			     iterator, data);
}

However, as I mentioned theoretically it is not safe entirely.

So, I think the easiest way is to maintains the vif/sta lists in driver when
::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to
access these lists. But, Johannes pointed out this is not a good idea [1].

[1] https://lore.kernel.org/linux-wireless/d61f3947cddec660cbb2a59e2424d2bd8c01346a.camel@sipsolutions.net/
--
Ping-Ke

Hi Ping-Ke,

On Mon, Jan 24, 2022 at 3:59 AM Pkshih <pkshih@realtek.com> wrote:
[...]
> > It seems to me that we should avoid using the mutex version of
> > ieee80211_iterate_*() because it can lead to more of these issues. So
> > from my point of view the general idea of the code from your attached
> > patch looks good. That said, I'm still very new to mac80211/cfg80211
> > so I'm also interested in other's opinions.
> >
>
> The attached patch can work "mostly", because both callers of iterate() and
> ::remove_interface hold rtwdev->mutex. Theoretically, the exception is a caller
> forks another work to iterate() between leaving ::remove_interface and mac80211
> doesn't yet free the vif, but the work executes after mac80211 free the vif.
> This will lead use-after-free, but I'm not sure if this scenario will happen.
> I need time to dig this, or you can help to do this.
>
> To avoid this, we can add a flag to struct rtw_vif, and set this flag
> when ::remove_interface. Then, only collect vif without this flag into list
> when we use iterate_actiom().
>
> As well as ieee80211_sta can do similar fix.
>
> > > So, I add wrappers to iterate rtw_iterate_stas() and rtw_iterate_vifs() that
> > > use _atomic version to collect sta and vif, and use list_for_each() to iterate.
> > > Reference code is attached, and I'm still thinking if we can have better method.
> > With "better method" do you mean something like in patch #2 from this
> > series (using unsigned int num_si and struct rtw_sta_info
> > *si[RTW_MAX_MAC_ID_NUM] inside the iter_data) are you thinking of a
> > better way in general?
> >
>
> I would like a straight method, for example, we can have another version of
> ieee80211_iterate_xxx() and do things in iterator, like original, so we just
> need to change the code slightly.
>
> Initially, I have an idea we can hold driver lock, like rtwdev->mutex, in both
> places where we use ieee80211_iterate_() and remove sta or vif. Hopefully,
> this can ensure it's safe to run iterator without other locks. Then, we can
> define another ieee80211_iterate_() version with a drv_lock argument, like
>
> #define ieee80211_iterate_active_interfaces_drv_lock(hw, iter_flags, iterator, data, drv_lock) \
> while (0) {     \
>         lockdep_assert_wiphy(drv_lock); \
>         ieee80211_iterate_active_interfaces_no_lock(hw, iter_flags, iterator, data); \
> }
>
> The driv_lock argument can avoid user forgetting to hold a lock, and we need
> a helper of no_lock version:
>
> void ieee80211_iterate_active_interfaces_no_lock(
>         struct ieee80211_hw *hw, u32 iter_flags,
>         void (*iterator)(void *data, u8 *mac,
>                          struct ieee80211_vif *vif),
>         void *data)
> {
>         struct ieee80211_local *local = hw_to_local(hw);
>
>         __iterate_interfaces(local, iter_flags | IEEE80211_IFACE_ITER_ACTIVE,
>                              iterator, data);
> }
>
> However, as I mentioned theoretically it is not safe entirely.
>
> So, I think the easiest way is to maintains the vif/sta lists in driver when
> ::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to
> access these lists. But, Johannes pointed out this is not a good idea [1].
Thank you for this detailed explanation! I appreciate that you took
the time to clearly explain this.

For the sta use-case I thought about adding a dedicated rwlock
(include/linux/rwlock.h) for rtw_dev->mac_id_map.
rtw_sta_{add,remove} would take a write-lock.
rtw_iterate_stas() takes the read-lock (the lock would be acquired
before calling into ieee80211_iterate_...). Additionally
rtw_iterate_stas() needs to check if the station is still valid
according to mac_id_map - if not: skip/ignore it for that iteration.
This could be combined with your
0001-rtw88-use-atomic-to-collect-stas-and-does-iterators.patch.

For the interface use-case it's not clear to me how this works at all.
rtw_ops_add_interface() has (in a simplified view):
    u8 port = 0;
    // the port variable is never changed
    rtwvif->port = port;
    rtwvif->conf = &rtw_vif_port[port];
    rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port);
How do multiple interfaces (vifs) work in rtw88 if the port is always
zero? Is some kind of tracking of the used ports missing (similar to
how we track the used station IDs - also called mac_id - in
rtw_dev->mac_id_map)?


Thank you again and best regards,
Martin

Hi,

Sorry, I kind of saw this fly by, read over it and wasn't sure I should
take a closer look, and then promptly forgot about it ...

> > So, I think the easiest way is to maintains the vif/sta lists in driver when
> > ::{add,remove }_interface/::sta_{add,remove}, and hold rtwdev->mutex lock to
> > access these lists. But, Johannes pointed out this is not a good idea [1].

> Thank you for this detailed explanation! I appreciate that you took
> the time to clearly explain this.
> 
> For the sta use-case I thought about adding a dedicated rwlock
> (include/linux/rwlock.h) for rtw_dev->mac_id_map.

You can't sleep under an rwlock either though? Wasn't that the point?

> rtw_sta_{add,remove} would take a write-lock.
> rtw_iterate_stas() takes the read-lock (the lock would be acquired
> before calling into ieee80211_iterate_...). Additionally
> rtw_iterate_stas() needs to check if the station is still valid
> according to mac_id_map - if not: skip/ignore it for that iteration.

All of that "still valid" seems fragile though, IMHO. Hard to reason
that it's not racy or prone to use-after-free situations.

IIUC, you're trying to iterate interfaces and stations in a sleepable
context, but you're worried about deadlocks with locking in mac80211, if
ieee80211_iterate_interfaces() or ieee80211_iterate_stations() take
locks that we already hold due to coming into the code from mac80211? Or
about ABBA deadlocks arising from this?

IMHO you should still do it, and just be careful about the locking. I'd
be happy to add APIs for e.g. ieee80211_iterate_stations_locked() when
you know you already hold local->sta_mtx, though whether or not that can
even happen today in any callbacks I don't know, haven't audited it, but
it shouldn't be that hard to audit the path(s) you want to create?
Unless you need this in some very frequently called function ...


Now all of this is potentially also (and just as) error prone as doing
your own iteration machinery, however, my longer-term plan is to unify
the locking more. Now that we're no longer relying on the RTNL so much,
I'm planning to see if we couldn't get rid of most locks in mac80211.
The thing is, that most of the time we're doing all this fine-grained
locking ... under the wdev->mtx, so it's entirely pointless, and in fact
just a bunch of extra work.

So now that from cfg80211 perspective we have everything running under
wdev lock, I think we could in mac80211 just remove all the locks and
take the wdev lock in all the async workers. We already do in many I
guess, or we take local->mtx which is kind of equivalent anyway.

The question is then if we keep wdev->mtx at all, and I'm not really
sure about that yet. There are arguments both ways, and maybe that means
we'd move some things under there rather than under the overall mutex.
One of the strongest arguments for this is probably that mac80211
doesn't really do anything expensive (there aren't really any expensive
calculations in it), so most time spent is in drivers ... and guess
what, drivers mostly just have their own global lock they take for
everything, which makes sense since they need to talk to the device or
firmware.

However we answer that question though, and I'm trending towards
removing the wdev/sdata locks, I (now) think all the mutexes that we
have at the 'local' level in mac80211 are fairly much pointless.

In essence then, with some work in the stack, we could basically have
all calls (apart from TX) into the drivers done with the wiphy->mtx
held, at which point drivers don't even really need their own lock (they
can acquire wiphy_lock() too in workers). Then this whole thing really
all collapses down to being able to do the iteration, just having to
ensure you wiphy_lock() your calls that aren't coming from mac80211 in
the first place.

I really think that's the medium-term strategy, and any help would be
welcome. I'm not sure I can dedicate time to this soon, and the RTNL
rework took I think over a year after I started thinking about it, so I
guess we'll see...

We can do this incrementally btw, like remove local->mtx now and use
wiphy_lock() in its place, then remove local->sta_mtx, local->iface_mtx,
one by one. The RTNL rework was one of the major stepping stones to this
I think, because it ensured a consistent handling of the wiphy_lock() in
cfg80211.

I realize this doesn't help you in the short term, but if you do a lot
of complicated things now, it'll also be hard to get rid of. If you use
the normal iteration functions now it'll be harder to reason about now,
but will sort of automatically become easier once the lock redux work I
outlined above starts. And like I said, any help welcome :)

johannes

Hi,

> -----Original Message-----
> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Sent: Friday, January 28, 2022 5:53 AM
> To: Pkshih <pkshih@realtek.com>
> Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org;
> johannes@sipsolutions.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou
> <neojou@gmail.com>; Jernej Skrabec <jernej.skrabec@gmail.com>; Ed Swierk <eswierk@gh.st>
> Subject: Re: [PATCH v3 0/8] rtw88: prepare locking for SDIO support
> 
> Hi Ping-Ke,
> 
> On Mon, Jan 24, 2022 at 3:59 AM Pkshih <pkshih@realtek.com> wrote:

[...]

> >
> > To avoid this, we can add a flag to struct rtw_vif, and set this flag
> > when ::remove_interface. Then, only collect vif without this flag into list
> > when we use iterate_actiom().
> >
> > As well as ieee80211_sta can do similar fix.
> >

I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta
and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can
check this flag to decide whether does thing or not.

[...]

> 
> For the sta use-case I thought about adding a dedicated rwlock
> (include/linux/rwlock.h) for rtw_dev->mac_id_map.
> rtw_sta_{add,remove} would take a write-lock.
> rtw_iterate_stas() takes the read-lock (the lock would be acquired
> before calling into ieee80211_iterate_...). Additionally
> rtw_iterate_stas() needs to check if the station is still valid
> according to mac_id_map - if not: skip/ignore it for that iteration.
> This could be combined with your
> 0001-rtw88-use-atomic-to-collect-stas-and-does-iterators.patch.

Using a 'disabled' flag within rtw_vif/rtw_sta will be intuitive and
better than bitmap of mac_id_map. Please reference my mention above.

> 
> For the interface use-case it's not clear to me how this works at all.
> rtw_ops_add_interface() has (in a simplified view):
>     u8 port = 0;
>     // the port variable is never changed
>     rtwvif->port = port;
>     rtwvif->conf = &rtw_vif_port[port];
>     rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port);
> How do multiple interfaces (vifs) work in rtw88 if the port is always
> zero? Is some kind of tracking of the used ports missing (similar to
> how we track the used station IDs - also called mac_id - in
> rtw_dev->mac_id_map)?

The port should be allocated dynamically if we support two or more vifs.
We have internal tree that is going to support p2p by second vif.


Ping-Ke

Hi Ping-Ke,

On Fri, Jan 28, 2022 at 1:51 AM Pkshih <pkshih@realtek.com> wrote:
[...]
>
> > >
> > > To avoid this, we can add a flag to struct rtw_vif, and set this flag
> > > when ::remove_interface. Then, only collect vif without this flag into list
> > > when we use iterate_actiom().
> > >
> > > As well as ieee80211_sta can do similar fix.
> > >
>
> I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta
> and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can
> check this flag to decide whether does thing or not.
That would indeed be a very straight forward approach and easy to read.
In net/mac80211/iface.c there's some cases where after
drv_remove_interface() (which internally calls our .remove_interface
op) will kfree the vif (sdata). Doesn't that then result in a
use-after-free if we rely on a boolean within rtw_vif?

[...]
> > For the interface use-case it's not clear to me how this works at all.
> > rtw_ops_add_interface() has (in a simplified view):
> >     u8 port = 0;
> >     // the port variable is never changed
> >     rtwvif->port = port;
> >     rtwvif->conf = &rtw_vif_port[port];
> >     rtw_info(rtwdev, "start vif %pM on port %d\n", vif->addr, rtwvif->port);
> > How do multiple interfaces (vifs) work in rtw88 if the port is always
> > zero? Is some kind of tracking of the used ports missing (similar to
> > how we track the used station IDs - also called mac_id - in
> > rtw_dev->mac_id_map)?
>
> The port should be allocated dynamically if we support two or more vifs.
> We have internal tree that is going to support p2p by second vif.
I see, thanks for clarifying this!


Best regards,
Martin

Hi,

On Sun, 2022-01-30 at 22:40 +0100, Martin Blumenstingl wrote:
> 
> On Fri, Jan 28, 2022 at 1:51 AM Pkshih <pkshih@realtek.com> wrote:
> [...]
> > > > To avoid this, we can add a flag to struct rtw_vif, and set this flag
> > > > when ::remove_interface. Then, only collect vif without this flag into list
> > > > when we use iterate_actiom().
> > > > 
> > > > As well as ieee80211_sta can do similar fix.
> > > > 
> > 
> > I would prefer my method that adds a 'bool disabled' flag to struct rtw_vif/rtw_sta
> > and set it when ::remove_interface/::sta_remove. Then rtw_iterate_stas() can
> > check this flag to decide whether does thing or not.
> That would indeed be a very straight forward approach and easy to read.
> In net/mac80211/iface.c there's some cases where after
> drv_remove_interface() (which internally calls our .remove_interface
> op) will kfree the vif (sdata). Doesn't that then result in a
> use-after-free if we rely on a boolean within rtw_vif?

The rtw_vif is drv_priv of ieee80211_vif, and they will be freed at
the same time. We must set 'bool disabled' after holding rtwdev->mutex
lock, and check this flag in iterator of ieee80211_iterate_active_interfaces_atomic()
to contruct a list of vif.

That means we never access this flag out of rtwdev->mutx or iterator.
Does it make sense?

--
Ping-Ke