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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87cy49g7q3.fsf@linux.dev>
Date: Fri, 19 Dec 2025 19:20:04 -0800
From: Roman Gushchin <roman.gushchin@...ux.dev>
To: Shakeel Butt <shakeel.butt@...ux.dev>
Cc: bpf@...r.kernel.org,  linux-mm@...ck.org,  linux-kernel@...r.kernel.org,
  JP Kobryn <inwardvessel@...il.com>,  Alexei Starovoitov <ast@...nel.org>,
  Daniel Borkmann <daniel@...earbox.net>,  Michal Hocko
 <mhocko@...nel.org>,  Johannes Weiner <hannes@...xchg.org>
Subject: Re: [PATCH bpf-next v1 6/6] bpf: selftests: selftests for memcg
 stat kfuncs

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

> On Thu, Dec 18, 2025 at 05:57:50PM -0800, Roman Gushchin wrote:
>> From: JP Kobryn <inwardvessel@...il.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@...il.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!