linux-kernel - Re: [PATCH v8 11/16] mm: replace vm_lock and detached flag with a reference count

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJuCfpH8eTU3OaWAPh+ndr9MCRxb7OYeYqbyPxWqXw+HxFpgZg@mail.gmail.com>
Date: Fri, 10 Jan 2025 14:37:31 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: akpm@...ux-foundation.org, peterz@...radead.org, willy@...radead.org, 
	liam.howlett@...cle.com, lorenzo.stoakes@...cle.com, mhocko@...e.com, 
	hannes@...xchg.org, mjguzik@...il.com, oliver.sang@...el.com, 
	mgorman@...hsingularity.net, david@...hat.com, peterx@...hat.com, 
	oleg@...hat.com, dave@...olabs.net, paulmck@...nel.org, brauner@...nel.org, 
	dhowells@...hat.com, hdanton@...a.com, hughd@...gle.com, 
	lokeshgidra@...gle.com, minchan@...gle.com, jannh@...gle.com, 
	shakeel.butt@...ux.dev, souravpanda@...gle.com, pasha.tatashin@...een.com, 
	klarasmodin@...il.com, richard.weiyang@...il.com, corbet@....net, 
	linux-doc@...r.kernel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	kernel-team@...roid.com
Subject: Re: [PATCH v8 11/16] mm: replace vm_lock and detached flag with a
 reference count

On Fri, Jan 10, 2025 at 2:26 PM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> On 1/10/25 16:56, Suren Baghdasaryan wrote:
> >> > --- a/mm/memory.c
> >> > +++ b/mm/memory.c
> >> > @@ -6370,9 +6370,41 @@ struct vm_area_struct *lock_mm_and_find_vma(struct mm_struct *mm,
> >> >  #endif
> >> >
> >> >  #ifdef CONFIG_PER_VMA_LOCK
> >> > +static inline bool __vma_enter_locked(struct vm_area_struct *vma, unsigned int tgt_refcnt)
> >> > +{
> >> > +     /*
> >> > +      * If vma is detached then only vma_mark_attached() can raise the
> >> > +      * vm_refcnt. mmap_write_lock prevents racing with vma_mark_attached().
> >> > +      */
> >> > +     if (!refcount_add_not_zero(VMA_LOCK_OFFSET, &vma->vm_refcnt))
> >> > +             return false;
> >> > +
> >> > +     rwsem_acquire(&vma->vmlock_dep_map, 0, 0, _RET_IP_);
> >> > +     rcuwait_wait_event(&vma->vm_mm->vma_writer_wait,
> >> > +                refcount_read(&vma->vm_refcnt) == tgt_refcnt,
> >> > +                TASK_UNINTERRUPTIBLE);
> >> > +     lock_acquired(&vma->vmlock_dep_map, _RET_IP_);
> >> > +
> >> > +     return true;
> >> > +}
> >> > +
> >> > +static inline void __vma_exit_locked(struct vm_area_struct *vma, bool *detached)
> >> > +{
> >> > +     *detached = refcount_sub_and_test(VMA_LOCK_OFFSET, &vma->vm_refcnt);
> >> > +     rwsem_release(&vma->vmlock_dep_map, _RET_IP_);
> >> > +}
> >> > +
> >> >  void __vma_start_write(struct vm_area_struct *vma, unsigned int mm_lock_seq)
> >> >  {
> >> > -     down_write(&vma->vm_lock.lock);
> >> > +     bool locked;
> >> > +
> >> > +     /*
> >> > +      * __vma_enter_locked() returns false immediately if the vma is not
> >> > +      * attached, otherwise it waits until refcnt is (VMA_LOCK_OFFSET + 1)
> >> > +      * indicating that vma is attached with no readers.
> >> > +      */
> >> > +     locked = __vma_enter_locked(vma, VMA_LOCK_OFFSET + 1);
> >>
> >> Wonder if it would be slightly better if tgt_refcount was just 1 (or 0
> >> below in vma_mark_detached()) and the VMA_LOCK_OFFSET added to it in
> >> __vma_enter_locked() itself as it's the one adding it in the first place.
> >
> > Well, it won't be called tgt_refcount then. Maybe "bool vma_attached"
> > and inside __vma_enter_locked() we do:
> >
> > unsigned int tgt_refcnt = VMA_LOCK_OFFSET + vma_attached ? 1 : 0;
> >
> > Is that better?
>
> Yeah I think so as it centralizes the target refcount logic into a single
> place __vma_enter_locked().
> Hm but then it's weird that __vma_start_write() would set vma_attached to
> true and yet it handles also a case where it's not attached.

Ah, good point.

> Maybe call the parameter "detaching" and switch the 0 and 1?

Yes, that would be less confusing. Thanks for the suggestion, I'll use it.