Message-ID: <6cdb0ba2-b340-4849-ac32-876e31beb87b@lucifer.local>
Date: Fri, 20 Jun 2025 06:08:14 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Zi Yan <ziy@...dia.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
        Dev Jain <dev.jain@....com>, Barry Song <baohua@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Lance Yang <ioworker0@...il.com>, SeongJae Park <sj@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [PATCH 1/5] mm/madvise: remove the visitor pattern and thread
 anon_vma state

On Thu, Jun 19, 2025 at 09:32:43PM -0400, Zi Yan wrote:
> On 19 Jun 2025, at 16:26, Lorenzo Stoakes wrote:
>
> > Now we have the madvise_behavior helper struct we no longer need to mess
> > around with void* pointers in order to propagate anon_vma_name, and this
> > means we can get rid of the confusing and inconsistent visitor pattern
> > implementation in madvise_vma_anon_name().
> >
> > This means we now have a single state object that threads through most of
> > madvise()'s logic and a single code path which executes the majority of
> > madvise() behaviour (we maintain separate logic for failure injection and
> > memory population for the time being).
> >
> > Note that users cannot inadvertently cause this behaviour to occur, as
> > madvise_behavior_valid() would reject it.
> >
> > Doing this results in a can_modify_vma_madv() check for anonymous VMA name
> > changes, however this will cause no issues as this operation is not
> > prohibited.
> >
> > We can also then reuse more code and drop the redundant
> > madvise_vma_anon_name() function altogether.
> >
> > Additionally separate out behaviours that update VMAs from those that do
> > not.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > ---
> >  mm/madvise.c | 167 +++++++++++++++++++++++++++------------------------
> >  1 file changed, 90 insertions(+), 77 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 070132f9842b..9dd935d64692 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -37,6 +37,9 @@
> >  #include "internal.h"
> >  #include "swap.h"
> >
> > +#define __MADV_SET_ANON_VMA_NAME (-1)
> > +#define __MADV_CLEAR_ANON_VMA_NAME (-2)
> > +
>
> These are stored in madvise_behavior.behavior field and used
> internally. At least you could add a comment in mman-common.h
> about this use, in case someone uses these values for MADV_*.
> Yes, these are large values that are very unlikely to be used,
> but who knows whether one will use them. :)

These are signed integers, so somebody would have to overflow >2.14bn, I don't
think this is really worth commenting there without adding confusion, and at any
rate, even if somebody did do that, madvise_behavior_valid() would catch it :)

>
> >  /*
> >   * Maximum number of attempts we make to install guard pages before we give up
> >   * and return -ERESTARTNOINTR to have userspace try again.
> > @@ -59,9 +62,13 @@ struct madvise_behavior {
> >  	int behavior;
> >  	struct mmu_gather *tlb;
> >  	enum madvise_lock_mode lock_mode;
> > +	struct anon_vma_name *anon_name;
> >  };
> >
> >  #ifdef CONFIG_ANON_VMA_NAME
> > +static int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> > +		unsigned long end, struct madvise_behavior *madv_behavior);
> > +
> >  struct anon_vma_name *anon_vma_name_alloc(const char *name)
> >  {
> >  	struct anon_vma_name *anon_name;
> > @@ -112,6 +119,48 @@ static int replace_anon_vma_name(struct vm_area_struct *vma,
> >
> >  	return 0;
> >  }
> > +
> > +int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > +			  unsigned long len_in, struct anon_vma_name *anon_name)
> > +{
> > +	unsigned long end;
> > +	unsigned long len;
> > +	struct madvise_behavior madv_behavior = {
> > +		.lock_mode = MADVISE_MMAP_WRITE_LOCK,
> > +		.anon_name = anon_name,
> > +	};
> > +
> > +	if (start & ~PAGE_MASK)
> > +		return -EINVAL;
> > +	len = (len_in + ~PAGE_MASK) & PAGE_MASK;
> > +
> > +	/* Check to see whether len was rounded up from small -ve to zero */
> > +	if (len_in && !len)
> > +		return -EINVAL;
> > +
> > +	end = start + len;
> > +	if (end < start)
> > +		return -EINVAL;
> > +
> > +	if (end == start)
> > +		return 0;
> > +
> > +	madv_behavior.behavior =
> > +		anon_name ? __MADV_SET_ANON_VMA_NAME : __MADV_CLEAR_ANON_VMA_NAME;
>
> How are __MADV_SET_ANON_VMA_NAME and __MADV_CLEAR_ANON_VMA_NAME used?
> It seems to me that madvise_vma_behavior() treats them the same.
> Why does anon_name is NULL or not make a difference?

Yeah you're right, I separated out because at one point I thought I needed to to
correctly handle the update vs. other updates but that's not actually the case,
will change.

>
> > +
> > +	return madvise_walk_vmas(mm, start, end, &madv_behavior);
> > +}
> > +
> > +static bool is_anon_vma_name(int behavior)
>
> Maybe update_anon_vma_name()? Otherwise the function reads like
> the behavior can be anon_vma_name.

Ack will change to is_set_anon_vma_name() or something like this to make
clearer.

>
> > +{
> > +	switch (behavior) {
> > +	case __MADV_SET_ANON_VMA_NAME:
> > +	case __MADV_CLEAR_ANON_VMA_NAME:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> >  #else /* CONFIG_ANON_VMA_NAME */
> >  static int replace_anon_vma_name(struct vm_area_struct *vma,
> >  				 struct anon_vma_name *anon_name)
> > @@ -121,6 +170,11 @@ static int replace_anon_vma_name(struct vm_area_struct *vma,
> >
> >  	return 0;
> >  }
> > +
> > +static bool is_anon_vma_name(int behavior)
> > +{
> > +	return false;
> > +}
> >  #endif /* CONFIG_ANON_VMA_NAME */
> >  /*
> >   * Update the vm_flags on region of a vma, splitting it or merging it as
> > @@ -1252,13 +1306,12 @@ static long madvise_guard_remove(struct vm_area_struct *vma,
> >  static int madvise_vma_behavior(struct vm_area_struct *vma,
> >  				struct vm_area_struct **prev,
> >  				unsigned long start, unsigned long end,
> > -				void *behavior_arg)
> > +				struct madvise_behavior *madv_behavior)
> >  {
> > -	struct madvise_behavior *arg = behavior_arg;
> > -	int behavior = arg->behavior;
> > -	int error;
> > -	struct anon_vma_name *anon_name;
> > +	int behavior = madv_behavior->behavior;
> > +	struct anon_vma_name *anon_name = madv_behavior->anon_name;
> >  	vm_flags_t new_flags = vma->vm_flags;
> > +	int error;
> >
> >  	if (unlikely(!can_modify_vma_madv(vma, behavior)))
> >  		return -EPERM;
> > @@ -1275,7 +1328,17 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> >  	case MADV_FREE:
> >  	case MADV_DONTNEED:
> >  	case MADV_DONTNEED_LOCKED:
> > -		return madvise_dontneed_free(vma, prev, start, end, arg);
> > +		return madvise_dontneed_free(vma, prev, start, end,
> > +					     madv_behavior);
> > +	case MADV_COLLAPSE:
> > +		return madvise_collapse(vma, prev, start, end);
> > +	case MADV_GUARD_INSTALL:
> > +		return madvise_guard_install(vma, prev, start, end);
> > +	case MADV_GUARD_REMOVE:
> > +		return madvise_guard_remove(vma, prev, start, end);
> > +
> > +	/* The below behaviours update VMAs via madvise_update_vma(). */
> > +
>
> Great comment and code move!

Thanks!

>
> >  	case MADV_NORMAL:
> >  		new_flags = new_flags & ~VM_RAND_READ & ~VM_SEQ_READ;
> >  		break;
> > @@ -1325,21 +1388,25 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> >  		if (error)
> >  			goto out;
> >  		break;
> > -	case MADV_COLLAPSE:
> > -		return madvise_collapse(vma, prev, start, end);
> > -	case MADV_GUARD_INSTALL:
> > -		return madvise_guard_install(vma, prev, start, end);
> > -	case MADV_GUARD_REMOVE:
> > -		return madvise_guard_remove(vma, prev, start, end);
> > +	case __MADV_SET_ANON_VMA_NAME:
> > +	case __MADV_CLEAR_ANON_VMA_NAME:
> > +		/* Only anonymous mappings can be named */
> > +		if (vma->vm_file && !vma_is_anon_shmem(vma))
> > +			return -EBADF;
> > +		break;
> >  	}
>
> __MADV_SET_ANON_VMA_NAME and __MADV_CLEAR_ANON_VMA_NAME are
> used here the code below. I do not see the functional difference
> of them. I understand a NULL anon_name means clear the name,
> but it is also just set the name to NULL.

Yeah you're right this was my mistake :) I had previously felt I should
differentiate as previously the code below was

if (madv_behavior->anon_name) {  ...

Which would have become:

if (behavior != __MADV_CLEAR_ANON_VMA_NAME && madv_behavior->anon_name) {  ...

But then realised we can just check the behavior flag and anyway avoid the
unnecessary get/put by doing this.

Will fixup.

>
> >
> >  	/* We cannot provide prev in this lock mode. */
> > -	VM_WARN_ON_ONCE(arg->lock_mode == MADVISE_VMA_READ_LOCK);
> > -	anon_name = anon_vma_name(vma);
> > -	anon_vma_name_get(anon_name);
> > +	VM_WARN_ON_ONCE(madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK);
> > +
> > +	if (!is_anon_vma_name(behavior)) {
> > +		anon_name = anon_vma_name(vma);
> > +		anon_vma_name_get(anon_name);
> > +	}
> >  	error = madvise_update_vma(vma, prev, start, end, new_flags,
> >  				   anon_name);
> > -	anon_vma_name_put(anon_name);
> > +	if (!is_anon_vma_name(behavior))
> > +		anon_vma_name_put(anon_name);
>
> Otherwise, the rest looks good to me.

Thanks!

>
> --
> Best Regards,
> Yan, Zi

Powered by blists - more mailing lists