| Message ID | 20220704084354.3556326-1-jeongik@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v1] wifi: mac80211_hwsim: fix race condition in pending packet | expand |
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
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
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
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
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
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)
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(-)