>-----Original Message-----
>From: Felix Fietkau <nbd@xxxxxxxx>
>Sent: Saturday, December 17, 2022 2:26 PM
>To: linux-wireless@xxxxxxxxxxxxxxx
>Cc: johannes@xxxxxxxxxxxxxxxx
>Subject: [PATCH v4] wifi: mac80211: fix initialization of rx->link and rx-
>>link_sta
>
>WARNING: This email originated from outside of Qualcomm. Please be wary
>of any links or attachments, and do not enable macros.
>
>There are some codepaths that do not initialize rx->link_sta properly. This
>causes a crash in places which assume that rx->link_sta is valid if rx->sta is
>valid.
>One known instance is triggered by __ieee80211_rx_h_amsdu being called
>from fast-rx. It results in a crash like this one:
>
> BUG: kernel NULL pointer dereference, address: 00000000000000a8
> #PF: supervisor write access in kernel mode
> #PF: error_code(0x0002) - not-present page PGD 0 P4D 0
> Oops: 0002 [#1] PREEMPT SMP PTI
> CPU: 1 PID: 506 Comm: mt76-usb-rx phy Tainted: G E 6.1.0-
>debian64x+1.7 #3
> Hardware name: ZOTAC ZBOX-ID92/ZBOX-IQ01/ZBOX-ID92/ZBOX-IQ01, BIOS
>B220P007 05/21/2014
> RIP: 0010:ieee80211_deliver_skb+0x62/0x1f0 [mac80211]
> Code: 00 48 89 04 24 e8 9e a7 c3 df 89 c0 48 03 1c c5 a0 ea 39 a1 4c 01 6b
>08 48 ff 03 48
> 83 7d 28 00 74 11 48 8b 45 30 48 63 55 44 <48> 83 84 d0 a8 00 00 00
>01 41 8b 86 c0
> 11 00 00 8d 50 fd 83 fa 01
> RSP: 0018:ffff999040803b10 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: ffffb9903f496480 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>0000000000000000
> RBP: ffff999040803ce0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff8d21828ac900
> R13: 000000000000004a R14: ffff8d2198ed89c0 R15: ffff8d2198ed8000
> FS: 0000000000000000(0000) GS:ffff8d24afe80000(0000)
>knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000000000a8 CR3: 0000000429810002 CR4:
>00000000001706e0 Call Trace:
> <TASK>
> __ieee80211_rx_h_amsdu+0x1b5/0x240 [mac80211]
> ? ieee80211_prepare_and_rx_handle+0xcdd/0x1320 [mac80211]
> ? __local_bh_enable_ip+0x3b/0xa0
> ieee80211_prepare_and_rx_handle+0xcdd/0x1320 [mac80211]
> ? prepare_transfer+0x109/0x1a0 [xhci_hcd]
> ieee80211_rx_list+0xa80/0xda0 [mac80211]
> mt76_rx_complete+0x207/0x2e0 [mt76]
> mt76_rx_poll_complete+0x357/0x5a0 [mt76]
> mt76u_rx_worker+0x4f5/0x600 [mt76_usb]
> ? mt76_get_min_avg_rssi+0x140/0x140 [mt76]
> __mt76_worker_fn+0x50/0x80 [mt76]
> kthread+0xed/0x120
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork+0x22/0x30
>
>Since the initialization of rx->link and rx->link_sta is rather convoluted and
>duplicated in many places, clean it up by using a helper function to set it.
>
>Fixes: ccdde7c74ffd ("wifi: mac80211: properly implement MLO key
>handling")
>Fixes: b320d6c456ff ("wifi: mac80211: use correct rx link_sta instead of
>default")
>Signed-off-by: Felix Fietkau <nbd@xxxxxxxx>
>---
>v4: fix regression in handling mgmt frames with AP_VLAN
>v3: include crash log
>v2: fix uninitialized variable
>
> net/mac80211/rx.c | 218 ++++++++++++++++++++--------------------------
> 1 file changed, 95 insertions(+), 123 deletions(-)
>
>diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index
>3fa7b36d4324..09cd0caded16 100644
>--- a/net/mac80211/rx.c
>+++ b/net/mac80211/rx.c
>@@ -4091,6 +4091,56 @@ static void
>ieee80211_invoke_rx_handlers(struct ieee80211_rx_data *rx) #undef
>CALL_RXH }
>
>+static bool
>+ieee80211_rx_is_valid_sta_link_id(struct ieee80211_sta *sta, u8
>+link_id) {
>+ if (!sta->mlo)
>+ return false;
>+
>+ return !!(sta->valid_links & BIT(link_id)); }
>+
>+static bool ieee80211_rx_data_set_link(struct ieee80211_rx_data *rx,
>+ u8 link_id) {
>+ if (!ieee80211_rx_is_valid_sta_link_id(&rx->sta->sta, link_id))
>+ return false;
>+
>+ rx->link_id = link_id;
>+ rx->link = rcu_dereference(rx->sdata->link[link_id]);
>+ rx->link_sta = rcu_dereference(rx->sta->link[link_id]);
>+
>+ return rx->link && rx->link_sta; }
>+
>+static bool ieee80211_rx_data_set_sta(struct ieee80211_rx_data *rx,
>+ struct ieee80211_sta *pubsta,
>+ int link_id) {
>+ struct sta_info *sta;
>+
>+ sta = container_of(pubsta, struct sta_info, sta);
>+
>+ rx->link_id = link_id;
>+ rx->sta = sta;
>+
>+ if (sta) {
>+ rx->local = sta->sdata->local;
>+ if (!rx->sdata)
>+ rx->sdata = sta->sdata;
>+ rx->link_sta = &sta->deflink;
>+
>+ if (link_id >= 0 &&
>+ !ieee80211_rx_data_set_link(rx, link_id))
>+ return false;
>+ }
>+
>+ if (link_id < 0)
>+ rx->link = &rx->sdata->deflink;
I guess this would set a wrong sdata link for a non ML STA associated to a ML AP, since
ieee80211_rx_is_valid_sta_link_id() adds condition above that a valid link id should be passed in Rx status from
a driver only for ML STA cases(sta->mlo check above).
Hence with no link id passed from driver for a Rx from a non ML STA, the link_id in this function would be -1
and hence will set the rx->link as the sdata->deflink .
A non ML STA could be associated to any of the links of a ML AP and hence the assumption of
using deflink for link_id < 0 would be wrong I think.
(May be this patch didn't change any behavior now, but the issue is already there I guess?)
Thanks,
Sriram.R
>+
>+ return true;
>+}
