linux-kernel - Re: [PATCH 08/10] mm: introduce commit_merge(), abstracting merge operation

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: <415d9d9c-7b63-47f0-9091-678f0d8d1268@lucifer.local>
Date: Tue, 6 Aug 2024 14:48:33 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Petr Tesařík <petr@...arici.cz>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH 08/10] mm: introduce commit_merge(), abstracting merge
 operation

On Tue, Aug 06, 2024 at 03:41:16PM GMT, Petr Tesařík wrote:
> On Mon,  5 Aug 2024 13:13:55 +0100
> Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
>
> > Pull this operation into its own function and have vma_expand() call
> > commit_merge() instead.
> >
> > This lays the groundwork for a subsequent patch which replaces vma_merge()
> > with a simpler function which can share the same code.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > ---
> >  mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 45 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index a404cf718f9e..b7e3c64d5d68 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm)
> >  }
> >  #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */
> >
> > +/* Actually perform the VMA merge operation. */
> > +static int commit_merge(struct vma_merge_struct *vmg,
> > +			struct vm_area_struct *adjust,
> > +			struct vm_area_struct *remove,
> > +			struct vm_area_struct *remove2,
> > +			long adj_start,
> > +			bool expanded)
> > +{
> > +	struct vma_prepare vp;
> > +
> > +	init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2);
> > +
> > +	if (expanded) {
> > +		vma_iter_config(vmg->vmi, vmg->start, vmg->end);
> > +	} else {
> > +		vma_iter_config(vmg->vmi, adjust->vm_start + adj_start,
> > +				adjust->vm_end);
> > +	}
>
> It's hard to follow the logic if you the "expanded" parameter is always
> true. I have to look at PATCH 09/10 first to see how it is expected to
> be used. Is there no other way?
>
> Note that this is not needed for adjust and adj_start, because they are
> merely moved here from vma_expand() and passed down as parameters to
> other functions.

See the next patch to understand how these are used, as the commit message
says, this lays the groundwork for the next patch which actually uses both
of these.

I have tried hard to clarify how these are used, however there is some
unavoidable and inherent complexity in this logic. If you don't believe me,
I suggest trying to follow the logic of the existing code :)

And if you want to _really_ have fun, I suggest you try to understand the
logic around v6.0 prior to Liam's interventions.

We might be able to try to improve the logic flow further, but it's one
step at a time with this.

>
> Petr T
>
> > +
> > +	if (vma_iter_prealloc(vmg->vmi, vmg->vma))
> > +		return -ENOMEM;
> > +
> > +	vma_prepare(&vp);
> > +	vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start);
> > +	vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff);
> > +
> > +	if (expanded)
> > +		vma_iter_store(vmg->vmi, vmg->vma);
> > +
> > +	if (adj_start) {
> > +		adjust->vm_start += adj_start;
> > +		adjust->vm_pgoff += PHYS_PFN(adj_start);
> > +		if (adj_start < 0) {
> > +			WARN_ON(expanded);
> > +			vma_iter_store(vmg->vmi, adjust);
> > +		}
> > +	}
> > +
> > +	vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm);
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * vma_merge_new_vma - Attempt to merge a new VMA into address space
> >   *
> > @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg)
> >  	bool remove_next = false;
> >  	struct vm_area_struct *vma = vmg->vma;
> >  	struct vm_area_struct *next = vmg->next;
> > -	struct vma_prepare vp;
> >
> >  	vma_start_write(vma);
> >  	if (next && (vma != next) && (vmg->end == next->vm_end)) {
> > @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg)
> >  			return ret;
> >  	}
> >
> > -	init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL);
> >  	/* Not merging but overwriting any part of next is not handled. */
> > -	VM_WARN_ON(next && !vp.remove &&
> > +	VM_WARN_ON(next && !remove_next &&
> >  		  next != vma && vmg->end > next->vm_start);
> >  	/* Only handles expanding */
> >  	VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end);
> >
> > -	/* Note: vma iterator must be pointing to 'start' */
> > -	vma_iter_config(vmg->vmi, vmg->start, vmg->end);
> > -	if (vma_iter_prealloc(vmg->vmi, vma))
> > +	if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true))
> >  		goto nomem;
> >
> > -	vma_prepare(&vp);
> > -	vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0);
> > -	vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff);
> > -	vma_iter_store(vmg->vmi, vma);
> > -
> > -	vma_complete(&vp, vmg->vmi, vma->vm_mm);
> >  	return 0;
> >
> >  nomem:
>