Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200625201857.almm27xgzburyxxu@wololo.home.arpa>
 <3d8546b66a4f2027a7fab1de291ec40f@adapt-ip.com> <e42b0d0e87b2461a9b023e432635e6a6@realtek.com>
In-Reply-To: <e42b0d0e87b2461a9b023e432635e6a6@realtek.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Fri, 20 Nov 2020 17:43:31 -0800
Message-ID: <CA+ASDXOPvnxZu7H2A8y-ApAVBOyK4vi7x7bTN0iF9SJe6qCU8g@mail.gmail.com>
Subject: Re: [PATCH] rtw88: fix skb_under_panic in tx path
To:     Yan-Hsuan Chuang <tony0620emma@gmail.com>,
        Nick Owens <mischief@offblast.org>
Cc:     Thomas Pedersen <thomas@adapt-ip.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

(Swapping out Yen-Hsuan's new email)

Necromancing an old thread, since it's still relevant, and I had it
sitting in my inbox to deal with. Now I have something useful to say!

On Mon, Jun 29, 2020 at 11:50 AM Tony Chuang <yhchuang@realtek.com> wrote:
> > On 2020-06-25 13:18, Nick Owens wrote:

> > > fixes the following panic on my thinkpad A485
> > >
> > > Oops#1 Part3
> > > <0>[ 3743.881656] skbuff: skb_under_panic: text:000000005f69fd98
> > > len:208 put:48 head:000000009e2719e8 data:00000000bd3795e0 tail:0xc2
> > > end:0x2c0 dev:wlp2s0
...
> > skb->head and skb->data here are really far (0.5GB) apart. Maybe
> > skb->data actually got corrupted earlier?

For the record, I've reproduced similar issues myself, and the problem
occurs when
(a) the initial SKB starts with minimal headroom (I find that many
SKBs come into mac80211 with plenty of headroom)
(b) the SKB participates in AMSDU aggregation
I've spotted a specific bug, which I'll point out below. But I remain
confused why in many cases the SKB ends up looking so corrupted.

...
> > If it is a headroom issue, you can actually express the needed headroom
> > needed by the driver in hw->extra_tx_headroom during init and avoid the
> > pskb_expand_head() here.
>
> Looks like a headroom issue, but the driver already assigned headroom.
>         max_tx_headroom = rtwdev->chip->tx_pkt_desc_sz;
>         hw->extra_tx_headroom = max_tx_headroom;
>
> Then I am not sure why this happens. Nick, can you help to dump_stack()
> so we can see where is the skb from?

That's not so easy, because of the layers and queueing involved, but
per the above, I've blamed mac80211's AMSDU aggregation. Specifically:

ieee80211_amsdu_aggregate() -> ieee80211_amsdu_prepare_head(), where
we pad out / expand the SKB to fit some additional AMSDU headers (and
later append additional data). But the padding function
(ieee80211_amsdu_realloc_pad()) accounts only for the 802.11 protocol
headroom, and not for the driver-specific headroom. So it chooses not
to expand the headroom, and instead eats into rtw88's space.

For such SKBs, they end up in the driver without sufficient headroom
-- thus, Nick's bug report.

NB: the seemingly-obvious fix (changing the headroom checks in
ieee80211_amsdu_realloc_pad()) does not seem to work, as I hit other
bugs along the way. Unfortunately, I haven't had the time to fix this
all myself properly, nor have I convinced Realtek to fix this
themselves. So in the meantime, Chrome OS is running with this:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/260a7d4939c323aebe80efc73610682ad2cb187a%5E%21/#F0

It's a similar idea.

We should of course fix the mac80211 bug, but I wonder if we also
deserve some patch similar to either the Chromium patch or Nick's
somewhere (perhaps with a loud warning, etc.), because it's much more
user friendly (in the face of similar future bugs) to do some
suboptimal memcpy()'s, etc., than to crash their systems

Brian