[PATCH v4] mac80211: Drop the packets whose source or destination mac address is empty

[PATCH v4] mac80211: Drop the packets whose source or destination mac address is empty

[Ming Chen]

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 <ming.chen@watchguard.com>
---
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

Re: [PATCH v4] mac80211: Drop the packets whose source or destination mac address is empty

[Toke Høiland-Jørgensen]

Ming Chen <ming032217@gmail.com> 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

Re: [PATCH v4] mac80211: Drop the packets whose source or destination mac address is empty

[Johannes Berg]

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

RE: [PATCH v4] mac80211: Drop the packets whose source or destination mac address is empty

[Ming Chen]

Thanks. Please see the inline reply.


Ming

> -----Original Message-----
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> Sent: Monday, November 18, 2019 3:33 AM
> To: Ming Chen <ming032217@gmail.com>; Johannes Berg
> <johannes@sipsolutions.net>
> Cc: linux-wireless@vger.kernel.org; Ming Chen
> <Ming.Chen@watchguard.com>
> Subject: Re: [PATCH v4] mac80211: Drop the packets whose source or
> destination mac address is empty
> 
> Ming Chen <ming032217@gmail.com> 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 :)
> 
[Ming Chen] As I said previously, this issue can be exposed when the radio works as a wireless client. The TX path will look up the sta(vap) by BSSID instead of mac address in SKB (see ieee80211_lookup_ra_sta). The BSSID would have been stored when the client is connected, so it could succeed to get the sta(vap). However, in the TX completion path, it will also compare the source mac in SKB and the one of the interface, after it retrieves the sta(vap) from hash_node table (see ieee80211_find_sta_by_ifaddr).

> > 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?
> 
> -Toke

Re: [PATCH v4] mac80211: Drop the packets whose source or destination mac address is empty

[Kalle Valo]

Ming Chen <Ming.Chen@watchguard.com> 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

RE: [PATCH v4] mac80211: Drop the packets whose source or destination mac address is empty

[Ming Chen]

> -----Original Message-----
> From: Kalle Valo <kvalo@codeaurora.org>
> Sent: Tuesday, November 19, 2019 1:42 AM
> To: Ming Chen <Ming.Chen@watchguard.com>
> Cc: Toke Høiland-Jørgensen <toke@redhat.com>; Ming Chen
> <ming032217@gmail.com>; Johannes Berg <johannes@sipsolutions.net>;
> linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v4] mac80211: Drop the packets whose source or
> destination mac address is empty
> 
> Ming Chen <Ming.Chen@watchguard.com> 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?
> 
[Ming Chen] I am not very familiar with the ARP processing in Linux Kernel, but I am sure I can see 
this kind of packets coming from the upper stack. I talked with the guys of this area of my company today, 
and they told me we didn't make any change on the ARP relevant processing.  
From my perspective, I think dropping this kind of packets is more or less reasonable. 
Anyway, I will make more investigation on how this kind of packets come into the driver. Thanks
> --
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingp
> atches