Re: [RFC PATCH v3 bpf-next 4/5] sched: add BPF-based per-cgroup NUMA balancing mode selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] mm: use-after-free on source folio in migrate_misplaced_folio()
--

commit 5de3ed8c82d3030544f5f025b8ae47dd8339bfeb
Author: Yafang Shao <laoar.shao@gmail.com>

sched: add BPF-based per-cgroup NUMA balancing mode selection

This commit introduces per-cgroup NUMA balancing mode selection managed via
the bpf_numab_set_mode() and bpf_numab_get_mode() kfuncs. Hot paths combine
requests along the task or folio's memcg cgroup hierarchy behind the
numab_cgroup_mode_set static key to override the global sysctl setting.

> diff --git a/mm/migrate.c b/mm/migrate.c
> index 211c29bd2a283..c0692c7b27467 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c

[ ... ]

> @@ -2770,7 +2770,7 @@ int migrate_misplaced_folio(struct folio *folio, int node)
>  	if (nr_succeeded) {
>  		count_vm_numa_events(NUMA_PAGE_MIGRATE, nr_succeeded);
>  		count_memcg_events(memcg, NUMA_PAGE_MIGRATE, nr_succeeded);
> -		if ((numab_in_tiering_mode())
> +		if (folio_numab_in_tiering_mode(folio)
>  		    && !node_is_toptier(folio_nid(folio))
>  		    && node_is_toptier(node))
>  			mod_lruvec_state(lruvec, PGPROMOTE_SUCCESS, nr_succeeded);

[Severity: Critical]
Has the source folio already been uncharged and freed by the time
migrate_misplaced_folio() calls folio_numab_in_tiering_mode(folio) here?

Before calling migrate_pages(), migrate_misplaced_folio() snapshots the
folio's memcg because migration transfers the charge and releases the
isolation reference on the source folio:

migrate_misplaced_folio()
  memcg = get_mem_cgroup_from_folio(folio);
  migrate_pages()
    migrate_pages_batch()
      migrate_folios_move()
        migrate_folio_move()
          move_to_new_folio()
            folio_migrate_flags()
              mem_cgroup_migrate()
                old->memcg_data = 0;
          migrate_folio_done()
            folio_put(src);

When nr_succeeded is non-zero, calling folio_numab_in_tiering_mode(folio) ->
folio_numab_mode(folio) -> folio_memcg(folio) -> folio_objcg(folio) reads
folio->memcg_data on the freed source folio:

static inline struct obj_cgroup *folio_objcg(const struct folio *folio)
{
	unsigned long memcg_data = folio->memcg_data;

	VM_BUG_ON_FOLIO(folio_test_slab(folio), folio);
	VM_BUG_ON_FOLIO(memcg_data & MEMCG_DATA_OBJEXTS, folio);

	return (struct obj_cgroup *)(memcg_data & ~OBJEXTS_FLAGS_MASK);
}

If the freed page has not been reallocated yet, folio->memcg_data is 0, so
folio_memcg(folio) returns NULL and folio_numab_mode(folio) falls back to
NUMAB_FOLLOW_SYSTEM rather than using the owner cgroup's tiering mode for
PGPROMOTE_SUCCESS accounting.

If another CPU reallocates the freed page before this check runs (for example
as a slab page setting PG_slab and MEMCG_DATA_OBJEXTS, or as a compound tail
page), can folio_objcg(folio) trigger VM_BUG_ON_FOLIO() or dereference a
slabobj_ext pointer as a struct obj_cgroup *?

Would it make sense to evaluate folio_numab_in_tiering_mode(folio) before
migrate_pages(), or derive the mode from the already-pinned memcg pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019064157.29885-1-laoar.shao@gmail.com?part=4