Re: [PATCH bpf-next] bpf: Annotate data races on bpf_lru_node type

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]

> bpf: Annotate data races on bpf_lru_node type

TL;DR: Commit message only: it may be inaccurate about the locking for the
node->type writes, and "the sanity check only feeds a warning" is questionable
since the check also causes an early return.

> bpf: Annotate data races on bpf_lru_node type

[ ... ]

> node->type is written under the global l->lock when a node moves between
> the active, inactive and free lists, and under the per-CPU loc_l->lock
> when it moves to or from a local list. It is read in two places that do
> not hold the lock the writer holds:

The commit message says node->type is written under the per-CPU
loc_l->lock when it moves to or from a local list.  Is that complete for
the writers this patch converts to WRITE_ONCE()?

The failure path in bpf_common_lru_pop_free() writes node->type after
taking loc_l->lock has failed:

kernel/bpf/bpf_lru_list.c:bpf_common_lru_pop_free() {
    ...
	if (raw_res_spin_lock_irqsave(&loc_l->lock, flags)) {
		/* ... */
		WRITE_ONCE(node->type, BPF_LRU_LOCAL_LIST_T_FREE);
    ...
}

so no lock is held for that write.  Could the commit message mention
that this write is safe because the stolen node is not on any list and
is owned by the current CPU at that point?

[ ... ]

> Both races are benign: the type is a single byte, the sanity check only
> feeds a warning, and the re-check exists precisely because the value can
> change concurrently (any change sends the node down the check_lru_list
> path, which revalidates under l->lock). Mark the two reads with

Is "the sanity check only feeds a warning" accurate?  In
bpf_lru_list_push_free() the WARN_ON_ONCE() condition also controls an
early return:

kernel/bpf/bpf_lru_list.c:bpf_lru_list_push_free() {
    ...
	if (WARN_ON_ONCE(IS_LOCAL_LIST_TYPE(READ_ONCE(node->type))))
		return;
    ...
}

so the racy value affects control flow and not only the warning.

Would it be more precise to say the race is benign because a concurrent
writer can only store BPF_LRU_LIST_T_ACTIVE or BPF_LRU_LIST_T_INACTIVE
for a node being pushed free, so IS_LOCAL_LIST_TYPE() gives the same
result for any value the read can observe?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/38500297852