* [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
@ 2026-10-25 10:43 David Hildenbrand (Arm)
2026-10-25 14:55 ` Suren Baghdasaryan
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: David Hildenbrand (Arm) @ 2026-10-25 10:43 UTC (permalink / raw)
To: Andrew Morton, Liam R. Howlett, Lorenzo Stoakes, Vlastimil Babka,
Jann Horn, Pedro Falcato, Suren Baghdasaryan, Shakeel Butt
Cc: linux-kernel, linux-mm, stable, David Hildenbrand (Arm)
Reading /proc/PID/maps under per-VMA locks can skip continuously mapped
address ranges during concurrent VMA splits or merges. This violates the
guarantee in Documentation/filesystems/proc.rst:
"If there is something at a given vaddr during the entirety of the
life of the smaps/maps walk, there will be some output for it."
lock_next_vma() reuses a tree iterator across calls under RCU. The iterator
remembers the range of its last entry. When its node is replaced,
mas_next_slot() walks the tree again at the last index of that range, then
advances past the entry containing that index. If VMA boundaries changed,
this can skip a range that has not been reported:
- A lookup finds [A, C), then mprotect() splits it into [A, B) and [B, C)
before vma_start_read() locks the first VMA. The reader reports [A, B),
but the iterator still covers [A, C). The next call starts from B yet
advances past C, omitting [B, C).
- The reader reports [A, B) and unlocks it. A concurrent mprotect() then
merges it with [B, C). On the next call, the iterator resumes at B - 1,
which now belongs to [A, C), and advances past C. Again, [B, C) is lost.
Checking the mmap_lock sequence only within the next call misses writers
that completed before that call began. Also, a NULL result is currently
accepted without validation, so a race can terminate the walk early.
Even a fresh lookup can skip a mapping. Suppose it finds a gap before
[B, C). A writer inserts an adjacent VMA at the end of that gap, then
merges it into [B, C), replacing the node. The lookup resumes at the old
gap's last index, now inside the merged VMA, and steps past it. The
original [B, C) remains mapped throughout. Repeating the forward lookup
is insufficient: another merge can make it skip the same mapping again,
returning the same later VMA or NULL.
The fresh-lookup race also affects /proc/PID/smaps, numa_maps and
smaps_rollup. Resetting the iterator after gathering each VMA's statistics
does not prevent a merge during the next lookup. Fixing lock_next_vma()
also prevents omitted entries and incomplete rollup statistics.
Keep a saved sequence in the proc maps locking context and validate it
before each lookup with mmap_lock_speculate_retry(). If it is even (no
pending writer) and still matches, reuse both the iterator and the saved
sequence. Otherwise, restart at from_addr and sample the current sequence
before looking up the VMA. Only refresh the sample when resetting the
iterator, so a writer between validation and the lookup cannot go
undetected.
The smp_rmb() in mmap_lock_speculate_retry() orders the previous lookup
before rereading the sequence. It pairs with smp_wmb() in
do_raw_write_seqcount_begin(), which separates the sequence increment in
mmap_write_lock() from subsequent tree updates.
Validate detected gaps (from_addr < vma->vm_start) and NULL from
vma_next() against the sequence. If there were concurrent writes, use
mas_walk() to check if the writer could have interfered with the range
of interest. If the range is not empty, something interfered concurrently
so we retry under mmap_lock. Checking for empty ranges avoids taking the
mmap_lock just because an unrelated writer changed the sequence.
Original fix generated by an LLM and sent by Josef Bacik in private.
Fixes: 5631da56c9a8 ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock")
Fixes: ea3085dd7a4e ("fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock")
Fixes: 0e985559d72a ("proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock")
Cc: stable@vger.kernel.org
Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
---
I neither understood nor liked the original LLM generated fix that was
provided to me, and I (and codex) think it was incomplete. So I tried to
come up with a better fix that avoids additional fallbacks to the mmap_lock
while trying to keep it simple.
I won't claim that I am now an expert on the code, or that this is the
most elegant fix. It is quite complicated what we built ... too
complicated. So if this goes into the wrong direction I'll be happy
if someone else wants to take over ;)
I reproduced the races with /proc/PID/maps, and with this patch they are
resolved. I did not manage to reproduce the races with the new
smaps/smaps_rollup, but the races are slightly different and harder to
trigger.
There is the issue with seqcount overflows that Lorenzo is tackling. It
probably interacts here as well? Likely, but that's an independent issue
to resolve.
---
fs/proc/internal.h | 1 +
fs/proc/task_mmu.c | 6 +++--
include/linux/mmap_lock.h | 5 ++--
mm/mmap_lock.c | 58 +++++++++++++++++++++++++++++++++++------------
4 files changed, 51 insertions(+), 19 deletions(-)
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index df873df9afc56..f3e846b574d79 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -386,6 +386,7 @@ struct mem_size_stats;
struct proc_maps_locking_ctx {
struct mm_struct *mm;
bool mmap_locked;
+ unsigned int mm_wr_seq;
struct vm_area_struct *locked_vma;
};
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index ee9fc2fd2b2fe..dc3adcffb6d9c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -148,6 +148,7 @@ static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
{
lock_ctx->locked_vma = NULL;
lock_ctx->mmap_locked = false;
+ lock_ctx->mm_wr_seq = UINT_MAX;
}
static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
@@ -160,7 +161,8 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
return vma_next(&priv->iter);
unlock_ctx_vma(lock_ctx);
- vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos);
+ vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos,
+ &lock_ctx->mm_wr_seq);
if (!IS_ERR_OR_NULL(vma))
lock_ctx->locked_vma = vma;
@@ -523,7 +525,7 @@ static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ct
unlock_ctx_vma(lock_ctx);
rcu_read_lock();
vma_iter_init(&vmi, mm, addr);
- vma = lock_next_vma(mm, &vmi, addr);
+ vma = lock_next_vma(mm, &vmi, addr, &lock_ctx->mm_wr_seq);
rcu_read_unlock();
if (!vma)
diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 28e3696ce9ff3..e33d15ce6a876 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -511,12 +511,13 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
* Locks next vma pointed by the iterator. Confirms the locked vma has not
* been modified and will retry under mmap_lock protection if modification
* was detected. Should be called from read RCU section.
+ * *last_seq must initially be odd and is kept across calls.
* Returns either a valid locked VMA, NULL if no more VMAs or -EINTR if the
* process was interrupted.
*/
struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
- struct vma_iterator *iter,
- unsigned long address);
+ struct vma_iterator *vmi, unsigned long from_addr,
+ unsigned int *last_seq);
static inline void vma_assert_can_modify(struct vm_area_struct *vma)
{
diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
index 2f94ee0fdee2a..0f3a4e0c5520d 100644
--- a/mm/mmap_lock.c
+++ b/mm/mmap_lock.c
@@ -400,21 +400,50 @@ static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm
return vma;
}
+static bool vma_range_is_empty(struct mm_struct *mm, unsigned long start,
+ unsigned long last)
+{
+ MA_STATE(mas, &mm->mm_mt, start, start);
+
+ return !mas_walk(&mas) && mas.last >= last;
+}
+
struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
- struct vma_iterator *vmi,
- unsigned long from_addr)
+ struct vma_iterator *vmi, unsigned long from_addr,
+ unsigned int *last_seq)
{
struct vm_area_struct *vma;
- unsigned int mm_wr_seq;
bool mmap_unlocked;
RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu read lock held");
retry:
- /* Start mmap_lock speculation in case we need to verify the vma later */
- mmap_unlocked = mmap_lock_speculate_try_begin(mm, &mm_wr_seq);
+ /*
+ * Restart by address if a writer could have changed the cached range.
+ * The retry check's smp_rmb() orders the previous lookup before reading
+ * the sequence and pairs with smp_wmb() in do_raw_write_seqcount_begin().
+ *
+ * The vma_range_is_empty() checks below avoid unnecessary mmap_lock
+ * fallbacks.
+ */
+ mmap_unlocked = !(*last_seq & 1);
+ if (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) {
+ vma_iter_set(vmi, from_addr);
+ mmap_unlocked = mmap_lock_speculate_try_begin(mm, last_seq);
+ }
+
vma = vma_next(vmi);
- if (!vma)
- return NULL;
+ if (!vma) {
+ /*
+ * There is no reason to fallback if there really isn't anything
+ * in the remaining range.
+ */
+ if ((mmap_unlocked && !mmap_lock_speculate_retry(mm, *last_seq)) ||
+ vma_range_is_empty(mm, from_addr, ULONG_MAX))
+ return NULL;
+
+ rcu_read_unlock();
+ goto fallback;
+ }
vma = vma_start_read(mm, vma);
if (IS_ERR_OR_NULL(vma)) {
@@ -442,15 +471,14 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
* it was not shrunk after we found it and another vma has not been
* installed ahead of it. Otherwise we might observe a gap that should
* not be there.
+ *
+ * If speculation fails, avoid taking the mmap_lock when a direct lookup
+ * confirms that the gap is empty.
*/
- if (from_addr < vma->vm_start) {
- /* Verify only if the address space might have changed since vma lookup. */
- if (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) {
- vma_iter_set(vmi, from_addr);
- if (vma != vma_next(vmi))
- goto fallback_unlock;
- }
- }
+ if (from_addr < vma->vm_start &&
+ (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) &&
+ !vma_range_is_empty(mm, from_addr, vma->vm_start - 1))
+ goto fallback_unlock;
return vma;
---
base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7
change-id: 20261025-per_vma_maps-ceab0eb860d5
--
Cheers,
David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
2026-10-25 10:43 [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends David Hildenbrand (Arm)
@ 2026-10-25 14:55 ` Suren Baghdasaryan
2026-10-25 19:01 ` Jose A. Perez de Azpillaga
2026-10-26 13:35 ` Lorenzo Stoakes (ARM)
2 siblings, 0 replies; 8+ messages in thread
From: Suren Baghdasaryan @ 2026-10-25 14:55 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Andrew Morton, Liam R. Howlett, Lorenzo Stoakes, Vlastimil Babka,
Jann Horn, Pedro Falcato, Shakeel Butt, linux-kernel, linux-mm,
stable
On Sun, Oct 25, 2026 at 3:43 AM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
> Reading /proc/PID/maps under per-VMA locks can skip continuously mapped
> address ranges during concurrent VMA splits or merges. This violates the
> guarantee in Documentation/filesystems/proc.rst:
>
> "If there is something at a given vaddr during the entirety of the
> life of the smaps/maps walk, there will be some output for it."
>
> lock_next_vma() reuses a tree iterator across calls under RCU. The iterator
> remembers the range of its last entry. When its node is replaced,
> mas_next_slot() walks the tree again at the last index of that range, then
> advances past the entry containing that index. If VMA boundaries changed,
> this can skip a range that has not been reported:
>
> - A lookup finds [A, C), then mprotect() splits it into [A, B) and [B, C)
> before vma_start_read() locks the first VMA. The reader reports [A, B),
> but the iterator still covers [A, C). The next call starts from B yet
> advances past C, omitting [B, C).
>
> - The reader reports [A, B) and unlocks it. A concurrent mprotect() then
> merges it with [B, C). On the next call, the iterator resumes at B - 1,
> which now belongs to [A, C), and advances past C. Again, [B, C) is lost.
>
> Checking the mmap_lock sequence only within the next call misses writers
> that completed before that call began. Also, a NULL result is currently
> accepted without validation, so a race can terminate the walk early.
>
> Even a fresh lookup can skip a mapping. Suppose it finds a gap before
> [B, C). A writer inserts an adjacent VMA at the end of that gap, then
> merges it into [B, C), replacing the node. The lookup resumes at the old
> gap's last index, now inside the merged VMA, and steps past it. The
> original [B, C) remains mapped throughout. Repeating the forward lookup
> is insufficient: another merge can make it skip the same mapping again,
> returning the same later VMA or NULL.
>
> The fresh-lookup race also affects /proc/PID/smaps, numa_maps and
> smaps_rollup. Resetting the iterator after gathering each VMA's statistics
> does not prevent a merge during the next lookup. Fixing lock_next_vma()
> also prevents omitted entries and incomplete rollup statistics.
>
> Keep a saved sequence in the proc maps locking context and validate it
> before each lookup with mmap_lock_speculate_retry(). If it is even (no
> pending writer) and still matches, reuse both the iterator and the saved
> sequence. Otherwise, restart at from_addr and sample the current sequence
> before looking up the VMA. Only refresh the sample when resetting the
> iterator, so a writer between validation and the lookup cannot go
> undetected.
>
> The smp_rmb() in mmap_lock_speculate_retry() orders the previous lookup
> before rereading the sequence. It pairs with smp_wmb() in
> do_raw_write_seqcount_begin(), which separates the sequence increment in
> mmap_write_lock() from subsequent tree updates.
>
> Validate detected gaps (from_addr < vma->vm_start) and NULL from
> vma_next() against the sequence. If there were concurrent writes, use
> mas_walk() to check if the writer could have interfered with the range
> of interest. If the range is not empty, something interfered concurrently
> so we retry under mmap_lock. Checking for empty ranges avoids taking the
> mmap_lock just because an unrelated writer changed the sequence.
>
> Original fix generated by an LLM and sent by Josef Bacik in private.
>
> Fixes: 5631da56c9a8 ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock")
> Fixes: ea3085dd7a4e ("fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock")
> Fixes: 0e985559d72a ("proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock")
> Cc: stable@vger.kernel.org
> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
Thanks for the report and the fix! Let me take a closer look. I took
extra care to ensure we do not skip any VMA but maybe I missed
something here.
> ---
> I neither understood nor liked the original LLM generated fix that was
> provided to me, and I (and codex) think it was incomplete. So I tried to
> come up with a better fix that avoids additional fallbacks to the mmap_lock
> while trying to keep it simple.
>
> I won't claim that I am now an expert on the code, or that this is the
> most elegant fix. It is quite complicated what we built ... too
> complicated. So if this goes into the wrong direction I'll be happy
> if someone else wants to take over ;)
>
> I reproduced the races with /proc/PID/maps, and with this patch they are
> resolved. I did not manage to reproduce the races with the new
> smaps/smaps_rollup, but the races are slightly different and harder to
> trigger.
>
> There is the issue with seqcount overflows that Lorenzo is tackling. It
> probably interacts here as well? Likely, but that's an independent issue
> to resolve.
> ---
> fs/proc/internal.h | 1 +
> fs/proc/task_mmu.c | 6 +++--
> include/linux/mmap_lock.h | 5 ++--
> mm/mmap_lock.c | 58 +++++++++++++++++++++++++++++++++++------------
> 4 files changed, 51 insertions(+), 19 deletions(-)
>
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index df873df9afc56..f3e846b574d79 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -386,6 +386,7 @@ struct mem_size_stats;
> struct proc_maps_locking_ctx {
> struct mm_struct *mm;
> bool mmap_locked;
> + unsigned int mm_wr_seq;
> struct vm_area_struct *locked_vma;
> };
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index ee9fc2fd2b2fe..dc3adcffb6d9c 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -148,6 +148,7 @@ static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> {
> lock_ctx->locked_vma = NULL;
> lock_ctx->mmap_locked = false;
> + lock_ctx->mm_wr_seq = UINT_MAX;
> }
>
> static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> @@ -160,7 +161,8 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> return vma_next(&priv->iter);
>
> unlock_ctx_vma(lock_ctx);
> - vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos);
> + vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos,
> + &lock_ctx->mm_wr_seq);
> if (!IS_ERR_OR_NULL(vma))
> lock_ctx->locked_vma = vma;
>
> @@ -523,7 +525,7 @@ static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ct
> unlock_ctx_vma(lock_ctx);
> rcu_read_lock();
> vma_iter_init(&vmi, mm, addr);
> - vma = lock_next_vma(mm, &vmi, addr);
> + vma = lock_next_vma(mm, &vmi, addr, &lock_ctx->mm_wr_seq);
> rcu_read_unlock();
>
> if (!vma)
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index 28e3696ce9ff3..e33d15ce6a876 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -511,12 +511,13 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> * Locks next vma pointed by the iterator. Confirms the locked vma has not
> * been modified and will retry under mmap_lock protection if modification
> * was detected. Should be called from read RCU section.
> + * *last_seq must initially be odd and is kept across calls.
> * Returns either a valid locked VMA, NULL if no more VMAs or -EINTR if the
> * process was interrupted.
> */
> struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> - struct vma_iterator *iter,
> - unsigned long address);
> + struct vma_iterator *vmi, unsigned long from_addr,
> + unsigned int *last_seq);
>
> static inline void vma_assert_can_modify(struct vm_area_struct *vma)
> {
> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> index 2f94ee0fdee2a..0f3a4e0c5520d 100644
> --- a/mm/mmap_lock.c
> +++ b/mm/mmap_lock.c
> @@ -400,21 +400,50 @@ static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm
> return vma;
> }
>
> +static bool vma_range_is_empty(struct mm_struct *mm, unsigned long start,
> + unsigned long last)
> +{
> + MA_STATE(mas, &mm->mm_mt, start, start);
> +
> + return !mas_walk(&mas) && mas.last >= last;
> +}
> +
> struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> - struct vma_iterator *vmi,
> - unsigned long from_addr)
> + struct vma_iterator *vmi, unsigned long from_addr,
> + unsigned int *last_seq)
> {
> struct vm_area_struct *vma;
> - unsigned int mm_wr_seq;
> bool mmap_unlocked;
>
> RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu read lock held");
> retry:
> - /* Start mmap_lock speculation in case we need to verify the vma later */
> - mmap_unlocked = mmap_lock_speculate_try_begin(mm, &mm_wr_seq);
> + /*
> + * Restart by address if a writer could have changed the cached range.
> + * The retry check's smp_rmb() orders the previous lookup before reading
> + * the sequence and pairs with smp_wmb() in do_raw_write_seqcount_begin().
> + *
> + * The vma_range_is_empty() checks below avoid unnecessary mmap_lock
> + * fallbacks.
> + */
> + mmap_unlocked = !(*last_seq & 1);
> + if (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) {
> + vma_iter_set(vmi, from_addr);
> + mmap_unlocked = mmap_lock_speculate_try_begin(mm, last_seq);
> + }
> +
> vma = vma_next(vmi);
> - if (!vma)
> - return NULL;
> + if (!vma) {
> + /*
> + * There is no reason to fallback if there really isn't anything
> + * in the remaining range.
> + */
> + if ((mmap_unlocked && !mmap_lock_speculate_retry(mm, *last_seq)) ||
> + vma_range_is_empty(mm, from_addr, ULONG_MAX))
> + return NULL;
> +
> + rcu_read_unlock();
> + goto fallback;
> + }
>
> vma = vma_start_read(mm, vma);
> if (IS_ERR_OR_NULL(vma)) {
> @@ -442,15 +471,14 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> * it was not shrunk after we found it and another vma has not been
> * installed ahead of it. Otherwise we might observe a gap that should
> * not be there.
> + *
> + * If speculation fails, avoid taking the mmap_lock when a direct lookup
> + * confirms that the gap is empty.
> */
> - if (from_addr < vma->vm_start) {
> - /* Verify only if the address space might have changed since vma lookup. */
> - if (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) {
> - vma_iter_set(vmi, from_addr);
> - if (vma != vma_next(vmi))
> - goto fallback_unlock;
> - }
> - }
> + if (from_addr < vma->vm_start &&
> + (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) &&
> + !vma_range_is_empty(mm, from_addr, vma->vm_start - 1))
> + goto fallback_unlock;
>
> return vma;
>
>
> ---
>
> base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7
>
> change-id: 20261025-per_vma_maps-ceab0eb860d5
>
> --
>
> Cheers,
>
> David
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
2026-10-25 10:43 [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends David Hildenbrand (Arm)
2026-10-25 14:55 ` Suren Baghdasaryan
@ 2026-10-25 19:01 ` Jose A. Perez de Azpillaga
2026-10-26 10:04 ` David Hildenbrand (Arm)
2026-10-26 13:35 ` Lorenzo Stoakes (ARM)
2 siblings, 1 reply; 8+ messages in thread
From: Jose A. Perez de Azpillaga @ 2026-10-25 19:01 UTC (permalink / raw)
To: David Hildenbrand (Arm),
Andrew Morton, Liam R. Howlett, Lorenzo Stoakes, Vlastimil Babka,
Jann Horn, Pedro Falcato, Suren Baghdasaryan, Shakeel Butt
Cc: linux-kernel, linux-mm, stable
On 10/25/26 11:43 AM, David Hildenbrand (Arm) wrote:
[snip]
> Fixes: 5631da56c9a8 ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock")
> Fixes: ea3085dd7a4e ("fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock")
> Fixes: 0e985559d72a ("proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock")
PROCMAP_QUERY does the same fresh lookup, shouldn't d9d1c2d81797
("fs/proc/task_mmu: execute PROCMAP_QUERY ioctl under per-vma locks") be
here as well? (commit tracked down by an LLM)
[snip]
> There is the issue with seqcount overflows that Lorenzo is tackling. It
> probably interacts here as well? Likely, but that's an independent issue
> to resolve.
the saved seq is reset on every m_start() and query, so a wrap would
need 2^31 write locks inside one read() or ioctl, I don't think it
matters in practice :)
I tested it on x86_64 in qemu with KASAN, lockdep and DEBUG_VM on top of
mm-unstable, the maps holes I hit there are gone. Also, the code LGTM:
Reviewed-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
Tested-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
--
cheers,
jose a. p-a
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
2026-10-25 19:01 ` Jose A. Perez de Azpillaga
@ 2026-10-26 10:04 ` David Hildenbrand (Arm)
2026-10-26 15:56 ` Andrew Morton
0 siblings, 1 reply; 8+ messages in thread
From: David Hildenbrand (Arm) @ 2026-10-26 10:04 UTC (permalink / raw)
To: Jose A. Perez de Azpillaga, Andrew Morton, Liam R. Howlett,
Lorenzo Stoakes, Vlastimil Babka, Jann Horn, Pedro Falcato,
Suren Baghdasaryan, Shakeel Butt
Cc: linux-kernel, linux-mm, stable
On 10/25/26 20:01, Jose A. Perez de Azpillaga wrote:
> On 10/25/26 11:43 AM, David Hildenbrand (Arm) wrote:
> [snip]
>
>> Fixes: 5631da56c9a8 ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock")
>> Fixes: ea3085dd7a4e ("fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock")
>> Fixes: 0e985559d72a ("proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock")
>
> PROCMAP_QUERY does the same fresh lookup, shouldn't d9d1c2d81797
> ("fs/proc/task_mmu: execute PROCMAP_QUERY ioctl under per-vma locks") be
> here as well? (commit tracked down by an LLM)
Hm, very likely I missed that. So yeah, let's just add it. I assume it won't
matter much because 5631da56c9a8 is the oldest one either way.
>
> [snip]
>
>> There is the issue with seqcount overflows that Lorenzo is tackling. It
>> probably interacts here as well? Likely, but that's an independent issue
>> to resolve.
>
> the saved seq is reset on every m_start() and query, so a wrap would
> need 2^31 write locks inside one read() or ioctl, I don't think it
> matters in practice :)
>
> I tested it on x86_64 in qemu with KASAN, lockdep and DEBUG_VM on top of
> mm-unstable, the maps holes I hit there are gone. Also, the code LGTM:
>
> Reviewed-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
> Tested-by: Jose A. Perez de Azpillaga <azpijr@gmail.com>
>
Thanks for the review!
--
Cheers,
David
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
2026-10-25 10:43 [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends David Hildenbrand (Arm)
2026-10-25 14:55 ` Suren Baghdasaryan
2026-10-25 19:01 ` Jose A. Perez de Azpillaga
@ 2026-10-26 13:35 ` Lorenzo Stoakes (ARM)
2026-10-26 13:38 ` Lorenzo Stoakes (ARM)
2026-10-26 16:16 ` Suren Baghdasaryan
2 siblings, 2 replies; 8+ messages in thread
From: Lorenzo Stoakes (ARM) @ 2026-10-26 13:35 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Andrew Morton, Liam R. Howlett, Vlastimil Babka, Jann Horn,
Pedro Falcato, Suren Baghdasaryan, Shakeel Butt, linux-kernel,
linux-mm, stable
Another note about this - we need to update the tests to actually add
something that can catch this. The selftests only check the last line of
one page and first of next, and latter always implies vma_iter_init() in
m_start() so it never tests the iterator actually being cached.
Obviously something for a follow-up.
On Sun, Oct 25, 2026 at 11:43:15AM +0100, David Hildenbrand (Arm) wrote:
> Reading /proc/PID/maps under per-VMA locks can skip continuously mapped
> address ranges during concurrent VMA splits or merges. This violates the
> guarantee in Documentation/filesystems/proc.rst:
>
> "If there is something at a given vaddr during the entirety of the
> life of the smaps/maps walk, there will be some output for it."
Right.
>
> lock_next_vma() reuses a tree iterator across calls under RCU. The iterator
> remembers the range of its last entry. When its node is replaced,
> mas_next_slot() walks the tree again at the last index of that range, then
> advances past the entry containing that index. If VMA boundaries changed,
> this can skip a range that has not been reported:
>
> - A lookup finds [A, C), then mprotect() splits it into [A, B) and [B, C)
> before vma_start_read() locks the first VMA. The reader reports [A, B),
> but the iterator still covers [A, C). The next call starts from B yet
> advances past C, omitting [B, C).
>
> - The reader reports [A, B) and unlocks it. A concurrent mprotect() then
> merges it with [B, C). On the next call, the iterator resumes at B - 1,
> which now belongs to [A, C), and advances past C. Again, [B, C) is lost.
OK so the key thing here is that the _iterator_ is the issue.
Essentially what 'next' means changes depending on the state of the
tree. All very subtle.
I wonder whether we shouldn't have a vma_iter_next_after() or something
where you can specify the position you are at in terms of an address rather
than a node.
But that's one for a follow-up.
>
> Checking the mmap_lock sequence only within the next call misses writers
> that completed before that call began. Also, a NULL result is currently
> accepted without validation, so a race can terminate the walk early.
Ugh.
>
> Even a fresh lookup can skip a mapping. Suppose it finds a gap before
> [B, C). A writer inserts an adjacent VMA at the end of that gap, then
> merges it into [B, C), replacing the node. The lookup resumes at the old
> gap's last index, now inside the merged VMA, and steps past it. The
> original [B, C) remains mapped throughout. Repeating the forward lookup
> is insufficient: another merge can make it skip the same mapping again,
> returning the same later VMA or NULL.
>
> The fresh-lookup race also affects /proc/PID/smaps, numa_maps and
> smaps_rollup. Resetting the iterator after gathering each VMA's statistics
> does not prevent a merge during the next lookup. Fixing lock_next_vma()
> also prevents omitted entries and incomplete rollup statistics.
>
> Keep a saved sequence in the proc maps locking context and validate it
> before each lookup with mmap_lock_speculate_retry(). If it is even (no
> pending writer) and still matches, reuse both the iterator and the saved
> sequence. Otherwise, restart at from_addr and sample the current sequence
> before looking up the VMA. Only refresh the sample when resetting the
> iterator, so a writer between validation and the lookup cannot go
> undetected.
I think this bit is unnecessary, see below.
>
> The smp_rmb() in mmap_lock_speculate_retry() orders the previous lookup
> before rereading the sequence. It pairs with smp_wmb() in
> do_raw_write_seqcount_begin(), which separates the sequence increment in
> mmap_write_lock() from subsequent tree updates.
I'm glad this is at least abstracted :)
>
> Validate detected gaps (from_addr < vma->vm_start) and NULL from
> vma_next() against the sequence. If there were concurrent writes, use
> mas_walk() to check if the writer could have interfered with the range
> of interest. If the range is not empty, something interfered concurrently
> so we retry under mmap_lock. Checking for empty ranges avoids taking the
> mmap_lock just because an unrelated writer changed the sequence.
>
> Original fix generated by an LLM and sent by Josef Bacik in private.
>
> Fixes: 5631da56c9a8 ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock")
> Fixes: ea3085dd7a4e ("fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock")
> Fixes: 0e985559d72a ("proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock")
> Cc: stable@vger.kernel.org
> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
> ---
> I neither understood nor liked the original LLM generated fix that was
> provided to me, and I (and codex) think it was incomplete. So I tried to
> come up with a better fix that avoids additional fallbacks to the mmap_lock
> while trying to keep it simple.
>
> I won't claim that I am now an expert on the code, or that this is the
> most elegant fix. It is quite complicated what we built ... too
> complicated. So if this goes into the wrong direction I'll be happy
Yep, I do think the VMA locking implementation is too complicated in general. I
did work to simplify the structure of the implementation at least so it was more
intelligible.
Hopefully with this and other fixes we get somewhere better.
I think we could do work to simplify the VMA iterators also.
> if someone else wants to take over ;)
I think we can make this a bit smaller. Patch attached.
But that incurs a perf penalty, so if not that, I also suggest some small
wrappers that de-duplicate and make things much clearer.
>
> I reproduced the races with /proc/PID/maps, and with this patch they are
> resolved. I did not manage to reproduce the races with the new
> smaps/smaps_rollup, but the races are slightly different and harder to
> trigger.
>
> There is the issue with seqcount overflows that Lorenzo is tackling. It
> probably interacts here as well? Likely, but that's an independent issue
> to resolve.
That impacts VMA seqcount, and this logic only touches mm seqcount so all
good (claude agrees...)
> ---
> fs/proc/internal.h | 1 +
> fs/proc/task_mmu.c | 6 +++--
> include/linux/mmap_lock.h | 5 ++--
> mm/mmap_lock.c | 58 +++++++++++++++++++++++++++++++++++------------
> 4 files changed, 51 insertions(+), 19 deletions(-)
>
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index df873df9afc56..f3e846b574d79 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -386,6 +386,7 @@ struct mem_size_stats;
> struct proc_maps_locking_ctx {
> struct mm_struct *mm;
> bool mmap_locked;
> + unsigned int mm_wr_seq;
> struct vm_area_struct *locked_vma;
> };
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index ee9fc2fd2b2fe..dc3adcffb6d9c 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -148,6 +148,7 @@ static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> {
> lock_ctx->locked_vma = NULL;
> lock_ctx->mmap_locked = false;
> + lock_ctx->mm_wr_seq = UINT_MAX;
Kinda horrible to just have to know this needs to be odd etc.
> }
>
> static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> @@ -160,7 +161,8 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> return vma_next(&priv->iter);
>
> unlock_ctx_vma(lock_ctx);
> - vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos);
> + vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos,
> + &lock_ctx->mm_wr_seq);
> if (!IS_ERR_OR_NULL(vma))
> lock_ctx->locked_vma = vma;
>
> @@ -523,7 +525,7 @@ static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ct
> unlock_ctx_vma(lock_ctx);
> rcu_read_lock();
> vma_iter_init(&vmi, mm, addr);
> - vma = lock_next_vma(mm, &vmi, addr);
> + vma = lock_next_vma(mm, &vmi, addr, &lock_ctx->mm_wr_seq);
I think this is challenging to reason about.
But with the suggestion simplification we can avoid it.
Otherwise, it should probably be tied to the iterator to which it belongs, like:
struct vma_rcu_iter {
struct vma_iterator vmi;
unsigned int mm_wr_seq;
};
And pass that around instead, e.g. change the signature to:
struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
struct vma_rcu_iter *rvmi, unsigned long from_addr)
And have something like:
static void vma_rcu_iter_init(struct vma_rcu_iter *rvmi,
struct mm_struct *mm, unsigned long from_addr)
{
vma_iter_init(&rvmi->vmi, mm, from_addr);
/* Set write-locked so initially treated as maybe changed. */
rvmi->mm_wr_seq = UINT_MAX;
}
> rcu_read_unlock();
>
> if (!vma)
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index 28e3696ce9ff3..e33d15ce6a876 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -511,12 +511,13 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> * Locks next vma pointed by the iterator. Confirms the locked vma has not
> * been modified and will retry under mmap_lock protection if modification
> * was detected. Should be called from read RCU section.
> + * *last_seq must initially be odd and is kept across calls.
It's quite odd to have callers need to know about this, feels like a leaked
abstraction.
> * Returns either a valid locked VMA, NULL if no more VMAs or -EINTR if the
> * process was interrupted.
> */
> struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> - struct vma_iterator *iter,
> - unsigned long address);
> + struct vma_iterator *vmi, unsigned long from_addr,
> + unsigned int *last_seq);
>
> static inline void vma_assert_can_modify(struct vm_area_struct *vma)
> {
> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> index 2f94ee0fdee2a..0f3a4e0c5520d 100644
> --- a/mm/mmap_lock.c
> +++ b/mm/mmap_lock.c
> @@ -400,21 +400,50 @@ static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm
> return vma;
> }
>
> +static bool vma_range_is_empty(struct mm_struct *mm, unsigned long start,
> + unsigned long last)
Be good to add an RCU assert (lockdep only). Yes, lock_next_vma() holds
the RCU lock over this but it also releases it at certain points and it
makes things clearer and zero cost.
> +{
> + MA_STATE(mas, &mm->mm_mt, start, start);
> +
> + return !mas_walk(&mas) && mas.last >= last;
> +}
> +
> struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> - struct vma_iterator *vmi,
> - unsigned long from_addr)
> + struct vma_iterator *vmi, unsigned long from_addr,
> + unsigned int *last_seq)
> {
> struct vm_area_struct *vma;
> - unsigned int mm_wr_seq;
> bool mmap_unlocked;
>
> RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu read lock held");
> retry:
> - /* Start mmap_lock speculation in case we need to verify the vma later */
> - mmap_unlocked = mmap_lock_speculate_try_begin(mm, &mm_wr_seq);
> + /*
> + * Restart by address if a writer could have changed the cached range.
> + * The retry check's smp_rmb() orders the previous lookup before reading
> + * the sequence and pairs with smp_wmb() in do_raw_write_seqcount_begin().
> + *
> + * The vma_range_is_empty() checks below avoid unnecessary mmap_lock
> + * fallbacks.
> + */
> + mmap_unlocked = !(*last_seq & 1);
If we keep this logic, this really has to be abstracted out, e.g.:
static bool mm_seq_is_write_locked(unsigned int mm_seq)
{
/* By convention, odd means locked, even means unlocked. */
return mm_seq & 1;
}
> + if (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) {
This is pretty subtle, and the VMA lock code is confusing enough as it is.
So abstracting the speculate/retry bit would be helpful I think.
Maybe something like:
static bool vma_tree_maybe_changed(struct mm_struct *mm, unsigned int last_seq)
{
/*
* Write lock held when sampled (VMA can change), or write lock acquired
* + dropped since.
*/
return mm_seq_is_write_locked(last_seq) ||
mmap_lock_speculate_retry(mm, last_seq);
}
/* Returns true if mmap write unlocked. */
static bool reset_walk_if_mmap_lock_dropped(struct vma_iterator *vmi,
struct mm_struct *mm, unsigned long from_addr,
unsigned int *last_seq)
{
if (vma_tree_maybe_changed(mm, *last_seq)) {
/* Reset walk and re-speculate. */
vma_iter_set(vmi, from_addr);
return mmap_lock_speculate_try_begin(mm, last_seq);
}
return true;
}
(Can return true at the end because the write lock can't be held at the
point of being sampled.)
Then this becomes the self-documenting:
mmap_unlocked = reset_walk_if_mmap_lock_dropped(vmi, mm, from_addr, last_seq);
> + vma_iter_set(vmi, from_addr);
> + mmap_unlocked = mmap_lock_speculate_try_begin(mm, last_seq);
> + }
> +
This seems like a bit of an over-optimisation for a fix, we can always
reintroduce it later.
Simpler I think to just keep the mmap_lock_speculate_try_begin() and
unconditionally perform the vma_iter_set().
Also easier to reason about. Better to reason that the vma iterator might be in
a questionable state, so just reset it.
Also eliminates adding last_seq + all the churn in the fix.
Patch suggestion attached below.
> vma = vma_next(vmi);
> - if (!vma)
> - return NULL;
> + if (!vma) {
> + /*
> + * There is no reason to fallback if there really isn't anything
> + * in the remaining range.
> + */
> + if ((mmap_unlocked && !mmap_lock_speculate_retry(mm, *last_seq)) ||
> + vma_range_is_empty(mm, from_addr, ULONG_MAX))
Quite a lot packed into this single expression.
If the write lock is not held (VMA can't change) and it wasn't acquired and
dropped since last we checked, or, if when explicitly checking the range to
see if anything is left, stop.
It seems to amount to me to 'can we trust that vma_next() didn't just skip
us over stuff?'
With the vma_tree_maybe_changed() helper could be:
if (!vma_tree_maybe_changed(mm, *last_seq) ||
vma_range_is_empty(mm, from_addr, ULONG_MAX))
return NULL;
Which self-documents this.
Actually given the logic below you could have:
static bool should_traverse_range(struct mm_struct *mm,
unsigned int last_seq, unsigned long from, unsigned long to)
{
return vma_tree_maybe_changed(mm, last_seq) &&
!vma_range_is_empty(mm, from, to);
}
Then this would simply be:
if (!should_traverse_range(mm, *last_seq, from_addr, ULONG_MAX))
return NULL;
> + return NULL;
> +
> + rcu_read_unlock();
> + goto fallback;
> + }
> vma = vma_start_read(mm, vma);
> if (IS_ERR_OR_NULL(vma)) {
> @@ -442,15 +471,14 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> * it was not shrunk after we found it and another vma has not been
> * installed ahead of it. Otherwise we might observe a gap that should
> * not be there.
> + *
> + * If speculation fails, avoid taking the mmap_lock when a direct lookup
> + * confirms that the gap is empty.
> */
> - if (from_addr < vma->vm_start) {
> - /* Verify only if the address space might have changed since vma lookup. */
> - if (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) {
> - vma_iter_set(vmi, from_addr);
> - if (vma != vma_next(vmi))
> - goto fallback_unlock;
> - }
> - }
> + if (from_addr < vma->vm_start &&
> + (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) &&
> + !vma_range_is_empty(mm, from_addr, vma->vm_start - 1))
> + goto fallback_unlock;
Again a lot of stuff on the same line, and a lot of duplication.
With above could be:
if (from_addr < vma->vm_start &&
should_traverse_range(mm, *last_seq, from_addr, vma->vm_start - 1))
goto fallback_unlock;
Which also self-documents.
So if the perf penalty for simplification is too much, this stuff can make
this a lot more understandable and avoid code duplication.
>
> return vma;
>
>
> ---
>
> base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7
>
> change-id: 20261025-per_vma_maps-ceab0eb860d5
>
> --
>
> Cheers,
>
> David
>
The simplification eats the cost of a new tree traversal each time from
root (ugh). It makes the fix a LOT simpler, but incurs (claude says)
13-17ns/VMA cost i.e. 10-25% of the per-line cost of reading
/proc/$pid/maps.
I think we care more about lock contention than raw /proc/$pid/maps
traversal time, however, so I'm not sure this matters.
In any case, an optimisation can be introduced with appropriate abstraction
as a follow-up, I think better to do the simpler-to-reason-about version as
the fix.
But if we are bothered about this, then it needs abstracting as suggested.
----8<----
From 2c87f4865b415b90b6b833b51323f2267d1a4f1f Mon Sep 17 00:00:00 2001
From: "Lorenzo Stoakes (ARM)" <ljs@kernel.org>
Date: Mon, 26 Oct 2026 12:11:21 +0000
Subject: [PATCH] simplification
---
mm/mmap_lock.c | 47 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 37 insertions(+), 10 deletions(-)
diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
index 83b7d3ded0dc..6568dda0aba4 100644
--- a/mm/mmap_lock.c
+++ b/mm/mmap_lock.c
@@ -428,6 +428,16 @@ static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm
return vma;
}
+static bool vma_range_is_empty(struct mm_struct *mm, unsigned long start,
+ unsigned long last)
+{
+ MA_STATE(mas, &mm->mm_mt, start, start);
+
+ RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu lock held");
+
+ return !mas_walk(&mas) && mas.last >= last;
+}
+
struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
struct vma_iterator *vmi,
unsigned long from_addr)
@@ -438,11 +448,29 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu read lock held");
retry:
+ /*
+ * No lock was held since the last call, so the iterator's cached range
+ * may no longer be valid. Always restart by address.
+ *
+ * The vma_range_is_empty() checks below avoid unnecessary mmap_lock
+ * fallbacks.
+ */
+ vma_iter_set(vmi, from_addr);
/* Start mmap_lock speculation in case we need to verify the vma later */
mmap_unlocked = mmap_lock_speculate_try_begin(mm, &mm_wr_seq);
vma = vma_next(vmi);
- if (!vma)
- return NULL;
+ if (!vma) {
+ /*
+ * There is no reason to fallback if there really isn't anything
+ * in the remaining range.
+ */
+ if ((mmap_unlocked && !mmap_lock_speculate_retry(mm, mm_wr_seq)) ||
+ vma_range_is_empty(mm, from_addr, ULONG_MAX))
+ return NULL;
+
+ rcu_read_unlock();
+ goto fallback;
+ }
vma = vma_start_read(mm, vma);
if (IS_ERR_OR_NULL(vma)) {
@@ -470,15 +498,14 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
* it was not shrunk after we found it and another vma has not been
* installed ahead of it. Otherwise we might observe a gap that should
* not be there.
+ *
+ * If speculation fails, avoid taking the mmap_lock when a direct lookup
+ * confirms that the gap is empty.
*/
- if (from_addr < vma->vm_start) {
- /* Verify only if the address space might have changed since vma lookup. */
- if (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) {
- vma_iter_set(vmi, from_addr);
- if (vma != vma_next(vmi))
- goto fallback_unlock;
- }
- }
+ if (from_addr < vma->vm_start &&
+ (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) &&
+ !vma_range_is_empty(mm, from_addr, vma->vm_start - 1))
+ goto fallback_unlock;
return vma;
--
2.56.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
2026-10-26 13:35 ` Lorenzo Stoakes (ARM)
@ 2026-10-26 13:38 ` Lorenzo Stoakes (ARM)
2026-10-26 16:16 ` Suren Baghdasaryan
1 sibling, 0 replies; 8+ messages in thread
From: Lorenzo Stoakes (ARM) @ 2026-10-26 13:38 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Andrew Morton, Liam R. Howlett, Vlastimil Babka, Jann Horn,
Pedro Falcato, Suren Baghdasaryan, Shakeel Butt, linux-kernel,
linux-mm, stable
On Mon, Oct 26, 2026 at 01:35:23PM +0000, Lorenzo Stoakes (ARM) wrote:
> /* Returns true if mmap write unlocked. */
> static bool reset_walk_if_mmap_lock_dropped(struct vma_iterator *vmi,
> struct mm_struct *mm, unsigned long from_addr,
> unsigned int *last_seq)
> {
> if (vma_tree_maybe_changed(mm, *last_seq)) {
> /* Reset walk and re-speculate. */
> vma_iter_set(vmi, from_addr);
> return mmap_lock_speculate_try_begin(mm, last_seq);
> }
>
> return true;
> }
Actually, nicer as:
/* Returns true if mmap write unlocked. */
static bool reset_walk_if_mmap_lock_dropped(struct vma_iterator *vmi,
struct mm_struct *mm, unsigned long from_addr,
unsigned int *last_seq)
{
if (!vma_tree_maybe_changed(mm, *last_seq))
return true;
/* Reset walk and re-speculate. */
vma_iter_set(vmi, from_addr);
return mmap_lock_speculate_try_begin(mm, last_seq);
}
--
Cheers, Lorenzo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
2026-10-26 10:04 ` David Hildenbrand (Arm)
@ 2026-10-26 15:56 ` Andrew Morton
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2026-10-26 15:56 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Jose A. Perez de Azpillaga, Liam R. Howlett, Lorenzo Stoakes,
Vlastimil Babka, Jann Horn, Pedro Falcato, Suren Baghdasaryan,
Shakeel Butt, linux-kernel, linux-mm, stable
On Mon, 26 Oct 2026 11:04:23 +0100 "David Hildenbrand (Arm)" <david@kernel.org> wrote:
> >
> > PROCMAP_QUERY does the same fresh lookup, shouldn't d9d1c2d81797
> > ("fs/proc/task_mmu: execute PROCMAP_QUERY ioctl under per-vma locks") be
> > here as well? (commit tracked down by an LLM)
>
> Hm, very likely I missed that. So yeah, let's just add it. I assume it won't
> matter much because 5631da56c9a8 is the oldest one either way.
I made that change to the mm.git copy of this patch.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends
2026-10-26 13:35 ` Lorenzo Stoakes (ARM)
2026-10-26 13:38 ` Lorenzo Stoakes (ARM)
@ 2026-10-26 16:16 ` Suren Baghdasaryan
1 sibling, 0 replies; 8+ messages in thread
From: Suren Baghdasaryan @ 2026-10-26 16:16 UTC (permalink / raw)
To: Lorenzo Stoakes (ARM)
Cc: David Hildenbrand (Arm),
Andrew Morton, Liam R. Howlett, Vlastimil Babka, Jann Horn,
Pedro Falcato, Shakeel Butt, linux-kernel, linux-mm, stable
On Mon, Oct 26, 2026 at 6:35 AM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
>
> Another note about this - we need to update the tests to actually add
> something that can catch this. The selftests only check the last line of
> one page and first of next, and latter always implies vma_iter_init() in
> m_start() so it never tests the iterator actually being cached.
>
> Obviously something for a follow-up.
Yeah, I'll look into it. David, you mentioned you were able to
reproduce the issue. Could you please share the reproducer? That would
help me cover this case in our selftests.
>
> On Sun, Oct 25, 2026 at 11:43:15AM +0100, David Hildenbrand (Arm) wrote:
> > Reading /proc/PID/maps under per-VMA locks can skip continuously mapped
> > address ranges during concurrent VMA splits or merges. This violates the
> > guarantee in Documentation/filesystems/proc.rst:
> >
> > "If there is something at a given vaddr during the entirety of the
> > life of the smaps/maps walk, there will be some output for it."
>
> Right.
>
> >
> > lock_next_vma() reuses a tree iterator across calls under RCU. The iterator
> > remembers the range of its last entry. When its node is replaced,
> > mas_next_slot() walks the tree again at the last index of that range, then
> > advances past the entry containing that index. If VMA boundaries changed,
> > this can skip a range that has not been reported:
> >
> > - A lookup finds [A, C), then mprotect() splits it into [A, B) and [B, C)
> > before vma_start_read() locks the first VMA. The reader reports [A, B),
> > but the iterator still covers [A, C). The next call starts from B yet
> > advances past C, omitting [B, C).
> >
> > - The reader reports [A, B) and unlocks it. A concurrent mprotect() then
> > merges it with [B, C). On the next call, the iterator resumes at B - 1,
> > which now belongs to [A, C), and advances past C. Again, [B, C) is lost.
>
> OK so the key thing here is that the _iterator_ is the issue.
>
> Essentially what 'next' means changes depending on the state of the
> tree. All very subtle.
Right. Subtle details of how iterators work...
>
> I wonder whether we shouldn't have a vma_iter_next_after() or something
> where you can specify the position you are at in terms of an address rather
> than a node.
>
> But that's one for a follow-up.
That would be nice. I'll discuss this with Liam. That would make this
fix very trivial.
>
> >
> > Checking the mmap_lock sequence only within the next call misses writers
> > that completed before that call began. Also, a NULL result is currently
> > accepted without validation, so a race can terminate the walk early.
>
> Ugh.
>
> >
> > Even a fresh lookup can skip a mapping. Suppose it finds a gap before
> > [B, C). A writer inserts an adjacent VMA at the end of that gap, then
> > merges it into [B, C), replacing the node. The lookup resumes at the old
> > gap's last index, now inside the merged VMA, and steps past it. The
> > original [B, C) remains mapped throughout. Repeating the forward lookup
> > is insufficient: another merge can make it skip the same mapping again,
> > returning the same later VMA or NULL.
> >
> > The fresh-lookup race also affects /proc/PID/smaps, numa_maps and
> > smaps_rollup. Resetting the iterator after gathering each VMA's statistics
> > does not prevent a merge during the next lookup. Fixing lock_next_vma()
> > also prevents omitted entries and incomplete rollup statistics.
> >
> > Keep a saved sequence in the proc maps locking context and validate it
> > before each lookup with mmap_lock_speculate_retry(). If it is even (no
> > pending writer) and still matches, reuse both the iterator and the saved
> > sequence. Otherwise, restart at from_addr and sample the current sequence
> > before looking up the VMA. Only refresh the sample when resetting the
> > iterator, so a writer between validation and the lookup cannot go
> > undetected.
>
> I think this bit is unnecessary, see below.
>
> >
> > The smp_rmb() in mmap_lock_speculate_retry() orders the previous lookup
> > before rereading the sequence. It pairs with smp_wmb() in
> > do_raw_write_seqcount_begin(), which separates the sequence increment in
> > mmap_write_lock() from subsequent tree updates.
>
> I'm glad this is at least abstracted :)
>
> >
> > Validate detected gaps (from_addr < vma->vm_start) and NULL from
> > vma_next() against the sequence. If there were concurrent writes, use
> > mas_walk() to check if the writer could have interfered with the range
> > of interest. If the range is not empty, something interfered concurrently
> > so we retry under mmap_lock. Checking for empty ranges avoids taking the
> > mmap_lock just because an unrelated writer changed the sequence.
> >
> > Original fix generated by an LLM and sent by Josef Bacik in private.
> >
> > Fixes: 5631da56c9a8 ("fs/proc/task_mmu: read proc/pid/maps under per-vma lock")
> > Fixes: ea3085dd7a4e ("fs/proc/task_mmu: read proc/pid/{smaps|numa_maps} under per-vma lock")
> > Fixes: 0e985559d72a ("proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
> > ---
> > I neither understood nor liked the original LLM generated fix that was
> > provided to me, and I (and codex) think it was incomplete. So I tried to
> > come up with a better fix that avoids additional fallbacks to the mmap_lock
> > while trying to keep it simple.
> >
> > I won't claim that I am now an expert on the code, or that this is the
> > most elegant fix. It is quite complicated what we built ... too
> > complicated. So if this goes into the wrong direction I'll be happy
>
> Yep, I do think the VMA locking implementation is too complicated in general. I
> did work to simplify the structure of the implementation at least so it was more
> intelligible.
>
> Hopefully with this and other fixes we get somewhere better.
>
> I think we could do work to simplify the VMA iterators also.
>
> > if someone else wants to take over ;)
I'll gladly do that.
>
> I think we can make this a bit smaller. Patch attached.
>
> But that incurs a perf penalty, so if not that, I also suggest some small
> wrappers that de-duplicate and make things much clearer.
>
> >
> > I reproduced the races with /proc/PID/maps, and with this patch they are
> > resolved. I did not manage to reproduce the races with the new
> > smaps/smaps_rollup, but the races are slightly different and harder to
> > trigger.
> >
> > There is the issue with seqcount overflows that Lorenzo is tackling. It
> > probably interacts here as well? Likely, but that's an independent issue
> > to resolve.
>
> That impacts VMA seqcount, and this logic only touches mm seqcount so all
> good (claude agrees...)
>
> > ---
> > fs/proc/internal.h | 1 +
> > fs/proc/task_mmu.c | 6 +++--
> > include/linux/mmap_lock.h | 5 ++--
> > mm/mmap_lock.c | 58 +++++++++++++++++++++++++++++++++++------------
> > 4 files changed, 51 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> > index df873df9afc56..f3e846b574d79 100644
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -386,6 +386,7 @@ struct mem_size_stats;
> > struct proc_maps_locking_ctx {
> > struct mm_struct *mm;
> > bool mmap_locked;
> > + unsigned int mm_wr_seq;
> > struct vm_area_struct *locked_vma;
> > };
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index ee9fc2fd2b2fe..dc3adcffb6d9c 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -148,6 +148,7 @@ static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > lock_ctx->locked_vma = NULL;
> > lock_ctx->mmap_locked = false;
> > + lock_ctx->mm_wr_seq = UINT_MAX;
>
> Kinda horrible to just have to know this needs to be odd etc.
Well, this interface was never meant to be used directly like this. If
we indeed go with this we should introduce an abstraction.
>
> > }
> >
> > static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > @@ -160,7 +161,8 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > return vma_next(&priv->iter);
> >
> > unlock_ctx_vma(lock_ctx);
> > - vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos);
> > + vma = lock_next_vma(lock_ctx->mm, &priv->iter, last_pos,
> > + &lock_ctx->mm_wr_seq);
> > if (!IS_ERR_OR_NULL(vma))
> > lock_ctx->locked_vma = vma;
> >
> > @@ -523,7 +525,7 @@ static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ct
> > unlock_ctx_vma(lock_ctx);
> > rcu_read_lock();
> > vma_iter_init(&vmi, mm, addr);
> > - vma = lock_next_vma(mm, &vmi, addr);
> > + vma = lock_next_vma(mm, &vmi, addr, &lock_ctx->mm_wr_seq);
>
> I think this is challenging to reason about.
>
> But with the suggestion simplification we can avoid it.
>
> Otherwise, it should probably be tied to the iterator to which it belongs, like:
>
> struct vma_rcu_iter {
> struct vma_iterator vmi;
> unsigned int mm_wr_seq;
> };
Agree.
>
> And pass that around instead, e.g. change the signature to:
>
> struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> struct vma_rcu_iter *rvmi, unsigned long from_addr)
>
> And have something like:
>
> static void vma_rcu_iter_init(struct vma_rcu_iter *rvmi,
> struct mm_struct *mm, unsigned long from_addr)
> {
> vma_iter_init(&rvmi->vmi, mm, from_addr);
> /* Set write-locked so initially treated as maybe changed. */
> rvmi->mm_wr_seq = UINT_MAX;
> }
Right.
>
> > rcu_read_unlock();
> >
> > if (!vma)
> > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> > index 28e3696ce9ff3..e33d15ce6a876 100644
> > --- a/include/linux/mmap_lock.h
> > +++ b/include/linux/mmap_lock.h
> > @@ -511,12 +511,13 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> > * Locks next vma pointed by the iterator. Confirms the locked vma has not
> > * been modified and will retry under mmap_lock protection if modification
> > * was detected. Should be called from read RCU section.
> > + * *last_seq must initially be odd and is kept across calls.
>
> It's quite odd to have callers need to know about this, feels like a leaked
> abstraction.
Yes it is. We should not expose these details directly.
>
> > * Returns either a valid locked VMA, NULL if no more VMAs or -EINTR if the
> > * process was interrupted.
> > */
> > struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> > - struct vma_iterator *iter,
> > - unsigned long address);
> > + struct vma_iterator *vmi, unsigned long from_addr,
> > + unsigned int *last_seq);
> >
> > static inline void vma_assert_can_modify(struct vm_area_struct *vma)
> > {
> > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > index 2f94ee0fdee2a..0f3a4e0c5520d 100644
> > --- a/mm/mmap_lock.c
> > +++ b/mm/mmap_lock.c
> > @@ -400,21 +400,50 @@ static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm
> > return vma;
> > }
> >
> > +static bool vma_range_is_empty(struct mm_struct *mm, unsigned long start,
> > + unsigned long last)
>
> Be good to add an RCU assert (lockdep only). Yes, lock_next_vma() holds
> the RCU lock over this but it also releases it at certain points and it
> makes things clearer and zero cost.
>
> > +{
> > + MA_STATE(mas, &mm->mm_mt, start, start);
> > +
> > + return !mas_walk(&mas) && mas.last >= last;
> > +}
> > +
> > struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> > - struct vma_iterator *vmi,
> > - unsigned long from_addr)
> > + struct vma_iterator *vmi, unsigned long from_addr,
> > + unsigned int *last_seq)
> > {
> > struct vm_area_struct *vma;
> > - unsigned int mm_wr_seq;
> > bool mmap_unlocked;
> >
> > RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu read lock held");
> > retry:
> > - /* Start mmap_lock speculation in case we need to verify the vma later */
> > - mmap_unlocked = mmap_lock_speculate_try_begin(mm, &mm_wr_seq);
> > + /*
> > + * Restart by address if a writer could have changed the cached range.
> > + * The retry check's smp_rmb() orders the previous lookup before reading
> > + * the sequence and pairs with smp_wmb() in do_raw_write_seqcount_begin().
> > + *
> > + * The vma_range_is_empty() checks below avoid unnecessary mmap_lock
> > + * fallbacks.
> > + */
> > + mmap_unlocked = !(*last_seq & 1);
>
> If we keep this logic, this really has to be abstracted out, e.g.:
>
> static bool mm_seq_is_write_locked(unsigned int mm_seq)
> {
> /* By convention, odd means locked, even means unlocked. */
> return mm_seq & 1;
> }
>
> > + if (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) {
>
> This is pretty subtle, and the VMA lock code is confusing enough as it is.
>
> So abstracting the speculate/retry bit would be helpful I think.
>
> Maybe something like:
>
> static bool vma_tree_maybe_changed(struct mm_struct *mm, unsigned int last_seq)
> {
> /*
> * Write lock held when sampled (VMA can change), or write lock acquired
> * + dropped since.
> */
> return mm_seq_is_write_locked(last_seq) ||
> mmap_lock_speculate_retry(mm, last_seq);
> }
If we follow your suggestion and introduce vma_rcu_iter then this can
be abstracted as an operation over vma_rcu_iter:
static bool is_vma_rcu_iter_valid(struct mm_struct *mm, struct
vma_rcu_iter *rvmi)
{
/*
* Write lock held when sampled (VMA can change), or write
lock acquired
* + dropped since.
*/
return mm_seq_is_write_locked(rvmi->mm_wr_seq) ||
mmap_lock_speculate_retry(mm, rvmi->mm_wr_seq);
}
>
> /* Returns true if mmap write unlocked. */
> static bool reset_walk_if_mmap_lock_dropped(struct vma_iterator *vmi,
> struct mm_struct *mm, unsigned long from_addr,
> unsigned int *last_seq)
> {
> if (vma_tree_maybe_changed(mm, *last_seq)) {
> /* Reset walk and re-speculate. */
> vma_iter_set(vmi, from_addr);
> return mmap_lock_speculate_try_begin(mm, last_seq);
> }
>
> return true;
> }
>
> (Can return true at the end because the write lock can't be held at the
> point of being sampled.)
>
> Then this becomes the self-documenting:
>
> mmap_unlocked = reset_walk_if_mmap_lock_dropped(vmi, mm, from_addr, last_seq);
>
> > + vma_iter_set(vmi, from_addr);
> > + mmap_unlocked = mmap_lock_speculate_try_begin(mm, last_seq);
> > + }
> > +
>
> This seems like a bit of an over-optimisation for a fix, we can always
> reintroduce it later.
>
> Simpler I think to just keep the mmap_lock_speculate_try_begin() and
> unconditionally perform the vma_iter_set().
>
> Also easier to reason about. Better to reason that the vma iterator might be in
> a questionable state, so just reset it.
>
> Also eliminates adding last_seq + all the churn in the fix.
>
> Patch suggestion attached below.
>
>
> > vma = vma_next(vmi);
> > - if (!vma)
> > - return NULL;
> > + if (!vma) {
> > + /*
> > + * There is no reason to fallback if there really isn't anything
> > + * in the remaining range.
> > + */
> > + if ((mmap_unlocked && !mmap_lock_speculate_retry(mm, *last_seq)) ||
> > + vma_range_is_empty(mm, from_addr, ULONG_MAX))
>
> Quite a lot packed into this single expression.
>
> If the write lock is not held (VMA can't change) and it wasn't acquired and
> dropped since last we checked, or, if when explicitly checking the range to
> see if anything is left, stop.
>
> It seems to amount to me to 'can we trust that vma_next() didn't just skip
> us over stuff?'
>
> With the vma_tree_maybe_changed() helper could be:
>
> if (!vma_tree_maybe_changed(mm, *last_seq) ||
> vma_range_is_empty(mm, from_addr, ULONG_MAX))
> return NULL;
>
> Which self-documents this.
>
> Actually given the logic below you could have:
>
> static bool should_traverse_range(struct mm_struct *mm,
> unsigned int last_seq, unsigned long from, unsigned long to)
> {
> return vma_tree_maybe_changed(mm, last_seq) &&
> !vma_range_is_empty(mm, from, to);
> }
>
> Then this would simply be:
>
> if (!should_traverse_range(mm, *last_seq, from_addr, ULONG_MAX))
> return NULL;
>
Right. Refactoring definitely makes sense.
> > + return NULL;
> > +
> > + rcu_read_unlock();
> > + goto fallback;
> > + }
> > vma = vma_start_read(mm, vma);
> > if (IS_ERR_OR_NULL(vma)) {
> > @@ -442,15 +471,14 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> > * it was not shrunk after we found it and another vma has not been
> > * installed ahead of it. Otherwise we might observe a gap that should
> > * not be there.
> > + *
> > + * If speculation fails, avoid taking the mmap_lock when a direct lookup
> > + * confirms that the gap is empty.
> > */
> > - if (from_addr < vma->vm_start) {
> > - /* Verify only if the address space might have changed since vma lookup. */
> > - if (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) {
> > - vma_iter_set(vmi, from_addr);
> > - if (vma != vma_next(vmi))
> > - goto fallback_unlock;
> > - }
> > - }
> > + if (from_addr < vma->vm_start &&
> > + (!mmap_unlocked || mmap_lock_speculate_retry(mm, *last_seq)) &&
> > + !vma_range_is_empty(mm, from_addr, vma->vm_start - 1))
> > + goto fallback_unlock;
>
> Again a lot of stuff on the same line, and a lot of duplication.
>
> With above could be:
>
> if (from_addr < vma->vm_start &&
> should_traverse_range(mm, *last_seq, from_addr, vma->vm_start - 1))
> goto fallback_unlock;
>
> Which also self-documents.
>
> So if the perf penalty for simplification is too much, this stuff can make
> this a lot more understandable and avoid code duplication.
>
> >
> > return vma;
> >
> >
> > ---
> >
> > base-commit: 21a7f44fd0765fcc5194130b28767786eba1d1e7
> >
> > change-id: 20261025-per_vma_maps-ceab0eb860d5
> >
> > --
> >
> > Cheers,
> >
> > David
> >
>
> The simplification eats the cost of a new tree traversal each time from
> root (ugh). It makes the fix a LOT simpler, but incurs (claude says)
> 13-17ns/VMA cost i.e. 10-25% of the per-line cost of reading
> /proc/$pid/maps.
>
> I think we care more about lock contention than raw /proc/$pid/maps
> traversal time, however, so I'm not sure this matters.
>
> In any case, an optimisation can be introduced with appropriate abstraction
> as a follow-up, I think better to do the simpler-to-reason-about version as
> the fix.
>
> But if we are bothered about this, then it needs abstracting as suggested.
>
> ----8<----
> From 2c87f4865b415b90b6b833b51323f2267d1a4f1f Mon Sep 17 00:00:00 2001
> From: "Lorenzo Stoakes (ARM)" <ljs@kernel.org>
> Date: Mon, 26 Oct 2026 12:11:21 +0000
> Subject: [PATCH] simplification
>
> ---
> mm/mmap_lock.c | 47 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> index 83b7d3ded0dc..6568dda0aba4 100644
> --- a/mm/mmap_lock.c
> +++ b/mm/mmap_lock.c
> @@ -428,6 +428,16 @@ static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm
> return vma;
> }
>
> +static bool vma_range_is_empty(struct mm_struct *mm, unsigned long start,
> + unsigned long last)
> +{
> + MA_STATE(mas, &mm->mm_mt, start, start);
> +
> + RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu lock held");
> +
> + return !mas_walk(&mas) && mas.last >= last;
> +}
> +
> struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> struct vma_iterator *vmi,
> unsigned long from_addr)
> @@ -438,11 +448,29 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
>
> RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "no rcu read lock held");
> retry:
> + /*
> + * No lock was held since the last call, so the iterator's cached range
> + * may no longer be valid. Always restart by address.
> + *
> + * The vma_range_is_empty() checks below avoid unnecessary mmap_lock
> + * fallbacks.
> + */
> + vma_iter_set(vmi, from_addr);
This is clearly simpler but this unconditional vmi reset is not ideal.
At this point we could make vmi just a local variable of this function
because we don't use the stored state anyway. I guess we can use this
as a quick temporary fix.
For the long-term fix I want to check with Liam about feasibility of
adding vma_iter_next_after(), in which case this issue can be fixes
easily and vmi reset can be removed.
Otherwise I can refactor David's patch to hide some of the details in
abstractions like Lorenzo suggested and post that.
> /* Start mmap_lock speculation in case we need to verify the vma later */
> mmap_unlocked = mmap_lock_speculate_try_begin(mm, &mm_wr_seq);
> vma = vma_next(vmi);
> - if (!vma)
> - return NULL;
> + if (!vma) {
> + /*
> + * There is no reason to fallback if there really isn't anything
> + * in the remaining range.
> + */
> + if ((mmap_unlocked && !mmap_lock_speculate_retry(mm, mm_wr_seq)) ||
> + vma_range_is_empty(mm, from_addr, ULONG_MAX))
> + return NULL;
> +
> + rcu_read_unlock();
> + goto fallback;
> + }
>
> vma = vma_start_read(mm, vma);
> if (IS_ERR_OR_NULL(vma)) {
> @@ -470,15 +498,14 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> * it was not shrunk after we found it and another vma has not been
> * installed ahead of it. Otherwise we might observe a gap that should
> * not be there.
> + *
> + * If speculation fails, avoid taking the mmap_lock when a direct lookup
> + * confirms that the gap is empty.
> */
> - if (from_addr < vma->vm_start) {
> - /* Verify only if the address space might have changed since vma lookup. */
> - if (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) {
> - vma_iter_set(vmi, from_addr);
> - if (vma != vma_next(vmi))
> - goto fallback_unlock;
> - }
> - }
> + if (from_addr < vma->vm_start &&
> + (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) &&
> + !vma_range_is_empty(mm, from_addr, vma->vm_start - 1))
> + goto fallback_unlock;
>
> return vma;
>
> --
> 2.56.0
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-10-26 16:16 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-25 10:43 [PATCH] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends David Hildenbrand (Arm)
2026-10-25 14:55 ` Suren Baghdasaryan
2026-10-25 19:01 ` Jose A. Perez de Azpillaga
2026-10-26 10:04 ` David Hildenbrand (Arm)
2026-10-26 15:56 ` Andrew Morton
2026-10-26 13:35 ` Lorenzo Stoakes (ARM)
2026-10-26 13:38 ` Lorenzo Stoakes (ARM)
2026-10-26 16:16 ` Suren Baghdasaryan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®