Re: [PATCH] bpf: Annotate bpf_obj_memcpy with data_race

[bot+bpf-ci]

> syzbot reported KCSAN write-write races when two tasks concurrently
> update the same map value. Both accesses reach the ordinary memcpy()
> paths in bpf_obj_memcpy() through copy_map_value().
>
> Unlocked map value updates are intentionally not serialized. Therefore,
> a concurrent update may result in a torn value. Callers requiring
> consistency must provide external synchronization or use BPF_F_LOCK
> where supported. bpf_long_memcpy() already documents and annotates the
> same behavior for long-aligned copies.

The BPF_F_LOCK remedy is mentioned here, but does it actually apply to
the map type in the syzbot report? The report shows
BPF_MAP_TYPE_PERCPU_ARRAY, which rejects BPF_F_LOCK in
bpf_percpu_array_update() and cannot have a bpf_spin_lock field per
map_check_btf(). Would it be more accurate to say that external
synchronization is the only option for per-cpu arrays, and that
BPF_F_LOCK is available for other map types where supported?

> Annotate the ordinary memcpy() operations in bpf_obj_memcpy() with
> data_race() as well. This documents the existing concurrency semantics
> and prevents KCSAN from reporting these intentional races. It does not
> add synchronization or change map update behavior.
>
> Place the annotations at the memcpy() sites in the common helper, matching
> bpf_long_memcpy(), without changing the existing copy helper interfaces.

This isn't a bug, but the last two paragraphs both describe where the
data_race() annotations were placed. Would folding them together keep
the changelog tighter without losing the rationale?

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index ffa5626411ac..68f022810fbc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -560,7 +560,16 @@ static inline void bpf_long_memcpy(void *dst, const void *src, u32 size)
>  		data_race(*ldst++ = *lsrc++);
>  }
>
> -/* copy everything but bpf_spin_lock, bpf_timer, and kptrs. There could be one of each. */
> +/*
> + * Copy everything but bpf_spin_lock, bpf_timer, and kptrs. There could
> + * be one of each.
> + *
> + * Map value copies can race with unlocked updates from BPF programs or
> + * syscalls. Such copies are best effort and may be torn, so annotate the
> + * ordinary byte copies as intentional data races. Callers that require a
> + * consistent value must provide synchronization or use BPF_F_LOCK where
> + * supported.
> + */

The comment states that all map value copies "are best effort and may be
torn", but is that accurate for every bpf_obj_memcpy() caller? Several
callers copy into freshly allocated, not-yet-published elements
(alloc_htab_elem, bpf_selem_alloc, bpf_sk_storage_clone_elem) or into
private syscall buffers (bpf_map_copy_value), where no other writer can
reach the destination. Would scoping the comment to in-place updates of
published map values be more precise?

Also, the BPF_F_LOCK advice appears again here. As with the commit
message, this doesn't apply to per-cpu arrays - bpf_percpu_array_update()
rejects the flag, and map_check_btf() won't allow a bpf_spin_lock field.
Should the comment mention that per-cpu maps have no BPF_F_LOCK option?

>  static inline void bpf_obj_memcpy(struct btf_record *rec,
>  				  void *dst, void *src, u32 size,
>  				  bool long_memcpy)

[ ... ]

>  		if (long_memcpy)
>  			bpf_long_memcpy(dst, src, size);
>  		else
> -			memcpy(dst, src, size);
> +			data_race(memcpy(dst, src, size));
>  		return;
>  	}
>
> @@ -580,10 +589,10 @@ static inline void bpf_obj_memcpy(struct btf_record *rec,
>  		u32 next_off = rec->fields[i].offset;
>  		u32 sz = next_off - curr_off;
>
> -		memcpy(dst + curr_off, src + curr_off, sz);
> +		data_race(memcpy(dst + curr_off, src + curr_off, sz));
>  		curr_off += rec->fields[i].size + sz;
>  	}
> -	memcpy(dst + curr_off, src + curr_off, size - curr_off);
> +	data_race(memcpy(dst + curr_off, src + curr_off, size - curr_off));
>  }

[ ... ]


---
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/32275499243