On Tue, Jul 11, 2023 at 12:21 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
