Re: [bug report] mm: replace vma->vm_flags direct modifications with modifier calls

[Suren Baghdasaryan <surenb@google.com>]

On Tue, Jul 11, 2023 at 3:21 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Jul 11, 2023 at 02:55:20PM -0700, Suren Baghdasaryan wrote:
> > On Tue, Jul 11, 2023 at 12:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > Hello Suren Baghdasaryan,
> > >
> > > The patch 1c71222e5f23: "mm: replace vma->vm_flags direct
> > > modifications with modifier calls" from Jan 26, 2023, leads to the
> > > following Smatch static checker warning:
> > >
> > >         ./include/linux/mm.h:729 vma_start_write()
> > >         warn: sleeping in atomic context
> > >
> > > include/linux/mm.h
> > >     722 static inline void vma_start_write(struct vm_area_struct *vma)
> > >     723 {
> > >     724         int mm_lock_seq;
> > >     725
> > >     726         if (__is_vma_write_locked(vma, &mm_lock_seq))
> > >     727                 return;
> > >     728
> > > --> 729         down_write(&vma->vm_lock->lock);
> > >     730         vma->vm_lock_seq = mm_lock_seq;
> > >     731         up_write(&vma->vm_lock->lock);
> > >     732 }
> > >
> > > The call tree is:
> > >
> > > gru_fault() <- disables preempt
> > > -> remap_pfn_range()
> > >    -> track_pfn_remap()
> > >    -> remap_pfn_range_notrack()
> > >       -> vm_flags_set()
> > >          -> vma_start_write()
> > >
> > > Before track_pfn_remap() and remap_pfn_range_notrack() would just do |=
> > > to set the flags but now they use vm_flags_set() so there is a potential
> > > they could sleep.
> >
> > Hi Dan,
> > Thanks for reporting! Looks like the page fault handler is modifying
> > the VMA flags, which has to be done under write-locked mmap_lock and I
> > don't see that being done here... I wonder if that should be allowed.
> > I'm CC'ing some MM folks to check if this is a valid VMA modification
> > and should be allowed. Matthew, this might be especially interesting
> > for you since gru_fault() handles file-backed page faults AFAIKT.
>
> I don't run the ->fault handler under RCU, only the ->map_pages()
> method.  I don't intend to change that.
>
> > Back to the issue at hand. If such modification should be indeed
> > allowed then the simplest fix I think would be to add new
> > remap_pfn_range_locked() function to be called from gru_fault() which
> > would use __vm_flags_mod() instead of vm_flags_set(). __vm_flags_mod()
> > does not lock the VMA, so would not have this issue. If the conclusion
> > is that this is a valid scenario then I can post a fix I described.
>
> I'm not certain, but calling remap_pfn_range() in the fault handler
> is definitely weird.  It's normally called _instead_ of having a fault
> handler.  The fault handler usually calls set_pte_at() directly.

Hmm. Is it weird enough to be considered invalid or weird but still ok?
Also, is it ok to modify VMA flags here without write-locking the
mmap_lock (and without write-locking the VMA)? The fault handler is
done under read-locked mmap_lock but I thought VMA modifications
require stronger locking...