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

[Ming Chen <Ming.Chen@watchguard.com>]

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