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

[Deepanshu Kartikey]

On Mon, Mar 16, 2026 at 2:06 PM Harry Yoo <harry.yoo@xxxxxxxxxx> wrote:
>
> 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?
>

Harry, you are right. The real bug is state.len = 0.
I withdraw my fix.
Thank you for the explanation