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@xxxxxxxxxxxxxxx>
>
>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@xxxxxxxxx>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
>> ---
>> 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
