Message-ID: <CAFBinCDjfKK3+WOXP2xbcAK-KToWof+kSzoxYztqRcc=7T1eyg@mail.gmail.com>
Date:   Thu, 27 Jan 2022 22:52:49 +0100
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Pkshih <pkshih@...ltek.com>
Cc:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "tony0620emma@...il.com" <tony0620emma@...il.com>,
        "kvalo@...eaurora.org" <kvalo@...eaurora.org>,
        "johannes@...solutions.net" <johannes@...solutions.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Neo Jou <neojou@...il.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Ed Swierk <eswierk@...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@...ltek.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

Powered by blists - more mailing lists