linux-kernel - Re: [PATCH v4 23/66] mm/mmap: Use advanced maple tree API for mmap

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 21 Jan 2022 18:11:44 +0000
From:   Liam Howlett <liam.howlett@...cle.com>
To:     Vlastimil Babka <vbabka@...e.cz>
CC:     "maple-tree@...ts.infradead.org" <maple-tree@...ts.infradead.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Song Liu <songliubraving@...com>,
        Davidlohr Bueso <dave@...olabs.net>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Laurent Dufour <ldufour@...ux.ibm.com>,
        David Rientjes <rientjes@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Rik van Riel <riel@...riel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Michel Lespinasse <walken.cr@...il.com>,
        Jerome Glisse <jglisse@...hat.com>,
        Minchan Kim <minchan@...gle.com>,
        Joel Fernandes <joelaf@...gle.com>,
        Rom Lemarchand <romlem@...gle.com>
Subject: Re: [PATCH v4 23/66] mm/mmap: Use advanced maple tree API for
 mmap_region()

* Vlastimil Babka <vbabka@...e.cz> [220117 11:39]:
> On 12/1/21 15:30, Liam Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
> > 
> > Changing mmap_region() to use the maple tree state and the advanced
> > maple tree interface allows for a lot less tree walking.
> > 
> > This change removes the last caller of munmap_vma_range(), so drop this
> > unused function.
> > 
> > Add vma_expand() to expand a VMA if possible by doing the necessary
> > hugepage check, uprobe_munmap of files, dcache flush, modifications then
> > undoing the detaches, etc.
> > 
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> > ---
> >  mm/mmap.c | 227 +++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 175 insertions(+), 52 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index c06c5b850e1e..b0b7e327bf8b 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -496,29 +496,6 @@ static inline struct vm_area_struct *__vma_next(struct mm_struct *mm,
> >  	return vma->vm_next;
> >  }
> >  
> > -/*
> > - * munmap_vma_range() - munmap VMAs that overlap a range.
> > - * @mm: The mm struct
> > - * @start: The start of the range.
> > - * @len: The length of the range.
> > - * @pprev: pointer to the pointer that will be set to previous vm_area_struct
> > - *
> > - * Find all the vm_area_struct that overlap from @start to
> > - * @end and munmap them.  Set @pprev to the previous vm_area_struct.
> > - *
> > - * Returns: -ENOMEM on munmap failure or 0 on success.
> > - */
> > -static inline int
> > -munmap_vma_range(struct mm_struct *mm, unsigned long start, unsigned long len,
> > -		 struct vm_area_struct **pprev, struct list_head *uf)
> > -{
> > -	// Needs optimization.
> > -	while (range_has_overlap(mm, start, start + len, pprev))
> > -		if (do_munmap(mm, start, len, uf))
> > -			return -ENOMEM;
> > -	return 0;
> > -}
> > -
> >  static unsigned long count_vma_pages_range(struct mm_struct *mm,
> >  		unsigned long addr, unsigned long end)
> >  {
> > @@ -619,6 +596,101 @@ static void __insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
> >  	mm->map_count++;
> >  }
> >  
> > +/*
> > + * vma_expand - Expand an existing VMA
> > + * @mas: The maple state
> > + * @vma: The vma to expand
> > + * @start: The start of the vma
> > + * @end: The exclusive end of the vma
> > + */
> 
> Looks like this, similarly to the brk case, replaces one path calling
> vma_merge->__vma_adjust() with something else. But this one is better
> encapsulated and visible, so less likely to be forgotten in case something
> changes. Would be even better if the brk case used it too :) seems like it
> doesn't, at least as of this patch.
> 
> But it would be great to improve the documentation here - some params are
> not documented, notably 'next', and I would explicitly state which scenarios
> it does cover - i.e. vma_merge() lists 8 scenarios and I assume this can
> handlea subset of those?

Yes, I will add the missing parameters to the documentation.  I will
also add nodes about how this handles expanding vma and may merge next,
but does not handle other scenarios.

> And scenarios not covered could be VM_WARN_ON'd?
> Without such stated assumptions, it's hard/impossible to review both the
> implementation against them and, and the callers against them.

Okay, I'll add some VM_WARN_ON's too.

> 
> > +inline int vma_expand(struct ma_state *mas, struct vm_area_struct *vma,
> > +		      unsigned long start, unsigned long end, pgoff_t pgoff,
> > +		      struct vm_area_struct *next)
> > +{
> > +	struct mm_struct *mm = vma->vm_mm;
> > +	struct address_space *mapping = NULL;
> > +	struct rb_root_cached *root = NULL;
> > +	struct anon_vma *anon_vma = vma->anon_vma;
> > +	struct file *file = vma->vm_file;
> > +	bool remove_next = false;
> > +	int error;
> > +
> > +	if (next && (vma != next) && (end == next->vm_end)) {
> 
> For example here this suggests that a case of 'end > next->vm_end' case is
> not covered? How do I know whether it's intended or a bug?
> 

Okay, thanks.  I will have a look to see how many I can come up with.