Re: [PATCH v6 mm-new 02/10] mm: thp: add a new kfunc bpf_mm_get_mem_cgroup()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Aug 27, 2025 at 01:50:18PM -0700, Shakeel Butt wrote:
> On Wed, Aug 27, 2025 at 04:34:48PM +0100, Lorenzo Stoakes wrote:
> > > +__bpf_kfunc_start_defs();
> > > +
> > > +/**
> > > + * bpf_mm_get_mem_cgroup - Get the memory cgroup associated with a mm_struct.
> > > + * @mm: The mm_struct to query
> > > + *
> > > + * The obtained mem_cgroup must be released by calling bpf_put_mem_cgroup().
> > > + *
> > > + * Return: The associated mem_cgroup on success, or NULL on failure. Note that
> > > + * this function depends on CONFIG_MEMCG being enabled - it will always return
> > > + * NULL if CONFIG_MEMCG is not configured.
> >
> > What kind of locking is assumed here?
> >
> > Are we protected against mmdrop() clearing out the mm?
>
> No locking is needed. Just the valid mm object or NULL. Usually the
> underlying function (get_mem_cgroup_from_mm) is called in page fault
> context where the current is holding mm. Here the only requirement is
> that mm is valid either through explicit reference or the context.

I mean this may be down to me being not so familiar with BPF, but my concern is
that we're handing _any_ mm here.

So presumably this could also be a remote mm?

If not then why are we accepting an mm parameter at all, when we could just grab
current->mm?

If it's a remote mm, then we need to be absolutely sure that we won't UAF.

I also feel we should talk about this in the kdoc, unless BPF always somehow
asserts these things to be the case + verifies them smoehow.

>
> >
> > > + */
> > > +__bpf_kfunc struct mem_cgroup *bpf_mm_get_mem_cgroup(struct mm_struct *mm)
> > > +{
> > > +	return get_mem_cgroup_from_mm(mm);
> > > +}
> > > +
> > > +/**
> > > + * bpf_put_mem_cgroup - Release a memory cgroup obtained from bpf_mm_get_mem_cgroup()
> > > + * @memcg: The memory cgroup to release
> > > + */
> > > +__bpf_kfunc void bpf_put_mem_cgroup(struct mem_cgroup *memcg)
> > > +{
> > > +#ifdef CONFIG_MEMCG
> > > +	if (!memcg)
> > > +		return;
> > > +	css_put(&memcg->css);
> >
> > Feels weird to have an ifdef here but not elsewhere, maybe the whole thing
> > should be ifdef...?
> >
> > Is there not a put equivalent for get_mem_cgroup_from_mm()? That is a bit weird.
> >
> > Also do we now refrence the memcg global? That's pretty gross, could we not
> > actually implement such a helper?
> >
> > Is it valid to do this also? Maybe cgroup people can chime in.
>
> There is mem_cgroup_put() which should handle !CONFIG_MEMCG configs.
>

OK Yafang - let's use this instead then?