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

[David Hildenbrand]

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@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.

... 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