* [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet
@ 2022-07-04 8:43 Jeongik Cha
2022-07-14 8:37 ` Jeongik Cha
2022-07-15 0:16 ` Guenter Roeck
0 siblings, 2 replies; 6+ messages in thread
From: Jeongik Cha @ 2022-07-04 8:43 UTC (permalink / raw)
To: Johannes Berg, Kalle Valo, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: adelva, kernel-team, jaeman, Jeongik Cha, linux-wireless, netdev,
linux-kernel
A pending packet uses a cookie as an unique key, but it can be duplicated
because it didn't use atomic operators.
And also, a pending packet can be null in hwsim_tx_info_frame_received_nl
due to race condition with mac80211_hwsim_stop.
For this,
* Use an atomic type and operator for a cookie
* Add a lock around the loop for pending packets
Signed-off-by: Jeongik Cha <jeongik@google.com>
---
drivers/net/wireless/mac80211_hwsim.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index c5bb97b381cf..ea006248ffcd 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -687,7 +687,7 @@ struct mac80211_hwsim_data {
bool ps_poll_pending;
struct dentry *debugfs;
- uintptr_t pending_cookie;
+ atomic64_t pending_cookie;
struct sk_buff_head pending; /* packets pending */
/*
* Only radios in the same group can communicate together (the
@@ -1358,7 +1358,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
int i;
struct hwsim_tx_rate tx_attempts[IEEE80211_TX_MAX_RATES];
struct hwsim_tx_rate_flag tx_attempts_flags[IEEE80211_TX_MAX_RATES];
- uintptr_t cookie;
+ u64 cookie;
if (data->ps != PS_DISABLED)
hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
@@ -1427,8 +1427,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
goto nla_put_failure;
/* We create a cookie to identify this skb */
- data->pending_cookie++;
- cookie = data->pending_cookie;
+ cookie = (u64)atomic64_inc_return(&data->pending_cookie);
info->rate_driver_data[0] = (void *)cookie;
if (nla_put_u64_64bit(skb, HWSIM_ATTR_COOKIE, cookie, HWSIM_ATTR_PAD))
goto nla_put_failure;
@@ -4178,6 +4177,7 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
const u8 *src;
unsigned int hwsim_flags;
int i;
+ unsigned long flags;
bool found = false;
if (!info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER] ||
@@ -4205,18 +4205,20 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
}
/* look for the skb matching the cookie passed back from user */
+ spin_lock_irqsave(&data2->pending.lock, flags);
skb_queue_walk_safe(&data2->pending, skb, tmp) {
u64 skb_cookie;
txi = IEEE80211_SKB_CB(skb);
- skb_cookie = (u64)(uintptr_t)txi->rate_driver_data[0];
+ skb_cookie = (u64)txi->rate_driver_data[0];
if (skb_cookie == ret_skb_cookie) {
- skb_unlink(skb, &data2->pending);
+ __skb_unlink(skb, &data2->pending);
found = true;
break;
}
}
+ spin_unlock_irqrestore(&data2->pending.lock, flags);
/* not found */
if (!found)
--
2.37.0.rc0.161.g10f37bed90-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet
2022-07-04 8:43 [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet Jeongik Cha
@ 2022-07-14 8:37 ` Jeongik Cha
2022-07-14 8:38 ` Johannes Berg
2022-07-15 0:16 ` Guenter Roeck
1 sibling, 1 reply; 6+ messages in thread
From: Jeongik Cha @ 2022-07-14 8:37 UTC (permalink / raw)
To: Johannes Berg, Kalle Valo, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: adelva, kernel-team, jaeman, linux-wireless, netdev, linux-kernel
On Mon, Jul 4, 2022 at 5:44 PM Jeongik Cha <jeongik@google.com> wrote:
>
> A pending packet uses a cookie as an unique key, but it can be duplicated
> because it didn't use atomic operators.
>
> And also, a pending packet can be null in hwsim_tx_info_frame_received_nl
> due to race condition with mac80211_hwsim_stop.
>
> For this,
> * Use an atomic type and operator for a cookie
> * Add a lock around the loop for pending packets
>
> Signed-off-by: Jeongik Cha <jeongik@google.com>
> ---
> drivers/net/wireless/mac80211_hwsim.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
> index c5bb97b381cf..ea006248ffcd 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -687,7 +687,7 @@ struct mac80211_hwsim_data {
> bool ps_poll_pending;
> struct dentry *debugfs;
>
> - uintptr_t pending_cookie;
> + atomic64_t pending_cookie;
> struct sk_buff_head pending; /* packets pending */
> /*
> * Only radios in the same group can communicate together (the
> @@ -1358,7 +1358,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
> int i;
> struct hwsim_tx_rate tx_attempts[IEEE80211_TX_MAX_RATES];
> struct hwsim_tx_rate_flag tx_attempts_flags[IEEE80211_TX_MAX_RATES];
> - uintptr_t cookie;
> + u64 cookie;
>
> if (data->ps != PS_DISABLED)
> hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
> @@ -1427,8 +1427,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
> goto nla_put_failure;
>
> /* We create a cookie to identify this skb */
> - data->pending_cookie++;
> - cookie = data->pending_cookie;
> + cookie = (u64)atomic64_inc_return(&data->pending_cookie);
> info->rate_driver_data[0] = (void *)cookie;
> if (nla_put_u64_64bit(skb, HWSIM_ATTR_COOKIE, cookie, HWSIM_ATTR_PAD))
> goto nla_put_failure;
> @@ -4178,6 +4177,7 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
> const u8 *src;
> unsigned int hwsim_flags;
> int i;
> + unsigned long flags;
> bool found = false;
>
> if (!info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER] ||
> @@ -4205,18 +4205,20 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
> }
>
> /* look for the skb matching the cookie passed back from user */
> + spin_lock_irqsave(&data2->pending.lock, flags);
> skb_queue_walk_safe(&data2->pending, skb, tmp) {
> u64 skb_cookie;
>
> txi = IEEE80211_SKB_CB(skb);
> - skb_cookie = (u64)(uintptr_t)txi->rate_driver_data[0];
> + skb_cookie = (u64)txi->rate_driver_data[0];
>
> if (skb_cookie == ret_skb_cookie) {
> - skb_unlink(skb, &data2->pending);
> + __skb_unlink(skb, &data2->pending);
> found = true;
> break;
> }
> }
> + spin_unlock_irqrestore(&data2->pending.lock, flags);
>
> /* not found */
> if (!found)
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
Hello Johannes!
It fixes kernel panics during a long test which uses mac80211_hwsim
driver. So I think it would be beneficial if we could merge this into
LTS branches. Could you share your opinion?
Thanks
Jeongik
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet
2022-07-14 8:37 ` Jeongik Cha
@ 2022-07-14 8:38 ` Johannes Berg
2022-07-14 8:40 ` Jeongik Cha
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2022-07-14 8:38 UTC (permalink / raw)
To: Jeongik Cha, Kalle Valo, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: adelva, kernel-team, jaeman, linux-wireless, netdev, linux-kernel
On Thu, 2022-07-14 at 17:37 +0900, Jeongik Cha wrote:
>
> It fixes kernel panics during a long test which uses mac80211_hwsim
> driver. So I think it would be beneficial if we could merge this into
> LTS branches. Could you share your opinion?
>
It also introduced two build compiler warning issues so I have two more
fixes ... I guess you can request that, but make sure you include the
other fixes (one of which hasn't landed yet) :-)
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet
2022-07-14 8:38 ` Johannes Berg
@ 2022-07-14 8:40 ` Jeongik Cha
0 siblings, 0 replies; 6+ messages in thread
From: Jeongik Cha @ 2022-07-14 8:40 UTC (permalink / raw)
To: Johannes Berg
Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, adelva, kernel-team, jaeman, linux-wireless, netdev,
linux-kernel
On Thu, Jul 14, 2022 at 5:39 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Thu, 2022-07-14 at 17:37 +0900, Jeongik Cha wrote:
> >
> > It fixes kernel panics during a long test which uses mac80211_hwsim
> > driver. So I think it would be beneficial if we could merge this into
> > LTS branches. Could you share your opinion?
> >
>
> It also introduced two build compiler warning issues so I have two more
> fixes ... I guess you can request that, but make sure you include the
> other fixes (one of which hasn't landed yet) :-)
>
> johannes
Will do! Thanks for letting me know.
Thanks,
Jeongik
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet
2022-07-04 8:43 [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet Jeongik Cha
2022-07-14 8:37 ` Jeongik Cha
@ 2022-07-15 0:16 ` Guenter Roeck
2022-07-15 1:53 ` Jeongik Cha
1 sibling, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2022-07-15 0:16 UTC (permalink / raw)
To: Jeongik Cha
Cc: Johannes Berg, Kalle Valo, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, adelva, kernel-team, jaeman,
linux-wireless, netdev, linux-kernel
On Mon, Jul 04, 2022 at 05:43:54PM +0900, Jeongik Cha wrote:
> A pending packet uses a cookie as an unique key, but it can be duplicated
> because it didn't use atomic operators.
>
> And also, a pending packet can be null in hwsim_tx_info_frame_received_nl
> due to race condition with mac80211_hwsim_stop.
>
> For this,
> * Use an atomic type and operator for a cookie
> * Add a lock around the loop for pending packets
>
> Signed-off-by: Jeongik Cha <jeongik@google.com>
Building i386:allyesconfig ... failed
--------------
Error log:
drivers/net/wireless/mac80211_hwsim.c: In function 'mac80211_hwsim_tx_frame_nl':
drivers/net/wireless/mac80211_hwsim.c:1431:37: error: cast to pointer from integer of different size
Also seen in other 32-bit builds.
Bisect log attached.
Guenter
---
# bad: [37b355fdaf31ee18bda9a93c2a438dc1cbf57ec9] Add linux-next specific files for 20220714
# good: [32346491ddf24599decca06190ebca03ff9de7f8] Linux 5.19-rc6
git bisect start 'HEAD' 'v5.19-rc6'
# bad: [6d30dd0872599b7004e26330fc2e476ad900e7f6] Merge branch 'drm-next' of git://git.freedesktop.org/git/drm/drm.git
git bisect bad 6d30dd0872599b7004e26330fc2e476ad900e7f6
# good: [6134a5c4db991084f2f7c2da6c6cf400e42e3a99] Merge branch 'docs-next' of git://git.lwn.net/linux.git
git bisect good 6134a5c4db991084f2f7c2da6c6cf400e42e3a99
# bad: [f6268862d21dc3233ced91b848a55b6dfa8d438b] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect bad f6268862d21dc3233ced91b848a55b6dfa8d438b
# good: [6d1ce9c03880c28a4a48f94d4a2dcb2e57c1b88e] net: phylink: fix SGMII inband autoneg enable
git bisect good 6d1ce9c03880c28a4a48f94d4a2dcb2e57c1b88e
# good: [480e10a33cdb7282f9ec91065fb624c0cd2f758f] Merge branch 'devfreq-next' of git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git
git bisect good 480e10a33cdb7282f9ec91065fb624c0cd2f758f
# good: [cfc6c2fcb686afdaea5bbca6f3dbb27815a23878] Merge branch 'phy-mxl-gpy-version-fix-and-improvements'
git bisect good cfc6c2fcb686afdaea5bbca6f3dbb27815a23878
# good: [8bc65d38ee466897a264c9e336fe21058818b1b1] wifi: nl80211: retrieve EHT related elements in AP mode
git bisect good 8bc65d38ee466897a264c9e336fe21058818b1b1
# good: [8f8df82f9cc2e76b48ba7cec3d08f4295e8f6ebb] Merge branch 'thermal/linux-next' of git://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git
git bisect good 8f8df82f9cc2e76b48ba7cec3d08f4295e8f6ebb
# good: [2635d2a8d4664b665bc12e15eee88e9b1b40ae7b] IB: Fix spelling of 'writable'
git bisect good 2635d2a8d4664b665bc12e15eee88e9b1b40ae7b
# good: [c18bd03474a070e80fee20f0628fd0a6728c2475] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm.git
git bisect good c18bd03474a070e80fee20f0628fd0a6728c2475
# good: [3c512307de4097aaaab3f4741c7a98fe88afa469] wifi: nl80211: fix sending link ID info of associated BSS
git bisect good 3c512307de4097aaaab3f4741c7a98fe88afa469
# bad: [736002fb6a09861c2663596011371884a8b7c0dd] Merge tag 'wireless-next-2022-07-13' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next
git bisect bad 736002fb6a09861c2663596011371884a8b7c0dd
# good: [37babce9127f3145366a8f36334f24afa9a5d196] wifi: mac80211: Use the bitmap API to allocate bitmaps
git bisect good 37babce9127f3145366a8f36334f24afa9a5d196
# bad: [58b6259d820d63c2adf1c7541b54cce5a2ae6073] wifi: mac80211_hwsim: add back erroneously removed cast
git bisect bad 58b6259d820d63c2adf1c7541b54cce5a2ae6073
# bad: [4ee186fa7e40ae06ebbfbad77e249e3746e14114] wifi: mac80211_hwsim: fix race condition in pending packet
git bisect bad 4ee186fa7e40ae06ebbfbad77e249e3746e14114
# first bad commit: [4ee186fa7e40ae06ebbfbad77e249e3746e14114] wifi: mac80211_hwsim: fix race condition in pending packet
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet
2022-07-15 0:16 ` Guenter Roeck
@ 2022-07-15 1:53 ` Jeongik Cha
0 siblings, 0 replies; 6+ messages in thread
From: Jeongik Cha @ 2022-07-15 1:53 UTC (permalink / raw)
To: Guenter Roeck
Cc: Johannes Berg, Kalle Valo, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, adelva, kernel-team, jaeman,
linux-wireless, netdev, linux-kernel
On Fri, Jul 15, 2022 at 9:16 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Mon, Jul 04, 2022 at 05:43:54PM +0900, Jeongik Cha wrote:
> > A pending packet uses a cookie as an unique key, but it can be duplicated
> > because it didn't use atomic operators.
> >
> > And also, a pending packet can be null in hwsim_tx_info_frame_received_nl
> > due to race condition with mac80211_hwsim_stop.
> >
> > For this,
> > * Use an atomic type and operator for a cookie
> > * Add a lock around the loop for pending packets
> >
> > Signed-off-by: Jeongik Cha <jeongik@google.com>
>
> Building i386:allyesconfig ... failed
> --------------
> Error log:
>
> drivers/net/wireless/mac80211_hwsim.c: In function 'mac80211_hwsim_tx_frame_nl':
> drivers/net/wireless/mac80211_hwsim.c:1431:37: error: cast to pointer from integer of different size
>
> Also seen in other 32-bit builds.
>
> Bisect log attached.
>
> Guenter
>
> ---
> # bad: [37b355fdaf31ee18bda9a93c2a438dc1cbf57ec9] Add linux-next specific files for 20220714
> # good: [32346491ddf24599decca06190ebca03ff9de7f8] Linux 5.19-rc6
> git bisect start 'HEAD' 'v5.19-rc6'
> # bad: [6d30dd0872599b7004e26330fc2e476ad900e7f6] Merge branch 'drm-next' of git://git.freedesktop.org/git/drm/drm.git
> git bisect bad 6d30dd0872599b7004e26330fc2e476ad900e7f6
> # good: [6134a5c4db991084f2f7c2da6c6cf400e42e3a99] Merge branch 'docs-next' of git://git.lwn.net/linux.git
> git bisect good 6134a5c4db991084f2f7c2da6c6cf400e42e3a99
> # bad: [f6268862d21dc3233ced91b848a55b6dfa8d438b] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> git bisect bad f6268862d21dc3233ced91b848a55b6dfa8d438b
> # good: [6d1ce9c03880c28a4a48f94d4a2dcb2e57c1b88e] net: phylink: fix SGMII inband autoneg enable
> git bisect good 6d1ce9c03880c28a4a48f94d4a2dcb2e57c1b88e
> # good: [480e10a33cdb7282f9ec91065fb624c0cd2f758f] Merge branch 'devfreq-next' of git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git
> git bisect good 480e10a33cdb7282f9ec91065fb624c0cd2f758f
> # good: [cfc6c2fcb686afdaea5bbca6f3dbb27815a23878] Merge branch 'phy-mxl-gpy-version-fix-and-improvements'
> git bisect good cfc6c2fcb686afdaea5bbca6f3dbb27815a23878
> # good: [8bc65d38ee466897a264c9e336fe21058818b1b1] wifi: nl80211: retrieve EHT related elements in AP mode
> git bisect good 8bc65d38ee466897a264c9e336fe21058818b1b1
> # good: [8f8df82f9cc2e76b48ba7cec3d08f4295e8f6ebb] Merge branch 'thermal/linux-next' of git://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git
> git bisect good 8f8df82f9cc2e76b48ba7cec3d08f4295e8f6ebb
> # good: [2635d2a8d4664b665bc12e15eee88e9b1b40ae7b] IB: Fix spelling of 'writable'
> git bisect good 2635d2a8d4664b665bc12e15eee88e9b1b40ae7b
> # good: [c18bd03474a070e80fee20f0628fd0a6728c2475] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm.git
> git bisect good c18bd03474a070e80fee20f0628fd0a6728c2475
> # good: [3c512307de4097aaaab3f4741c7a98fe88afa469] wifi: nl80211: fix sending link ID info of associated BSS
> git bisect good 3c512307de4097aaaab3f4741c7a98fe88afa469
> # bad: [736002fb6a09861c2663596011371884a8b7c0dd] Merge tag 'wireless-next-2022-07-13' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next
> git bisect bad 736002fb6a09861c2663596011371884a8b7c0dd
> # good: [37babce9127f3145366a8f36334f24afa9a5d196] wifi: mac80211: Use the bitmap API to allocate bitmaps
> git bisect good 37babce9127f3145366a8f36334f24afa9a5d196
> # bad: [58b6259d820d63c2adf1c7541b54cce5a2ae6073] wifi: mac80211_hwsim: add back erroneously removed cast
> git bisect bad 58b6259d820d63c2adf1c7541b54cce5a2ae6073
> # bad: [4ee186fa7e40ae06ebbfbad77e249e3746e14114] wifi: mac80211_hwsim: fix race condition in pending packet
> git bisect bad 4ee186fa7e40ae06ebbfbad77e249e3746e14114
> # first bad commit: [4ee186fa7e40ae06ebbfbad77e249e3746e14114] wifi: mac80211_hwsim: fix race condition in pending packet
I think https://patchwork.kernel.org/project/linux-wireless/patch/20220713211645.0d320e00e5b6.Ida11d2308dbf999d8bb9b1c49aa6e73af8fd3d33@changeid/
is the fix for this.
Thanks,
Jeongik
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-15 1:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-04 8:43 [PATCH v1] wifi: mac80211_hwsim: fix race condition in pending packet Jeongik Cha
2022-07-14 8:37 ` Jeongik Cha
2022-07-14 8:38 ` Johannes Berg
2022-07-14 8:40 ` Jeongik Cha
2022-07-15 0:16 ` Guenter Roeck
2022-07-15 1:53 ` Jeongik Cha
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).