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

[Peter Xu]

On Thu, Jul 11, 2024 at 11:51:41PM +0200, David Hildenbrand wrote:
> 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.

Ah yes, I had one rfc patch for that, I temporarily put that aside as it
seemed nobody cared except myself.. it's here:

https://lore.kernel.org/all/20240523223745.395337-2-peterx@xxxxxxxxxx

I didn't know it can already cause real trouble.  It looks like that patch
should fix this.

Thanks,

-- 
Peter Xu