linux-kernel - Re: [PATCH 1/5] mm/madvise: remove the visitor pattern and thread anon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b3d1976-16f7-423a-98a2-45a5c550b5e4@lucifer.local>
Date: Fri, 20 Jun 2025 14:17:58 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>, Zi Yan <ziy@...dia.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>,
        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 Fri, Jun 20, 2025 at 03:05:02PM +0200, Vlastimil Babka wrote:
> On 6/19/25 22: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.
>
> This paragraph is a bit confusing. I've inferred from the code you're
> talking about the new internal negative values, but the preceding paragraphs
> don't mention them. Could you explain in more detail what the patch does?
> I.e. adding the new struct madvise_behavior field and the new behavior value(s).

Sure will update on respin.

>
> > 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>
>
> > @@ -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;
> >  	}
> >
> >  	/* 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);
>
> This is not new, but the refactoring made it very visible that we're doing
> get/put on anon_name exactly in cases where we're not messing with anon_name
> so it might look buggy. Some explanatory comment would be thus nice,
> otherwise people need to git blame for commit 942341dcc5748.

Yeah I was confused myself until you mentioned that commit and - of course -
it's because of merge :P which maybe I should have figured out right away but
there we are :>)

So for my own sake as well as others I will add on respin.

>
> Otherwise LGTM, will wait with tag for v2 as you replied elsewhere there
> will be changes. Thanks!
>

Thanks!