Re: [PATCH] mm/vma: avoid spurious write locks when mm->mm_lock_seq overflows

[Lorenzo Stoakes (ARM)]

It seems likely that you're using AI to generate these review points. Please do
say if you're doing that, thanks.

Normally that'd be totally unwelcome, but both Sashiko + local AI missed these,
and I have made mistakes here I need to fix, so this is actually useful...

On Fri, Oct 16, 2026 at 09:41:28AM +0200, Jose A. Perez de Azpillaga wrote:
> On 10/15/26 10:31 PM, Lorenzo Stoakes (ARM) wrote:
> [snip]
>
> > The reset, performed in reset_vma_seqcounts(), only needs to WRITE_ONCE()
> > the value. The stores follow the smp_wmb() in do_raw_write_seqcount_end(),
> > so a reader observing a reset value also observes the VMA modifications
> > made under the write lock.
>
> the writer side looks fine, but what orders it on the reader side?
> raw_read_seqcount() is just an smp_load_acquire() since d0dd066a0fa2,
> not the smp_rmb() the comment mentions, so the earlier
> READ_ONCE(vma->vm_lock_seq) isn't ordered against the later vma reads.
> couldn't a reader on arm64 see the reset 0 with the old odd
> mm_lock_seq, pass the parity check and read stale vma fields? would
> smp_load_acquire(&vma->vm_lock_seq) be enough?

Yup that's a mistake, will fix on respin.

>
> [snip]
>
> > Cc: stable@xxxxxxxxxxxxxxx
>
> the parity check relies on e5e7fb278e59. before that, mm_lock_seq only
> increments on unlock, so locked values can be odd or even. should this
> be limited to 6.14+, with a note that older trees need a different fix?
> a straight backport of the odd check would ignore write locks taken on
> even cycles.

I have a plan for older kernels (just make the value 64 bit...)

>
> [snip]
>
> > +	/*
> > +	 * The VMA seqcount is always odd if locked. An even value can only be
> > +	 * the zero stored by reset_vma_seqcounts(), never a write lock.
> > +	 */
>
> vma_lock_init() now stores 0 too, so this isn't quite true, is it?

Yes it is true, because as the commit message and I think even comments say, the
VMA is not visible when detached, but it is visible when attached, at which
point a write lock will have been acquired by definition, hence non-zero.

I guess I could add a point about attached here.

>
> [snip]
>
> > -static inline struct vm_area_struct *vma_start_read(struct mm_struct *mm,
> > -						    struct vm_area_struct *vma)
> > +static struct vm_area_struct *vma_start_read(struct mm_struct *mm,
> > +					     struct vm_area_struct *vma)
>
> is dropping the inline intended?

Yes. inline in C files is useless.

>
> [snip]
>
> > +EXPORT_SYMBOL(reset_vma_seqcounts);
>
> since it has to be exported for the inline mmap_write_unlock(), would a
> __ prefix make sense, so nobody calls it outside vma_end_write_all()?

Yup, that's fair, will update.

>
> I tested it on x86 in qemu with KASAN, lockdep and DEBUG_VM on top of
> mm-unstable and saw no issues, but I'm hesitant about the points above.
> happy to give a Tested-by on the next version :)

Thanks!

--
Cheers, Lorenzo