Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:
- [High] Missing fixup of `prot.parent` linkage when dynamically creating dmem 
cgroup pools breaks hierarchical protection.
--

commit 534b5f88933b6e08b308d664bd9c8b28861c0c8c
Author: linuszeng <[email protected]>

mm: page_counter: add page_counter_protection struct and init API

This commit extracts the hierarchical protection state (memory.min and
memory.low) from struct page_counter into a new page_counter_protection
structure. It introduces page_counter_init_protection() to attach this
context, saving space for counters that don't support protection.

> diff --git a/kernel/cgroup/dmem.c b/kernel/cgroup/dmem.c
> index 4683f3d680226..e33d807cdea89 100644
> --- a/kernel/cgroup/dmem.c
> +++ b/kernel/cgroup/dmem.c
> @@ -426,8 +427,9 @@ alloc_pool_single(struct dmemcg_state *dmemcs, struct 
> dmem_cgroup_region *region
>       if (parent)
>               ppool = find_cg_pool_locked(parent, region);
>  
> -     page_counter_init(&pool->cnt,
> -                       ppool ? &ppool->cnt : NULL, true);
> +     page_counter_init(&pool->cnt, ppool ? &ppool->cnt : NULL);
> +     page_counter_init_protection(&pool->cnt, &pool->prot,
> +                                  ppool ? &ppool->prot : NULL);
>       reset_all_resource_limits(pool);
>       refcount_set(&pool->ref, 1);
>       kref_get(&region->ref);

[Severity: High]
Does get_cg_pool_locked() also need to be updated to fix up the prot.parent
linkage?

When alloc_pool_single() is called during a bottom-up dynamic pool creation,
ppool can be NULL. This sets pool->prot.parent to NULL here. Later in
get_cg_pool_locked(), the missing parent pools are created and a fixup loop
links the counters:

kernel/cgroup/dmem.c:get_cg_pool_locked() {
    ...
                /* ppool was created if it didn't exist by above loop. */
                ppool = find_cg_pool_locked(pp, region);

                /* Fix up parent links, mark as inited. */
                pool->cnt.parent = &ppool->cnt;
                if (ppool && !pool->parent) {
    ...
}

Because pool->prot.parent = &ppool->prot; is missing in this fixup loop,
prot.parent permanently remains NULL. This causes propagate_protected_usage()
to silently abort due to !prot->parent, breaking hierarchical protection
for the dmem cgroup.

-- 
Sashiko AI review ยท 
https://sashiko.dev/#/patchset/[email protected]?part=1

Reply via email to