Re: [Bug 217061] commit 2286a6914c776ec34cd97e4573b1466d055cb9de breaks do_mprotect_pkey

Re: [Bug 217061] commit 2286a6914c776ec34cd97e4573b1466d055cb9de breaks do_mprotect_pkey

[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?

Re: [Bug 217061] commit 2286a6914c776ec34cd97e4573b1466d055cb9de breaks do_mprotect_pkey

[Bert Karwatzki]

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.  

Re: [Bug 217061] commit 2286a6914c776ec34cd97e4573b1466d055cb9de breaks do_mprotect_pkey

[Liam R. Howlett]

* 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

Re: [Bug 217061] commit 2286a6914c776ec34cd97e4573b1466d055cb9de breaks do_mprotect_pkey

[Liam R. Howlett]

...

> 
> 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.

The patch has been sent out and I've submitted my testing to LTP:


https://github.com/linux-test-project/ltp/commit/3cbaaddf6f785d91aeb370beb75d0623f8c48624

The testcase is mprotect05

Thanks,
Liam