Re: [PATCH v2 5/6] mm/page_owner: add bpf_page_owner_get_memcg_info() kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: TOCTOU race in bpf_page_owner_get_memcg_info() leads to page fault
- [High] mm: wild pointer dereference in get_page_memcg_info() for tail pages
--

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

mm/page_owner: add bpf_page_owner_get_memcg_info() kfunc

This commit exposes per-page memcg information to BPF programs.
It splits memcg info collection out of print_page_owner_memcg() into
a new get_page_memcg_info() function and adds the
bpf_page_owner_get_memcg_info() kfunc.

Link: https://lore.kernel.org/linux-mm/20261009112308.240769-6-zhen.ni@easystack.cn/

> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 113e4e7e0b3a1..664121be66863 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -538,16 +538,22 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m,

[ ... ]

> +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;
> +	int ret = -ENODATA;
> +
> +	*info = (struct memcg_info){};
>  
>  	rcu_read_lock();
>  	memcg_data = READ_ONCE(page->memcg_data);
> +	if (!memcg_data)
> +		goto out_unlock;

[Severity: High]
Does removing the PageTail() guard here expose a wild pointer dereference?

In struct folio, the first tail page's _nr_pages field aliases exactly with
the memcg_data offset when CONFIG_MEMCG is enabled. Due to a lack of memory
barriers in __update_page_owner_handle(), page_owner_next_eligible() can
observe PAGE_EXT_OWNER_ALLOCATED before page_owner->order is updated,
yielding a tail page.

If a tail page is passed here, the non-zero _nr_pages value (e.g., 512 for a
large folio) will bypass the !memcg_data check and be dereferenced as an
obj_cgroup pointer later in the function, leading to a kernel panic.

[ ... ]

> diff --git a/mm/page_owner_iter.c b/mm/page_owner_iter.c
> index a2b90c8748520..86e0b4b52bfc7 100644
> --- a/mm/page_owner_iter.c
> +++ b/mm/page_owner_iter.c
> @@ -177,4 +177,21 @@ __bpf_kfunc int bpf_page_owner_stack_snprint(struct page_owner_scan *scan,
>  				   0);
>  }
>  
> +__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)
> +{
> +	/* Guard against the section being offlined after scan. */
> +	struct page *page = pfn_to_online_page(scan->pfn);
> +
> +	if (!page) {
> +		*info__uninit = (struct memcg_info){};
> +		if (buf__uninit && buf__sz)
> +			buf__uninit[0] = '\0';
> +		return -ENODATA;
> +	}
> +
> +	return get_page_memcg_info(page, info__uninit, buf__uninit, buf__sz);
> +}

[Severity: High]
Is this vulnerable to a time-of-check to time-of-use race with memory
hot-remove?

The pfn_to_online_page() check and subsequent get_page_memcg_info()
dereference occur without holding get_online_mems(). Because this is a
sleepable BPF kfunc (KF_SLEEPABLE), the thread can be preempted between the
check and the dereference.

If memory hot-remove offlines and removes the memory section concurrently, it
tears down the vmemmap backing the struct page. When the thread resumes and
get_page_memcg_info() attempts to read page->memcg_data, it will fault on the
unmapped address.

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