On Mon, Oct 28, 2024 at 11:07:16AM -0400, Liam R. Howlett wrote:
>* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [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@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.
>>
>> >
>> > 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;
>
>
>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
