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

Harry Yoo
2026-03-16T08:36:46Z

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@oracle.com> 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@gmail.com
> > >
>
> 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