From: Pkshih <pkshih@realtek.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
"tony0620emma@gmail.com" <tony0620emma@gmail.com>,
"kvalo@codeaurora.org" <kvalo@codeaurora.org>,
"johannes@sipsolutions.net" <johannes@sipsolutions.net>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@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
Date: Mon, 24 Jan 2022 02:59:15 +0000 [thread overview]
Message-ID: <5ef8ab4f78e448df9f823385d0daed88@realtek.com> (raw)
In-Reply-To: <CAFBinCBVEndU0t-6d5atE31OFYHzPyk7pOe78v0XrrFWcBec9w@mail.gmail.com>
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
next prev parent reply other threads:[~2022-01-24 2:59 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-08 0:55 [PATCH v3 0/8] rtw88: prepare locking for SDIO support Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 1/8] rtw88: Move rtw_chip_cfg_csi_rate() out of rtw_vif_watch_dog_iter() Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 2/8] rtw88: Move rtw_update_sta_info() out of rtw_ra_mask_info_update_iter() Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 3/8] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 4/8] rtw88: Use rtw_iterate_stas " Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 5/8] rtw88: Replace usage of rtw_iterate_keys_rcu() with rtw_iterate_keys() Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 6/8] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 7/8] rtw88: hci: Convert rf_lock from a spinlock to a mutex Martin Blumenstingl
2022-01-08 0:55 ` [PATCH v3 8/8] rtw88: fw: Convert h2c.lock " Martin Blumenstingl
2022-01-19 9:38 ` [PATCH v3 0/8] rtw88: prepare locking for SDIO support Pkshih
2022-01-21 8:10 ` Pkshih
2022-01-23 19:03 ` Martin Blumenstingl
2022-01-24 2:59 ` Pkshih [this message]
2022-01-27 21:52 ` Martin Blumenstingl
2022-01-28 0:51 ` Pkshih
2022-01-30 21:40 ` Martin Blumenstingl
2022-01-31 3:06 ` Pkshih
2022-02-03 22:26 ` Johannes Berg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5ef8ab4f78e448df9f823385d0daed88@realtek.com \
--to=pkshih@realtek.com \
--cc=eswierk@gh.st \
--cc=jernej.skrabec@gmail.com \
--cc=johannes@sipsolutions.net \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=martin.blumenstingl@googlemail.com \
--cc=neojou@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=tony0620emma@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).