Re: [RFC PATCH bpf-next v7 11/11] samples/bpf: Add memcg priority control and async reclaim example

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Non-atomic updates to the global `aggregation_map` in the `handle_count_memcg_events` tracepoint lead to data races and lost PGFAULT updates.
- [Medium] Atomic one-shot guards permanently leak in error paths, deadlocking the async reclaim functionality.
- [Low] `parse_u64` relies entirely on `errno` and fails to detect completely non-numeric input strings, silently returning 0.
- [Low] Ignoring the return value of `__atomic_add_fetch` and using a subsequent `READ_ONCE` introduces an unnecessary race window.
--

commit 0eac8db0526f6826e19ed8ba2b922d7d358b6032
Author: Hui Zhu <zhuhui@kylinos.cn>
Subject: samples/bpf: Add memcg priority control and async reclaim example

This commit adds a sample program demonstrating the memcg_bpf_ops feature.
It provides priority-based memory throttling and workqueue-driven
asynchronous page reclaim using a BPF program and userspace loader.

> diff --git a/samples/bpf/memcg.bpf.c b/samples/bpf/memcg.bpf.c
> new file mode 100644
> index 0000000000000..0995284794ac7
> --- /dev/null
> +++ b/samples/bpf/memcg.bpf.c
[ ... ]
> +static int async_free(void *map, int *key, void *value)
> +{
> +	struct cgroup_memcg cm;
> +	bool started_wq = false;
> +	int i;
> +
> +	if (get_cgroup_memcg_from_id(local_config.low_cgroup_id, &cm) != 0)
> +		return 0;

[Severity: Medium]
Can the async_free_run guard permanently leak if get_cgroup_memcg_from_id
fails? Returning early here skips the lock release at the end of the
function, which would deadlock the async reclaim functionality.

> +
> +	for (i = 0; i < ASYNC_FREE_LOOP_MAX; i++) {
[ ... ]
> +	}
> +
> +	put_cgroup_memcg(&cm);
> +
> +	if (!started_wq)
> +		__atomic_exchange_n(&async_free_run, 0, __ATOMIC_RELEASE);
> +	return 0;
> +}
[ ... ]
> +SEC("tp/memcg/count_memcg_events")
> +int handle_count_memcg_events(
> +		struct trace_event_raw_memcg_rstat_events *ctx)
> +{
[ ... ]
> +	current_ts = bpf_ktime_get_ns();
> +
> +	if (current_ts - data->window_start_ts < ONE_SECOND_NS) {
> +		data->sum += ctx->val;

[Severity: Medium]
Can this non-atomic update to the global aggregation_map lead to data races
and lost PGFAULT updates? Since this map is shared across all CPUs,
concurrent PGFAULT events executing the tracepoint might perform read,
modify, write operations simultaneously.

Should BPF atomic instructions or a bpf_spin_lock be used here?

> +	} else {
> +		data->window_start_ts = current_ts;
> +		data->sum = ctx->val;
> +	}
[ ... ]
> +static __always_inline unsigned int
> +__memcg_charged_impl(struct mem_cgroup *memcg, unsigned int nr_pages)
> +{
> +	struct wq_elem *elem;
> +	__u32 key = 0;
> +	u64 expected = 0;
> +	s64 cur_allocated;
> +	s64 cur_old_allocated;
> +
> +	__atomic_add_fetch(&allocated, nr_pages, __ATOMIC_RELAXED);
> +	cur_allocated = READ_ONCE(allocated);

[Severity: Low]
Is there a race window introduced by ignoring the return value of
__atomic_add_fetch and performing a separate READ_ONCE? A concurrent
increment by another CPU between the add and the read could cause
cur_allocated to observe an inconsistent or skipped value. 

Could the return value of __atomic_add_fetch be assigned directly to
eliminate this window?

> +	cur_old_allocated = READ_ONCE(old_allocated);
> +	if (abs_diff(cur_allocated, cur_old_allocated) < ONE_MB_PAGE_COUNT)
> +		goto out;
> +	WRITE_ONCE(old_allocated, cur_allocated);
> +
> +	if (get_usage() < local_config.async_trigger_bytes)
> +		goto out;
> +
> +	if (__atomic_compare_exchange_n(&async_free_run,
> +					&expected, 1,
> +					false,
> +					__ATOMIC_ACQ_REL,
> +					__ATOMIC_RELAXED)) {
> +		elem = bpf_map_lookup_elem(&wq_map, &key);
> +		if (!elem)
> +			return 0;

[Severity: Medium]
Does this early return also leak the async_free_run guard if the map lookup
fails? Like the early return in async_free, this skips resetting the guard
and could deadlock the state machine.

> +
> +		bpf_wq_start(&elem->work, 0);
> +	}
> +
> +out:
> +	return 0;
> +}
[ ... ]
> diff --git a/samples/bpf/memcg.c b/samples/bpf/memcg.c
> new file mode 100644
> index 0000000000000..0929d868e6d8f
> --- /dev/null
> +++ b/samples/bpf/memcg.c
[ ... ]
> +static uint64_t parse_u64(const char *str, const char *prog)
> +{
> +	uint64_t value;
> +
> +	errno = 0;
> +	value = strtoull(str, NULL, 10);
> +	if (errno != 0) {

[Severity: Low]
Does this code silently return 0 for completely non-numeric input strings?

The strtoull function does not set errno if the input string contains no
valid digits. Since the endptr argument is NULL, could malformed arguments
like --async_trigger_bytes abc silently return 0 instead of raising an
error?

> +		fprintf(stderr, "ERROR: strtoull '%s' failed: %d\n",
> +			str, errno);
> +		usage(prog);
> +		exit(-errno);
> +	}
> +	return value;
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779760876.git.zhuhui@kylinos.cn?part=11