Re: [PATCH v2 4/15] userfaultfd: introduce mfill_get_vma() and mfill_put_vma()

[Harry Yoo]

On Mon, Mar 16, 2026 at 01:35:38PM +0530, Deepanshu Kartikey wrote:
> On Mon, Mar 16, 2026 at 1:19 PM Harry Yoo <harry.yoo@xxxxxxxxxx> wrote:
> >
> > > It seems there's another attempt to fix the syzbot report from
> > > Deepanshu Kartikey [2], which I didn't take a deeper look.
> > >
> > > At first look [2] looks a bit wrong way to fix to me though,
> > > because it allows operating only on a single VMA nothing should really split
> > > or shrink the VMA if somebody is holding the VMA lock in read mode
> > > (and the validation of the range is done while holding the lock).
> > >
> > > [2] https://lore.kernel.org/linux-mm/20260316070039.549506-1-kartikey406@xxxxxxxxx
> > >
> 
> Harry,
> 
> You are correct that once vm_refcnt > 0, nobody can split the VMA.
> However the split can happen in the race window BEFORE vm_refcnt++
> in vma_start_read(), and CHECK 2 can miss this if mmap_write_unlock()
> completes before CHECK 2 runs.
> 
> Here is the exact race:
> 
> vma_start_read():
> 
>     /* CHECK 1 */
>     if (READ_ONCE(vma->vm_lock_seq) == READ_ONCE(mm->mm_lock_seq.sequence))
>         goto err;
> 
>     /*
>      * RACE WINDOW: vm_refcnt is still 0 here!
>      * UFFDIO_UNREGISTER can run:
>      *
>      *   mmap_write_lock()    -> mm_lock_seq = 11
>      *   vma_start_write(vma) -> vm_lock_seq = 11
>      *   __split_vma()        -> vma->vm_end = 0x4ca000
>      *   mmap_write_unlock()  -> mm_lock_seq = 12
>      *
>      * writer completes entirely before vm_refcnt++!
>      */
> 
>     __refcount_inc_not_zero_limited_acquire(&vma->vm_refcnt, ...);
>     /* vm_refcnt = 1 now, but vma->vm_end already modified! */

It is true that vma->vm_end might have changed before acquiring the vma lock,
but it doesn't matter as long as you verify the range after acquiring
the lock, no? (that's what uffd_mfill_lock() does)

You're not really supposed to read vma->vm_end before acquiring
the vma lock and use the value because nothing guarantees that
the VMA is stable until the lock is acquired.

Or am I still missing something?

>     /* CHECK 2 */
>     if (unlikely(vma->vm_lock_seq == raw_read_seqcount(&mm->mm_lock_seq)))
>     /*
>      * vm_lock_seq(11) == mm_lock_seq(12)?
>      * NO! writer already finished and unlocked!
>      * mm_lock_seq incremented to 12 (even=unlocked)
>      * CHECK 2 MISSES the race!
>      */
>     return vma;
>     /*
>      * returns split vma with vm_end=0x4ca000
>      * but vm_refcnt=1 (lock held)
>      */

-- 
Cheers,
Harry / Hyeonggon