linux-kernel - Re: [PATCH 1/5] mm: simplify vma merge structure and expand comments

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: <d457c298-0bc8-4f8a-a2e1-fc45610493de@lucifer.local>
Date: Tue, 28 Jan 2025 14:05:10 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Jann Horn <jannh@...gle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] mm: simplify vma merge structure and expand comments

On Tue, Jan 28, 2025 at 12:38:48PM +0100, Vlastimil Babka wrote:
> On 1/27/25 16:50, Lorenzo Stoakes wrote:
> > The merge code, while much improved, still has a number of points of
> > confusion. As part of a broader series cleaning this up to make this more
> > maintainable, we start by addressing some confusion around vma_merge_struct
> > fields.
> >
> > So far, the caller either provides no vmg->vma (a new VMA) or supplies the
> > existing VMA which is being altered, setting vmg->start,end,pgoff to the
> > proposed VMA dimensions.
> >
> > vmg->vma is then updated, as are vmg->start,end,pgoff as the merge process
> > proceeds and the appropriate merge strategy is determined.
> >
> > This is rather confusing, as vmg->vma starts off as the 'middle' VMA
> > between vmg->prev,next, but becomes the 'target' VMA, except in one
> > specific edge case (merge next, shrink middle).
> >
> > Int his patch we introduce vmg->middle to describe the VMA that is between
> > vmg->prev and vmg->next, and does NOT change during the merge operation.
> >
> > We replace vmg->vma with vmg->target, and use this only during the merge
> > operation itself.
>
> Yeah that's much better.

Yes, and part of a number of steps that gradually improve things (though
some of the incremental states are not quite beautiful, the final result is
good :)

>
> > Aside from the merge right, shrink middle case, this becomes the VMA that
> > forms the basis of the VMA that is returned. This edge case can be
> > addressed in a future commit.
> >
> > We also add a number of comments to explain what is going on.
> >
> > Finally, we adjust the ASCII diagrams showing each merge case in
> > vma_merge_existing_range() to be clearer - the arrow range previously
> > showed the vmg->start, end spanned area, but it is clearer to change this
> > to show the final merged VMA.
> >
> > This patch has no change in functional behaviour.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@...e.cz>

Thanks!

>
> > --- a/mm/vma.h
> > +++ b/mm/vma.h
> > @@ -69,16 +69,48 @@ enum vma_merge_flags {
> >  	VMG_FLAG_JUST_EXPAND = 1 << 0,
> >  };
> >
> > -/* Represents a VMA merge operation. */
> > +/*
> > + * Describes a VMA merge operation and is threaded throughout it.
> > + *
> > + * Any of the fields may be mutated by the merge operation, so no guarantees are
> > + * made to the contents of this structure after a merge operation has completed.
> > + */
>
> Well this patch seems like a step in the direction to limit what's mutated,
> and perhaps defining some of the guarantees (via const?) could be then possible?

Yeah, I was thinking about doing this, but perhaps as a follow-up. We'd
have to differentiate between:

const struct foo *bar;

and

struct foo * const bar;

To indicate in some cases we are fine with changing the pointer but not the
underlying struct and vice-versa.

>
> >  struct vma_merge_struct {
> >  	struct mm_struct *mm;
> >  	struct vma_iterator *vmi;
> > -	pgoff_t pgoff;
> > +	/*
> > +	 * Adjacent VMAs, any of which may be NULL if not present:
> > +	 *
> > +	 * |------|--------|------|
> > +	 * | prev | middle | next |
> > +	 * |------|--------|------|
> > +	 *
> > +	 * middle may not yet exist in the case of a proposed new VMA being
> > +	 * merged, or it may be an existing VMA.
> > +	 *
> > +	 * next may be assigned by the caller.
>
> Caller of what?

vma_merge_new_range() requires you to specify it, we document this there:

 * ASSUMPTIONS:
 ...
 * - The caller must have specified the next vma in @vmg->next.

In the case of vma_merge_existing_range() ,the caller should _not_ specify
next:

 * ASSUMPTIONS:
 * - The caller must not set @vmg->next, as we determine this.

Yes, this is insane, and I tried in the original series to avoid the need
for this stupid situation, but it ended up being more complicated than just
documenting and checking for this in the vma_merge_existing_range() case
(in the vma_merge_new_range() case we can't, since it may legitimately be
NULL if the proposed VMA is the last in the virtual address space).

Another one for a future fixup :)

>
>