Re: [PATCH bpf-next v10 1/2] mm/bpf: Add bpf_proactive_reclaim kfunc

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Missing newline (\n) in pr_warn() format strings.

Pre-existing issues:
- [High] Missing negative bounds checks for enum array indices in `bpf_mem_cgroup_memory_events()` and `bpf_mem_cgroup_vm_events()`.
--

commit 21e3edc4898900dd57bbb4035bb4acd286a2047b
Author: Hui Zhu <zhuhui@kylinos.cn>

mm/bpf: Add bpf_proactive_reclaim kfunc

This commit introduces a sleepable kfunc bpf_proactive_reclaim() to allow
BPF programs to trigger proactive memory reclaim on a memory cgroup. The
kfunc caps the reclaim target at MEMCG_CHARGE_BATCH and is limited to
BPF_PROG_TYPE_SYSCALL programs to avoid filesystem lock deadlocks.

> diff --git a/mm/bpf_memcontrol.c b/mm/bpf_memcontrol.c
> index 716df49d76477..d8827bc388efc 100644
> --- a/mm/bpf_memcontrol.c
> +++ b/mm/bpf_memcontrol.c
[ ... ]
> @@ -159,6 +161,74 @@ __bpf_kfunc void bpf_mem_cgroup_flush_stats(struct mem_cgroup *memcg)
>  	mem_cgroup_flush_stats(memcg);
>  }
>  

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed that
the existing bpf_mem_cgroup_memory_events() and bpf_mem_cgroup_vm_events()
kfuncs do not properly check for negative enum indices.

Since the BPF verifier treats enums as signed integers and does not validate
enum ranges, can a BPF program pass a negative event enum value? The
existing bounds check in bpf_mem_cgroup_memory_events() is:

        if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
                return (unsigned long)-1;

A negative event value bypasses this upper-bound check due to signed integer
comparison, which leads to an arbitrary out-of-bounds kernel memory read
when the negative index is accessed here:

        return atomic_long_read(&memcg->memory_events[event]);

Similarly, memcg_vm_event_item_valid() permits negative values for
bpf_mem_cgroup_vm_events(), leading to an out-of-bounds read in
mem_cgroup_events_index[idx]. Does this require a check for negative values,
or perhaps casting the event argument to an unsigned type before comparison?

> +/**
> + * bpf_proactive_reclaim - proactively reclaim memory from a memory
> + *                         cgroup
[ ... ]
> @@ -174,19 +244,35 @@ BTF_ID_FLAGS(func, bpf_mem_cgroup_flush_stats, KF_SLEEPABLE)
[ ... ]
>  static int __init bpf_memcontrol_init(void)
>  {
>  	int err;
>  
>  	err = register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC,
>  					&bpf_memcontrol_kfunc_set);
> -	if (err)
> +	if (err) {
>  		pr_warn("error while registering bpf memcontrol kfuncs: %d", err);
> +		return err;
> +	}
> +
> +	err = register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL,
> +					&bpf_memcontrol_reclaim_kfunc_set);
> +	if (err)
> +		pr_warn("error registering bpf reclaim kfuncs: %d", err);

[Severity: Low]
Does this pr_warn() call need a trailing newline (\n) in the format string?

Without it, subsequent log messages might be incorrectly appended to the
same line in dmesg.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789091855.git.zhuhui@kylinos.cn?part=1