[PATCH bpf-next v7 0/3] bpf: Implement stack_map_get_build_id_offset_sleepable()

[PATCH bpf-next v7 0/3] bpf: Implement stack_map_get_build_id_offset_sleepable()

[Ihor Solodrai]

The series introduces stack_map_get_build_id_offset_sleepable(),
fixing a gap with parsing build_id in sleepable context in stackmap.c

In particular, this fixes a deadlock in
stack_map_get_build_id_offset() doing a blocking __kernel_read(),
which happens since commit 777a8560fd29 ("lib/buildid: use
__kernel_read() for sleepable context").

See previous revisions for more details.

---

v6->v7:
  * Addressed feedback from Andrii (mostly patch #2):
    * implement proper CONFIG_PER_VMA_LOCK=n support, following a
      VMA locking pattern similar to one used in PROCMAP_QUERY
    * change the contract of stack_map_lock_vma(): if a non-NULL VMA
      is returned, then a read lock is held
      * remove now unnecessary vma_locked flag
    * and various other nits
  * Add vma_is_anonymous() checks where appropriate (AIs)
v6: https://lore.kernel.org/bpf/20260521225022.2695755-1-ihor.solodrai@linux.dev/

v5->v6:
  * Misc refactoring (Andrii):
    * add stack_map_build_id_set_valid() helper
    * simplify control flow in stack_map_get_build_id_offset_sleepable()
v5: https://lore.kernel.org/bpf/20260515005244.1333013-1-ihor.solodrai@linux.dev/

v4->v5:
  * Add comments explaining mmap_read_trylock() (Shakeel)
  * Rebase on bpf-next (Alexei)
v4: https://lore.kernel.org/bpf/20260514184727.1067141-1-ihor.solodrai@linux.dev/

v3->v4:
  * Change Fixes tag in patch #2 (AI)
  * Nit in caching implementation (Mykyta)
v3: https://lore.kernel.org/bpf/20260512032906.2670326-1-ihor.solodrai@linux.dev/

v2->v3:
  * Split patch #2 in two: stack_map_get_build_id_offset_sleepable()
    implementation, and then introduce caching
  * Drop taking mmap_lock if CONFIG_PER_VMA_LOCK=n, fall back to raw
    IPs instead
  * Cache vm_{start,end} in addition to prev_file (Mykyta)
v2: https://lore.kernel.org/bpf/20260409010604.1439087-1-ihor.solodrai@linux.dev/

v1->v2:
  * Addressed feedback from Puranjay:
    * split out a small refactoring patch
    * use mmap_read_trylock()
    * take into account CONFIG_PER_VMA_LOCK
    * replace find_vma() with vma_lookup()
    * cache prev_build_id to avoid re-parsing the same file
  * Snapshot vm_pgoff and vm_start before unlocking (AI)
  * To avoid repetitive unlocking statements, introduce struct
    stack_map_vma_lock to hold relevant lock state info and add an
    unlock helper
v1: https://lore.kernel.org/bpf/20260407223003.720428-1-ihor.solodrai@linux.dev/

---


Ihor Solodrai (3):
  bpf: Factor out stack_map build ID helpers
  bpf: Avoid faultable build ID reads under mm locks
  bpf: Cache build IDs in sleepable stackmap path

 kernel/bpf/stackmap.c | 215 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 198 insertions(+), 17 deletions(-)

-- 
2.54.0

[PATCH bpf-next v7 1/3] bpf: Factor out stack_map build ID helpers

[Ihor Solodrai]

Factor out helpers from stack_map_get_build_id_offset() in
preparation for adding a sleepable build ID resolution path:
stack_map_build_id_set_ip(), stack_map_build_id_offset(), and
stack_map_build_id_set_valid().

While here, refactor stack_map_get_build_id_offset():
  * use continue-driven control flow in the main loop and remove
    build_id_valid label
  * update prev_vma and prev_build_id on the fall-back-to-IP branch so
    the cache reflects the actual VMA seen on the previous IP [1]
  * guard fetch_build_id() with vma_is_anonymous() [2] to skip parse
    attempts that would otherwise fail the ELF magic check

[1] https://lore.kernel.org/bpf/CAEf4Bzac9uWWqBvzH0iFzKvJcq3vxscZ3pKm0sUHmN-F-z9wVQ@mail.gmail.com/
[2] https://lore.kernel.org/bpf/226398c1ff3f2b686c0aeb010408d85fb15df13f9ff60a045bee31e79b9e41e9@mail.kernel.org/

Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 kernel/bpf/stackmap.c | 57 ++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index da3d328f5c15..e23be7d44503 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -152,6 +152,28 @@ static int fetch_build_id(struct vm_area_struct *vma, unsigned char *build_id, b
 			 : build_id_parse_nofault(vma, build_id, NULL);
 }
 
+static inline void stack_map_build_id_set_ip(struct bpf_stack_build_id *id)
+{
+	id->status = BPF_STACK_BUILD_ID_IP;
+	memset(id->build_id, 0, BUILD_ID_SIZE_MAX);
+}
+
+static inline u64 stack_map_build_id_offset(unsigned long vm_pgoff,
+					    unsigned long vm_start, u64 ip)
+{
+	return (vm_pgoff << PAGE_SHIFT) + ip - vm_start;
+}
+
+static inline void stack_map_build_id_set_valid(struct bpf_stack_build_id *id,
+						u64 offset,
+						const unsigned char *build_id)
+{
+	id->status = BPF_STACK_BUILD_ID_VALID;
+	id->offset = offset;
+	if (id->build_id != build_id)
+		memcpy(id->build_id, build_id, BUILD_ID_SIZE_MAX);
+}
+
 /*
  * Expects all id_offs[i].ip values to be set to correct initial IPs.
  * They will be subsequently:
@@ -165,44 +187,45 @@ static int fetch_build_id(struct vm_area_struct *vma, unsigned char *build_id, b
 static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 					  u32 trace_nr, bool user, bool may_fault)
 {
-	int i;
 	struct mmap_unlock_irq_work *work = NULL;
 	bool irq_work_busy = bpf_mmap_unlock_get_irq_work(&work);
+	bool has_user_ctx = user && current && current->mm;
 	struct vm_area_struct *vma, *prev_vma = NULL;
-	const char *prev_build_id;
+	const unsigned char *prev_build_id = NULL;
+	int i;
 
 	/* If the irq_work is in use, fall back to report ips. Same
 	 * fallback is used for kernel stack (!user) on a stackmap with
 	 * build_id.
 	 */
-	if (!user || !current || !current->mm || irq_work_busy ||
-	    !mmap_read_trylock(current->mm)) {
+	if (!has_user_ctx || irq_work_busy || !mmap_read_trylock(current->mm)) {
 		/* cannot access current->mm, fall back to ips */
-		for (i = 0; i < trace_nr; i++) {
-			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
-			memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
-		}
+		for (i = 0; i < trace_nr; i++)
+			stack_map_build_id_set_ip(&id_offs[i]);
 		return;
 	}
 
 	for (i = 0; i < trace_nr; i++) {
 		u64 ip = READ_ONCE(id_offs[i].ip);
+		u64 offset;
 
-		if (range_in_vma(prev_vma, ip, ip)) {
+		if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
 			vma = prev_vma;
-			memcpy(id_offs[i].build_id, prev_build_id, BUILD_ID_SIZE_MAX);
-			goto build_id_valid;
+			offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
+			stack_map_build_id_set_valid(&id_offs[i], offset, prev_build_id);
+			continue;
 		}
 		vma = find_vma(current->mm, ip);
-		if (!vma || fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
+		if (!vma || vma_is_anonymous(vma) ||
+		    fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
 			/* per entry fall back to ips */
-			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
-			memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
+			stack_map_build_id_set_ip(&id_offs[i]);
+			prev_vma = vma;
+			prev_build_id = NULL;
 			continue;
 		}
-build_id_valid:
-		id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ip - vma->vm_start;
-		id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
+		offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
+		stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
 		prev_vma = vma;
 		prev_build_id = id_offs[i].build_id;
 	}
-- 
2.54.0

[PATCH bpf-next v7 2/3] bpf: Avoid faultable build ID reads under mm locks

[Ihor Solodrai]

Sleepable build ID parsing can block in __kernel_read() [1], so the
stackmap sleepable path must not call it while holding mmap_lock or a
per-VMA read lock.

The issue and the fix are conceptually similar to a recent procfs
patch [2]. A similar VMA locking pattern has already been used in
PROCMAP_QUERY [3].

Resolve each covered VMA with a stable read-side reference, preferring
lock_vma_under_rcu() and falling back to mmap_read_trylock() only long
enough to acquire the VMA read lock. Take a reference to the backing
file, drop the VMA lock, and then parse the build ID through
(sleepable) build_id_parse_file().

We have to use mmap_read_trylock() (and give up on failure) in this
context because taking mmap_read_lock() is generally unsafe on code
paths reachable from BPF programs [4], and may lead to deadlocks.

[1] https://lore.kernel.org/all/20251218005818.614819-1-shakeel.butt@linux.dev/
[2] https://lore.kernel.org/all/20260128183232.2854138-1-andrii@kernel.org/
[3] https://lore.kernel.org/all/20250808152850.2580887-1-surenb@google.com/
[4] https://lore.kernel.org/bpf/2895ecd8-df1e-4cc0-b9f9-aef893dc2360@linux.dev/

Fixes: d4dd9775ec24 ("bpf: wire up sleepable bpf_get_stack() and bpf_get_task_stack() helpers")
Suggested-by: Puranjay Mohan <puranjay@kernel.org>
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 kernel/bpf/stackmap.c | 109 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index e23be7d44503..c53cfd9a67cf 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -9,6 +9,7 @@
 #include <linux/perf_event.h>
 #include <linux/btf_ids.h>
 #include <linux/buildid.h>
+#include <linux/mmap_lock.h>
 #include "percpu_freelist.h"
 #include "mmap_unlock_work.h"
 
@@ -174,6 +175,109 @@ static inline void stack_map_build_id_set_valid(struct bpf_stack_build_id *id,
 		memcpy(id->build_id, build_id, BUILD_ID_SIZE_MAX);
 }
 
+struct stack_map_vma_lock {
+	struct vm_area_struct *vma;
+	struct mm_struct *mm;
+};
+
+/*
+ * Acquire a stable read-side reference on the VMA covering @ip.
+ *
+ * With CONFIG_PER_VMA_LOCK=y this returns a VMA with its per-VMA read
+ * lock held and mmap_lock dropped, so the caller may sleep.
+ *
+ * With CONFIG_PER_VMA_LOCK=n it returns a VMA with mmap_lock still
+ * held; the caller must snapshot any fields it needs and pin vm_file
+ * with get_file() before stack_map_unlock_vma() drops mmap_lock, as
+ * the VMA may be split, merged, or freed after that.
+ *
+ * Returns NULL on failure, in which case no lock is held.
+ */
+static struct vm_area_struct *
+stack_map_lock_vma(struct stack_map_vma_lock *lock, unsigned long ip)
+{
+	struct mm_struct *mm = lock->mm;
+	struct vm_area_struct *vma;
+
+	/* noop under !CONFIG_PER_VMA_LOCK */
+	vma = lock_vma_under_rcu(mm, ip);
+	if (vma) {
+		lock->vma = vma;
+		return vma;
+	}
+
+	/*
+	 * Taking mmap_read_lock() is unsafe here, because the caller BPF
+	 * program might already hold it, causing a deadlock.
+	 */
+	if (!mmap_read_trylock(mm))
+		return NULL;
+
+	vma = vma_lookup(mm, ip);
+	if (!vma) {
+		mmap_read_unlock(mm);
+		return NULL;
+	}
+
+#ifdef CONFIG_PER_VMA_LOCK
+	if (!vma_start_read_locked(vma)) {
+		mmap_read_unlock(mm);
+		return NULL;
+	}
+	mmap_read_unlock(mm);
+#endif
+
+	lock->vma = vma;
+	return vma;
+}
+
+static void stack_map_unlock_vma(struct stack_map_vma_lock *lock)
+{
+#ifdef CONFIG_PER_VMA_LOCK
+	vma_end_read(lock->vma);
+#else
+	mmap_read_unlock(lock->mm);
+#endif
+	lock->vma = NULL;
+}
+
+static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *id_offs,
+						    u32 trace_nr)
+{
+	struct mm_struct *mm = current->mm;
+	struct stack_map_vma_lock lock = { .mm = mm };
+	struct vm_area_struct *vma;
+	struct file *file;
+	u64 offset;
+	u64 ip;
+
+	for (u32 i = 0; i < trace_nr; i++) {
+		ip = READ_ONCE(id_offs[i].ip);
+
+		vma = stack_map_lock_vma(&lock, ip);
+		if (!vma) {
+			stack_map_build_id_set_ip(&id_offs[i]);
+			continue;
+		}
+		if (vma_is_anonymous(vma) || !vma->vm_file) {
+			stack_map_build_id_set_ip(&id_offs[i]);
+			stack_map_unlock_vma(&lock);
+			continue;
+		}
+
+		file = get_file(vma->vm_file);
+		offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
+		stack_map_unlock_vma(&lock);
+
+		/* build_id_parse_file() may block on filesystem reads */
+		if (build_id_parse_file(file, id_offs[i].build_id, NULL))
+			stack_map_build_id_set_ip(&id_offs[i]);
+		else
+			stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
+		fput(file);
+	}
+}
+
 /*
  * Expects all id_offs[i].ip values to be set to correct initial IPs.
  * They will be subsequently:
@@ -194,6 +298,11 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 	const unsigned char *prev_build_id = NULL;
 	int i;
 
+	if (may_fault && has_user_ctx) {
+		stack_map_get_build_id_offset_sleepable(id_offs, trace_nr);
+		return;
+	}
+
 	/* If the irq_work is in use, fall back to report ips. Same
 	 * fallback is used for kernel stack (!user) on a stackmap with
 	 * build_id.
-- 
2.54.0

[PATCH bpf-next v7 3/3] bpf: Cache build IDs in sleepable stackmap path

[Ihor Solodrai]

Stack traces often contain adjacent IPs from the same VMA or from
different VMAs backed by the same ELF file. Cache the last successfully
parsed build id together with the resolved VMA range and backing file
so the sleepable build id path can avoid repeated VMA locking and file
parsing in common cases.

Suggested-by: Mykyta Yatsenko <yatsenko@meta.com>
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
---
 kernel/bpf/stackmap.c | 61 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index c53cfd9a67cf..77ba03216c09 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -246,6 +246,14 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
 {
 	struct mm_struct *mm = current->mm;
 	struct stack_map_vma_lock lock = { .mm = mm };
+	struct {
+		struct file *file;
+		const unsigned char *build_id;
+		unsigned long vm_start;
+		unsigned long vm_end;
+		unsigned long vm_pgoff;
+	} cache = {};
+	unsigned long vm_pgoff, vm_start, vm_end;
 	struct vm_area_struct *vma;
 	struct file *file;
 	u64 offset;
@@ -254,6 +262,17 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
 	for (u32 i = 0; i < trace_nr; i++) {
 		ip = READ_ONCE(id_offs[i].ip);
 
+		/*
+		 * Range cache fast path: if ip falls within the previously
+		 * resolved VMA range, reuse the cache build_id without
+		 * re-acquiring the VMA lock.
+		 */
+		if (cache.build_id && ip >= cache.vm_start && ip < cache.vm_end) {
+			offset = stack_map_build_id_offset(cache.vm_pgoff, cache.vm_start, ip);
+			stack_map_build_id_set_valid(&id_offs[i], offset, cache.build_id);
+			continue;
+		}
+
 		vma = stack_map_lock_vma(&lock, ip);
 		if (!vma) {
 			stack_map_build_id_set_ip(&id_offs[i]);
@@ -265,17 +284,47 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
 			continue;
 		}
 
-		file = get_file(vma->vm_file);
-		offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
+		file = vma->vm_file;
+		vm_pgoff = vma->vm_pgoff;
+		vm_start = vma->vm_start;
+		vm_end = vma->vm_end;
+		offset = stack_map_build_id_offset(vm_pgoff, vm_start, ip);
+
+		/*
+		 * Same backing file as previous (e.g. different VMAs
+		 * of the same ELF binary). Reuse the cache build_id.
+		 */
+		if (file == cache.file) {
+			stack_map_unlock_vma(&lock);
+			stack_map_build_id_set_valid(&id_offs[i], offset, cache.build_id);
+			cache.vm_start = vm_start;
+			cache.vm_end = vm_end;
+			cache.vm_pgoff = vm_pgoff;
+			continue;
+		}
+
+		file = get_file(file);
 		stack_map_unlock_vma(&lock);
 
 		/* build_id_parse_file() may block on filesystem reads */
-		if (build_id_parse_file(file, id_offs[i].build_id, NULL))
+		if (build_id_parse_file(file, id_offs[i].build_id, NULL)) {
 			stack_map_build_id_set_ip(&id_offs[i]);
-		else
-			stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
-		fput(file);
+			fput(file);
+			continue;
+		}
+
+		stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
+		if (cache.file)
+			fput(cache.file);
+		cache.file = file;
+		cache.build_id = id_offs[i].build_id;
+		cache.vm_start = vm_start;
+		cache.vm_end = vm_end;
+		cache.vm_pgoff = vm_pgoff;
 	}
+
+	if (cache.file)
+		fput(cache.file);
 }
 
 /*
-- 
2.54.0

Re: [PATCH bpf-next v7 1/3] bpf: Factor out stack_map build ID helpers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The refactored VMA caching logic bypasses the cache for fallback cases and evicts valid VMAs, directly contradicting the patch's stated goal.
--

commit e38559ed451d61a994765f3bfb260cc14edd3569
Author: Ihor Solodrai <ihor.solodrai@linux.dev>

bpf: Factor out stack_map build ID helpers

This patch refactors stack_map_get_build_id_offset() to use helpers and
continue-driven control flow. It also attempts to optimize VMA caching
by updating the cached VMA on the fallback-to-IP branch to prevent
redundant parsing attempts for anonymous or failing VMAs.

Link: https://lore.kernel.org/bpf/CAEf4Bzac9uWWqBvzH0iFzKvJcq3vxscZ3pKm0sUHmN-F-z9wVQ@mail.gmail.com/
Link: https://lore.kernel.org/bpf/226398c1ff3f2b686c0aeb010408d85fb15df13f9ff60a045bee31e79b9e41e9@mail.kernel.org/

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index da3d328f5c15a..e23be7d445038 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c

[ ... ]

> @@ -165,44 +187,45 @@ static int fetch_build_id(struct vm_area_struct *vma, unsigned char *build_id, b
>  static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>  					  u32 trace_nr, bool user, bool may_fault)
>  {

[ ... ]

>  	for (i = 0; i < trace_nr; i++) {
>  		u64 ip = READ_ONCE(id_offs[i].ip);
> +		u64 offset;
>  
> -		if (range_in_vma(prev_vma, ip, ip)) {
> +		if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
>  			vma = prev_vma;
> -			memcpy(id_offs[i].build_id, prev_build_id, BUILD_ID_SIZE_MAX);
> -			goto build_id_valid;
> +			offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> +			stack_map_build_id_set_valid(&id_offs[i], offset, prev_build_id);
> +			continue;
>  		}
>  		vma = find_vma(current->mm, ip);
> -		if (!vma || fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
> +		if (!vma || vma_is_anonymous(vma) ||
> +		    fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
>  			/* per entry fall back to ips */
> -			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> -			memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
> +			stack_map_build_id_set_ip(&id_offs[i]);
> +			prev_vma = vma;
> +			prev_build_id = NULL;

[Severity: Medium]
Does setting prev_build_id to NULL here inadvertently defeat the VMA caching
optimization mentioned in the commit message?

The commit message states the intent is to update prev_vma and prev_build_id
on this fallback branch so the cache reflects the actual VMA seen. However,
the cache check at the top of the loop now requires prev_build_id to be
non-NULL:

    if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {

Since prev_build_id is set to NULL on failure, won't consecutive IPs falling
into the same anonymous or failing VMA bypass the cache completely and trigger
redundant find_vma() lookups?

Also, does overwriting prev_vma with the failing VMA while disabling the
cache hit (via NULL prev_build_id) evict the last successful VMA from the
cache, potentially degrading performance if stack frames interleave?

> +			continue;
>  		}
> -build_id_valid:
> -		id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ip - vma->vm_start;
> -		id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
> +		offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> +		stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
>  		prev_vma = vma;
>  		prev_build_id = id_offs[i].build_id;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525223948.1920986-1-ihor.solodrai@linux.dev?part=1

Re: [PATCH bpf-next v7 1/3] bpf: Factor out stack_map build ID helpers

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 2095 bytes --]

> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index da3d328f5c15..e23be7d44503 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -152,6 +152,28 @@ static int fetch_build_id(struct vm_area_struct *vma, unsigned char *build_id, b
>  			 : build_id_parse_nofault(vma, build_id, NULL);
>  }
>
> +static inline void stack_map_build_id_set_ip(struct bpf_stack_build_id *id)
> +{
> +	id->status = BPF_STACK_BUILD_ID_IP;
> +	memset(id->build_id, 0, BUILD_ID_SIZE_MAX);
> +}
> +
> +static inline u64 stack_map_build_id_offset(unsigned long vm_pgoff,
> +					    unsigned long vm_start, u64 ip)
> +{
> +	return (vm_pgoff << PAGE_SHIFT) + ip - vm_start;
> +}
> +
> +static inline void stack_map_build_id_set_valid(struct bpf_stack_build_id *id,
> +						u64 offset,
> +						const unsigned char *build_id)
> +{
> +	id->status = BPF_STACK_BUILD_ID_VALID;
> +	id->offset = offset;
> +	if (id->build_id != build_id)
> +		memcpy(id->build_id, build_id, BUILD_ID_SIZE_MAX);
> +}

In bpf_get_stack(), the destination buffer can be a BPF map value that is
concurrently readable locklessly by user-space. Since the buffer initially
contains the raw instruction pointer (ip), does setting status to VALID
before writing the offset create a data race?

A concurrent lockless reader polling the status field might observe the
VALID status while the union still holds the raw ip instead of the correct
file offset, and before the build_id string is fully copied.

Prior to this patch, the offset was written before the status. Could this
reordering of assignments cause concurrent readers to observe stale data?

This was raised by sashiko-bot in v6 review and acknowledged by the author
as needing to be fixed in the next version, but was not addressed:

https://lore.kernel.org/bpf/20260521231600.E14FD1F000E9@smtp.kernel.org/

[ ... ]


---
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/26422948694

Re: [PATCH bpf-next v7 1/3] bpf: Factor out stack_map build ID helpers

[Andrii Nakryiko]

On Mon, May 25, 2026 at 4:11 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The refactored VMA caching logic bypasses the cache for fallback cases and evicts valid VMAs, directly contradicting the patch's stated goal.
> --
>
> commit e38559ed451d61a994765f3bfb260cc14edd3569
> Author: Ihor Solodrai <ihor.solodrai@linux.dev>
>
> bpf: Factor out stack_map build ID helpers
>
> This patch refactors stack_map_get_build_id_offset() to use helpers and
> continue-driven control flow. It also attempts to optimize VMA caching
> by updating the cached VMA on the fallback-to-IP branch to prevent
> redundant parsing attempts for anonymous or failing VMAs.
>
> Link: https://lore.kernel.org/bpf/CAEf4Bzac9uWWqBvzH0iFzKvJcq3vxscZ3pKm0sUHmN-F-z9wVQ@mail.gmail.com/
> Link: https://lore.kernel.org/bpf/226398c1ff3f2b686c0aeb010408d85fb15df13f9ff60a045bee31e79b9e41e9@mail.kernel.org/
>
> > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > index da3d328f5c15a..e23be7d445038 100644
> > --- a/kernel/bpf/stackmap.c
> > +++ b/kernel/bpf/stackmap.c
>
> [ ... ]
>
> > @@ -165,44 +187,45 @@ static int fetch_build_id(struct vm_area_struct *vma, unsigned char *build_id, b
> >  static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> >                                         u32 trace_nr, bool user, bool may_fault)
> >  {
>
> [ ... ]
>
> >       for (i = 0; i < trace_nr; i++) {
> >               u64 ip = READ_ONCE(id_offs[i].ip);
> > +             u64 offset;
> >
> > -             if (range_in_vma(prev_vma, ip, ip)) {
> > +             if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
> >                       vma = prev_vma;
> > -                     memcpy(id_offs[i].build_id, prev_build_id, BUILD_ID_SIZE_MAX);
> > -                     goto build_id_valid;
> > +                     offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> > +                     stack_map_build_id_set_valid(&id_offs[i], offset, prev_build_id);
> > +                     continue;
> >               }
> >               vma = find_vma(current->mm, ip);
> > -             if (!vma || fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
> > +             if (!vma || vma_is_anonymous(vma) ||
> > +                 fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
> >                       /* per entry fall back to ips */
> > -                     id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> > -                     memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
> > +                     stack_map_build_id_set_ip(&id_offs[i]);
> > +                     prev_vma = vma;
> > +                     prev_build_id = NULL;
>
> [Severity: Medium]
> Does setting prev_build_id to NULL here inadvertently defeat the VMA caching
> optimization mentioned in the commit message?
>
> The commit message states the intent is to update prev_vma and prev_build_id
> on this fallback branch so the cache reflects the actual VMA seen. However,
> the cache check at the top of the loop now requires prev_build_id to be
> non-NULL:
>
>     if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
>
> Since prev_build_id is set to NULL on failure, won't consecutive IPs falling
> into the same anonymous or failing VMA bypass the cache completely and trigger
> redundant find_vma() lookups?

yeah, it seems so to me... we have prev_build_id and prev_vma, we
should set prev_build_id to NULL if build ID fetching failed, but we
should preserve prev_vma always to avoid unnecessary find_vma() calls.
And if it so happens that we have matching prev_vma, then depending of
prev_build_id being NULL or not we either set absolute ip or
build_id+offset, respectively

Let's do a follow up to improve this, nothing is really broken, I
think, so applying.


>
> Also, does overwriting prev_vma with the failing VMA while disabling the
> cache hit (via NULL prev_build_id) evict the last successful VMA from the
> cache, potentially degrading performance if stack frames interleave?
>
> > +                     continue;
> >               }
> > -build_id_valid:
> > -             id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ip - vma->vm_start;
> > -             id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
> > +             offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> > +             stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
> >               prev_vma = vma;
> >               prev_build_id = id_offs[i].build_id;
> >       }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260525223948.1920986-1-ihor.solodrai@linux.dev?part=1
>

Re: [PATCH bpf-next v7 3/3] bpf: Cache build IDs in sleepable stackmap path

[Andrii Nakryiko]

On Mon, May 25, 2026 at 3:40 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>
> Stack traces often contain adjacent IPs from the same VMA or from
> different VMAs backed by the same ELF file. Cache the last successfully
> parsed build id together with the resolved VMA range and backing file
> so the sleepable build id path can avoid repeated VMA locking and file
> parsing in common cases.
>
> Suggested-by: Mykyta Yatsenko <yatsenko@meta.com>
> Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> ---
>  kernel/bpf/stackmap.c | 61 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index c53cfd9a67cf..77ba03216c09 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -246,6 +246,14 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
>  {
>         struct mm_struct *mm = current->mm;
>         struct stack_map_vma_lock lock = { .mm = mm };
> +       struct {
> +               struct file *file;
> +               const unsigned char *build_id;
> +               unsigned long vm_start;
> +               unsigned long vm_end;
> +               unsigned long vm_pgoff;
> +       } cache = {};
> +       unsigned long vm_pgoff, vm_start, vm_end;
>         struct vm_area_struct *vma;
>         struct file *file;
>         u64 offset;
> @@ -254,6 +262,17 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
>         for (u32 i = 0; i < trace_nr; i++) {
>                 ip = READ_ONCE(id_offs[i].ip);
>
> +               /*
> +                * Range cache fast path: if ip falls within the previously
> +                * resolved VMA range, reuse the cache build_id without
> +                * re-acquiring the VMA lock.
> +                */
> +               if (cache.build_id && ip >= cache.vm_start && ip < cache.vm_end) {
> +                       offset = stack_map_build_id_offset(cache.vm_pgoff, cache.vm_start, ip);
> +                       stack_map_build_id_set_valid(&id_offs[i], offset, cache.build_id);
> +                       continue;
> +               }
> +

Same issue as with patch #1, this is a lost opportunity to avoid
unnecessary work if VMA has no matching build ID. We should do
negative caching (i.e., have build_id set to NULL, but use
vm_start/vm_end for subsequent ip matches) and avoid unnecessary vma
lookups. Let's treat it as optimization and address this in a follow
up patch.

>                 vma = stack_map_lock_vma(&lock, ip);
>                 if (!vma) {
>                         stack_map_build_id_set_ip(&id_offs[i]);
> @@ -265,17 +284,47 @@ static void stack_map_get_build_id_offset_sleepable(struct bpf_stack_build_id *i
>                         continue;
>                 }
>
> -               file = get_file(vma->vm_file);
> -               offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
> +               file = vma->vm_file;
> +               vm_pgoff = vma->vm_pgoff;
> +               vm_start = vma->vm_start;
> +               vm_end = vma->vm_end;
> +               offset = stack_map_build_id_offset(vm_pgoff, vm_start, ip);
> +
> +               /*
> +                * Same backing file as previous (e.g. different VMAs
> +                * of the same ELF binary). Reuse the cache build_id.
> +                */
> +               if (file == cache.file) {
> +                       stack_map_unlock_vma(&lock);
> +                       stack_map_build_id_set_valid(&id_offs[i], offset, cache.build_id);
> +                       cache.vm_start = vm_start;
> +                       cache.vm_end = vm_end;
> +                       cache.vm_pgoff = vm_pgoff;
> +                       continue;
> +               }
> +
> +               file = get_file(file);
>                 stack_map_unlock_vma(&lock);
>
>                 /* build_id_parse_file() may block on filesystem reads */
> -               if (build_id_parse_file(file, id_offs[i].build_id, NULL))
> +               if (build_id_parse_file(file, id_offs[i].build_id, NULL)) {
>                         stack_map_build_id_set_ip(&id_offs[i]);
> -               else
> -                       stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
> -               fput(file);
> +                       fput(file);

we should probably negative-cache this lack of build ID and update
vm_start/vm_end here, in a follow up


> +                       continue;
> +               }
> +
> +               stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
> +               if (cache.file)
> +                       fput(cache.file);
> +               cache.file = file;
> +               cache.build_id = id_offs[i].build_id;
> +               cache.vm_start = vm_start;
> +               cache.vm_end = vm_end;
> +               cache.vm_pgoff = vm_pgoff;
>         }
> +
> +       if (cache.file)
> +               fput(cache.file);
>  }
>
>  /*
> --
> 2.54.0
>

Re: [PATCH bpf-next v7 0/3] bpf: Implement stack_map_get_build_id_offset_sleepable()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Mon, 25 May 2026 15:39:45 -0700 you wrote:
> The series introduces stack_map_get_build_id_offset_sleepable(),
> fixing a gap with parsing build_id in sleepable context in stackmap.c
> 
> In particular, this fixes a deadlock in
> stack_map_get_build_id_offset() doing a blocking __kernel_read(),
> which happens since commit 777a8560fd29 ("lib/buildid: use
> __kernel_read() for sleepable context").
> 
> [...]

Here is the summary with links:
  - [bpf-next,v7,1/3] bpf: Factor out stack_map build ID helpers
    https://git.kernel.org/bpf/bpf-next/c/fc99547a8bda
  - [bpf-next,v7,2/3] bpf: Avoid faultable build ID reads under mm locks
    https://git.kernel.org/bpf/bpf-next/c/fad3021faf7b
  - [bpf-next,v7,3/3] bpf: Cache build IDs in sleepable stackmap path
    https://git.kernel.org/bpf/bpf-next/c/5e9099d8ff24

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next v7 1/3] bpf: Factor out stack_map build ID helpers

[Ihor Solodrai]

On 2026-05-28 2:57 p.m., Andrii Nakryiko wrote:
> On Mon, May 25, 2026 at 4:11 PM <sashiko-bot@kernel.org> wrote:
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [Medium] The refactored VMA caching logic bypasses the cache for fallback cases and evicts valid VMAs, directly contradicting the patch's stated goal.
>> --
>>
>> commit e38559ed451d61a994765f3bfb260cc14edd3569
>> Author: Ihor Solodrai <ihor.solodrai@linux.dev>
>>
>> bpf: Factor out stack_map build ID helpers
>>
>> This patch refactors stack_map_get_build_id_offset() to use helpers and
>> continue-driven control flow. It also attempts to optimize VMA caching
>> by updating the cached VMA on the fallback-to-IP branch to prevent
>> redundant parsing attempts for anonymous or failing VMAs.
>>
>> Link: https://lore.kernel.org/bpf/CAEf4Bzac9uWWqBvzH0iFzKvJcq3vxscZ3pKm0sUHmN-F-z9wVQ@mail.gmail.com/
>> Link: https://lore.kernel.org/bpf/226398c1ff3f2b686c0aeb010408d85fb15df13f9ff60a045bee31e79b9e41e9@mail.kernel.org/
>>
>>> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
>>> index da3d328f5c15a..e23be7d445038 100644
>>> --- a/kernel/bpf/stackmap.c
>>> +++ b/kernel/bpf/stackmap.c
>>
>> [ ... ]
>>
>>> @@ -165,44 +187,45 @@ static int fetch_build_id(struct vm_area_struct *vma, unsigned char *build_id, b
>>>   static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
>>>                                          u32 trace_nr, bool user, bool may_fault)
>>>   {
>>
>> [ ... ]
>>
>>>        for (i = 0; i < trace_nr; i++) {
>>>                u64 ip = READ_ONCE(id_offs[i].ip);
>>> +             u64 offset;
>>>
>>> -             if (range_in_vma(prev_vma, ip, ip)) {
>>> +             if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
>>>                        vma = prev_vma;
>>> -                     memcpy(id_offs[i].build_id, prev_build_id, BUILD_ID_SIZE_MAX);
>>> -                     goto build_id_valid;
>>> +                     offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
>>> +                     stack_map_build_id_set_valid(&id_offs[i], offset, prev_build_id);
>>> +                     continue;
>>>                }
>>>                vma = find_vma(current->mm, ip);
>>> -             if (!vma || fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
>>> +             if (!vma || vma_is_anonymous(vma) ||
>>> +                 fetch_build_id(vma, id_offs[i].build_id, may_fault)) {
>>>                        /* per entry fall back to ips */
>>> -                     id_offs[i].status = BPF_STACK_BUILD_ID_IP;
>>> -                     memset(id_offs[i].build_id, 0, BUILD_ID_SIZE_MAX);
>>> +                     stack_map_build_id_set_ip(&id_offs[i]);
>>> +                     prev_vma = vma;
>>> +                     prev_build_id = NULL;
>>
>> [Severity: Medium]
>> Does setting prev_build_id to NULL here inadvertently defeat the VMA caching
>> optimization mentioned in the commit message?
>>
>> The commit message states the intent is to update prev_vma and prev_build_id
>> on this fallback branch so the cache reflects the actual VMA seen. However,
>> the cache check at the top of the loop now requires prev_build_id to be
>> non-NULL:
>>
>>      if (prev_build_id && range_in_vma(prev_vma, ip, ip)) {
>>
>> Since prev_build_id is set to NULL on failure, won't consecutive IPs falling
>> into the same anonymous or failing VMA bypass the cache completely and trigger
>> redundant find_vma() lookups?
> 
> yeah, it seems so to me... we have prev_build_id and prev_vma, we
> should set prev_build_id to NULL if build ID fetching failed, but we
> should preserve prev_vma always to avoid unnecessary find_vma() calls.
> And if it so happens that we have matching prev_vma, then depending of
> prev_build_id being NULL or not we either set absolute ip or
> build_id+offset, respectively
> 
> Let's do a follow up to improve this, nothing is really broken, I
> think, so applying.

Hi Andrii, thanks for applying.

I'll send a follow up as soon as I can.

> 
> 
>>
>> Also, does overwriting prev_vma with the failing VMA while disabling the
>> cache hit (via NULL prev_build_id) evict the last successful VMA from the
>> cache, potentially degrading performance if stack frames interleave?
>>
>>> +                     continue;
>>>                }
>>> -build_id_valid:
>>> -             id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ip - vma->vm_start;
>>> -             id_offs[i].status = BPF_STACK_BUILD_ID_VALID;
>>> +             offset = stack_map_build_id_offset(vma->vm_pgoff, vma->vm_start, ip);
>>> +             stack_map_build_id_set_valid(&id_offs[i], offset, id_offs[i].build_id);
>>>                prev_vma = vma;
>>>                prev_build_id = id_offs[i].build_id;
>>>        }
>>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/20260525223948.1920986-1-ihor.solodrai@linux.dev?part=1
>>