* Re: [PATCH net] ath9k_htc: fix possibly missing barrier in ath9k_htc_rxep()
2026-08-03 10:44 [PATCH net] ath9k_htc: fix possibly missing barrier in ath9k_htc_rxep() Thomas Fourier
2026-09-08 8:00 ` Toke Høiland-Jørgensen
@ 2026-09-08 15:04 ` Jeff Johnson
2026-09-08 16:43 ` Toke Høiland-Jørgensen
1 sibling, 1 reply; 4+ messages in thread
From: Jeff Johnson @ 2026-09-08 15:04 UTC (permalink / raw)
To: Thomas Fourier
Cc: stable, Toke Høiland-Jørgensen, Tetsuo Handa,
open list:QUALCOMM ATHEROS ATH9K WIRELESS DRIVER, open list
On 8/3/2026 3:44 AM, Thomas Fourier wrote:
> ath9k_rx_init() initialises the rx buffer and its lock then calls a
> memory barrier and then sets the priv->rx.initialized flag. However,
> ath9k_htc_rxep() reads that flag and imidiatly takes the lock. This may
s/imidiatly/immediately/
> cause the lock to be taken while not fully initialized.
>
> Add a barrier to prevent speculative read of the lock before checking
> the initized flag.
s/initized/initialized/ (or initialised)
>
> Fixes: b0ec7e55fce6 ("ath9k_htc: fix NULL pointer dereference at ath9k_htc_rxep()")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> ---
> drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
> index bed7ea2425a0..97d61f3f0aad 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
> @@ -1145,6 +1145,12 @@ void ath9k_htc_rxep(void *drv_priv, struct sk_buff *skb,
> if (!data_race(priv->rx.initialized))
> goto err;
>
> + /*
> + * Make sure all the ath9k_rx_init() memory writes are visible before
> + * proceeding.
> + */
> + smp_rmb();
> +
> spin_lock_irqsave(&priv->rx.rxbuflock, flags);
> list_for_each_entry(tmp_buf, &priv->rx.rxbuf, list) {
> if (!tmp_buf->in_process) {
I decided to run this though my review agent, which has the following analysis:
### Overview
The parent commit `b0ec7e55fce6` added an `initialized` flag guarded by
`smp_wmb()` (writer) + `data_race()` (reader) to prevent `ath9k_htc_rxep()`
from operating before `ath9k_rx_init()` completes. This patch argues that the
reader side is missing a corresponding `smp_rmb()` barrier, allowing the CPU
to speculatively read `rxbuflock` (and related structures) before the
`initialized` flag check is confirmed.
### Correctness Analysis — **This is the central question**
The claim is that without `smp_rmb()`, the CPU can read `rxbuflock` before
the flag check. This deserves careful scrutiny.
**The actual memory ordering concern:**
In `ath9k_rx_init()`:
```c
spin_lock_init(&priv->rx.rxbuflock); // writes to rxbuflock struct
list_add_tail(...); // writes to rxbuf list
smp_wmb(); // store barrier
priv->rx.initialized = true; // flag store
```
In `ath9k_htc_rxep()` (before this patch):
```c
if (!data_race(priv->rx.initialized)) // flag load (unordered)
goto err;
spin_lock_irqsave(&priv->rx.rxbuflock, ...); // acquires lock
```
**Problem with the patch's reasoning:**
The `data_race()` annotation means this read is intentionally not ordered —
it is a deliberate racy read. For the `smp_wmb()`/`smp_rmb()` pairing to
work, the flag load must itself be an acquiring/ordered read. `data_race()`
is not `READ_ONCE()` and does not constitute the load half of a barrier pair.
**The correct fix** would be to replace `data_race(priv->rx.initialized)`
with `smp_load_acquire(&priv->rx.initialized)` and drop the `smp_wmb()` on
the writer side in favor of `smp_store_release()`. That gives a proper
acquire/release pair. Or, simpler: replace both the `smp_wmb()` + plain store
with `smp_store_release()`, and replace the `data_race()` read with
`smp_load_acquire()`.
Adding `smp_rmb()` after `data_race()` is also questionable because on
architectures with weak ordering (e.g., ARM64), the `data_race()` load of
`initialized` can still be observed as `true` via a stale/forwarded value
while the `smp_rmb()` fence only prevents reordering of subsequent loads
relative to *prior* loads — but the load that matters (the flag) was
already done without ordering guarantees.
**In practice on x86:** The `smp_rmb()` compiles to a no-op (x86 TSO provides
load/load ordering). The concern is real only on weakly-ordered architectures
(ARM64, POWER). There `data_race()` + `smp_rmb()` is not a sound barrier pair.
### Specific Issues
1. **Wrong primitives for the problem.** The correct idiom for "publish a
data structure, then set a flag that a reader checks before using the
structure" is:
- Writer: `smp_store_release(&priv->rx.initialized, true)` (replaces
`smp_wmb()` + plain store)
- Reader: `if (!smp_load_acquire(&priv->rx.initialized)) goto err;`
(replaces `data_race()` + separate `smp_rmb()`)
This is the standard kernel pattern (see
`Documentation/memory-barriers.txt`, "RELEASE-ACQUIRE" pairing). The
`data_race()` annotation is semantically wrong here — it indicates "I know
this is racy and accept it", not "do an ordered load."
2. **`data_race()` semantics mismatch.** `data_race()` suppresses KCSAN
warnings but is not a substitute for `READ_ONCE()` and provides no ordering.
The comment in `b0ec7e55fce6` used `data_race()` to suppress KCSAN since this
is a benign early-exit check — but that means `smp_rmb()` after it has no
memory ordering contract to stand on.
I ran the above through another LLM instance to check the analysis:
Issue 1 + 2 (barrier primitive mismatch) — Partially valid, but the framing is
overstated.
The reviewer's core claim: data_race() + smp_rmb() is not a sound barrier
pair. This deserves scrutiny.
data_race() is purely a KCSAN annotation — it disables the KCSAN
instrumentation and compiles down to a plain C load. On ARM64, the sequence as
patched is:
ldr w0, [initialized] // data_race() → plain load
cbz w0, goto_err // branch
dmb ishld // smp_rmb()
ldr x1, [rxbuflock] // spin_lock_irqsave
The dmb ishld prevents loads after it from being observed before loads before
it in program order. Since ldr [initialized] is before the dmb ishld and ldr
[rxbuflock] is after it, the ordering holds on ARM64 in practice. The
conditional branch plus smp_rmb() does close the window.
However, the reviewer is correct that the idiom is non-idiomatic and
semantically contradictory:
- data_race() explicitly declares "this is an intentional race / unordered
access" — which contradicts using it as the ordered-flag-read half of a
barrier pair.
- The standard kernel idiom for "publish-then-flag / check-flag-then-use" is
smp_store_release() + smp_load_acquire().
Verdict: Valid, but the stronger "technically unsound" characterization is
debatable. The right fix is to replace both primitives for clarity and idiom
compliance, not because smp_rmb() alone is provably broken with a conditional
branch preceding it.
The reviewer's recommended fix is correct:
/* ath9k_rx_init() — writer side, replaces smp_wmb() + plain store */
smp_store_release(&priv->rx.initialized, true);
/* ath9k_htc_rxep() — reader side, replaces data_race() + smp_rmb() */
if (!smp_load_acquire(&priv->rx.initialized))
goto err;
/* no separate smp_rmb() needed */
So my question: Are there any flaws with that analysis?
^ permalink raw reply [flat|nested] 4+ messages in thread