linux-kernel - Re: [PATCH 04/10] mm: abstract parameters for vma

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] [day] [month] [year] [list]

Message-ID: <d1c80a35-0a96-4dd3-9e91-ce4f062d90d8@lucifer.local>
Date: Wed, 14 Aug 2024 14:53:47 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: "Vlastimil Babka (SUSE)" <vbabka@...nel.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>
Subject: Re: [PATCH 04/10] mm: abstract parameters for vma_expand/shrink()

On Thu, Aug 08, 2024 at 04:45:53PM GMT, Lorenzo Stoakes wrote:
> On Thu, Aug 08, 2024 at 04:20:26PM GMT, Vlastimil Babka (SUSE) wrote:
> > On 8/5/24 14:13, Lorenzo Stoakes wrote:
> > > Equally use struct vma_merge_struct to abstract parameters for VMA
> > > expansion and shrinking.
> > >
> > > This leads the way to further refactoring and de-duplication by
> > > standardising the interface.
> > >
> > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > ---
> > >  mm/mmap.c               | 30 +++++++++++--------
> > >  mm/vma.c                | 66 ++++++++++++++++++-----------------------
> > >  mm/vma.h                |  8 ++---
> > >  tools/testing/vma/vma.c | 18 +++++++++--
> > >  4 files changed, 65 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 721ced6e37b0..04145347c245 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -1367,7 +1367,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > >  	pgoff_t pglen = len >> PAGE_SHIFT;
> > >  	unsigned long charged = 0;
> > >  	unsigned long end = addr + len;
> > > -	unsigned long merge_start = addr, merge_end = end;
> > >  	bool writable_file_mapping = false;
> > >  	int error;
> > >  	VMA_ITERATOR(vmi, mm, addr);
> > > @@ -1423,28 +1422,26 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > >  	/* Attempt to expand an old mapping */
> > >  	/* Check next */
> > >  	if (next && next->vm_start == end && can_vma_merge_before(&vmg)) {
> > > -		merge_end = next->vm_end;
> > > -		vma = next;
> > > +		/* We can adjust this as can_vma_merge_after() doesn't touch */
> > > +		vmg.end = next->vm_end;
> >
> > Ugh, ok but wonder how fragile that is.
>
> Yeah you're right this is a bit horrid, I'll find a way to make this less
> brittle.

FYI for when I send out the v2 respin:

Actually, as I work through it now, I think this is OK as-is (I'll remove
the comment as it's confusing though).

The next block checks prev, so the end of the VMA doesn't really matter,
and in any case isn't checked by can_vma_merge_after(), but rather by the
prev->vm_end == addr conditional below.

I've addressed your other comments.

[snip]