* [PATCH bpf v3 1/3] bpf: fix mm lifecycle in open-coded task_vma iterator
2026-03-11 22:57 [PATCH bpf v3 0/3] bpf: fix and improve open-coded task_vma iterator Puranjay Mohan
@ 2026-03-11 22:57 ` Puranjay Mohan
2026-03-11 22:57 ` [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks Puranjay Mohan
2026-03-11 22:57 ` [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator Puranjay Mohan
2 siblings, 0 replies; 11+ messages in thread
From: Puranjay Mohan @ 2026-03-11 22:57 UTC (permalink / raw)
To: bpf
Cc: Puranjay Mohan, Puranjay Mohan, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Mykyta Yatsenko,
kernel-team
The open-coded task_vma iterator reads task->mm and acquires
mmap_read_trylock() but never calls mmget(). The mm can reach
mm_users == 0 if the task exits while the iterator holds the lock.
Add mmget_not_zero() before mmap_read_trylock(). Drop the mm reference
via mmput_async(). mmput_async() -> schedule_work() -> __queue_work()
takes pool->lock. BPF programs on traceable functions or tracepoints
called under pool->lock, or programs running in NMI, can reach here
and try to re-acquire pool->lock, causing a deadlock. Guard with
irqs_disabled() because queue_work() disables IRQs before taking
pool->lock. Defer to irq_work when IRQs are disabled.
Widen the mmput_async() #if guard to include CONFIG_BPF_SYSCALL,
following the same approach used for CONFIG_FUTEX_PRIVATE_HASH.
Fixes: 4ac454682158 ("bpf: Introduce task_vma open-coded iterator kfuncs")
Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
include/linux/sched/mm.h | 2 +-
kernel/bpf/task_iter.c | 55 +++++++++++++++++++++++++++++++++++++---
kernel/fork.c | 2 +-
3 files changed, 53 insertions(+), 6 deletions(-)
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 95d0040df584..5908de0c2f82 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -140,7 +140,7 @@ static inline bool mmget_not_zero(struct mm_struct *mm)
/* mmput gets rid of the mappings and all user-space */
extern void mmput(struct mm_struct *);
-#if defined(CONFIG_MMU) || defined(CONFIG_FUTEX_PRIVATE_HASH)
+#if defined(CONFIG_MMU) || defined(CONFIG_FUTEX_PRIVATE_HASH) || defined(CONFIG_BPF_SYSCALL)
/* same as above but performs the slow path from the async context. Can
* be called from the atomic context as well
*/
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 98d9b4c0daff..2ccdc3228063 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -9,6 +9,7 @@
#include <linux/bpf_mem_alloc.h>
#include <linux/btf_ids.h>
#include <linux/mm_types.h>
+#include <linux/sched/mm.h>
#include "mmap_unlock_work.h"
static const char * const iter_task_type_names[] = {
@@ -799,6 +800,7 @@ struct bpf_iter_task_vma_kern_data {
struct mm_struct *mm;
struct mmap_unlock_irq_work *work;
struct vma_iterator vmi;
+ struct irq_work irq_work;
};
struct bpf_iter_task_vma {
@@ -813,6 +815,16 @@ struct bpf_iter_task_vma_kern {
struct bpf_iter_task_vma_kern_data *data;
} __attribute__((aligned(8)));
+static void do_bpf_iter_mmput(struct irq_work *entry)
+{
+ struct bpf_iter_task_vma_kern_data *data;
+
+ data = container_of(entry, struct bpf_iter_task_vma_kern_data,
+ irq_work);
+ mmput_async(data->mm);
+ bpf_mem_free(&bpf_global_ma, data);
+}
+
__bpf_kfunc_start_defs();
__bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
@@ -842,17 +854,38 @@ __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
/* kit->data->work == NULL is valid after bpf_mmap_unlock_get_irq_work */
irq_work_busy = bpf_mmap_unlock_get_irq_work(&kit->data->work);
- if (irq_work_busy || !mmap_read_trylock(kit->data->mm)) {
+ if (irq_work_busy) {
err = -EBUSY;
goto err_cleanup_iter;
}
+ if (!mmget_not_zero(kit->data->mm)) {
+ err = -ENOENT;
+ goto err_cleanup_iter;
+ }
+
+ init_irq_work(&kit->data->irq_work, do_bpf_iter_mmput);
+
+ if (!mmap_read_trylock(kit->data->mm)) {
+ err = -EBUSY;
+ goto err_cleanup_mmget;
+ }
+
vma_iter_init(&kit->data->vmi, kit->data->mm, addr);
return 0;
+err_cleanup_mmget:
+ put_task_struct(kit->data->task);
+ if (!irqs_disabled()) {
+ mmput_async(kit->data->mm);
+ bpf_mem_free(&bpf_global_ma, kit->data);
+ } else {
+ irq_work_queue(&kit->data->irq_work);
+ }
+ kit->data = NULL;
+ return err;
err_cleanup_iter:
- if (kit->data->task)
- put_task_struct(kit->data->task);
+ put_task_struct(kit->data->task);
bpf_mem_free(&bpf_global_ma, kit->data);
/* NULL kit->data signals failed bpf_iter_task_vma initialization */
kit->data = NULL;
@@ -875,7 +908,21 @@ __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
if (kit->data) {
bpf_mmap_unlock_mm(kit->data->work, kit->data->mm);
put_task_struct(kit->data->task);
- bpf_mem_free(&bpf_global_ma, kit->data);
+ /*
+ * mmput_async() -> schedule_work() -> __queue_work()
+ * takes pool->lock. BPF programs on traceable functions
+ * or tracepoints called under pool->lock, or programs
+ * running in NMI, can reach here and try to re-acquire
+ * pool->lock, causing a deadlock. queue_work() disables
+ * IRQs before taking pool->lock, so irqs_disabled()
+ * detects both cases.
+ */
+ if (!irqs_disabled()) {
+ mmput_async(kit->data->mm);
+ bpf_mem_free(&bpf_global_ma, kit->data);
+ } else {
+ irq_work_queue(&kit->data->irq_work);
+ }
}
}
diff --git a/kernel/fork.c b/kernel/fork.c
index 65113a304518..d0411a63d4ab 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1198,7 +1198,7 @@ void mmput(struct mm_struct *mm)
}
EXPORT_SYMBOL_GPL(mmput);
-#if defined(CONFIG_MMU) || defined(CONFIG_FUTEX_PRIVATE_HASH)
+#if defined(CONFIG_MMU) || defined(CONFIG_FUTEX_PRIVATE_HASH) || defined(CONFIG_BPF_SYSCALL)
static void mmput_async_fn(struct work_struct *work)
{
struct mm_struct *mm = container_of(work, struct mm_struct,
--
2.52.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks
2026-03-11 22:57 [PATCH bpf v3 0/3] bpf: fix and improve open-coded task_vma iterator Puranjay Mohan
2026-03-11 22:57 ` [PATCH bpf v3 1/3] bpf: fix mm lifecycle in " Puranjay Mohan
@ 2026-03-11 22:57 ` Puranjay Mohan
2026-03-11 23:53 ` bot+bpf-ci
2026-03-11 22:57 ` [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator Puranjay Mohan
2 siblings, 1 reply; 11+ messages in thread
From: Puranjay Mohan @ 2026-03-11 22:57 UTC (permalink / raw)
To: bpf
Cc: Puranjay Mohan, Puranjay Mohan, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Mykyta Yatsenko,
kernel-team
The open-coded task_vma iterator holds mmap_lock for the entire duration
of iteration, increasing contention on this highly contended lock.
Switch to per-VMA locking. Find the next VMA via an RCU-protected maple
tree walk and lock it with lock_vma_under_rcu(). lock_next_vma() is not
used because its fallback takes mmap_read_lock(), and the iterator must
work in non-sleepable contexts.
lock_vma_under_rcu() is a point lookup (mas_walk) that finds the VMA
containing a given address but cannot iterate across gaps. An
RCU-protected vma_next() walk (mas_find) first locates the next VMA's
vm_start to pass to lock_vma_under_rcu().
Between the RCU walk and the lock, the VMA may be removed, shrunk, or
write-locked. On failure, advance past it using vm_end from the RCU
walk. Because the VMA slab is SLAB_TYPESAFE_BY_RCU, vm_end may be
stale; fall back to PAGE_SIZE advancement when it does not make forward
progress. Concurrent VMA insertions at addresses already passed by the
iterator are not detected.
CONFIG_PER_VMA_LOCK is required; return -EOPNOTSUPP without it.
Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
kernel/bpf/task_iter.c | 105 +++++++++++++++++++++++++++++------------
1 file changed, 76 insertions(+), 29 deletions(-)
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 2ccdc3228063..50dbeaeafd86 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -9,6 +9,7 @@
#include <linux/bpf_mem_alloc.h>
#include <linux/btf_ids.h>
#include <linux/mm_types.h>
+#include <linux/mmap_lock.h>
#include <linux/sched/mm.h>
#include "mmap_unlock_work.h"
@@ -798,9 +799,9 @@ const struct bpf_func_proto bpf_find_vma_proto = {
struct bpf_iter_task_vma_kern_data {
struct task_struct *task;
struct mm_struct *mm;
- struct mmap_unlock_irq_work *work;
- struct vma_iterator vmi;
+ struct vm_area_struct *locked_vma;
struct irq_work irq_work;
+ u64 next_addr;
};
struct bpf_iter_task_vma {
@@ -831,12 +832,16 @@ __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
struct task_struct *task, u64 addr)
{
struct bpf_iter_task_vma_kern *kit = (void *)it;
- bool irq_work_busy = false;
int err;
BUILD_BUG_ON(sizeof(struct bpf_iter_task_vma_kern) != sizeof(struct bpf_iter_task_vma));
BUILD_BUG_ON(__alignof__(struct bpf_iter_task_vma_kern) != __alignof__(struct bpf_iter_task_vma));
+ if (!IS_ENABLED(CONFIG_PER_VMA_LOCK)) {
+ kit->data = NULL;
+ return -EOPNOTSUPP;
+ }
+
/* is_iter_reg_valid_uninit guarantees that kit hasn't been initialized
* before, so non-NULL kit->data doesn't point to previously
* bpf_mem_alloc'd bpf_iter_task_vma_kern_data
@@ -852,38 +857,16 @@ __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
goto err_cleanup_iter;
}
- /* kit->data->work == NULL is valid after bpf_mmap_unlock_get_irq_work */
- irq_work_busy = bpf_mmap_unlock_get_irq_work(&kit->data->work);
- if (irq_work_busy) {
- err = -EBUSY;
- goto err_cleanup_iter;
- }
-
if (!mmget_not_zero(kit->data->mm)) {
err = -ENOENT;
goto err_cleanup_iter;
}
+ kit->data->locked_vma = NULL;
init_irq_work(&kit->data->irq_work, do_bpf_iter_mmput);
-
- if (!mmap_read_trylock(kit->data->mm)) {
- err = -EBUSY;
- goto err_cleanup_mmget;
- }
-
- vma_iter_init(&kit->data->vmi, kit->data->mm, addr);
+ kit->data->next_addr = addr;
return 0;
-err_cleanup_mmget:
- put_task_struct(kit->data->task);
- if (!irqs_disabled()) {
- mmput_async(kit->data->mm);
- bpf_mem_free(&bpf_global_ma, kit->data);
- } else {
- irq_work_queue(&kit->data->irq_work);
- }
- kit->data = NULL;
- return err;
err_cleanup_iter:
put_task_struct(kit->data->task);
bpf_mem_free(&bpf_global_ma, kit->data);
@@ -892,13 +875,76 @@ __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
return err;
}
+/*
+ * Find and lock the next VMA at or after data->next_addr.
+ *
+ * lock_vma_under_rcu() is a point lookup (mas_walk): it finds the VMA
+ * containing a given address but cannot iterate. An RCU-protected
+ * maple tree walk with vma_next() (mas_find) is needed first to locate
+ * the next VMA's vm_start across any gap.
+ *
+ * Between the RCU walk and the lock, the VMA may be removed, shrunk,
+ * or write-locked. On failure, advance past it using vm_end from the
+ * RCU walk. SLAB_TYPESAFE_BY_RCU can make vm_end stale, so fall back
+ * to PAGE_SIZE advancement to guarantee forward progress.
+ */
+static struct vm_area_struct *
+bpf_iter_task_vma_find_next(struct bpf_iter_task_vma_kern_data *data)
+{
+ struct vm_area_struct *vma;
+ struct vma_iterator vmi;
+ unsigned long start, end;
+
+retry:
+ rcu_read_lock();
+ vma_iter_init(&vmi, data->mm, data->next_addr);
+ vma = vma_next(&vmi);
+ if (!vma) {
+ rcu_read_unlock();
+ return NULL;
+ }
+ start = vma->vm_start;
+ end = vma->vm_end;
+ rcu_read_unlock();
+
+ vma = lock_vma_under_rcu(data->mm, start);
+ if (!vma) {
+ if (end > data->next_addr)
+ data->next_addr = end;
+ else
+ data->next_addr += PAGE_SIZE;
+ goto retry;
+ }
+
+ if (unlikely(data->next_addr >= vma->vm_end)) {
+ data->next_addr += PAGE_SIZE;
+ vma_end_read(vma);
+ goto retry;
+ }
+
+ return vma;
+}
+
__bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
{
struct bpf_iter_task_vma_kern *kit = (void *)it;
+ struct vm_area_struct *vma;
if (!kit->data) /* bpf_iter_task_vma_new failed */
return NULL;
- return vma_next(&kit->data->vmi);
+
+ if (kit->data->locked_vma) {
+ vma_end_read(kit->data->locked_vma);
+ kit->data->locked_vma = NULL;
+ }
+
+ vma = bpf_iter_task_vma_find_next(kit->data);
+ if (!vma)
+ return NULL;
+
+ kit->data->locked_vma = vma;
+ kit->data->next_addr = vma->vm_end;
+ return vma;
}
__bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
@@ -906,7 +952,8 @@ __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
struct bpf_iter_task_vma_kern *kit = (void *)it;
if (kit->data) {
- bpf_mmap_unlock_mm(kit->data->work, kit->data->mm);
+ if (kit->data->locked_vma)
+ vma_end_read(kit->data->locked_vma);
put_task_struct(kit->data->task);
/*
* mmput_async() -> schedule_work() -> __queue_work()
--
2.52.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks
2026-03-11 22:57 ` [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks Puranjay Mohan
@ 2026-03-11 23:53 ` bot+bpf-ci
2026-03-12 0:35 ` Puranjay Mohan
0 siblings, 1 reply; 11+ messages in thread
From: bot+bpf-ci @ 2026-03-11 23:53 UTC (permalink / raw)
To: puranjay, bpf
Cc: puranjay, puranjay12, ast, andrii, daniel, martin.lau, eddyz87,
memxor, mykyta.yatsenko5, kernel-team, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index 2ccdc3228063..50dbeaeafd86 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
[ ... ]
> +__bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
> +{
> + struct bpf_iter_task_vma_kern *kit = (void *)it;
> + struct vm_area_struct *vma;
> +
> + if (!kit->data) /* bpf_iter_task_vma_new failed */
> + return NULL;
> +
> + if (kit->data->locked_vma) {
> + vma_end_read(kit->data->locked_vma);
> + kit->data->locked_vma = NULL;
> + }
[ ... ]
> __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
> {
> struct bpf_iter_task_vma_kern *kit = (void *)it;
>
> if (kit->data) {
> - bpf_mmap_unlock_mm(kit->data->work, kit->data->mm);
> + if (kit->data->locked_vma)
> + vma_end_read(kit->data->locked_vma);
> put_task_struct(kit->data->task);
> /*
> * mmput_async() -> schedule_work() -> __queue_work()
> * takes pool->lock. BPF programs on traceable functions
> * or tracepoints called under pool->lock, or programs
> * running in NMI, can reach here and try to re-acquire
> * pool->lock, causing a deadlock. queue_work() disables
> * IRQs before taking pool->lock, so irqs_disabled()
> * detects both cases.
> */
Is vma_end_read() safe to call from NMI context here?
The comment above notes that BPF programs running in NMI can reach
this code. The old code deferred the mmap_lock release via irq_work
when irqs_disabled() to avoid exactly this class of problem.
vma_end_read() calls vma_refcount_put(), which can call
rcuwait_wake_up() when a VMA writer is waiting:
vma_end_read()
vma_refcount_put()
rcuwait_wake_up(&mm->vma_writer_wait)
wake_up_process(task)
raw_spin_lock_irqsave(&p->pi_lock, ...)
If an NMI fires while the interrupted context holds pi_lock or
rq->__lock, this path deadlocks. The same concern applies to the
vma_end_read() calls in bpf_iter_task_vma_next() and
bpf_iter_task_vma_find_next().
The task_vma iterator kfuncs are in common_btf_ids registered for
BPF_PROG_TYPE_UNSPEC, so perf event programs running in NMI context
can use them.
> if (!irqs_disabled()) {
> mmput_async(kit->data->mm);
> bpf_mem_free(&bpf_global_ma, kit->data);
> } else {
> irq_work_queue(&kit->data->irq_work);
> }
> }
> }
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/22978681697
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks
2026-03-11 23:53 ` bot+bpf-ci
@ 2026-03-12 0:35 ` Puranjay Mohan
2026-03-12 16:38 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: Puranjay Mohan @ 2026-03-12 0:35 UTC (permalink / raw)
To: bot+bpf-ci
Cc: bpf, ast, andrii, daniel, martin.lau, eddyz87, memxor,
mykyta.yatsenko5, kernel-team, yonghong.song, clm, ihor.solodrai
On Wed, Mar 11, 2026 at 11:53 PM <bot+bpf-ci@kernel.org> wrote:
>
> > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> > index 2ccdc3228063..50dbeaeafd86 100644
> > --- a/kernel/bpf/task_iter.c
> > +++ b/kernel/bpf/task_iter.c
>
> [ ... ]
>
> > +__bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
> > +{
> > + struct bpf_iter_task_vma_kern *kit = (void *)it;
> > + struct vm_area_struct *vma;
> > +
> > + if (!kit->data) /* bpf_iter_task_vma_new failed */
> > + return NULL;
> > +
> > + if (kit->data->locked_vma) {
> > + vma_end_read(kit->data->locked_vma);
> > + kit->data->locked_vma = NULL;
> > + }
>
> [ ... ]
>
> > __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
> > {
> > struct bpf_iter_task_vma_kern *kit = (void *)it;
> >
> > if (kit->data) {
> > - bpf_mmap_unlock_mm(kit->data->work, kit->data->mm);
> > + if (kit->data->locked_vma)
> > + vma_end_read(kit->data->locked_vma);
> > put_task_struct(kit->data->task);
> > /*
> > * mmput_async() -> schedule_work() -> __queue_work()
> > * takes pool->lock. BPF programs on traceable functions
> > * or tracepoints called under pool->lock, or programs
> > * running in NMI, can reach here and try to re-acquire
> > * pool->lock, causing a deadlock. queue_work() disables
> > * IRQs before taking pool->lock, so irqs_disabled()
> > * detects both cases.
> > */
>
> Is vma_end_read() safe to call from NMI context here?
>
> The comment above notes that BPF programs running in NMI can reach
> this code. The old code deferred the mmap_lock release via irq_work
> when irqs_disabled() to avoid exactly this class of problem.
>
> vma_end_read() calls vma_refcount_put(), which can call
> rcuwait_wake_up() when a VMA writer is waiting:
>
> vma_end_read()
> vma_refcount_put()
> rcuwait_wake_up(&mm->vma_writer_wait)
> wake_up_process(task)
> raw_spin_lock_irqsave(&p->pi_lock, ...)
>
This is a valid concern and fput() will have similar problems because
it calls schedule_delayed_work(). I will have to think about how to
fix it, the easiest would be to add the following to new:
if (irqs_disabled()) {
kit->data = NULL;
return -EBUSY;
}
Then the mmput_async()'s deferral to irq_work is not needed because
the iterator returns early in _new().
But this will make the iteratory useless in NMI because
irqs_disabled() would always return true in NMI (unless I am missing
something)
> If an NMI fires while the interrupted context holds pi_lock or
> rq->__lock, this path deadlocks. The same concern applies to the
> vma_end_read() calls in bpf_iter_task_vma_next() and
> bpf_iter_task_vma_find_next().
>
> The task_vma iterator kfuncs are in common_btf_ids registered for
> BPF_PROG_TYPE_UNSPEC, so perf event programs running in NMI context
> can use them.
>
> > if (!irqs_disabled()) {
> > mmput_async(kit->data->mm);
> > bpf_mem_free(&bpf_global_ma, kit->data);
> > } else {
> > irq_work_queue(&kit->data->irq_work);
> > }
> > }
> > }
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/22978681697
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks
2026-03-12 0:35 ` Puranjay Mohan
@ 2026-03-12 16:38 ` Andrii Nakryiko
2026-03-13 3:19 ` Alexei Starovoitov
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2026-03-12 16:38 UTC (permalink / raw)
To: Puranjay Mohan
Cc: bot+bpf-ci, bpf, ast, andrii, daniel, martin.lau, eddyz87, memxor,
mykyta.yatsenko5, kernel-team, yonghong.song, clm, ihor.solodrai
On Wed, Mar 11, 2026 at 5:35 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> On Wed, Mar 11, 2026 at 11:53 PM <bot+bpf-ci@kernel.org> wrote:
> >
> > > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> > > index 2ccdc3228063..50dbeaeafd86 100644
> > > --- a/kernel/bpf/task_iter.c
> > > +++ b/kernel/bpf/task_iter.c
> >
> > [ ... ]
> >
> > > +__bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
> > > +{
> > > + struct bpf_iter_task_vma_kern *kit = (void *)it;
> > > + struct vm_area_struct *vma;
> > > +
> > > + if (!kit->data) /* bpf_iter_task_vma_new failed */
> > > + return NULL;
> > > +
> > > + if (kit->data->locked_vma) {
> > > + vma_end_read(kit->data->locked_vma);
> > > + kit->data->locked_vma = NULL;
> > > + }
> >
> > [ ... ]
> >
> > > __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
> > > {
> > > struct bpf_iter_task_vma_kern *kit = (void *)it;
> > >
> > > if (kit->data) {
> > > - bpf_mmap_unlock_mm(kit->data->work, kit->data->mm);
> > > + if (kit->data->locked_vma)
> > > + vma_end_read(kit->data->locked_vma);
> > > put_task_struct(kit->data->task);
> > > /*
> > > * mmput_async() -> schedule_work() -> __queue_work()
> > > * takes pool->lock. BPF programs on traceable functions
> > > * or tracepoints called under pool->lock, or programs
> > > * running in NMI, can reach here and try to re-acquire
> > > * pool->lock, causing a deadlock. queue_work() disables
> > > * IRQs before taking pool->lock, so irqs_disabled()
> > > * detects both cases.
> > > */
> >
> > Is vma_end_read() safe to call from NMI context here?
> >
> > The comment above notes that BPF programs running in NMI can reach
> > this code. The old code deferred the mmap_lock release via irq_work
> > when irqs_disabled() to avoid exactly this class of problem.
> >
> > vma_end_read() calls vma_refcount_put(), which can call
> > rcuwait_wake_up() when a VMA writer is waiting:
> >
> > vma_end_read()
> > vma_refcount_put()
> > rcuwait_wake_up(&mm->vma_writer_wait)
> > wake_up_process(task)
> > raw_spin_lock_irqsave(&p->pi_lock, ...)
> >
>
> This is a valid concern and fput() will have similar problems because
> it calls schedule_delayed_work(). I will have to think about how to
> fix it, the easiest would be to add the following to new:
>
> if (irqs_disabled()) {
> kit->data = NULL;
> return -EBUSY;
> }
>
> Then the mmput_async()'s deferral to irq_work is not needed because
> the iterator returns early in _new().
>
> But this will make the iteratory useless in NMI because
> irqs_disabled() would always return true in NMI (unless I am missing
> something)
It seems like having task_vma iterator work in NMI and irqs_disabled()
conditions is untenable due to all these issues... deferring mmput
through async irq_work is fine as it's fixed cost to keep track of
that. But doing async VMA releases for each VMA seems like a bigger
problem, as we'll need to dynamically allocate memory to keep a list
of VMAs yet to be released, which sucks. So yeah, perhaps just giving
up and making task_vma work only in "more normal" is what we have to
do? With do have task_work to work around this, so task_vma is still
extremely useful once add VMA snapshotting
>
> > If an NMI fires while the interrupted context holds pi_lock or
> > rq->__lock, this path deadlocks. The same concern applies to the
> > vma_end_read() calls in bpf_iter_task_vma_next() and
> > bpf_iter_task_vma_find_next().
> >
> > The task_vma iterator kfuncs are in common_btf_ids registered for
> > BPF_PROG_TYPE_UNSPEC, so perf event programs running in NMI context
> > can use them.
> >
> > > if (!irqs_disabled()) {
> > > mmput_async(kit->data->mm);
> > > bpf_mem_free(&bpf_global_ma, kit->data);
> > > } else {
> > > irq_work_queue(&kit->data->irq_work);
> > > }
> > > }
> > > }
> >
> >
> > ---
> > AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> > See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> >
> > CI run summary: https://github.com/kernel-patches/bpf/actions/runs/22978681697
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks
2026-03-12 16:38 ` Andrii Nakryiko
@ 2026-03-13 3:19 ` Alexei Starovoitov
0 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2026-03-13 3:19 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Puranjay Mohan, bot+bpf-ci, bpf, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Eduard,
Kumar Kartikeya Dwivedi, Mykyta Yatsenko, Kernel Team,
Yonghong Song, Chris Mason, Ihor Solodrai
On Thu, Mar 12, 2026 at 9:38 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 11, 2026 at 5:35 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >
> > On Wed, Mar 11, 2026 at 11:53 PM <bot+bpf-ci@kernel.org> wrote:
> > >
> > > > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> > > > index 2ccdc3228063..50dbeaeafd86 100644
> > > > --- a/kernel/bpf/task_iter.c
> > > > +++ b/kernel/bpf/task_iter.c
> > >
> > > [ ... ]
> > >
> > > > +__bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
> > > > +{
> > > > + struct bpf_iter_task_vma_kern *kit = (void *)it;
> > > > + struct vm_area_struct *vma;
> > > > +
> > > > + if (!kit->data) /* bpf_iter_task_vma_new failed */
> > > > + return NULL;
> > > > +
> > > > + if (kit->data->locked_vma) {
> > > > + vma_end_read(kit->data->locked_vma);
> > > > + kit->data->locked_vma = NULL;
> > > > + }
> > >
> > > [ ... ]
> > >
> > > > __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
> > > > {
> > > > struct bpf_iter_task_vma_kern *kit = (void *)it;
> > > >
> > > > if (kit->data) {
> > > > - bpf_mmap_unlock_mm(kit->data->work, kit->data->mm);
> > > > + if (kit->data->locked_vma)
> > > > + vma_end_read(kit->data->locked_vma);
> > > > put_task_struct(kit->data->task);
> > > > /*
> > > > * mmput_async() -> schedule_work() -> __queue_work()
> > > > * takes pool->lock. BPF programs on traceable functions
> > > > * or tracepoints called under pool->lock, or programs
> > > > * running in NMI, can reach here and try to re-acquire
> > > > * pool->lock, causing a deadlock. queue_work() disables
> > > > * IRQs before taking pool->lock, so irqs_disabled()
> > > > * detects both cases.
> > > > */
> > >
> > > Is vma_end_read() safe to call from NMI context here?
> > >
> > > The comment above notes that BPF programs running in NMI can reach
> > > this code. The old code deferred the mmap_lock release via irq_work
> > > when irqs_disabled() to avoid exactly this class of problem.
> > >
> > > vma_end_read() calls vma_refcount_put(), which can call
> > > rcuwait_wake_up() when a VMA writer is waiting:
> > >
> > > vma_end_read()
> > > vma_refcount_put()
> > > rcuwait_wake_up(&mm->vma_writer_wait)
> > > wake_up_process(task)
> > > raw_spin_lock_irqsave(&p->pi_lock, ...)
> > >
> >
> > This is a valid concern and fput() will have similar problems because
> > it calls schedule_delayed_work(). I will have to think about how to
> > fix it, the easiest would be to add the following to new:
> >
> > if (irqs_disabled()) {
> > kit->data = NULL;
> > return -EBUSY;
> > }
> >
> > Then the mmput_async()'s deferral to irq_work is not needed because
> > the iterator returns early in _new().
> >
> > But this will make the iteratory useless in NMI because
> > irqs_disabled() would always return true in NMI (unless I am missing
> > something)
>
> It seems like having task_vma iterator work in NMI and irqs_disabled()
> conditions is untenable due to all these issues... deferring mmput
> through async irq_work is fine as it's fixed cost to keep track of
> that. But doing async VMA releases for each VMA seems like a bigger
> problem, as we'll need to dynamically allocate memory to keep a list
> of VMAs yet to be released, which sucks. So yeah, perhaps just giving
> up and making task_vma work only in "more normal" is what we have to
> do? With do have task_work to work around this, so task_vma is still
> extremely useful once add VMA snapshotting
Since the fix is targeting bpf tree let's do the simplest/minimal fix
if (irqs_disabled()) // EBUSY
No irq_work deferral at all.
Once the fix reaches bpf-next we can restart this discussion on how
to make it usable in a wider context.
I feel that with task_work all such cases of "nmi makes it hard"
should be punted to humans to refactor their code to use task_work.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator
2026-03-11 22:57 [PATCH bpf v3 0/3] bpf: fix and improve open-coded task_vma iterator Puranjay Mohan
2026-03-11 22:57 ` [PATCH bpf v3 1/3] bpf: fix mm lifecycle in " Puranjay Mohan
2026-03-11 22:57 ` [PATCH bpf v3 2/3] bpf: switch task_vma iterator from mmap_lock to per-VMA locks Puranjay Mohan
@ 2026-03-11 22:57 ` Puranjay Mohan
2026-03-11 23:53 ` bot+bpf-ci
2026-03-12 16:44 ` Andrii Nakryiko
2 siblings, 2 replies; 11+ messages in thread
From: Puranjay Mohan @ 2026-03-11 22:57 UTC (permalink / raw)
To: bpf
Cc: Puranjay Mohan, Puranjay Mohan, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau,
Eduard Zingerman, Kumar Kartikeya Dwivedi, Mykyta Yatsenko,
kernel-team
Holding the per-VMA lock across the BPF program body creates a lock
ordering problem when helpers acquire locks that depend on mmap_lock:
vm_lock -> i_rwsem -> mmap_lock -> vm_lock
Snapshot the VMA under the per-VMA lock in _next() via memcpy(), then
drop the lock before returning. The BPF program accesses only the
snapshot.
The verifier only trusts vm_mm and vm_file pointers (see
BTF_TYPE_SAFE_TRUSTED_OR_NULL in verifier.c). vm_file is reference-
counted with get_file() under the lock and released via fput() on the
next iteration or in _destroy(). vm_mm is set to the mm pointer held
via mmget(). All other pointers are left as-is by memcpy() since the
verifier treats them as untrusted.
Fixes: 4ac454682158 ("bpf: Introduce task_vma open-coded iterator kfuncs")
Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
---
kernel/bpf/task_iter.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 50dbeaeafd86..83e3c3983341 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -799,7 +799,7 @@ const struct bpf_func_proto bpf_find_vma_proto = {
struct bpf_iter_task_vma_kern_data {
struct task_struct *task;
struct mm_struct *mm;
- struct vm_area_struct *locked_vma;
+ struct vm_area_struct snapshot;
struct irq_work irq_work;
u64 next_addr;
};
@@ -862,7 +862,7 @@ __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
goto err_cleanup_iter;
}
- kit->data->locked_vma = NULL;
+ kit->data->snapshot.vm_file = NULL;
init_irq_work(&kit->data->irq_work, do_bpf_iter_mmput);
kit->data->next_addr = addr;
return 0;
@@ -928,23 +928,37 @@ bpf_iter_task_vma_find_next(struct bpf_iter_task_vma_kern_data *data)
__bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
{
struct bpf_iter_task_vma_kern *kit = (void *)it;
- struct vm_area_struct *vma;
+ struct vm_area_struct *snap, *vma;
if (!kit->data) /* bpf_iter_task_vma_new failed */
return NULL;
- if (kit->data->locked_vma) {
- vma_end_read(kit->data->locked_vma);
- kit->data->locked_vma = NULL;
+ snap = &kit->data->snapshot;
+
+ if (snap->vm_file) {
+ fput(snap->vm_file);
+ snap->vm_file = NULL;
}
vma = bpf_iter_task_vma_find_next(kit->data);
if (!vma)
return NULL;
- kit->data->locked_vma = vma;
+ memcpy(snap, vma, sizeof(*snap));
+
+ /*
+ * The verifier only trusts vm_mm and vm_file (see
+ * BTF_TYPE_SAFE_TRUSTED_OR_NULL in verifier.c). Take a reference
+ * on vm_file; set vm_mm to the mm held via mmget(). All other
+ * pointers are untrusted by the verifier and left as-is.
+ */
+ if (snap->vm_file)
+ get_file(snap->vm_file);
+ snap->vm_mm = kit->data->mm;
+
kit->data->next_addr = vma->vm_end;
- return vma;
+ vma_end_read(vma);
+ return snap;
}
__bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
@@ -952,8 +966,8 @@ __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
struct bpf_iter_task_vma_kern *kit = (void *)it;
if (kit->data) {
- if (kit->data->locked_vma)
- vma_end_read(kit->data->locked_vma);
+ if (kit->data->snapshot.vm_file)
+ fput(kit->data->snapshot.vm_file);
put_task_struct(kit->data->task);
/*
* mmput_async() -> schedule_work() -> __queue_work()
--
2.52.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator
2026-03-11 22:57 ` [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator Puranjay Mohan
@ 2026-03-11 23:53 ` bot+bpf-ci
2026-03-12 16:44 ` Andrii Nakryiko
1 sibling, 0 replies; 11+ messages in thread
From: bot+bpf-ci @ 2026-03-11 23:53 UTC (permalink / raw)
To: puranjay, bpf
Cc: puranjay, puranjay12, ast, andrii, daniel, martin.lau, eddyz87,
memxor, mykyta.yatsenko5, kernel-team, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index 50dbeaeafd86..83e3c3983341 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
[ ... ]
> @@ -928,23 +928,37 @@ bpf_iter_task_vma_find_next(struct bpf_iter_task_vma_kern_data *data)
> __bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
> {
> struct bpf_iter_task_vma_kern *kit = (void *)it;
> - struct vm_area_struct *vma;
> + struct vm_area_struct *snap, *vma;
>
> if (!kit->data) /* bpf_iter_task_vma_new failed */
> return NULL;
>
> - if (kit->data->locked_vma) {
> - vma_end_read(kit->data->locked_vma);
> - kit->data->locked_vma = NULL;
> + snap = &kit->data->snapshot;
> +
> + if (snap->vm_file) {
> + fput(snap->vm_file);
> + snap->vm_file = NULL;
> }
Andrii Nakryiko noted in his v2 review that this fput + NULL
cleanup pattern is duplicated in bpf_iter_task_vma_destroy(),
and suggested adding a common helper to keep them in sync as
more trusted fields may be added in the future:
https://lore.kernel.org/bpf/CAEf4Bzb9aPiAxOfo+_L6iaZPGrk3mPQ+6yuA6msTUHOV=xbGfg@mail.gmail.com/
Would it make sense to factor this into a small
bpf_iter_task_vma_snapshot_cleanup() or similar, as he
suggested?
[ ... ]
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/22978681697
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator
2026-03-11 22:57 ` [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator Puranjay Mohan
2026-03-11 23:53 ` bot+bpf-ci
@ 2026-03-12 16:44 ` Andrii Nakryiko
2026-03-12 18:08 ` Puranjay Mohan
1 sibling, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2026-03-12 16:44 UTC (permalink / raw)
To: Puranjay Mohan
Cc: bpf, Puranjay Mohan, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Mykyta Yatsenko, kernel-team
On Wed, Mar 11, 2026 at 3:58 PM Puranjay Mohan <puranjay@kernel.org> wrote:
>
> Holding the per-VMA lock across the BPF program body creates a lock
> ordering problem when helpers acquire locks that depend on mmap_lock:
>
> vm_lock -> i_rwsem -> mmap_lock -> vm_lock
>
> Snapshot the VMA under the per-VMA lock in _next() via memcpy(), then
> drop the lock before returning. The BPF program accesses only the
> snapshot.
>
> The verifier only trusts vm_mm and vm_file pointers (see
> BTF_TYPE_SAFE_TRUSTED_OR_NULL in verifier.c). vm_file is reference-
> counted with get_file() under the lock and released via fput() on the
> next iteration or in _destroy(). vm_mm is set to the mm pointer held
> via mmget(). All other pointers are left as-is by memcpy() since the
> verifier treats them as untrusted.
>
> Fixes: 4ac454682158 ("bpf: Introduce task_vma open-coded iterator kfuncs")
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
> kernel/bpf/task_iter.c | 34 ++++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> index 50dbeaeafd86..83e3c3983341 100644
> --- a/kernel/bpf/task_iter.c
> +++ b/kernel/bpf/task_iter.c
> @@ -799,7 +799,7 @@ const struct bpf_func_proto bpf_find_vma_proto = {
> struct bpf_iter_task_vma_kern_data {
> struct task_struct *task;
> struct mm_struct *mm;
> - struct vm_area_struct *locked_vma;
> + struct vm_area_struct snapshot;
> struct irq_work irq_work;
> u64 next_addr;
> };
> @@ -862,7 +862,7 @@ __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
> goto err_cleanup_iter;
> }
>
> - kit->data->locked_vma = NULL;
> + kit->data->snapshot.vm_file = NULL;
> init_irq_work(&kit->data->irq_work, do_bpf_iter_mmput);
> kit->data->next_addr = addr;
> return 0;
> @@ -928,23 +928,37 @@ bpf_iter_task_vma_find_next(struct bpf_iter_task_vma_kern_data *data)
> __bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
> {
> struct bpf_iter_task_vma_kern *kit = (void *)it;
> - struct vm_area_struct *vma;
> + struct vm_area_struct *snap, *vma;
>
> if (!kit->data) /* bpf_iter_task_vma_new failed */
> return NULL;
>
> - if (kit->data->locked_vma) {
> - vma_end_read(kit->data->locked_vma);
> - kit->data->locked_vma = NULL;
> + snap = &kit->data->snapshot;
> +
> + if (snap->vm_file) {
> + fput(snap->vm_file);
> + snap->vm_file = NULL;
> }
>
> vma = bpf_iter_task_vma_find_next(kit->data);
> if (!vma)
> return NULL;
>
> - kit->data->locked_vma = vma;
> + memcpy(snap, vma, sizeof(*snap));
> +
> + /*
> + * The verifier only trusts vm_mm and vm_file (see
> + * BTF_TYPE_SAFE_TRUSTED_OR_NULL in verifier.c). Take a reference
> + * on vm_file; set vm_mm to the mm held via mmget(). All other
> + * pointers are untrusted by the verifier and left as-is.
> + */
> + if (snap->vm_file)
> + get_file(snap->vm_file);
> + snap->vm_mm = kit->data->mm;
ummm... why? you were making a copy while VMA was locked. If you are
afraid that VMA was moved to a different mm_struct, we should detect
and retry instead of overwriting snapshot's mm, no?
> +
> kit->data->next_addr = vma->vm_end;
> - return vma;
> + vma_end_read(vma);
> + return snap;
> }
>
> __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
> @@ -952,8 +966,8 @@ __bpf_kfunc void bpf_iter_task_vma_destroy(struct bpf_iter_task_vma *it)
> struct bpf_iter_task_vma_kern *kit = (void *)it;
>
> if (kit->data) {
> - if (kit->data->locked_vma)
> - vma_end_read(kit->data->locked_vma);
> + if (kit->data->snapshot.vm_file)
> + fput(kit->data->snapshot.vm_file);
> put_task_struct(kit->data->task);
> /*
> * mmput_async() -> schedule_work() -> __queue_work()
> --
> 2.52.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH bpf v3 3/3] bpf: return VMA snapshot from task_vma iterator
2026-03-12 16:44 ` Andrii Nakryiko
@ 2026-03-12 18:08 ` Puranjay Mohan
0 siblings, 0 replies; 11+ messages in thread
From: Puranjay Mohan @ 2026-03-12 18:08 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Eduard Zingerman, Kumar Kartikeya Dwivedi,
Mykyta Yatsenko, kernel-team
On Thu, Mar 12, 2026 at 4:44 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 11, 2026 at 3:58 PM Puranjay Mohan <puranjay@kernel.org> wrote:
> >
> > Holding the per-VMA lock across the BPF program body creates a lock
> > ordering problem when helpers acquire locks that depend on mmap_lock:
> >
> > vm_lock -> i_rwsem -> mmap_lock -> vm_lock
> >
> > Snapshot the VMA under the per-VMA lock in _next() via memcpy(), then
> > drop the lock before returning. The BPF program accesses only the
> > snapshot.
> >
> > The verifier only trusts vm_mm and vm_file pointers (see
> > BTF_TYPE_SAFE_TRUSTED_OR_NULL in verifier.c). vm_file is reference-
> > counted with get_file() under the lock and released via fput() on the
> > next iteration or in _destroy(). vm_mm is set to the mm pointer held
> > via mmget(). All other pointers are left as-is by memcpy() since the
> > verifier treats them as untrusted.
> >
> > Fixes: 4ac454682158 ("bpf: Introduce task_vma open-coded iterator kfuncs")
> > Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> > ---
> > kernel/bpf/task_iter.c | 34 ++++++++++++++++++++++++----------
> > 1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
> > index 50dbeaeafd86..83e3c3983341 100644
> > --- a/kernel/bpf/task_iter.c
> > +++ b/kernel/bpf/task_iter.c
> > @@ -799,7 +799,7 @@ const struct bpf_func_proto bpf_find_vma_proto = {
> > struct bpf_iter_task_vma_kern_data {
> > struct task_struct *task;
> > struct mm_struct *mm;
> > - struct vm_area_struct *locked_vma;
> > + struct vm_area_struct snapshot;
> > struct irq_work irq_work;
> > u64 next_addr;
> > };
> > @@ -862,7 +862,7 @@ __bpf_kfunc int bpf_iter_task_vma_new(struct bpf_iter_task_vma *it,
> > goto err_cleanup_iter;
> > }
> >
> > - kit->data->locked_vma = NULL;
> > + kit->data->snapshot.vm_file = NULL;
> > init_irq_work(&kit->data->irq_work, do_bpf_iter_mmput);
> > kit->data->next_addr = addr;
> > return 0;
> > @@ -928,23 +928,37 @@ bpf_iter_task_vma_find_next(struct bpf_iter_task_vma_kern_data *data)
> > __bpf_kfunc struct vm_area_struct *bpf_iter_task_vma_next(struct bpf_iter_task_vma *it)
> > {
> > struct bpf_iter_task_vma_kern *kit = (void *)it;
> > - struct vm_area_struct *vma;
> > + struct vm_area_struct *snap, *vma;
> >
> > if (!kit->data) /* bpf_iter_task_vma_new failed */
> > return NULL;
> >
> > - if (kit->data->locked_vma) {
> > - vma_end_read(kit->data->locked_vma);
> > - kit->data->locked_vma = NULL;
> > + snap = &kit->data->snapshot;
> > +
> > + if (snap->vm_file) {
> > + fput(snap->vm_file);
> > + snap->vm_file = NULL;
> > }
> >
> > vma = bpf_iter_task_vma_find_next(kit->data);
> > if (!vma)
> > return NULL;
> >
> > - kit->data->locked_vma = vma;
> > + memcpy(snap, vma, sizeof(*snap));
> > +
> > + /*
> > + * The verifier only trusts vm_mm and vm_file (see
> > + * BTF_TYPE_SAFE_TRUSTED_OR_NULL in verifier.c). Take a reference
> > + * on vm_file; set vm_mm to the mm held via mmget(). All other
> > + * pointers are untrusted by the verifier and left as-is.
> > + */
> > + if (snap->vm_file)
> > + get_file(snap->vm_file);
> > + snap->vm_mm = kit->data->mm;
>
> ummm... why? you were making a copy while VMA was locked. If you are
> afraid that VMA was moved to a different mm_struct, we should detect
> and retry instead of overwriting snapshot's mm, no?
This is just to make sure the person who reads this sees it that only
these two are trusted. We don't need it because snap->vm_mm and
kit->data->mm will be equal anyway, I will remove it in the next
version.
^ permalink raw reply [flat|nested] 11+ messages in thread