[PATCH] mm/mlock: set the correct prev on failure

[PATCH] mm/mlock: set the correct prev on failure

[Wei Yang]

After commit 94d7d9233951 ("mm: abstract the vma_merge()/split_vma()
pattern for mprotect() et al."), if vma_modify_flags() return error, the
vma is set to an error code. This will lead to an invalid prev be
returned.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/mlock.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/mlock.c b/mm/mlock.c
index e3e3dc2b2956..8c3f9cf8f960 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -478,11 +478,12 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
 		/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
 		goto out;
 
-	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
-	if (IS_ERR(vma)) {
-		ret = PTR_ERR(vma);
+	*prev = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
+	if (IS_ERR(*prev)) {
+		ret = PTR_ERR(*prev);
 		goto out;
 	}
+	vma = *prev;
 
 	/*
 	 * Keep track of amount of locked VM.
-- 
2.34.1

Re: [PATCH] mm/mlock: set the correct prev on failure

[Lorenzo Stoakes]

+ Vlastimil, Liam, Jann as this is VMA-related.

We really need to bring all VMA-ish files under the VMA MAINTAINERS
block... will maybe address that once things around that file... calm down
a bit.

But please cc all of us on anything that even vaguely relates to VMAs,
thanks!

On Sun, Oct 27, 2024 at 02:56:29AM +0000, Wei Yang wrote:
> After commit 94d7d9233951 ("mm: abstract the vma_merge()/split_vma()
> pattern for mprotect() et al."), if vma_modify_flags() return error, the
> vma is set to an error code. This will lead to an invalid prev be
> returned.

This is a great spot, but this commit message is missing critical
details. This is only meaningful for apply_mlockall_flags() which is both
ignoring errors AND assuming mlock_fixup(), even on error, is correctly
updating the prev state. Which is imo wrong.

So I'd _add_ a bit more information here like:

Generally this shouldn't matter as the caller should treat an error as
indicating state is now invalidated, however unfortunately
apply_mlockall_flags() does not check for errors and assumes that
mlock_fixup() correctly maintains prev even if an error were to occur.

This patch fixes that assumption.

We'll also need to backport this, so a:

Fixes: 94d7d9233951 ("mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.")
Cc: <stable@vger.kernel.org>

Needs to be added, and make the next revision [PATCH hotfix 6.12 v2] to
make it clear this needs to go to 6.12.

>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/mlock.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mlock.c b/mm/mlock.c
> index e3e3dc2b2956..8c3f9cf8f960 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -478,11 +478,12 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  		/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
>  		goto out;
>
> -	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> -	if (IS_ERR(vma)) {
> -		ret = PTR_ERR(vma);
> +	*prev = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> +	if (IS_ERR(*prev)) {
> +		ret = PTR_ERR(*prev);
>  		goto out;
>  	}
> +	vma = *prev;

Yeah sorry I hate this, it was icky before and it's kinda disgusting to
assign *prev then *prev to vma, then, if success, vma to *prev - yeah
that's super confusing :)

I mean a better alternative if you were to do this approach would be to
have a new vma local but I don't actually think that's the correct
approach.

Really the caller _must_ deal with errors, and not assume any state is
valid after an error occurs.

So I think the fix should be in apply_mlockall_flags() instead like:

	...

	for_each_vma(vmi, vma) {
		...
		int error;

		...

		error = mlock_fixup(&vmi, vma, &prev, vma->vm_start, vma->vm_end,
				    newflags);
		/* Ignore errors, but prev needs fixing up. */
		if (error)
			prev = vma;

		...
	}

This is also a smaller delta for backporting.


>
>  	/*
>  	 * Keep track of amount of locked VM.
> --
> 2.34.1
>
>

I'm happy for you to resubmit like this and take full credit by the way! :)
assuming you agree with this approach.

This is also reminding me that I need to refactor all this crap, the whole
passing prev around and looping like that is horrible. Also the outer loop
should be maintaining prev, not the inner one.

This is going on my TODO list!

Re: [PATCH] mm/mlock: set the correct prev on failure

[Wei Yang]

On Sun, Oct 27, 2024 at 11:41:13AM +0000, Lorenzo Stoakes wrote:
>+ Vlastimil, Liam, Jann as this is VMA-related.
>
>We really need to bring all VMA-ish files under the VMA MAINTAINERS
>block... will maybe address that once things around that file... calm down
>a bit.
>
>But please cc all of us on anything that even vaguely relates to VMAs,
>thanks!
>

Sure, will add them in later change.

>On Sun, Oct 27, 2024 at 02:56:29AM +0000, Wei Yang wrote:
>> After commit 94d7d9233951 ("mm: abstract the vma_merge()/split_vma()
>> pattern for mprotect() et al."), if vma_modify_flags() return error, the
>> vma is set to an error code. This will lead to an invalid prev be
>> returned.
>
>This is a great spot, but this commit message is missing critical
>details. This is only meaningful for apply_mlockall_flags() which is both
>ignoring errors AND assuming mlock_fixup(), even on error, is correctly
>updating the prev state. Which is imo wrong.
>

Yes.

>So I'd _add_ a bit more information here like:
>
>Generally this shouldn't matter as the caller should treat an error as
>indicating state is now invalidated, however unfortunately
>apply_mlockall_flags() does not check for errors and assumes that
>mlock_fixup() correctly maintains prev even if an error were to occur.
>
>This patch fixes that assumption.
>
>We'll also need to backport this, so a:
>
>Fixes: 94d7d9233951 ("mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.")
>Cc: <stable@vger.kernel.org>
>
>Needs to be added, and make the next revision [PATCH hotfix 6.12 v2] to
>make it clear this needs to go to 6.12.
>

Will follow this.

>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> ---
>>  mm/mlock.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index e3e3dc2b2956..8c3f9cf8f960 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -478,11 +478,12 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>>  		/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
>>  		goto out;
>>
>> -	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
>> -	if (IS_ERR(vma)) {
>> -		ret = PTR_ERR(vma);
>> +	*prev = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
>> +	if (IS_ERR(*prev)) {
>> +		ret = PTR_ERR(*prev);
>>  		goto out;
>>  	}
>> +	vma = *prev;
>
>Yeah sorry I hate this, it was icky before and it's kinda disgusting to
>assign *prev then *prev to vma, then, if success, vma to *prev - yeah
>that's super confusing :)
>
>I mean a better alternative if you were to do this approach would be to
>have a new vma local but I don't actually think that's the correct
>approach.
>
>Really the caller _must_ deal with errors, and not assume any state is
>valid after an error occurs.
>
>So I think the fix should be in apply_mlockall_flags() instead like:
>
>	...
>
>	for_each_vma(vmi, vma) {
>		...
>		int error;
>
>		...
>
>		error = mlock_fixup(&vmi, vma, &prev, vma->vm_start, vma->vm_end,
>				    newflags);
>		/* Ignore errors, but prev needs fixing up. */
>		if (error)
>			prev = vma;
>
>		...
>	}
>
>This is also a smaller delta for backporting.
>

I have to say this one look better.

Thanks

>
>>
>>  	/*
>>  	 * Keep track of amount of locked VM.
>> --
>> 2.34.1
>>
>>
>
>I'm happy for you to resubmit like this and take full credit by the way! :)
>assuming you agree with this approach.
>
>This is also reminding me that I need to refactor all this crap, the whole
>passing prev around and looping like that is horrible. Also the outer loop
>should be maintaining prev, not the inner one.
>
>This is going on my TODO list!

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/mlock: set the correct prev on failure

[Wei Yang]

On Sun, Oct 27, 2024 at 11:41:13AM +0000, Lorenzo Stoakes wrote:
[...]
>>
>>
>
>I'm happy for you to resubmit like this and take full credit by the way! :)
>assuming you agree with this approach.
>
>This is also reminding me that I need to refactor all this crap, the whole
>passing prev around and looping like that is horrible. Also the outer loop
>should be maintaining prev, not the inner one.
>

Looping like for_each_vma()/for_each_vma_range(). Any other stuff?

If you would like to name others, I will pay attention later.

>This is going on my TODO list!

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm/mlock: set the correct prev on failure

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [241027 07:43]:
> + Vlastimil, Liam, Jann as this is VMA-related.
> 
> We really need to bring all VMA-ish files under the VMA MAINTAINERS
> block... will maybe address that once things around that file... calm down
> a bit.
> 
> But please cc all of us on anything that even vaguely relates to VMAs,
> thanks!
> 
> On Sun, Oct 27, 2024 at 02:56:29AM +0000, Wei Yang wrote:
> > After commit 94d7d9233951 ("mm: abstract the vma_merge()/split_vma()
> > pattern for mprotect() et al."), if vma_modify_flags() return error, the
> > vma is set to an error code. This will lead to an invalid prev be
> > returned.
> 
> This is a great spot, but this commit message is missing critical
> details. This is only meaningful for apply_mlockall_flags() which is both
> ignoring errors AND assuming mlock_fixup(), even on error, is correctly
> updating the prev state. Which is imo wrong.
> 
> So I'd _add_ a bit more information here like:
> 
> Generally this shouldn't matter as the caller should treat an error as
> indicating state is now invalidated, however unfortunately
> apply_mlockall_flags() does not check for errors and assumes that
> mlock_fixup() correctly maintains prev even if an error were to occur.
> 
> This patch fixes that assumption.
> 
> We'll also need to backport this, so a:
> 
> Fixes: 94d7d9233951 ("mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.")
> Cc: <stable@vger.kernel.org>
> 
> Needs to be added, and make the next revision [PATCH hotfix 6.12 v2] to
> make it clear this needs to go to 6.12.
> 
> >
> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  mm/mlock.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/mlock.c b/mm/mlock.c
> > index e3e3dc2b2956..8c3f9cf8f960 100644
> > --- a/mm/mlock.c
> > +++ b/mm/mlock.c
> > @@ -478,11 +478,12 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  		/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
> >  		goto out;
> >
> > -	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> > -	if (IS_ERR(vma)) {
> > -		ret = PTR_ERR(vma);
> > +	*prev = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
> > +	if (IS_ERR(*prev)) {
> > +		ret = PTR_ERR(*prev);
> >  		goto out;
> >  	}
> > +	vma = *prev;


If we move the assignment *prev = vma to the start of this function,
then we can just get rid of the "out:" label and return on errors.

But the v2 seems fine,
Liam

Re: [PATCH] mm/mlock: set the correct prev on failure

[Wei Yang]

On Mon, Oct 28, 2024 at 11:07:16AM -0400, Liam R. Howlett wrote:
>* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [241027 07:43]:
>> + Vlastimil, Liam, Jann as this is VMA-related.
>> 
>> We really need to bring all VMA-ish files under the VMA MAINTAINERS
>> block... will maybe address that once things around that file... calm down
>> a bit.
>> 
>> But please cc all of us on anything that even vaguely relates to VMAs,
>> thanks!
>> 
>> On Sun, Oct 27, 2024 at 02:56:29AM +0000, Wei Yang wrote:
>> > After commit 94d7d9233951 ("mm: abstract the vma_merge()/split_vma()
>> > pattern for mprotect() et al."), if vma_modify_flags() return error, the
>> > vma is set to an error code. This will lead to an invalid prev be
>> > returned.
>> 
>> This is a great spot, but this commit message is missing critical
>> details. This is only meaningful for apply_mlockall_flags() which is both
>> ignoring errors AND assuming mlock_fixup(), even on error, is correctly
>> updating the prev state. Which is imo wrong.
>> 
>> So I'd _add_ a bit more information here like:
>> 
>> Generally this shouldn't matter as the caller should treat an error as
>> indicating state is now invalidated, however unfortunately
>> apply_mlockall_flags() does not check for errors and assumes that
>> mlock_fixup() correctly maintains prev even if an error were to occur.
>> 
>> This patch fixes that assumption.
>> 
>> We'll also need to backport this, so a:
>> 
>> Fixes: 94d7d9233951 ("mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.")
>> Cc: <stable@vger.kernel.org>
>> 
>> Needs to be added, and make the next revision [PATCH hotfix 6.12 v2] to
>> make it clear this needs to go to 6.12.
>> 
>> >
>> > Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> > ---
>> >  mm/mlock.c | 7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/mm/mlock.c b/mm/mlock.c
>> > index e3e3dc2b2956..8c3f9cf8f960 100644
>> > --- a/mm/mlock.c
>> > +++ b/mm/mlock.c
>> > @@ -478,11 +478,12 @@ static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
>> >  		/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
>> >  		goto out;
>> >
>> > -	vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
>> > -	if (IS_ERR(vma)) {
>> > -		ret = PTR_ERR(vma);
>> > +	*prev = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
>> > +	if (IS_ERR(*prev)) {
>> > +		ret = PTR_ERR(*prev);
>> >  		goto out;
>> >  	}
>> > +	vma = *prev;
>
>
>If we move the assignment *prev = vma to the start of this function,
>then we can just get rid of the "out:" label and return on errors.
>

Maybe not, if my understanding is correct.

  vma = vma_modify_flags(, vma, ...)
      vma_modify()
          merged = vma_merge_existing_range()
	  return merged

For example, if we merge left, the returned one would be the prev instead of
vma we passed in. Even we may release the original vma. 

>But the v2 seems fine,
>Liam

-- 
Wei Yang
Help you, Help me