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

[David Hildenbrand]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=35a4414f6e247f515443
> > Tested-by: syzbot+35a4414f6e247f515443@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Pei Li <peili.dev@xxxxxxxxx>
> > ---
> > 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.

If everything fails, we could drop the assert, but I'm hoping we can 
avoid that. We really want most users of follow_pte() to do the right thing.

--
Cheers,

David / dhildenb