Re: [PATCH bpf-next v1 6/6] bpf: selftests: selftests for memcg stat kfuncs

[Roman Gushchin <roman.gushchin@linux.dev>]

Shakeel Butt <shakeel.butt@linux.dev> writes:

> On Thu, Dec 18, 2025 at 05:57:50PM -0800, Roman Gushchin wrote:
>> From: JP Kobryn <inwardvessel@gmail.com>
>> 
>> Add test coverage for the kfuncs that fetch memcg stats. Using some common
>> stats, test scenarios ensuring that the given stat increases by some
>> arbitrary amount. The stats selected cover the three categories represented
>> by the enums: node_stat_item, memcg_stat_item, vm_event_item.
>> 
>> Since only a subset of all stats are queried, use a static struct made up
>> of fields for each stat. Write to the struct with the fetched values when
>> the bpf program is invoked and read the fields in the user mode program for
>> verification.
>> 
>> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
>
> Need your signoff

Sure, will add, thanks.

>
> [...]
>> +
>> +#define NR_PIPES 64
>> +static void test_kmem(struct bpf_link *link, struct memcg_query *memcg_query)
>> +{
>> +	int fds[NR_PIPES][2], i;
>> +
>> +	/*
>> +	 * Increase kmem value by creating pipes which will allocate some
>> +	 * kernel buffers.
>> +	 */
>> +	for (i = 0; i < NR_PIPES; i++) {
>> +		if (!ASSERT_OK(pipe(fds[i]), "pipe"))
>> +			goto cleanup;
>> +	}
>> +
>> +	if (!ASSERT_OK(read_stats(link), "read stats"))
>> +		goto cleanup;
>> +
>> +	ASSERT_GT(memcg_query->memcg_kmem, 0, "kmem value");
>> +
>> +cleanup:
>> +	for (i = 0; i < NR_PIPES; i++) {
>
> Instead of from 0 to NR_PIPES, we need to go from i-1 to (and equal to) 0
> otherwise we can potentially close() junk values.

Good catch, will fix.

>
>> +		close(fds[i][0]);
>> +		close(fds[i][1]);
>> +	}
>> +}
>> +
>
> [...]
>
>> +
>> +SEC("iter.s/cgroup")
>> +int cgroup_memcg_query(struct bpf_iter__cgroup *ctx)
>> +{
>> +	struct cgroup *cgrp = ctx->cgroup;
>> +	struct cgroup_subsys_state *css;
>> +	struct mem_cgroup *memcg;
>> +
>> +	if (!cgrp)
>> +		return 1;
>> +
>> +	css = &cgrp->self;
>> +	if (!css)
>
> Will css ever be NULL here?

Hm, I think previously the verifier wasn't smart enough to understand
that it's always a valid pointer, but I just tested it with linux-next
and it worked well. I'll drop the check.

Thanks!