Re: [PATCH bpf-next 1/4] mm/bpf: Add bpf_try_to_free_mem_cgroup_pages kfunc

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Recursive invocation of try_to_free_mem_cgroup_pages() via bpf_try_to_free_mem_cgroup_pages() clobbers current->reclaim_state, leading to a kernel crash or corrupted slab reclaim accounting.
--

--- Patch [1]: [PATCH bpf-next 1/4] mm/bpf: Add bpf_try_to_free_mem_cgroup_pages kfunc ---
commit df416a25d32f502ac37a07ea5346f80fe7a52345
Author: Hui Zhu <[email protected]>

mm/bpf: Add bpf_try_to_free_mem_cgroup_pages kfunc

This commit exposes the memory cgroup reclaim interface to BPF programs by
adding the bpf_try_to_free_mem_cgroup_pages kfunc. The wrapper translates
the swappiness argument before delegating to the core reclaim path.

> diff --git a/mm/bpf_memcontrol.c b/mm/bpf_memcontrol.c
> index 716df49d76477..3f7a5c97e1355 100644
> --- a/mm/bpf_memcontrol.c
> +++ b/mm/bpf_memcontrol.c
[ ... ]
> @@ -159,6 +160,61 @@ __bpf_kfunc void bpf_mem_cgroup_flush_stats(struct mem_cgroup *memcg)
>  	mem_cgroup_flush_stats(memcg);
>  }
>  
> +/**
> + * bpf_try_to_free_mem_cgroup_pages - attempt to reclaim pages from
[ ... ]
> + */
> +__bpf_kfunc unsigned long
> +bpf_try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
> +				 unsigned long nr_pages,
> +				 gfp_t gfp_mask,
> +				 unsigned int reclaim_options,
> +				 int swappiness)
> +{

[Severity: High]
If this kfunc is called from a sleepable BPF program that was triggered during
memory reclaim (e.g., via an fentry hook on a function like shrink_slab), could
it corrupt the reclaim state and crash the kernel?

Looking at the underlying try_to_free_mem_cgroup_pages() in mm/vmscan.c, it
unconditionally overwrites current->reclaim_state and sets it to NULL upon
return:

    set_task_reclaim_state(current, &sc.reclaim_state);
    ...
    nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
    ...
    set_task_reclaim_state(current, NULL);

If the outer reclaim path resumes and attempts to dereference
current->reclaim_state (such as in set_mm_walk() or clear_mm_walk() in MGLRU),
wouldn't it hit a fatal NULL pointer dereference?

Should there be a mechanism here to prevent nested reclaim calls, or should the
underlying function save and restore the previous reclaim state?

> +	int *swapiness_ptr;
> +
> +	if (swappiness > SWAPPINESS_ANON_ONLY)
> +		return 0;
> +	else if (swappiness < MIN_SWAPPINESS)
> +		swapiness_ptr = NULL;
> +	else
> +		swapiness_ptr = &swappiness;
> +
> +	return try_to_free_mem_cgroup_pages(memcg, nr_pages, gfp_mask,
> +					    reclaim_options, swapiness_ptr);
> +}
> +
>  __bpf_kfunc_end_defs();
[ ... ]

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