We found ath9k could occasionally receive some frames from Linux IP stack with empty source
and destination mac address, especially when the radio mode works as a wireless client and
configure a static IP. If the ADDBA has been negotiated, this kind of error packets will cause
the driver failed to find the opposite node (VAP) while in the function of processing these frame's TX
complete interrupt.
The above failure happens inside the TX complete processing
function ath_tx_process_buffer while calling ieee80211_find_sta_by_ifaddr.
Inside the function ieee80211_find_sta_by_ifaddr,
the condition of ether_addr_equal(sta->sdata->vif.addr, localaddr) will return false
since localaddr(hdr->addr2, 802.3 source mac) is an empty mac address.
Finally, this function will return NULL to ath_tx_process_buffer.
And then ath_tx_process_buffer will call ath_tx_complete_aggr to complete the frame(s),
However, the sta is NULL at this moment, so it could complete this kind
of the frame(s) but doesn't (and cannot) update the BA window.
Please see the below snippet of ath_tx_complete_aggr
if (!sta) {
INIT_LIST_HEAD(&bf_head);
while (bf) {
bf_next = bf->bf_next;
if (!bf->bf_state.stale || bf_next != NULL)
list_move_tail(&bf->list, &bf_head);
ath_tx_complete_buf(sc, bf, txq, &bf_head, NULL, ts, 0);
bf = bf_next;
}
return;
}
To fix this issue, we could remove the comparison of localaddr of ieee80211_find_sta_by_ifaddr
when works as a wireless client - it won't have more than one sta (VAP) found, but I don't think
it is the best solution. Dropping this kind of error packet before it
goes into the driver, should be the right direction.
Signed-off-by: Ming Chen <[email protected]>
---
v4:
-Add more details for the changelog
v3:
-Fix s-o-b location
v2:
-According to review feedback, use the is_zero_ether_addr to check if the mac address is empty.
---
net/mac80211/tx.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index db38be1b75fa..b18745a3f6b0 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2489,6 +2489,13 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
if (IS_ERR(sta))
sta = NULL;
+ /* drop this skb when source mac or destination mac is empty */
+ if (is_zero_ether_addr(skb->data) ||
+ is_zero_ether_addr(skb->data + ETH_ALEN)) {
+ ret = -ENOTCONN;
+ goto free;
+ }
+
#ifdef CONFIG_MAC80211_DEBUGFS
if (local->force_tx_status)
info_flags |= IEEE80211_TX_CTL_REQ_TX_STATUS;
@@ -3435,6 +3442,11 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
if (skb->sk && skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS)
return false;
+ /* drop this skb when source mac or destination mac is empty */
+ if (is_zero_ether_addr(skb->data) ||
+ is_zero_ether_addr(skb->data + ETH_ALEN))
+ return false;
+
if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
--
2.17.1
Ming Chen <[email protected]> writes:
> We found ath9k could occasionally receive some frames from Linux IP stack with empty source
> and destination mac address, especially when the radio mode works as a wireless client and
> configure a static IP. If the ADDBA has been negotiated, this kind of error packets will cause
> the driver failed to find the opposite node (VAP) while in the function of processing these frame's TX
> complete interrupt.
>
> The above failure happens inside the TX complete processing
> function ath_tx_process_buffer while calling ieee80211_find_sta_by_ifaddr.
> Inside the function ieee80211_find_sta_by_ifaddr,
> the condition of ether_addr_equal(sta->sdata->vif.addr, localaddr) will return false
> since localaddr(hdr->addr2, 802.3 source mac) is an empty mac address.
>
> Finally, this function will return NULL to ath_tx_process_buffer.
> And then ath_tx_process_buffer will call ath_tx_complete_aggr to complete the frame(s),
> However, the sta is NULL at this moment, so it could complete this kind
> of the frame(s) but doesn't (and cannot) update the BA window.
> Please see the below snippet of ath_tx_complete_aggr
> if (!sta) {
> INIT_LIST_HEAD(&bf_head);
> while (bf) {
> bf_next = bf->bf_next;
>
> if (!bf->bf_state.stale || bf_next != NULL)
> list_move_tail(&bf->list, &bf_head);
>
> ath_tx_complete_buf(sc, bf, txq, &bf_head, NULL, ts, 0);
>
> bf = bf_next;
> }
> return;
> }
>
> To fix this issue, we could remove the comparison of localaddr of ieee80211_find_sta_by_ifaddr
> when works as a wireless client - it won't have more than one sta (VAP) found, but I don't think
> it is the best solution.
Ah, so the TX path doesn't do any lookups when the device is a sta, but
the TX completion path does? This was the information I was looking for;
please explain this in the commit message, you don't need to paste in
the code :)
> Dropping this kind of error packet before it goes into the driver,
> should be the right direction.
So I still wonder why this happens from higher up in the stack. If
there's a legitimate reason, maybe dropping the packet is not the right
thing? And if there is *no* legitimate reason, maybe the packet should
be dropped higher up in the stack instead?
What kind of packets does this happen with?
-Toke
On Mon, 2019-11-18 at 12:32 +0100, Toke Høiland-Jørgensen wrote:
> > Dropping this kind of error packet before it goes into the driver,
> > should be the right direction.
>
> So I still wonder why this happens from higher up in the stack. If
> there's a legitimate reason, maybe dropping the packet is not the right
> thing? And if there is *no* legitimate reason, maybe the packet should
> be dropped higher up in the stack instead?
Agree, this is really weird, it'd be good to know the actual packet this
happens with. Don't think I've ever seen this.
johannes
Ming Chen <[email protected]> writes:
>> > Dropping this kind of error packet before it goes into the driver,
>> > should be the right direction.
>>
>> So I still wonder why this happens from higher up in the stack. If there's a
>> legitimate reason, maybe dropping the packet is not the right thing? And if
>> there is *no* legitimate reason, maybe the packet should be dropped higher
>> up in the stack instead?
>>
>> What kind of packets does this happen with?
>
> [Ming Chen] It should an ARP packet. I can see this kind of packet
> before ARP table is complete. If so, how about dropping it in the
> function of ieee80211_subif_start_xmit?
The question here is why are you seeing this but nobody else? Are you
using some special protocol, do you have some changes in the kernel
which cause this or what could explain this behaviour?
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches