Re: [PATCH net 1/1] net: bridge: use a stable FDB dst snapshot in RCU readers

On Mon, Apr 13, 2026 at 05:08:46PM +0800, Ren Wei wrote:
> From: Zhengchuan Liang <[email protected]>
> 
> Local FDB entries can be rewritten in place by `fdb_delete_local()`, which
> updates `f->dst` to another port or to `NULL` while keeping the entry
> alive. Several bridge RCU readers inspect `f->dst`, including
> `br_fdb_fillbuf()` through the `brforward_read()` sysfs path.
> 
> These readers currently load `f->dst` multiple times and can therefore
> observe inconsistent values across the check and later dereference.
> In `br_fdb_fillbuf()`, this means a concurrent local-FDB update can change
> `f->dst` after the NULL check and before the `port_no` dereference,
> leading to a NULL-ptr-deref.
> 
> Fix this by taking a single `READ_ONCE()` snapshot of `f->dst` in each
> affected RCU reader and using that snapshot for the rest of the access
> sequence. Also publish the in-place `f->dst` updates in `fdb_delete_local()`
> with `WRITE_ONCE()` so the readers and writer use matching access patterns.

Sashiko is complaining [1] about missing READ_ONCE() annotations in some
places, but I can handle them in net-next in a similar fashion to commit
3e19ae7c6fd6 ("net: bridge: use READ_ONCE() and WRITE_ONCE() compiler
barriers for fdb->dst").

It's also complaining [2] about a not very interesting possible bug in
br_fdb_dump() which is pre-existing.

> 
> Fixes: 960b589f86c7 ("bridge: Properly check if local fdb entry can be deleted in br_fdb_change_mac_address")
> Cc: [email protected]
> Reported-by: Yifan Wu <[email protected]>
> Reported-by: Juefei Pu <[email protected]>
> Co-developed-by: Yuan Tan <[email protected]>
> Signed-off-by: Yuan Tan <[email protected]>
> Suggested-by: Xin Liu <[email protected]>
> Tested-by: Ren Wei <[email protected]>
> Signed-off-by: Zhengchuan Liang <[email protected]>
> Signed-off-by: Ren Wei <[email protected]>

Reviewed-by: Ido Schimmel <[email protected]>

[1]
"
Are there other RCU readers that still need this protection?

For instance, in br_dev_xmit(), br_fdb_find_rcu() returns a local FDB entry
which is then passed to br_forward(). If a concurrent fdb_delete_local()
sets the entry's dst to NULL, could this cause a NULL pointer dereference if
br_forward() is inlined and the compiler emits multiple loads?

Similarly, br_handle_frame_finish() appears to perform an unmarked read of
dst->dst, which might race with br_fdb_update().

Also, in br_fdb_delete_by_port(), f->dst is read directly without
READ_ONCE(). While called under br->hash_lock, the br_fdb_update()
fast path updates f->dst locklessly. Could this trigger KCSAN warnings due
to an unmarked data race?
"

[2]
"
Does passing f to fdb_fill_info() allow a concurrent update to change
the destination port after the filtering check?

fdb_fill_info() executes a new READ_ONCE(fdb->dst). If f->dst changes
between the filter_dev check above and the call to fdb_fill_info(), the
dumped entry might claim to be on a device that doesn't match the requested
filter_dev.

Should fdb_fill_info() be updated to accept the dst snapshot instead?
"