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

[Luka Gejak]

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 am posting it rather than waiting for your comments on v6, since
patches 4 and 6 changed enough that reviewing the v6 versions would not
be a good use of your time.

Best regards,
Luka Gejak