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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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