Re: [PATCH v2 3/4] mm: introduce bpf_mthp_ops struct ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] BPF architectural layering violation resulting in a Use-After-Free risk.
- [High] Broken cgroup inheritance prevents overriding parent policies.
- [High] Unconditional atomic operations and SRCU locking on the page fault hot path.
- [Low] Build failures due to incomplete types and missing header includes.
--

commit 5151cfb43b99be8eef5398ab31786c76a122dc79
Author: Vernon Yang <yanglincheng@kylinos.cn>

mm: introduce bpf_mthp_ops struct ops
    
This commit introduces a new BPF struct_ops called bpf_mthp_ops to allow eBPF
programs to customize the sizes of multi-size THP (mTHP) for different
cgroups. It adds bpf_mthp_choose() to the page fault path to retrieve the
BPF-specified orders.

> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 1f084ee71443e..7db83ca81e6e7 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -5836,6 +5836,8 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name,
>  	if (ret)
>  		goto out_stat_exit;
>  
> +	cgroup_bpf_set_mthp_ops(cgrp, parent);

Does this file need to include <linux/bpf_huge_memory.h> for this function
call? This might cause a build failure due to a missing declaration.

> +
>  	for (tcgrp = cgrp; tcgrp; tcgrp = cgroup_parent(tcgrp))
>  		cgrp->ancestors[tcgrp->level] = tcgrp;

[ ... ]

> diff --git a/mm/bpf_huge_memory.c b/mm/bpf_huge_memory.c
> new file mode 100644
> index 0000000000000..851c6ebe29334
> --- /dev/null
> +++ b/mm/bpf_huge_memory.c
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
[ ... ]
> +#include <linux/bpf.h>
> +#include <linux/srcu.h>

Does this file also need to include <linux/memcontrol.h> and <linux/cgroup.h>?
Functions like get_mem_cgroup_from_mm() and css_for_each_descendant_pre() are
used below, which might lead to build failures.

> +
> +/* Protects cgrp->mthp_ops pointer for read and write. */
> +DEFINE_SRCU(mthp_bpf_srcu);
> +
> +unsigned long bpf_mthp_choose(struct mm_struct *mm, unsigned long orders)
> +{
> +	struct cgroup *cgrp;
> +	struct mem_cgroup *memcg;
> +	struct bpf_mthp_ops *ops;
> +	int idx;
> +
> +	memcg = get_mem_cgroup_from_mm(mm);

Since thp_vma_allowable_orders() calls this function unconditionally on every
THP allocation, does executing get_mem_cgroup_from_mm() and modifying atomic
reference counts on the page fault hot path introduce significant overhead?

Could a static key be used to bypass these atomic operations and the SRCU
locking below when the BPF feature is inactive?

> +	if (!memcg)
> +		return orders;
> +
> +	cgrp = memcg->css.cgroup;
> +
> +	idx = srcu_read_lock(&mthp_bpf_srcu);
> +	ops = READ_ONCE(cgrp->mthp_ops);
> +	if (unlikely(ops && ops->mthp_choose))
> +		orders = ops->mthp_choose(cgrp, orders);
> +	srcu_read_unlock(&mthp_bpf_srcu, idx);
> +
> +	mem_cgroup_put(memcg);
> +
> +	return orders;
> +}

[ ... ]

> +static int bpf_mthp_ops_reg(void *kdata, struct bpf_link *link)
> +{
> +	struct bpf_struct_ops_link *st_link = (struct bpf_struct_ops_link *)link;
> +	struct bpf_mthp_ops *ops = kdata;
> +	struct cgroup_subsys_state *child;
> +	struct cgroup *cgrp;
> +
> +	if (!link)
> +		return -EOPNOTSUPP;
> +
> +	cgrp = st_link->cgroup;

Can this code successfully compile? struct bpf_struct_ops_link appears to be an
incomplete type defined privately in kernel/bpf/bpf_struct_ops.c, and it lacks
a cgroup field.

Additionally, does bpf_mthp_ops_reg() need to acquire a reference count on the
target cgroup using cgroup_get()? If the cgroup is deleted while the link
remains active, could st_link->cgroup become a dangling pointer and lead to a
use-after-free when bpf_mthp_ops_unreg() is later called?

> +	if (!cgrp)
> +		return -EINVAL;
> +
> +	cgroup_lock();
> +	css_for_each_descendant_pre(child, &cgrp->self) {
> +		if (READ_ONCE(child->cgroup->mthp_ops)) {
> +			pr_warn("sub-cgroup has already registered.\n");
> +			cgroup_unlock();
> +			return -EBUSY;
> +		}
> +	}

Since cgroup_create() copies parent->mthp_ops directly to the child cgroup,
wouldn't all descendants inherently have mthp_ops set if their parent does?
Does this block users from registering an overriding BPF program on the child
cgroup, or block the parent if a child registers first?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508150055.680136-1-vernon2gm@gmail.com?part=3