Re: [Bug 217061] commit 2286a6914c776ec34cd97e4573b1466d055cb9de breaks do_mprotect_pkey

["Liam R. Howlett" <Liam.Howlett@Oracle.com>]

* Bert Karwatzki <spasswolf@web.de> [230223 15:36]:
> Am Donnerstag, dem 23.02.2023 um 12:04 -0800 schrieb Andrew Morton:
> > (switched to email.  Please respond via emailed reply-to-all, not via
> > the
> > bugzilla web interface).
> > 
> > On Wed, 22 Feb 2023 18:27:37 +0000 bugzilla-daemon@kernel.org wrote:
> > 
> > > https://bugzilla.kernel.org/show_bug.cgi?id=217061
> > > 
> > > --- Comment #5 from spasswolf@web.de ---
> > > This fixes the problem for me in next-20230220
> > > 
> > > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > > index 1d4843c97c2a..f70f9a7b545e 100644
> > > --- a/mm/mprotect.c
> > > +++ b/mm/mprotect.c
> > > @@ -832,6 +832,7 @@ static int do_mprotect_pkey(unsigned long
> > > start, size_t
> > > len,
> > >                 if (error)
> > >                         break;
> > > 
> > > +               tmp = prev->vm_end;
> > >                 nstart = tmp;
> > >                 prot = reqprot;
> > >         }
> > > 
> > > Next I'll try to construct a simpler testcase without steam and
> > > stellaris.
> > 
> > Thanks for figuring this out.
> > 
> > Liam, this has been bisected to 2286a6914c776e ("mm: change
> > mprotect_fixup to vma iterator").  Please take a look?
> > 
> While a still don't have a simple example for this bug I can at least
> explain what it takes to trigger it:
> The memory area for which mprotect is called must have (at least) 3
> vm_area_struct of which the first to must be mergeable. Then the
> following happens in the for_each_vma_range loop inside
> do_mprotect_pkey:
> At the beggining of the loop vma points to the first vm_area_struct.
> Then mprotect_fixup is called and merges our first two vm_area_structs
> but tmp still points to the beginning of the second vm_area_struct. At
> the beginning of the next iteration vma already points to the third
> vm_area_struct and so the check vma->vm_start != tmp gives a false
> error.
>   Setting tmp=prev->vm_end after mprotect fixup sets tmp to the
> beginning of the next unprocessed vma and fixes this error.  
> 

Thank you for the analysis.

Just for clarity, tmp is used to ensure there isn't a hole in the range
we are iterating over.  I don't particularly like how this was done, but
I tried (unsuccessfully) not to break it.

It looks like I've missed the case where a merge succeeds and so tmp is
now out of sync with the iterator.

I would change this tmp = prev->vm_end idea slightly and use the vma
iterator end location:

tmp = vma_iter_end(&vmi);

I'll send out a patch once I test this.

Thanks,
Liam