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

[Suren Baghdasaryan <surenb@google.com>]

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.

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.
Thanks,
Suren.

>
> regards,
> dan carpenter