Re: [PATCH v2 2/3] mm: page_counter: track protection state in page_counter_protection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent dmem cgroup eviction can observe a partially initialized pool and trigger a NULL pointer dereference.
--

commit 505dda1717b6dcbcc1903e6c7cc5e90077d70bb6
Author: linuszeng <linuszeng@tencent.com>
Subject: mm: page_counter: track protection state in page_counter_protection

This patch moves the read/write side of hierarchical protection from struct
page_counter to struct page_counter_protection. It updates
propagate_protected_usage and calculate_protection, along with accessors to
operate on the new context structure. The patch intends to introduce no
functional changes.

> diff --git a/mm/page_counter.c b/mm/page_counter.c
> index 38cb99f5f50ed..401201c8e3902 100644
> --- a/mm/page_counter.c
> +++ b/mm/page_counter.c

[ ... ]

> @@ -463,23 +479,24 @@ void page_counter_calculate_protection(struct page_counter *root,
>  		return;
>  
>  	if (parent == root) {
> -		counter->emin = READ_ONCE(counter->min);
> -		counter->elow = READ_ONCE(counter->low);
> +		prot->emin = READ_ONCE(prot->min);
> +		prot->elow = READ_ONCE(prot->low);
>  		return;
>  	}
>  
> +	parent_prot = parent->prot;

[Severity: High]
This is a pre-existing issue, but could this code encounter a null pointer
dereference if concurrent dmem cgroup eviction observes a partially
initialized pool?

Looking at alloc_pool_single in kernel/cgroup/dmem.c, a new pool is
published to the RCU-protected pools list before its parent link is fully
established:

alloc_pool_single()
    ...
    list_add_tail_rcu(&pool->css_node, &dmemcs->pools);
    ...

If a concurrent thread running dmem_cgroup_state_evict_valuable or
dmem_cgroup_below_min iterates over the cgroup descendants via
css_for_each_descendant_pre, it can find this partially initialized pool and
pass it to page_counter_calculate_protection.

Since counter->parent is null and does not match the root argument, the
early return above is skipped, leading to a dereference of parent->prot
here when parent is null.

Does the eviction path need to synchronize with the completion of pool
initialization, or should page_counter_calculate_protection check if parent
is null before dereferencing it?

>  	parent_usage = page_counter_read(parent);
>  
> -	WRITE_ONCE(counter->emin, effective_protection(usage, parent_usage,
> -			READ_ONCE(counter->min),
> -			READ_ONCE(parent->emin),

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-descriptive-name-v2-0-d7dd7c099049@tencent.com?part=2