Re: [PATCH] mm: Fix mmap_assert_locked() in follow_pte()

David Hildenbrand
2024-07-11T21:51:48Z

On 11.07.24 23:45, David Hildenbrand wrote:
> On 11.07.24 23:33, David Hildenbrand wrote:
>> On 11.07.24 07:13, Pei Li wrote:
>>> This patch fixes this warning by acquiring read lock before entering
>>> untrack_pfn() while write lock is not held.
>>>
>>> syzbot has tested the proposed patch and the reproducer did not
>>> trigger any issue.
>>>
>>> Reported-by: syzbot+35a4414f6e247f515443@syzkaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=35a4414f6e247f515443
>>> Tested-by: syzbot+35a4414f6e247f515443@syzkaller.appspotmail.com
>>> Signed-off-by: Pei Li <peili.dev@gmail.com>
>>> ---
>>> Syzbot reported the following warning in follow_pte():
>>>
>>> WARNING: CPU: 3 PID: 5192 at include/linux/rwsem.h:195 rwsem_assert_held include/linux/rwsem.h:195 [inline]
>>> WARNING: CPU: 3 PID: 5192 at include/linux/rwsem.h:195 mmap_assert_locked include/linux/mmap_lock.h:65 [inline]
>>> WARNING: CPU: 3 PID: 5192 at include/linux/rwsem.h:195 follow_pte+0x414/0x4c0 mm/memory.c:5980
>>>
>>> This is because we are assuming that mm->mmap_lock should be held when
>>> entering follow_pte(). This is added in commit c5541ba378e3 (mm:
>>> follow_pte() improvements).
>>>
>>> However, in the following call stack, we are not acquring the lock:
>>> follow_phys arch/x86/mm/pat/memtype.c:957 [inline]
>>> get_pat_info+0xf2/0x510 arch/x86/mm/pat/memtype.c:991
>>> untrack_pfn+0xf7/0x4d0 arch/x86/mm/pat/memtype.c:1104
>>> unmap_single_vma+0x1bd/0x2b0 mm/memory.c:1819
>>> zap_page_range_single+0x326/0x560 mm/memory.c:1920
>>
>> That implies that unmap_vmas() is called without the mmap lock in read
>> mode, correct?
>>
>> Do we know how this happens?
>>
>> * exit_mmap() holds the mmap lock in read mode
>> * unmap_region is documented to hold the mmap lock in read mode
>
> I think this is it (missed the call from zap_page_range_single()):
>
> follow_phys arch/x86/mm/pat/memtype.c:957 [inline]
> get_pat_info+0xf2/0x510 arch/x86/mm/pat/memtype.c:991
> untrack_pfn+0xf7/0x4d0 arch/x86/mm/pat/memtype.c:1104
> unmap_single_vma+0x1bd/0x2b0 mm/memory.c:1819
> zap_page_range_single+0x326/0x560 mm/memory.c:1920
> unmap_mapping_range_vma mm/memory.c:3684 [inline]
> unmap_mapping_range_tree mm/memory.c:3701 [inline]
> unmap_mapping_pages mm/memory.c:3767 [inline]
> unmap_mapping_range+0x1ee/0x280 mm/memory.c:3804
> truncate_pagecache+0x53/0x90 mm/truncate.c:731
> simple_setattr+0xf2/0x120 fs/libfs.c:886
> notify_change+0xec6/0x11f0 fs/attr.c:499
> do_truncate+0x15c/0x220 fs/open.c:65
> handle_truncate fs/namei.c:3308 [inline]
>
> I think Peter recently questioned whether untrack_pfn() should be even
> called from the place, but I might misremember things.
>
> Fix should work (I suspect we are not violating some locking rules?),
> PFNMAP should not happen there too often that we really care.

... thinking again, likely we reach this point with "!mm_wr_locked" and
the mmap lock already held in read mode. So I suspect the fix won't work
as is.

--
Cheers,

David / dhildenb