| 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
| ||
|
Message-ID: <76e112da-a8b6-4004-9bce-2dcc29dea7fc@suse.cz>
Date: Fri, 23 Jan 2026 11:02:04 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: David Hildenbrand <david@...nel.org>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>, Mike Rapoport
<rppt@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>,
Michal Hocko <mhocko@...e.com>, Shakeel Butt <shakeel.butt@...ux.dev>,
Jann Horn <jannh@...gle.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Will Deacon <will@...nel.org>, Boqun Feng <boqun.feng@...il.com>,
Waiman Long <longman@...hat.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Clark Williams <clrkwllms@...nel.org>, Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH RESEND v3 07/10] mm/vma: introduce helper struct + thread
through exclusive lock fns
On 1/22/26 14:01, Lorenzo Stoakes wrote:
> It is confusing to have __vma_enter_exclusive_locked() return 0, 1 or an
> error (but only when waiting for readers in TASK_KILLABLE state), and
> having the return value be stored in a stack variable called 'locked' is
> further confusion.
>
> More generally, we are doing a lock of rather finnicky things during the
> acquisition of a state in which readers are excluded and moving out of this
> state, including tracking whether we are detached or not or whether an
> error occurred.
>
> We are implementing logic in __vma_enter_exclusive_locked() that
> effectively acts as if 'if one caller calls us do X, if another then do Y',
> which is very confusing from a control flow perspective.
>
> Introducing the shared helper object state helps us avoid this, as we can
> now handle the 'an error arose but we're detached' condition correctly in
> both callers - a warning if not detaching, and treating the situation as if
> no error arose in the case of a VMA detaching.
>
> This also acts to help document what's going on and allows us to add some
> more logical debug asserts.
>
> Also update vma_mark_detached() to add a guard clause for the likely
> 'already detached' state (given we hold the mmap write lock), and add a
> comment about ephemeral VMA read lock reference count increments to clarify
> why we are entering/exiting an exclusive locked state here.
>
> No functional change intended.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> ---
> mm/mmap_lock.c | 144 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 91 insertions(+), 53 deletions(-)
>
> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> index f73221174a8b..75166a43ffa4 100644
> --- a/mm/mmap_lock.c
> +++ b/mm/mmap_lock.c
> @@ -46,20 +46,40 @@ EXPORT_SYMBOL(__mmap_lock_do_trace_released);
> #ifdef CONFIG_MMU
> #ifdef CONFIG_PER_VMA_LOCK
>
> +/* State shared across __vma_[enter, exit]_exclusive_locked(). */
> +struct vma_exclude_readers_state {
> + /* Input parameters. */
> + struct vm_area_struct *vma;
> + int state; /* TASK_KILLABLE or TASK_UNINTERRUPTIBLE. */
> + bool detaching;
> +
> + bool detached;
> + bool exclusive; /* Are we exclusively locked? */
> +};
> +
> /*
> * Now that all readers have been evicted, mark the VMA as being out of the
> * 'exclude readers' state.
> *
> * Returns true if the VMA is now detached, otherwise false.
> */
> -static bool __must_check __vma_exit_exclusive_locked(struct vm_area_struct *vma)
> +static void __vma_exit_exclusive_locked(struct vma_exclude_readers_state *ves)
> {
> - bool detached;
> + struct vm_area_struct *vma = ves->vma;
> +
> + VM_WARN_ON_ONCE(ves->detached);
> + VM_WARN_ON_ONCE(!ves->exclusive);
I think this will triger when called on wait failure from
__vma_enter_exclusive_locked(). Given the other things Suren raised about
the field, I wonder if it's worth keeping it?
> - detached = refcount_sub_and_test(VM_REFCNT_EXCLUDE_READERS_FLAG,
> - &vma->vm_refcnt);
> + ves->detached = refcount_sub_and_test(VM_REFCNT_EXCLUDE_READERS_FLAG,
> + &vma->vm_refcnt);
> __vma_lockdep_release_exclusive(vma);
> - return detached;
> +}
> +
> @@ -151,7 +176,12 @@ EXPORT_SYMBOL_GPL(__vma_start_write);
>
> void vma_mark_detached(struct vm_area_struct *vma)
> {
> - bool detached;
> + struct vma_exclude_readers_state ves = {
> + .vma = vma,
> + .state = TASK_UNINTERRUPTIBLE,
> + .detaching = true,
> + };
> + int err;
>
> vma_assert_write_locked(vma);
> vma_assert_attached(vma);
> @@ -160,18 +190,26 @@ void vma_mark_detached(struct vm_area_struct *vma)
> * See the comment describing the vm_area_struct->vm_refcnt field for
> * details of possible refcnt values.
> */
> - detached = __vma_refcount_put(vma, NULL);
> - if (unlikely(!detached)) {
> - /* Wait until vma is detached with no readers. */
> - if (__vma_enter_exclusive_locked(vma, true, TASK_UNINTERRUPTIBLE)) {
> - /*
> - * Once this is complete, no readers can increment the
> - * reference count, and the VMA is marked detached.
> - */
> - detached = __vma_exit_exclusive_locked(vma);
> - WARN_ON_ONCE(!detached);
> - }
> + if (likely(__vma_refcount_put(vma, NULL)))
> + return;
Seems to me it would be worthwhile splitting this function to an
static-inline-in-header vma_mark_detached() that does only the asserts and
__vma_refcount_put(), and keeping the function here as __vma_mark_detached()
(or maybe differently named since the detaching kinda already happened with
the refcount put... __vma_mark_detached_finish()?) handling the rare case
__vma_refcount_put() returns false.
> +
> + /*
> + * Wait until the VMA is detached with no readers. Since we hold the VMA
> + * write lock, the only read locks that might be present are those from
> + * threads trying to acquire the read lock and incrementing the
> + * reference count before realising the write lock is held and
> + * decrementing it.
> + */
> + err = __vma_enter_exclusive_locked(&ves);
> + if (!err && !ves.detached) {
> + /*
> + * Once this is complete, no readers can increment the
> + * reference count, and the VMA is marked detached.
> + */
> + __vma_exit_exclusive_locked(&ves);
> }
> + /* If an error arose but we were detached anyway, we don't care. */
> + WARN_ON_ONCE(!ves.detached);
> }
>
> /*
> --
> 2.52.0
Powered by blists - more mailing lists