Re: [PATCH v5 0/6] wifi: rtw88: preparations for RTL8723B/RTL8723BS

[Peter Robinson]

Hi Luka,

On Thu, 20 Aug 2026 at 10:02, Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
>
> On August 14, 2026 12:53:20 PM GMT+02:00, luka.gejak@xxxxxxxxx wrote:
> >From: Luka Gejak <luka.gejak@xxxxxxxxx>
> >
> >This is the first of two series adding support for the Realtek RTL8723B
> >802.11n chipset and its RTL8723BS SDIO variant to rtw88. It contains
> >only the changes to the shared rtw88 core that the chip driver depends
> >on. The chip itself, the build glue and the MAINTAINERS entry are a
> >second series.
> >
>
> ...
>
> Hi Ping-Ke,
>
> Please do not apply patches 4 and 6 of v6. I found real bugs in both
> after posting and a v7 is on the way.
>
> Patch 6 touches the skb after handing it to the TX worker:
>
>         skb_queue_tail(&rtwsdio->tx_queue[queue], skb);
>
>         rtw_sdio_8723bs_stop_tx_queue(rtwdev, queue,
>                                       skb_get_queue_mapping(skb));
>
> The worker can dequeue, transmit and free that skb before the second
> statement runs, so reading it there is a use after free. v7 reads the
> queue index into a local before the enqueue.
>
> Patch 4 concerns you more than me. It passed the padded size to
> rtw_sdio_get_tx_addr(), where upstream passes skb->len. That value is
> encoded into the CMD53 address as the transfer length, so on every other
> SDIO chip the encoded length changed whenever sdio_align_size() padded.
> For the RTL8723BS both expressions are the same value, so the change
> bought this chip nothing and only affected the other parts. The padding
> itself also ran on the generic path, giving those chips an allocation
> and an -ENOMEM path they did not have. v7 restores skb->len and gates
> the padding on the chip.
>
> The other v7 changes, all in patches 4 and 6:
>
>   - the free page check, the output queue wait and the accounting are
>     serialised. The TX worker and the H2C path both reach
>     rtw_sdio_write_port(), and two writers could each pass the checks
>     and claim the same pages and output queue entry, after which the
>     chip discards one transfer silently. The vendor driver has one
>     transmit thread and never needed this.
>   - the padding called skb_put_zero() after checking only tailroom, so a
>     cloned skb had its shared buffer written. It uses __skb_pad() now.
>   - the back-pressure stop could race the drain: the worker could empty
>     the queue while the stop was still in progress, see the flag clear,
>     and never wake it, leaving an access category stopped with nothing
>     left to wake it.
>
> Patches 1, 2, 3 and 5 are unchanged and keep your acks; patch 5 shifts
> only by hunk offsets because patch 4 grew above it. I dropped your ack
> from patch 4, since the rework is too substantive to carry it.
>
> Most of these came from Sashiko's automated review of v6. I checked each
> against the code before acting on it, and one of its eight findings did
> not hold up.
>
> v7 follows shortly. It is hardware tested: 60 idle pings at 0% loss with
> power save on, bidirectional load with no stall, 19.6 Mbit/s up and 36.8
> down, UDP at 0% loss, three scans, three reconnects and a clean log.

I had done some basic testing with v5, and had left a v6 to build
overnight. I'll grab this shortly and do a build with v7 and do some
testing.It was generally looking stable on the Pine64+ board I was
using for testing.

I think I may also have a device(s) with a 8723cs and/or a 8723ds I
can possibly test with, I'll see if I can remember/find it over the
weekend and get it running for some testing of that too.

Peter