linux-kernel - Re: [PATCH v2 06/44] mm/mmap: convert brk to use vma iterator

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: <20230109163759.6dwusx6ellqumh4a@revolver>
Date:   Mon, 9 Jan 2023 16:38:10 +0000
From:   Liam Howlett <liam.howlett@...cle.com>
To:     Vernon Yang <vernon2gm@...il.com>
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>
Subject: Re: [PATCH v2 06/44] mm/mmap: convert brk to use vma iterator

* Vernon Yang <vernon2gm@...il.com> [230109 10:19]:
> Hello Liam,
> 
> On Thu, Jan 05, 2023 at 07:15:54PM +0000, Liam Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
> >
> > Use the vma iterator API for the brk() system call.  This will provide
> > type safety at compile time.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> > ---
> >  mm/mmap.c | 47 +++++++++++++++++++++++------------------------
> >  1 file changed, 23 insertions(+), 24 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 9318f2ac8a6e..4a6f42ab3560 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c

...

> > @@ -2998,7 +2998,7 @@ static int do_brk_munmap(struct ma_state *mas, struct vm_area_struct *vma,
> >   * do not match then create a new anonymous VMA.  Eventually we may be able to
> >   * do some brk-specific accounting here.
> >   */
> > -static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> > +static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  		unsigned long addr, unsigned long len, unsigned long flags)
> >  {
> >  	struct mm_struct *mm = current->mm;
> > @@ -3025,8 +3025,7 @@ static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> >  	if (vma && vma->vm_end == addr && !vma_policy(vma) &&
> >  	    can_vma_merge_after(vma, flags, NULL, NULL,
> >  				addr >> PAGE_SHIFT, NULL_VM_UFFD_CTX, NULL)) {
> > -		mas_set_range(mas, vma->vm_start, addr + len - 1);
> 
> Why was mas_set_range() removed here, but below [1] it’s left?

Set range does two things: 1. it sets the range, and 2. it resets the
maple state so that the store will occur in the correct location.  This
is so that you do not use an invalid maple state for a store.

We don't need to move the maple state in this case since we are already
pointing at the vma we are merging with.

Furthermore, the API for the vma iterator below "vma_iter_prealloc()"
also ensures that the state is okay.

> 
> > -		if (mas_preallocate(mas, vma, GFP_KERNEL))
> > +		if (vma_iter_prealloc(vmi, vma))
> >  			goto unacct_fail;
> >
> >  		vma_adjust_trans_huge(vma, vma->vm_start, addr + len, 0);
> > @@ -3036,7 +3035,7 @@ static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> >  		}
> >  		vma->vm_end = addr + len;
> >  		vma->vm_flags |= VM_SOFTDIRTY;
> > -		mas_store_prealloc(mas, vma);
> > +		vma_iter_store(vmi, vma);
> >
> >  		if (vma->anon_vma) {
> >  			anon_vma_interval_tree_post_update_vma(vma);
> > @@ -3057,8 +3056,8 @@ static int do_brk_flags(struct ma_state *mas, struct vm_area_struct *vma,
> >  	vma->vm_pgoff = addr >> PAGE_SHIFT;
> >  	vma->vm_flags = flags;
> >  	vma->vm_page_prot = vm_get_page_prot(flags);
> > -	mas_set_range(mas, vma->vm_start, addr + len - 1);
> > -	if (mas_store_gfp(mas, vma, GFP_KERNEL))
> > +	mas_set_range(&vmi->mas, vma->vm_start, vma->vm_end - 1);
> 
> [1]. the mas_set_range() here has been reserved.

The "vma_iter_store_gfp()" API call does not check the state and the
state could be invalid.

> 
> > +	if (vma_iter_store_gfp(vmi, vma, GFP_KERNEL))
> >  		goto mas_store_fail;
> >
> >  	mm->map_count++;
...

This situation happened because I added the sanity check to the state
later in the development cycle.

I will fix this inconsistency and remove the mas_set_range().  Thanks
for catching this.