Re: [PATCH v3 6/7] mm/page_owner: add bpf_page_owner_get_memcg_info() kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/page_owner: tail page _nr_pages dereferenced as objcg in get_page_memcg_info()
- [Medium] mm/page_owner: __uninit in bpf_page_owner_get_memcg_info() poisons stack
- [Medium] mm/page_owner: missing __nullable on buf in bpf_page_owner_get_memcg_info()
--

commit 6c73498afe7c643ebd28b236a8d852daad168a14
Author: Zhen Ni <zhen.ni@easystack.cn>

mm/page_owner: add bpf_page_owner_get_memcg_info() kfunc

This commit factors per-page memcg lookup out of print_page_owner_memcg()
into get_page_memcg_info() and adds the bpf_page_owner_get_memcg_info()
kfunc so BPF page_owner iterators can retrieve memcg metadata and the full
cgroup path.

[ ... ]

> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 8bc15bcdc24e9..7470b5d002be4 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -538,16 +538,22 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,
>  
>  #ifdef CONFIG_MEMCG
>  /*
> - * Looking for memcg information and print it out
> + * Collect memcg information of @page into @info; when @buf and a
> + * nonzero @size are given, also copy the full cgroup path of the
> + * charged memcg into @buf.
> + *
> + * Return: 0 if @page is charged to an attributable memcg; -ENODATA
> + * otherwise.
>   */
> -static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
> -					 struct page *page)
> +int get_page_memcg_info(struct page *page, struct memcg_info *info,
> +			char *buf, int size)
>  {
>  	unsigned long memcg_data;
>  	struct obj_cgroup *objcg;
>  	struct mem_cgroup *memcg;
> -	bool online;
> -	char name[80];
> +	int ret = -ENODATA;
> +
> +	*info = (struct memcg_info){};
>  
>  	rcu_read_lock();
>  	memcg_data = READ_ONCE(page->memcg_data);
> @@ -555,31 +561,68 @@ static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret,
>  		goto out_unlock;
>  
>  	if (memcg_data & MEMCG_DATA_OBJEXTS) {
> -		ret += scnprintf(kbuf + ret, count - ret,
> -				"Slab cache page\n");
> +		info->is_slab = true;
>  		goto out_unlock;
>  	}
>  
>  	objcg = (void *)(memcg_data & ~OBJEXTS_FLAGS_MASK);
> -	memcg = objcg ? obj_cgroup_memcg(objcg) : NULL;
> +	if (!objcg)
> +		goto out_unlock;
> +
> +	memcg = obj_cgroup_memcg(objcg);

[Severity: High]
Can get_page_memcg_info() in mm/page_owner.c dereference folio->_nr_pages as
a struct obj_cgroup pointer when inspecting tail page 1 of a compound folio?

In struct folio (include/linux/mm_types.h), the first tail page (__page_1)
places _nr_pages at the same offset as struct page::memcg_data:

include/linux/mm_types.h:struct folio {
    ...
			atomic_t _mapcount_1;
			atomic_t _refcount_1;
#ifdef NR_PAGES_IN_LARGE_FOLIO
			unsigned int _nr_pages;
#endif
		};
		struct page __page_1;
    ...
}

For a compound folio with order >= 3, _nr_pages is 1U << order (for example,
0x8 or 0x200), so its low bits matching OBJEXTS_FLAGS_MASK are zero.

Because bpf_page_owner_get_memcg_info() in mm/page_owner_iter.c calls
get_page_memcg_info() on scan->pfn without stabilizing the page reference or
folio state (and struct page_owner_scan is a scalar-only struct that a BPF
program can also populate on the stack or in a map), page can refer to tail
page 1 of a compound folio.

In get_page_memcg_info(), page->memcg_data is read before checking
PageTail(page):

	memcg_data = READ_ONCE(page->memcg_data);
	if (!memcg_data || PageTail(page))
		goto out_unlock;

If tail page 1 is concurrently freed via __free_pages_prepare() ->
free_tail_page_prepare() -> clear_compound_head() after READ_ONCE() reads
_nr_pages into memcg_data, or if the folio is concurrently split to order 0
in mm/huge_memory.c:__split_folio_to_order() where clear_compound_head()
clears PageTail(new_head) before new_folio->memcg_data overwrites _nr_pages:

mm/huge_memory.c:__split_folio_to_order() {
    ...
		clear_compound_head(new_head);
		if (new_order) {
			prep_compound_page(new_head, new_order);
			folio_set_large_rmappable(new_folio);
		}
    ...
#ifdef CONFIG_MEMCG
		new_folio->memcg_data = folio->memcg_data;
#endif
}

wouldn't PageTail(page) evaluate to false while memcg_data holds 1UL << order,
causing obj_cgroup_memcg(objcg) to dereference that page count as a pointer?

[ ... ]

> diff --git a/mm/page_owner_iter.c b/mm/page_owner_iter.c
> index b7e0ce1dfa80a..a649e1207e246 100644
> --- a/mm/page_owner_iter.c
> +++ b/mm/page_owner_iter.c
> @@ -181,4 +181,52 @@ __bpf_kfunc int bpf_page_owner_stack_snprint(struct page_owner_scan *scan,
>  	return len;
>  }
>  
> +/**
> + * bpf_page_owner_get_memcg_info - Report the memcg charged for the current page
> + * @scan: page_owner scan handle.
> + * @info__uninit: output buffer for the memcg_info.
> + * @buf__uninit: optional output buffer for the full cgroup path.
> + * @buf__sz: size of @buf__uninit.
> + *
> + * Fills @info__uninit with the memcg charged for the page backing
> + * @scan->pfn.  If @buf__uninit and @buf__sz are provided, the full
> + * cgroup path is also copied out, so a BPF program can attribute a
> + * page_owner allocation to a specific cgroup.
> + *
> + * @scan->pfn is resolved to a struct page under get_online_mems(),
> + * so the page cannot be accessed after its memory section is offlined
> + * or removed.
> + *
> + * Returns:
> + * * 0 on success
> + * * -ENODATA if the page is not online or not charged to a memcg
> + * * -EOPNOTSUPP if built without CONFIG_MEMCG
> + */
> +__bpf_kfunc int bpf_page_owner_get_memcg_info(struct page_owner_scan *scan,
> +					      struct memcg_info *info__uninit,
> +					      char *buf__uninit, u32 buf__sz)

[Severity: Medium]
Will the __uninit suffix on info__uninit and buf__uninit in
bpf_page_owner_get_memcg_info() (mm/page_owner_iter.c) cause the BPF verifier
to reject valid programs that pass stack buffers across a pruning checkpoint?

In kernel/bpf/verifier.c:bpf_kfunc_stack_access_bytes(), any kfunc argument
matching is_kfunc_arg_uninit() returns a negative size:

	if (is_kfunc_arg_uninit(btf, &args[arg]))
		return -size;
	return size;

That negative size tells record_stack_access_off() in kernel/bpf/liveness.c
that the kfunc call overwrites the stack slots without reading them, so static
stack liveness analysis marks those slots dead prior to the call. When
clean_verifier_state() -> __clean_func_state() in kernel/bpf/states.c runs at
an earlier pruning checkpoint (such as the second iteration of
bpf_iter_page_owner_next(), or a branch/call after stack allocation), it
poisons those dead stack slots with STACK_POISON:

	for (j = start; j < end; j++)
		st->stack[i].slot_type[j] = STACK_POISON;

However, check_kfunc_args() in kernel/bpf/verifier.c only honors
is_kfunc_arg_uninit() for KF_ARG_PTR_TO_DYNPTR. For KF_ARG_PTR_TO_MEM
(info__uninit) and KF_ARG_MEM_SIZE (buf__uninit), check_kfunc_args() validates
the buffers with BPF_READ | BPF_WRITE:

	ret = check_mem_reg(env, reg, argno, type_size, BPF_READ | BPF_WRITE,
			    meta, &known_memory);
    ...
	ret = check_mem_size_reg(env, buff_reg, size_reg, buff_argno, argno,
				 BPF_READ | BPF_WRITE, true, meta, &failure);

When check_stack_range_initialized() in kernel/bpf/verifier.c performs the
BPF_READ check with allow_poison == false, it hits *stype == STACK_POISON and
fails verification with -EACCES ("slot poisoned by dead code elimination").

> +{
> +	struct page *page;
> +	int ret = -ENODATA;
> +
> +	if (!static_branch_unlikely(&page_owner_inited))
> +		goto nodata;
> +
> +	get_online_mems();
> +	page = pfn_to_online_page(scan->pfn);
> +	if (!page) {
> +		put_online_mems();
> +		goto nodata;
> +	}
> +	ret = get_page_memcg_info(page, info__uninit, buf__uninit, buf__sz);
> +	put_online_mems();
> +	return ret;
> +
> +nodata:
> +	*info__uninit = (struct memcg_info){};
> +	if (buf__uninit && buf__sz)
> +		buf__uninit[0] = '\0';

[Severity: Medium]
Should the optional cgroup path buffer parameter in
bpf_page_owner_get_memcg_info() (mm/page_owner_iter.c) use a __nullable
suffix so BPF programs can pass NULL when they only need struct memcg_info?

Both bpf_page_owner_get_memcg_info() and get_page_memcg_info() check whether
buf is NULL, and the kernel-doc describes buf__uninit as an optional output
buffer. However, in kernel/bpf/verifier.c:get_kfunc_arg_type(), PTR_MAYBE_NULL
is only added when is_kfunc_arg_nullable() matches the "__nullable" (or
"__arena") suffix:

static bool is_kfunc_arg_nullable(const struct btf *btf, const struct btf_param *arg)
{
	return btf_param_match_suffix(btf, arg, "__nullable") ||
	       btf_param_match_suffix(btf, arg, "__arena");
}

Without a __nullable suffix on the buffer argument, check_kfunc_args() in
kernel/bpf/verifier.c rejects a BPF program that passes NULL (or a
PTR_MAYBE_NULL register) for arg#3 with -EACCES ("Possibly NULL pointer passed
to trusted arg#3").

> +	return ret;
> +}
> +
>  __bpf_kfunc_end_defs();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021112437.1799213-1-zhen.ni@easystack.cn?part=6