Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
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
In-Reply-To: <20191116060833.45752-1-ming.chen@watchguard.com>
References: <20191116060833.45752-1-ming.chen@watchguard.com>
Date:   Mon, 18 Nov 2019 12:32:39 +0100
Message-ID: <87blt9ctd4.fsf@toke.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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 wi=
reless client and
> configure a static IP. If the ADDBA has been negotiated, this kind of err=
or packets will cause
> the driver failed to find the opposite node (VAP) while in the function o=
f 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 r=
eturn 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 =3D bf->bf_next;
>
>                 if (!bf->bf_state.stale || bf_next !=3D NULL)
>                         list_move_tail(&bf->list, &bf_head);
>
>                 ath_tx_complete_buf(sc, bf, txq, &bf_head, NULL, ts, 0);
>
>                 bf =3D bf_next;
>         }
>         return;
> }
>
> To fix this issue, we could remove the comparison of localaddr of ieee802=
11_find_sta_by_ifaddr
> when works as a wireless client - it won't have more than one sta (VAP) f=
ound, 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