bpf: Disable preemption in stack map code
Subsystems (3)
Jiri Olsa
reviewercover bpf: Disable preemption in stack map code
hi,
we need to disable preemption for get_perf_callchain and keep it disabled as long as we are accessing its returned trace entries buffer.
This patchset refactors both bpf_get_stack and bpf_get_stackid helpers as suggested by Andrii [1] before applying the actual preemption fix.
Note the initial fix was sent by Tao Chen [2], but there was no follow up on this since February, hence this post.
thanks,
jirka
[1] https://lore.kernel.org/bpf/CAEf4BzZwvAUgLwz-M0Y_NJLTmedyY9U6s7LrSmn751hQdTP4Uw@mail.gmail.com/
[2] https://lore.kernel.org/bpf/[email protected]/
---
Daniel Borkmann (1):
bpf: Disable preemption in __bpf_get_stack
Jiri Olsa (8):
bpf: Factor stackid_init function from __bpf_get_stackid
bpf: Factor stackid_fastpath function from __bpf_get_stackid
bpf: Factor stackid_new_bucket from __bpf_get_stackid
bpf: Use stack id functions instead of __bpf_get_stackid
bpf: Disable preemption in bpf_get_stackid
bpf: Factor callchain_store function from __bpf_get_stack
bpf: Factor callchain_finalize function from __bpf_get_stack
bpf: Remove trace_in argument from __bpf_get_stack
kernel/bpf/stackmap.c | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------- 1 file changed, 198 insertions(+), 94 deletions(-)
Jiri Olsa
reviewerPATCH 1/9 bpf: Factor stackid_init function from __bpf_get_stackid
The new stackid_init function stores all the necessary bits for stackid trace and it will be used by other functions in following changes.
--- kernel/bpf/stackmap.c | 95 +++++++++++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 36 deletions(-)
kernel/bpf/stackmap.c +59 -36
@@ -504,33 +504,54 @@ get_callchain_entry_for_task(struct task_struct *task, u32 max_depth)#endif } -static long __bpf_get_stackid (struct bpf_map *map, -struct perf_callchain_entry *trace,u64 flags) +struct stackid { +struct stack_map_bucket *bucket; +u64 *ips; +u32 nr; +u32 len; +u32 hash; +u32 id; +}; + +static int stackid_init (struct stackid *stackid,struct bpf_map *map, +struct perf_callchain_entry *trace,u64 flags) { struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); -struct stack_map_bucket *bucket, *new_bucket, *old_bucket; -u32 hash, id, trace_nr, trace_len, i, max_depth;u32 skip = flags &BPF_F_SKIP_FIELD_MASK ; -bool user = flags &BPF_F_USER_STACK ; -u64 *ips; -bool hash_matches; +u32 max_depth;if (trace->nr <= skip)/* skipping more than usable stack trace */ return -EFAULT ; max_depth =stack_map_calculate_max_depth (map->value_size,stack_map_data_size (map), flags); - trace_nr =min_t (u32, trace->nr - skip, max_depth - skip); - trace_len = trace_nr *sizeof (u64); - ips = trace->ip + skip; - hash =jhash2 ((u32 *)ips, trace_len /sizeof (u32),0 ); - id = hash & (smap->n_buckets -1 ); - bucket =READ_ONCE (smap->buckets[id]); + stackid->nr =min_t (u32, trace->nr - skip, max_depth - skip); + stackid->len = stackid->nr *sizeof (u64); + stackid->ips = trace->ip + skip; + stackid->hash =jhash2 ((u32 *)stackid->ips, stackid->len /sizeof (u32),0 ); + stackid->id = stackid->hash & (smap->n_buckets -1 ); + stackid->bucket =READ_ONCE (smap->buckets[stackid->id]); +return 0 ; +} - hash_matches = bucket && bucket->hash == hash; +static long __bpf_get_stackid (struct stackid *stackid,struct bpf_map *map, +struct perf_callchain_entry *trace,u64 flags) +{ + struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); +struct stack_map_bucket *new_bucket, *old_bucket; +bool user = flags &BPF_F_USER_STACK ; +bool hash_matches; +u32 trace_len, i; +int err; + + err =stackid_init (stackid, map, trace, flags); +if (err) +return err; + + hash_matches = stackid->bucket && stackid->bucket->hash == stackid->hash;/* fast cmp */ if (hash_matches && flags &BPF_F_FAST_STACK_CMP ) -return id; +return stackid->id;if (stack_map_use_build_id (map)) {struct bpf_stack_build_id *id_offs;
@@ -540,42 +561,42 @@ static long __bpf_get_stackid(struct bpf_map *map,pcpu_freelist_pop (&smap->freelist);if (unlikely (!new_bucket))return -ENOMEM ; - new_bucket->nr = trace_nr; + new_bucket->nr = stackid->nr; id_offs = (struct bpf_stack_build_id *)new_bucket->data; -for (i =0 ; i < trace_nr; i++) - id_offs[i].ip = ips[i]; -stack_map_get_build_id_offset (id_offs, trace_nr, user, false/* !may_fault */ ); - trace_len = trace_nr *sizeof (struct bpf_stack_build_id ); -if (hash_matches && bucket->nr == trace_nr && -memcmp (bucket->data, new_bucket->data, trace_len) ==0 ) { +for (i =0 ; i < stackid->nr; i++) + id_offs[i].ip = stackid->ips[i]; +stack_map_get_build_id_offset (id_offs, stackid->nr, user, false/* !may_fault */ ); + trace_len = stackid->nr *sizeof (struct bpf_stack_build_id ); +if (hash_matches && stackid->bucket->nr == stackid->nr && +memcmp (stackid->bucket->data, new_bucket->data, trace_len) ==0 ) {pcpu_freelist_push (&smap->freelist, &new_bucket->fnode); -return id; +return stackid->id; } -if (bucket && !(flags &BPF_F_REUSE_STACKID )) { +if (stackid->bucket && !(flags &BPF_F_REUSE_STACKID )) {pcpu_freelist_push (&smap->freelist, &new_bucket->fnode);return -EEXIST ; } }else { -if (hash_matches && bucket->nr == trace_nr && -memcmp (bucket->data, ips, trace_len) ==0 ) -return id; -if (bucket && !(flags &BPF_F_REUSE_STACKID )) +if (hash_matches && stackid->bucket->nr == stackid->nr && +memcmp (stackid->bucket->data, stackid->ips, stackid->len) ==0 ) +return stackid->id; +if (stackid->bucket && !(flags &BPF_F_REUSE_STACKID ))return -EEXIST ; new_bucket = (struct stack_map_bucket *)pcpu_freelist_pop (&smap->freelist);if (unlikely (!new_bucket))return -ENOMEM ; -memcpy (new_bucket->data, ips, trace_len); +memcpy (new_bucket->data, stackid->ips, stackid->len); } - new_bucket->hash = hash; - new_bucket->nr = trace_nr; + new_bucket->hash = stackid->hash; + new_bucket->nr = stackid->nr; - old_bucket =xchg (&smap->buckets[id], new_bucket); + old_bucket =xchg (&smap->buckets[stackid->id], new_bucket);if (old_bucket)pcpu_freelist_push (&smap->freelist, &old_bucket->fnode); -return id; +return stackid->id; } BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map,
@@ -584,6 +605,7 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map,u32 elem_size =stack_map_data_size (map);bool user = flags &BPF_F_USER_STACK ;struct perf_callchain_entry *trace; +struct stackid stackid;bool kernel = !user;u32 max_depth;
@@ -599,7 +621,7 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map,/* couldn't fetch the stack trace */ return -EFAULT ; -return __bpf_get_stackid (map, trace, flags); +return __bpf_get_stackid (&stackid, map, trace, flags); }const struct bpf_func_proto bpf_get_stackid_proto = {
@@ -628,6 +650,7 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, {struct perf_event *event = ctx->event;struct perf_callchain_entry *trace; +struct stackid stackid;bool kernel, user;__u64 nr_kernel;int ret;
@@ -653,7 +676,7 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx,if (kernel) { trace->nr = nr_kernel; - ret =__bpf_get_stackid (map, trace, flags); + ret =__bpf_get_stackid (&stackid, map, trace, flags); }else {/* user */ u64 skip = flags &BPF_F_SKIP_FIELD_MASK ;
@@ -662,7 +685,7 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx,return -EFAULT ; flags = (flags & ~BPF_F_SKIP_FIELD_MASK ) | skip; - ret =__bpf_get_stackid (map, trace, flags); + ret =__bpf_get_stackid (&stackid, map, trace, flags); }/* restore nr */
Signature · 1 line
2.54.0
Jiri Olsa
reviewerPATCH 2/9 bpf: Factor stackid_fastpath function from __bpf_get_stackid
In reply to Jiri OlsaThe new stackid_fastpath does the fast stack hash and trace check, that does not need new bucket allocation. It covers both just-ip and buildid code paths.
--- kernel/bpf/stackmap.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-)
kernel/bpf/stackmap.c +28 -15
@@ -511,6 +511,7 @@ struct stackid {u32 len;u32 hash;u32 id; +bool hash_matches; }; static int stackid_init(struct stackid *stackid, struct bpf_map *map,
@@ -531,28 +532,46 @@ static int stackid_init(struct stackid *stackid, struct bpf_map *map, stackid->hash =jhash2 ((u32 *)stackid->ips, stackid->len /sizeof (u32),0 ); stackid->id = stackid->hash & (smap->n_buckets -1 ); stackid->bucket =READ_ONCE (smap->buckets[stackid->id]); + stackid->hash_matches = stackid->bucket && stackid->bucket->hash == stackid->hash;return 0 ; } +static int stackid_fastpath (struct stackid *stackid,struct bpf_map *map, +struct perf_callchain_entry *trace,u64 flags) +{ +int err; + + err =stackid_init (stackid, map, trace, flags); +if (err) +return err; + +/* fast cmp */ +if (stackid->hash_matches && flags &BPF_F_FAST_STACK_CMP ) +return stackid->id; + +if (stack_map_use_build_id (map)) +return -ENOENT ; +if (stackid->hash_matches && stackid->bucket->nr == stackid->nr && +memcmp (stackid->bucket->data, stackid->ips, stackid->len) ==0 ) +return stackid->id; +if (stackid->bucket && !(flags &BPF_F_REUSE_STACKID )) +return -EEXIST ; +return -ENOENT ; +} +static long __bpf_get_stackid (struct stackid *stackid,struct bpf_map *map,struct perf_callchain_entry *trace,u64 flags) { struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map);struct stack_map_bucket *new_bucket, *old_bucket;bool user = flags &BPF_F_USER_STACK ; -bool hash_matches;u32 trace_len, i;int err; - err =stackid_init (stackid, map, trace, flags); -if (err) + err =stackid_fastpath (stackid, map, trace, flags); +if (err != -ENOENT )return err; - hash_matches = stackid->bucket && stackid->bucket->hash == stackid->hash; -/* fast cmp */ -if (hash_matches && flags &BPF_F_FAST_STACK_CMP ) -return stackid->id; -if (stack_map_use_build_id (map)) {struct bpf_stack_build_id *id_offs;
@@ -567,7 +586,7 @@ static long __bpf_get_stackid(struct stackid *stackid, struct bpf_map *map, id_offs[i].ip = stackid->ips[i];stack_map_get_build_id_offset (id_offs, stackid->nr, user, false/* !may_fault */ ); trace_len = stackid->nr *sizeof (struct bpf_stack_build_id ); -if (hash_matches && stackid->bucket->nr == stackid->nr && +if (stackid->hash_matches && stackid->bucket->nr == stackid->nr &&memcmp (stackid->bucket->data, new_bucket->data, trace_len) ==0 ) {pcpu_freelist_push (&smap->freelist, &new_bucket->fnode);return stackid->id;
@@ -577,12 +596,6 @@ static long __bpf_get_stackid(struct stackid *stackid, struct bpf_map *map,return -EEXIST ; } }else { -if (hash_matches && stackid->bucket->nr == stackid->nr && -memcmp (stackid->bucket->data, stackid->ips, stackid->len) ==0 ) -return stackid->id; -if (stackid->bucket && !(flags &BPF_F_REUSE_STACKID )) -return -EEXIST ; - new_bucket = (struct stack_map_bucket *)pcpu_freelist_pop (&smap->freelist);if (unlikely (!new_bucket))
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH bpf-next 2/9] bpf: Factor stackid_fastpath function from __bpf_get_stackid
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] A pre-existing TOCTOU race condition in stackid_init involving sysctl_perf_event_max_stack can cause a max_depth - skip underflow, leading to a kernel heap buffer overflow. --
--- Patch [2]: [PATCH bpf-next 2/9] bpf: Factor stackid_fastpath function from __bpf_get_stackid --- commit 1a0215d43cbe727ac7583d25380f0acdc326abc9
Author: Jiri Olsa <[email protected]>
bpf: Factor stackid_fastpath function from __bpf_get_stackid
This commit extracts the fast stack hash and trace check logic from __bpf_get_stackid into a new stackid_fastpath function. It optimizes both the just-ip and buildid code paths by avoiding new bucket allocation during the fast path check.
kernel/bpf/stackmap.c static int stackid_init(struct stackid *stackid, struct bpf_map *map, · line 537
Quoted 11 lines
kernel/bpf/stackmap.c +1
-0@@ -531,28 +532,46 @@ static int stackid_init(struct stackid *stackid, struct bpf_map *map, stackid->hash =jhash2 ((u32 *)stackid->ips, stackid->len /sizeof (u32),0 ); stackid->id = stackid->hash & (smap->n_buckets -1 ); stackid->bucket =READ_ONCE (smap->buckets[stackid->id]); + stackid->hash_matches = stackid->bucket && stackid->bucket->hash == stackid->hash;return 0 ; }
[Severity: High]
This isn't a bug introduced by this patch, but could the stackid calculation in this function underflow and cause an out-of-bounds write?
Just above this diff in stackid_init(), stackid->nr is calculated like this:
max_depth = stack_map_calculate_max_depth(map->value_size, stack_map_data_size(map), flags); stackid->nr = min_t(u32, trace->nr - skip, max_depth - skip);
If sysctl_perf_event_max_stack is lowered concurrently while a BPF program is executing, max_depth gets capped to the new smaller sysctl value.
If the BPF program provides a skip value larger than the new sysctl but smaller than the old one, max_depth - skip will underflow to a large u32 value.
The min_t() would then incorrectly select trace->nr - skip, resulting in an unsafely large stackid->len.
Later, this length is used to write into a bucket that was allocated based on the smaller map->value_size:
memcpy(new_bucket->data, stackid->ips, stackid->len);
Does this sequence allow a kernel heap buffer overflow to occur?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH bpf-next 2/9] bpf: Factor stackid_fastpath function from __bpf_get_stackid
Quoted 35 lines · On Mon, Jul 20, 2026 at 09:09:51AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] A pre-existing TOCTOU race condition instackid_initinvolvingsysctl_perf_event_max_stackcan cause amax_depth - skipunderflow, leading to a kernel heap buffer overflow. ----- Patch [2]: [PATCH bpf-next 2/9] bpf: Factor
stackid_fastpathfunction from__bpf_get_stackid--- commit1a0215d43cbe727ac7583d25380f0acdc326abc9
Author: Jiri Olsa <[email protected]>bpf: Factor
stackid_fastpathfunction from__bpf_get_stackidThis commit extracts the fast stack hash and trace check logic from
__bpf_get_stackidinto a newstackid_fastpathfunction. It optimizes both the just-ip and buildid code paths by avoiding new bucket allocation during the fast path check.kernel/bpf/stackmap.c static int stackid_init(struct stackid *stackid, struct bpf_map *map, · line 537
Quoted 11 lines
kernel/bpf/stackmap.c +1
-0@@ -531,28 +532,46 @@ static int stackid_init(struct stackid *stackid, struct bpf_map *map, stackid->hash =jhash2 ((u32 *)stackid->ips, stackid->len /sizeof (u32),0 ); stackid->id = stackid->hash & (smap->n_buckets -1 ); stackid->bucket =READ_ONCE (smap->buckets[stackid->id]); + stackid->hash_matches = stackid->bucket && stackid->bucket->hash == stackid->hash;return 0 ; }[Severity: High]
This isn't a bug introduced by this patch, but could the stackid calculation in this function underflow and cause an out-of-bounds write?Just above this diff in
stackid_init(),stackid->nris calculated like this:
Quoted 7 lines
max_depth = stack_map_calculate_max_depth(map->value_size, stack_map_data_size(map), flags); stackid->nr = min_t(u32, trace->nr - skip, max_depth - skip);If
sysctl_perf_event_max_stackis lowered concurrently while a BPF program is executing,max_depthgets capped to the new smaller sysctl value.
that can't happen, perf_event_max_stack_handler needs zero in nr_callchain_events to change the sysctl_perf_event_max_stack, but it's held by stackmap
jirka
Quoted 17 lines
If the BPF program provides a skip value larger than the new sysctl but smaller than the old one,
max_depth- skip will underflow to a large u32 value.The
min_t()would then incorrectly selecttrace->nr- skip, resulting in an unsafely largestackid->len.Later, this length is used to write into a bucket that was allocated based on the smaller
map->value_size:memcpy(new_bucket->data, stackid->ips, stackid->len);Does this sequence allow a kernel heap buffer overflow to occur?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Jiri Olsa
reviewerPATCH 3/9 bpf: Factor stackid_new_bucket from __bpf_get_stackid
In reply to Jiri OlsaThe new stackid_new_bucket allocates the new bucket and initializes it with the trace data.
--- kernel/bpf/stackmap.c | 49 +++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 18 deletions(-)
kernel/bpf/stackmap.c +31 -18
@@ -559,31 +559,53 @@ static int stackid_fastpath(struct stackid *stackid, struct bpf_map *map,return -ENOENT ; } +static struct stack_map_bucket * +stackid_new_bucket (struct stackid *stackid,struct bpf_map *map, +struct perf_callchain_entry *trace,u64 flags) +{ + struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); +struct bpf_stack_build_id *id_offs; +struct stack_map_bucket *bucket; +u32 i; + + bucket = (struct stack_map_bucket *)pcpu_freelist_pop (&smap->freelist); +if (unlikely (!bucket)) +return NULL ; + +if (stack_map_use_build_id (map)) { + id_offs = (struct bpf_stack_build_id *)bucket->data; +for (i =0 ; i < stackid->nr; i++) + id_offs[i].ip = stackid->ips[i]; + }else { +memcpy (bucket->data, stackid->ips, stackid->len); + } + + bucket->hash = stackid->hash; + bucket->nr = stackid->nr; +return bucket; +} +static long __bpf_get_stackid (struct stackid *stackid,struct bpf_map *map,struct perf_callchain_entry *trace,u64 flags) { struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map);struct stack_map_bucket *new_bucket, *old_bucket;bool user = flags &BPF_F_USER_STACK ; -u32 trace_len, i; +u32 trace_len;int err; err =stackid_fastpath (stackid, map, trace, flags);if (err != -ENOENT )return err; + new_bucket =stackid_new_bucket (stackid, map, trace, flags); +if (!new_bucket) +return -ENOMEM ; +if (stack_map_use_build_id (map)) {struct bpf_stack_build_id *id_offs; -/* for build_id+offset, pop a bucket before slow cmp */ - new_bucket = (struct stack_map_bucket *) -pcpu_freelist_pop (&smap->freelist); -if (unlikely (!new_bucket)) -return -ENOMEM ; - new_bucket->nr = stackid->nr; id_offs = (struct bpf_stack_build_id *)new_bucket->data; -for (i =0 ; i < stackid->nr; i++) - id_offs[i].ip = stackid->ips[i];stack_map_get_build_id_offset (id_offs, stackid->nr, user, false/* !may_fault */ ); trace_len = stackid->nr *sizeof (struct bpf_stack_build_id ); if (stackid->hash_matches && stackid->bucket->nr == stackid->nr &&
@@ -595,17 +617,8 @@ static long __bpf_get_stackid(struct stackid *stackid, struct bpf_map *map,pcpu_freelist_push (&smap->freelist, &new_bucket->fnode);return -EEXIST ; } - }else { - new_bucket = (struct stack_map_bucket *) -pcpu_freelist_pop (&smap->freelist); -if (unlikely (!new_bucket)) -return -ENOMEM ; -memcpy (new_bucket->data, stackid->ips, stackid->len); } - new_bucket->hash = stackid->hash; - new_bucket->nr = stackid->nr; - old_bucket =xchg (&smap->buckets[stackid->id], new_bucket);if (old_bucket)pcpu_freelist_push (&smap->freelist, &old_bucket->fnode);
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH bpf-next 3/9] bpf: Factor stackid_new_bucket from __bpf_get_stackid
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The newly introduced `stackid_new_bucket` function accepts `trace` and `flags` parameters which are…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly introduced stackid_new_bucket function accepts trace and flags parameters which are completely unused within its body. --
--- Patch [3]: [PATCH bpf-next 3/9] bpf: Factor stackid_new_bucket from __bpf_get_stackid --- commit cee0ea77a9c1144b54fb0472b1788623044b5650
Author: Jiri Olsa <[email protected]>
bpf: Factor stackid_new_bucket from __bpf_get_stackid
This commit factors out the bucket allocation and initialization logic from __bpf_get_stackid into a new stackid_new_bucket helper function.
kernel/bpf/stackmap.c static int stackid_fastpath(struct stackid *stackid, struct bpf_map *map, · line 564
Quoted 11 lines
kernel/bpf/stackmap.c +3
-0@@ -559,31 +559,53 @@ static int stackid_fastpath(struct stackid *stackid, struct bpf_map *map,return -ENOENT ; } +static struct stack_map_bucket * +stackid_new_bucket (struct stackid *stackid,struct bpf_map *map, +struct perf_callchain_entry *trace,u64 flags)
[Severity: Low]
Are the trace and flags parameters needed here?
Looking at the implementation of stackid_new_bucket() below, neither trace nor flags are used anywhere in the function body. Could they be removed from the function signature to clean up the API?
Quoted 22 lines
kernel/bpf/stackmap.c +22
-0+{ + struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); +struct bpf_stack_build_id *id_offs; +struct stack_map_bucket *bucket; +u32 i; + + bucket = (struct stack_map_bucket *)pcpu_freelist_pop (&smap->freelist); +if (unlikely (!bucket)) +return NULL ; + +if (stack_map_use_build_id (map)) { + id_offs = (struct bpf_stack_build_id *)bucket->data; +for (i =0 ; i < stackid->nr; i++) + id_offs[i].ip = stackid->ips[i]; + }else { +memcpy (bucket->data, stackid->ips, stackid->len); + } + + bucket->hash = stackid->hash; + bucket->nr = stackid->nr; +return bucket; +}
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH bpf-next 3/9] bpf: Factor stackid_new_bucket from __bpf_get_stackid
Quoted 28 lines · On Mon, Jul 20, 2026 at 09:01:40AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly introducedstackid_new_bucketfunction acceptstraceandflagsparameters which are completely unused within its body. ----- Patch [3]: [PATCH bpf-next 3/9] bpf: Factor
stackid_new_bucketfrom__bpf_get_stackid--- commitcee0ea77a9c1144b54fb0472b1788623044b5650
Author: Jiri Olsa <[email protected]>bpf: Factor
stackid_new_bucketfrom__bpf_get_stackidThis commit factors out the bucket allocation and initialization logic from
__bpf_get_stackidinto a newstackid_new_buckethelper function.kernel/bpf/stackmap.c static int stackid_fastpath(struct stackid *stackid, struct bpf_map *map, · line 564
Quoted 11 lines
kernel/bpf/stackmap.c +3
-0@@ -559,31 +559,53 @@ static int stackid_fastpath(struct stackid *stackid, struct bpf_map *map,return -ENOENT ; } +static struct stack_map_bucket * +stackid_new_bucket (struct stackid *stackid,struct bpf_map *map, +struct perf_callchain_entry *trace,u64 flags)[Severity: Low]
Are the trace and flags parameters needed here?
Quoted 3 lines
Looking at the implementation of
stackid_new_bucket()below, neither trace nor flags are used anywhere in the function body. Could they be removed from the function signature to clean up the API?
right, will remove
jirka
Quoted 28 lines
Quoted 23 lines
kernel/bpf/stackmap.c +22
-0+{ + struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); +struct bpf_stack_build_id *id_offs; +struct stack_map_bucket *bucket; +u32 i; + + bucket = (struct stack_map_bucket *)pcpu_freelist_pop (&smap->freelist); +if (unlikely (!bucket)) +return NULL ; + +if (stack_map_use_build_id (map)) { + id_offs = (struct bpf_stack_build_id *)bucket->data; +for (i =0 ; i < stackid->nr; i++) + id_offs[i].ip = stackid->ips[i]; + }else { +memcpy (bucket->data, stackid->ips, stackid->len); + } + + bucket->hash = stackid->hash; + bucket->nr = stackid->nr; +return bucket; +}[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Jiri Olsa
reviewerPATCH 4/9 bpf: Use stack id functions instead of __bpf_get_stackid
In reply to Jiri OlsaReplacing __bpf_get_stackid calls with sequence of following functions:
stackid_fastpath
stackid_new_bucket
stackid_install
This makes code more structured and allows us to easily disable preemption only in bpf_get_stackid in following changes.
--- kernel/bpf/stackmap.c | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-)
kernel/bpf/stackmap.c +27 -16
@@ -585,22 +585,13 @@ stackid_new_bucket(struct stackid *stackid, struct bpf_map *map,return bucket; } -static long __bpf_get_stackid (struct stackid *stackid,struct bpf_map *map, -struct perf_callchain_entry *trace,u64 flags) +static long stackid_install (struct stackid *stackid,struct bpf_map *map, +struct stack_map_bucket *new_bucket,u64 flags) { struct bpf_stack_map *smap = container_of(map, struct bpf_stack_map, map); -struct stack_map_bucket *new_bucket, *old_bucket;bool user = flags &BPF_F_USER_STACK ; +struct stack_map_bucket *old_bucket;u32 trace_len; -int err; - - err =stackid_fastpath (stackid, map, trace, flags); -if (err != -ENOENT ) -return err; - - new_bucket =stackid_new_bucket (stackid, map, trace, flags); -if (!new_bucket) -return -ENOMEM ;if (stack_map_use_build_id (map)) {struct bpf_stack_build_id *id_offs;
@@ -630,10 +621,12 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, {u32 elem_size =stack_map_data_size (map);bool user = flags &BPF_F_USER_STACK ; +struct stack_map_bucket *new_bucket;struct perf_callchain_entry *trace;struct stackid stackid;bool kernel = !user;u32 max_depth; +int err;if (unlikely (flags & ~(BPF_F_SKIP_FIELD_MASK |BPF_F_USER_STACK |BPF_F_FAST_STACK_CMP |BPF_F_REUSE_STACKID )))
@@ -647,7 +640,15 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map,/* couldn't fetch the stack trace */ return -EFAULT ; -return __bpf_get_stackid (&stackid, map, trace, flags); + err =stackid_fastpath (&stackid, map, trace, flags); +if (err != -ENOENT ) +return err; + + new_bucket =stackid_new_bucket (&stackid, map, trace, flags); +if (!new_bucket) +return -ENOMEM ; + +return stackid_install (&stackid, map, new_bucket, flags); }const struct bpf_func_proto bpf_get_stackid_proto = {
@@ -675,6 +676,7 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx, struct bpf_map *, map, u64, flags) {struct perf_event *event = ctx->event; +struct stack_map_bucket *new_bucket;struct perf_callchain_entry *trace;struct stackid stackid;bool kernel, user;
@@ -702,7 +704,6 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx,if (kernel) { trace->nr = nr_kernel; - ret =__bpf_get_stackid (&stackid, map, trace, flags); }else {/* user */ u64 skip = flags &BPF_F_SKIP_FIELD_MASK ;
@@ -711,12 +712,22 @@ BPF_CALL_3(bpf_get_stackid_pe, struct bpf_perf_event_data_kern *, ctx,return -EFAULT ; flags = (flags & ~BPF_F_SKIP_FIELD_MASK ) | skip; - ret =__bpf_get_stackid (&stackid, map, trace, flags); } + ret =stackid_fastpath (&stackid, map, trace, flags); +if (ret != -ENOENT ) + goto error; + + new_bucket =stackid_new_bucket (&stackid, map, trace, flags); +if (new_bucket) { + trace->nr = nr; +return stackid_install (&stackid, map, new_bucket, flags); + } + ret = -ENOMEM ; + +error:/* restore nr */ trace->nr = nr; -return ret; }
Signature · 1 line
2.54.0
Jiri Olsa
reviewerPATCH 5/9 bpf: Disable preemption in bpf_get_stackid
In reply to Jiri OlsaThe get_perf_callchain call needs disabled preemption plus we need it disabled as long as we access its returned trace entries buffer.
Note the bpf_get_stackid_pe function is executed already with preemption disabled.
--- kernel/bpf/stackmap.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
kernel/bpf/stackmap.c +13 -6
@@ -625,30 +625,37 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map,struct perf_callchain_entry *trace;struct stackid stackid;bool kernel = !user; +int err = -EFAULT ;u32 max_depth; -int err;if (unlikely (flags & ~(BPF_F_SKIP_FIELD_MASK |BPF_F_USER_STACK |BPF_F_FAST_STACK_CMP |BPF_F_REUSE_STACKID )))return -EINVAL ; max_depth =stack_map_calculate_max_depth (map->value_size, elem_size, flags); + +preempt_disable (); trace =get_perf_callchain (regs, kernel, user, max_depth, false, false,0 );if (unlikely (!trace))/* couldn't fetch the stack trace */ -return -EFAULT ; + goto out; err =stackid_fastpath (&stackid, map, trace, flags);if (err != -ENOENT ) -return err; + goto out; new_bucket =stackid_new_bucket (&stackid, map, trace, flags); -if (!new_bucket) -return -ENOMEM ; +if (new_bucket) { +preempt_enable (); +return stackid_install (&stackid, map, new_bucket, flags); + } + err = -ENOMEM ; -return stackid_install (&stackid, map, new_bucket, flags); +out: +preempt_enable (); +return err; }const struct bpf_func_proto bpf_get_stackid_proto = {
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH bpf-next 5/9] bpf: Disable preemption in bpf_get_stackid
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete fix for preemption vulnerability in stack helpers (__bpf_get_stack) --
--- Patch [5]: [PATCH bpf-next 5/9] bpf: Disable preemption in bpf_get_stackid --- commit eb097d7647defa7379264dcdf95711bc573a419a
Author: Jiri Olsa <[email protected]>
bpf: Disable preemption in bpf_get_stackid
This commit adds preempt_disable() and preempt_enable() around the get_perf_callchain() call and trace buffer usage in bpf_get_stackid() to prevent the per-CPU trace entries buffer from being corrupted if the task is preempted.
Quoted 11 lines
kernel/bpf/stackmap.c +1
-1@@ -625,30 +625,37 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map,struct perf_callchain_entry *trace;struct stackid stackid;bool kernel = !user; +int err = -EFAULT ;u32 max_depth; -int err;
kernel/bpf/stackmap.c BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, · line 639
Quoted 10 lines
kernel/bpf/stackmap.c +2
-0if (unlikely (flags & ~(BPF_F_SKIP_FIELD_MASK |BPF_F_USER_STACK |BPF_F_FAST_STACK_CMP |BPF_F_REUSE_STACKID )))return -EINVAL ; max_depth =stack_map_calculate_max_depth (map->value_size, elem_size, flags); + +preempt_disable (); trace =get_perf_callchain (regs, kernel, user, max_depth, false, false,0 );
[Severity: High]
This isn't a bug introduced by this patch, but does __bpf_get_stack() also need this preempt_disable() protection?
__bpf_get_stack() has a similar pattern where it calls get_perf_callchain() or get_callchain_entry_for_task() and dereferences trace->ip via memcpy().
While it uses rcu_read_lock() when may_fault is true, on CONFIG_PREEMPT_RCU kernels this does not prevent preemption.
Could a preempting task trigger another stack trace on the same CPU before the memcpy() finishes, overwriting the per-CPU buffer in __bpf_get_stack()?
Quoted 4 lines
kernel/bpf/stackmap.c +1
-1if (unlikely (!trace))/* couldn't fetch the stack trace */ -return -EFAULT ; + goto out;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
Re: [PATCH bpf-next 5/9] bpf: Disable preemption in bpf_get_stackid
Quoted 45 lines · On Mon, Jul 20, 2026 at 09:04:10AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete fix for preemption vulnerability in stack helpers (__bpf_get_stack) ----- Patch [5]: [PATCH bpf-next 5/9] bpf: Disable preemption in
bpf_get_stackid--- commiteb097d7647defa7379264dcdf95711bc573a419a
Author: Jiri Olsa <[email protected]>bpf: Disable preemption in
bpf_get_stackidThis commit adds
preempt_disable()andpreempt_enable()around theget_perf_callchain()call and trace buffer usage inbpf_get_stackid()to prevent the per-CPU trace entries buffer from being corrupted if the task is preempted.Quoted 11 lines
kernel/bpf/stackmap.c +1
-1@@ -625,30 +625,37 @@ BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map,struct perf_callchain_entry *trace;struct stackid stackid;bool kernel = !user; +int err = -EFAULT ;u32 max_depth; -int err;kernel/bpf/stackmap.c BPF_CALL_3(bpf_get_stackid, struct pt_regs *, regs, struct bpf_map *, map, · line 639
Quoted 10 lines
kernel/bpf/stackmap.c +2
-0if (unlikely (flags & ~(BPF_F_SKIP_FIELD_MASK |BPF_F_USER_STACK |BPF_F_FAST_STACK_CMP |BPF_F_REUSE_STACKID )))return -EINVAL ; max_depth =stack_map_calculate_max_depth (map->value_size, elem_size, flags); + +preempt_disable (); trace =get_perf_callchain (regs, kernel, user, max_depth, false, false,0 );[Severity: High]
This isn't a bug introduced by this patch, but does__bpf_get_stack()also need thispreempt_disable()protection?
Quoted 8 lines
__bpf_get_stack()has a similar pattern where it callsget_perf_callchain()orget_callchain_entry_for_task()and dereferencestrace->ipviamemcpy().While it uses
rcu_read_lock()whenmay_faultis true, onCONFIG_PREEMPT_RCUkernels this does not prevent preemption.Could a preempting task trigger another stack trace on the same CPU before the
memcpy()finishes, overwriting the per-CPU buffer in__bpf_get_stack()?
__bpf_get_stack is taken care of in the following patches
jirka
Jiri Olsa
reviewerPATCH 6/9 bpf: Factor callchain_store function from __bpf_get_stack
In reply to Jiri OlsaThe new callchain_store function stores trace entries buffer into user supplied buffer. It covers both just-ip and buildid data.
--- kernel/bpf/stackmap.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-)
kernel/bpf/stackmap.c +25 -14
@@ -747,6 +747,29 @@ const struct bpf_func_proto bpf_get_stackid_proto_pe = { .arg3_type = ARG_ANYTHING, }; +static u32 callchain_store (struct perf_callchain_entry *trace,void *buf,u32 size, +u32 elem_size,u64 flags) +{ +bool user_build_id = flags &BPF_F_USER_BUILD_ID ; +u32 skip = flags &BPF_F_SKIP_FIELD_MASK ; +u32 trace_nr, copy_len; +u64 *ips; + + trace_nr = trace->nr - skip; + copy_len = trace_nr * elem_size; + + ips = trace->ip + skip; +if (user_build_id) { +struct bpf_stack_build_id *id_offs = buf; + +for (u32 i =0 ; i < trace_nr; i++) + id_offs[i].ip = ips[i]; + }else { +memcpy (buf, ips, copy_len); + } +return trace_nr; +} +static long __bpf_get_stack (struct pt_regs *regs,struct task_struct *task,struct perf_callchain_entry *trace_in,void *buf,u32 size,u64 flags,bool may_fault)
@@ -759,7 +782,6 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,struct perf_callchain_entry *trace;bool kernel = !user;int err = -EINVAL ; -u64 *ips;if (unlikely (flags & ~(BPF_F_SKIP_FIELD_MASK |BPF_F_USER_STACK |BPF_F_USER_BUILD_ID )))
@@ -804,21 +826,10 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, goto err_fault; } - trace_nr = trace->nr - skip; + trace_nr =callchain_store (trace, buf, size, elem_size, flags); copy_len = trace_nr * elem_size; - ips = trace->ip + skip; -if (user_build_id) { -struct bpf_stack_build_id *id_offs = buf; -u32 i; - -for (i =0 ; i < trace_nr; i++) - id_offs[i].ip = ips[i]; - }else { -memcpy (buf, ips, copy_len); - } - -/* trace/ips should not be dereferenced after this point */ +/* trace should not be dereferenced after this point */ if (may_fault)rcu_read_unlock ();
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH bpf-next 6/9] bpf: Factor callchain_store function from __bpf_get_stack
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The 'size' parameter in callchain_store() is unused. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The 'size' parameter in callchain_store() is unused.
--
--- Patch [6]: [PATCH bpf-next 6/9] bpf: Factor callchain_store function from __bpf_get_stack --- commit aee65ba10526fcf4c6320ea660fc50a555551cc1
Author: Jiri Olsa <[email protected]>
bpf: Factor callchain_store function from __bpf_get_stack
This commit extracts the logic for storing trace entries into a user-supplied buffer from __bpf_get_stack() and places it into a new helper function named callchain_store().
Quoted 4 lines
kernel/bpf/stackmap.c +0
-0
[ ... ]
kernel/bpf/stackmap.c const struct bpf_func_proto bpf_get_stackid_proto_pe = { · line 751
Quoted 6 lines
kernel/bpf/stackmap.c +2
-0@@ -747,6 +747,29 @@ const struct bpf_func_proto bpf_get_stackid_proto_pe = { .arg3_type = ARG_ANYTHING, }; +static u32 callchain_store (struct perf_callchain_entry *trace,void *buf,u32 size, +u32 elem_size,u64 flags)
[Severity: Low]
This isn't a bug, but the 'size' parameter doesn't appear to be used anywhere within callchain_store() in kernel/bpf/stackmap.c. Could it be safely removed from the function signature since the bounds checking is handled in the caller?
Quoted 20 lines
kernel/bpf/stackmap.c +20
-0+{ +bool user_build_id = flags &BPF_F_USER_BUILD_ID ; +u32 skip = flags &BPF_F_SKIP_FIELD_MASK ; +u32 trace_nr, copy_len; +u64 *ips; + + trace_nr = trace->nr - skip; + copy_len = trace_nr * elem_size; + + ips = trace->ip + skip; +if (user_build_id) { +struct bpf_stack_build_id *id_offs = buf; + +for (u32 i =0 ; i < trace_nr; i++) + id_offs[i].ip = ips[i]; + }else { +memcpy (buf, ips, copy_len); + } +return trace_nr; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Re: [PATCH bpf-next 6/9] bpf: Factor callchain_store function from __bpf_get_stack
Quoted 23 lines · On Mon, Jul 20, 2026 at 09:07:20AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The 'size' parameter incallchain_store()is unused.
----- Patch [6]: [PATCH bpf-next 6/9] bpf: Factor
callchain_storefunction from__bpf_get_stack--- commitaee65ba10526fcf4c6320ea660fc50a555551cc1
Author: Jiri Olsa <[email protected]>bpf: Factor
callchain_storefunction from__bpf_get_stackThis commit extracts the logic for storing trace entries into a user-supplied buffer from
__bpf_get_stack()and places it into a new helper function namedcallchain_store().Quoted 4 lines
kernel/bpf/stackmap.c +0
-0[ ... ]
Quoted 4 lines
kernel/bpf/stackmap.c +0
-0@@ -747,6 +747,29 @@ const struct bpf_func_proto bpf_get_stackid_proto_pe = { .arg3_type = ARG_ANYTHING, };
Quoted 7 lines
kernel/bpf/stackmap.c const struct bpf_func_proto bpf_get_stackid_proto_pe = { · line 751
Quoted 2 lines
kernel/bpf/stackmap.c +2
-0+static u32 callchain_store (struct perf_callchain_entry *trace,void *buf,u32 size, +u32 elem_size,u64 flags)[Severity: Low]
This isn't a bug, but the 'size' parameter doesn't appear to be used anywhere withincallchain_store()inkernel/bpf/stackmap.c. Could it be safely removed from the function signature since the bounds checking is handled in the caller?
true, will remove
jirka
Jiri Olsa
reviewerPATCH 7/9 bpf: Factor callchain_finalize function from __bpf_get_stack
In reply to Jiri OlsaThe new callchain_finalize function calls the build-id retrieval (if needed) and zeroes the buffer.
--- kernel/bpf/stackmap.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
kernel/bpf/stackmap.c +15 -8
@@ -770,16 +770,29 @@ static u32 callchain_store(struct perf_callchain_entry *trace, void *buf, u32 sireturn trace_nr; } +static long callchain_finalize (void *buf,u32 size,u32 trace_nr,u32 elem_size, +bool user_build_id,bool user,bool may_fault) +{ +u32 copy_len = trace_nr * elem_size; + +if (user_build_id) +stack_map_get_build_id_offset (buf, trace_nr, user, may_fault); + +if (size > copy_len) +memset (buf + copy_len,0 , size - copy_len); +return copy_len; +} +static long __bpf_get_stack (struct pt_regs *regs,struct task_struct *task,struct perf_callchain_entry *trace_in,void *buf,u32 size,u64 flags,bool may_fault) { -u32 trace_nr, copy_len, elem_size, max_depth;bool user_build_id = flags &BPF_F_USER_BUILD_ID ;bool crosstask = task && task != current;u32 skip = flags &BPF_F_SKIP_FIELD_MASK ;bool user = flags &BPF_F_USER_STACK ;struct perf_callchain_entry *trace; +u32 trace_nr, elem_size, max_depth;bool kernel = !user;int err = -EINVAL ;
@@ -827,18 +840,12 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, } trace_nr =callchain_store (trace, buf, size, elem_size, flags); - copy_len = trace_nr * elem_size;/* trace should not be dereferenced after this point */ if (may_fault)rcu_read_unlock (); -if (user_build_id) -stack_map_get_build_id_offset (buf, trace_nr, user, may_fault); - -if (size > copy_len) -memset (buf + copy_len,0 , size - copy_len); -return copy_len; +return callchain_finalize (buf, size, trace_nr, elem_size, user_build_id, user, may_fault); err_fault: err = -EFAULT ;
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH bpf-next 7/9] bpf: Factor callchain_finalize function from __bpf_get_stack
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Sleepable BPF programs calling bpf_get_stack() or bpf_get_task_stack() can corrupt per-CPU perf callchain buffers and trigger preemption warnings. --
--- Patch [7]: [PATCH bpf-next 7/9] bpf: Factor callchain_finalize function from __bpf_get_stack --- commit a181c39a42a0e8c840b9a2a09b02e1efa512170b
Author: Jiri Olsa <[email protected]>
bpf: Factor callchain_finalize function from __bpf_get_stack
The new callchain_finalize function calls the build-id retrieval (if needed) and zeroes the buffer.
Quoted 4 lines
kernel/bpf/stackmap.c +0
-0
[ ... ]
kernel/bpf/stackmap.c static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, · line 846
Quoted 9 lines
kernel/bpf/stackmap.c +0
-1@@ -827,18 +840,12 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, } trace_nr =callchain_store (trace, buf, size, elem_size, flags); - copy_len = trace_nr * elem_size;/* trace should not be dereferenced after this point */ if (may_fault)rcu_read_unlock ();
[Severity: High]
This is a pre-existing issue, but can sleepable BPF programs calling this function corrupt per-CPU perf callchain buffers and trigger preemption warnings?
When may_fault is true (such as from bpf_get_stack_sleepable or bpf_get_task_stack_sleepable), this code relies on rcu_read_lock() earlier in the function. However, on PREEMPT_RCU kernels, rcu_read_lock() does not disable preemption.
The earlier call to get_perf_callchain() eventually reaches get_callchain_entry(), which relies on smp_processor_id() and requires preemption to be explicitly disabled:
kernel/events/callchain.c:get_callchain_entry() {
...
cpu = smp_processor_id();
return (((void *)entries->cpu_entries[cpu]) +
(*rctx * perf_callchain_entry__sizeof()));
}Since preemption is not disabled during this sequence, can the task be preempted during or after fetching the per-CPU buffer, allowing another BPF program on the same CPU to overwrite the buffer before it is copied by callchain_store()?
Quoted 9 lines
kernel/bpf/stackmap.c +1
-6-if (user_build_id) -stack_map_get_build_id_offset (buf, trace_nr, user, may_fault); - -if (size > copy_len) -memset (buf + copy_len,0 , size - copy_len); -return copy_len; +return callchain_finalize (buf, size, trace_nr, elem_size, user_build_id, user, may_fault); err_fault:
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
Re: [PATCH bpf-next 7/9] bpf: Factor callchain_finalize function from __bpf_get_stack
Quoted 51 lines · On Mon, Jul 20, 2026 at 09:08:10AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Sleepable BPF programs callingbpf_get_stack()orbpf_get_task_stack()can corrupt per-CPU perf callchain buffers and trigger preemption warnings. ----- Patch [7]: [PATCH bpf-next 7/9] bpf: Factor
callchain_finalizefunction from__bpf_get_stack--- commita181c39a42a0e8c840b9a2a09b02e1efa512170b
Author: Jiri Olsa <[email protected]>bpf: Factor
callchain_finalizefunction from__bpf_get_stackThe new
callchain_finalizefunction calls the build-id retrieval (if needed) and zeroes the buffer.Quoted 4 lines
kernel/bpf/stackmap.c +0
-0[ ... ]
kernel/bpf/stackmap.c static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, · line 846
Quoted 9 lines
kernel/bpf/stackmap.c +0
-1@@ -827,18 +840,12 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, } trace_nr =callchain_store (trace, buf, size, elem_size, flags); - copy_len = trace_nr * elem_size;/* trace should not be dereferenced after this point */ if (may_fault)rcu_read_unlock ();[Severity: High]
This is a pre-existing issue, but can sleepable BPF programs calling this function corrupt per-CPU perf callchain buffers and trigger preemption warnings?When
may_faultis true (such as frombpf_get_stack_sleepableorbpf_get_task_stack_sleepable), this code relies onrcu_read_lock()earlier in the function. However, onPREEMPT_RCUkernels,rcu_read_lock()does not disable preemption.The earlier call to
get_perf_callchain()eventually reachesget_callchain_entry(), which relies onsmp_processor_id()and requires preemption to be explicitly disabled:kernel/events/callchain.c:get_callchain_entry() { ... cpu = smp_processor_id(); return (((void *)entries->cpu_entries[cpu]) + (*rctx * perf_callchain_entry__sizeof())); }
Quoted 4 lines
Since preemption is not disabled during this sequence, can the task be preempted during or after fetching the per-CPU buffer, allowing another BPF program on the same CPU to overwrite the buffer before it is copied by
callchain_store()?
we disable the preemption in patxh # 9
jirka
Jiri Olsa
reviewerPATCH 8/9 bpf: Remove trace_in argument from __bpf_get_stack
In reply to Jiri OlsaNow with the new callchain_* helper functions we can process trace_in case directly in bpf_get_stack_pe function and remove it from __bpf_get_stack which makes things easier for preemption fix in following change.
--- kernel/bpf/stackmap.c | 50 +++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 14 deletions(-)
kernel/bpf/stackmap.c +36 -14
@@ -784,7 +784,6 @@ static long callchain_finalize(void *buf, u32 size, u32 trace_nr, u32 elem_size, }static long __bpf_get_stack (struct pt_regs *regs,struct task_struct *task, -struct perf_callchain_entry *trace_in,void *buf,u32 size,u64 flags,bool may_fault) {bool user_build_id = flags &BPF_F_USER_BUILD_ID ;
@@ -823,10 +822,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task,if (may_fault)rcu_read_lock ();/* need RCU for perf's callchain below */ -if (trace_in) { - trace = trace_in; - trace->nr =min_t (u32, trace->nr, max_depth); - }else if (kernel && task) { +if (kernel && task) { trace =get_callchain_entry_for_task (task, max_depth); }else { trace = get_perf_callchain(regs, kernel, user, max_depth,
@@ -857,7 +853,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, BPF_CALL_4(bpf_get_stack, struct pt_regs *, regs, void *, buf, u32, size, u64, flags) { -return __bpf_get_stack (regs,NULL ,NULL , buf, size, flags, false/* !may_fault */ ); +return __bpf_get_stack (regs,NULL , buf, size, flags, false/* !may_fault */ ); }const struct bpf_func_proto bpf_get_stack_proto = {
@@ -873,7 +869,7 @@ const struct bpf_func_proto bpf_get_stack_proto = { BPF_CALL_4(bpf_get_stack_sleepable, struct pt_regs *, regs, void *, buf, u32, size, u64, flags) { -return __bpf_get_stack (regs,NULL ,NULL , buf, size, flags, true/* may_fault */ ); +return __bpf_get_stack (regs,NULL , buf, size, flags, true/* may_fault */ ); }const struct bpf_func_proto bpf_get_stack_sleepable_proto = {
@@ -897,7 +893,7 @@ static long __bpf_get_task_stack(struct task_struct *task, void *buf, u32 size, regs =task_pt_regs (task);if (regs) - res =__bpf_get_stack (regs, task,NULL , buf, size, flags, may_fault); + res =__bpf_get_stack (regs, task, buf, size, flags, may_fault);put_task_stack (task);return res;
@@ -937,6 +933,33 @@ const struct bpf_func_proto bpf_get_task_stack_sleepable_proto = { .arg4_type = ARG_ANYTHING, }; +static int __bpf_get_stack_pe (struct perf_callchain_entry *trace,void *buf,u32 size, +u64 flags) +{ +bool user_build_id = flags &BPF_F_USER_BUILD_ID ; +u64 skip = flags &BPF_F_SKIP_FIELD_MASK ; +bool user = flags &BPF_F_USER_STACK ; +u32 elem_size, max_depth, nr_trace; +bool kernel = !user; + +if (kernel && user_build_id) +return -EINVAL ; + + elem_size = user_build_id ?sizeof (struct bpf_stack_build_id ) :sizeof (u64); +if (unlikely (size % elem_size)) +return -EINVAL ; + + max_depth =stack_map_calculate_max_depth (size, elem_size, flags); + trace->nr =min_t (u32, trace->nr, max_depth); + +if (trace->nr < skip) +return -EFAULT ; + + nr_trace =callchain_store (trace, buf, size, elem_size, flags); +return callchain_finalize (buf, size, nr_trace, elem_size, user_build_id, + user, false/* !may_fault */ ); +} + BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx, void *, buf, u32, size, u64, flags) {
@@ -948,7 +971,7 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,__u64 nr_kernel;if (!(event->attr.sample_type &PERF_SAMPLE_CALLCHAIN )) -return __bpf_get_stack (regs,NULL ,NULL , buf, size, flags, false/* !may_fault */ ); +return __bpf_get_stack (regs,NULL , buf, size, flags, false/* !may_fault */ );if (unlikely (flags & ~(BPF_F_SKIP_FIELD_MASK |BPF_F_USER_STACK |BPF_F_USER_BUILD_ID )))
@@ -968,7 +991,7 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx,__u64 nr = trace->nr; trace->nr = nr_kernel; - err =__bpf_get_stack (regs,NULL , trace, buf, size, flags, false/* !may_fault */ ); + err =__bpf_get_stack_pe (trace, buf, size, flags);/* restore nr */ trace->nr = nr;
@@ -978,14 +1001,13 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx, skip += nr_kernel;if (skip >BPF_F_SKIP_FIELD_MASK ) goto clear; - flags = (flags & ~BPF_F_SKIP_FIELD_MASK ) | skip; - err =__bpf_get_stack (regs,NULL , trace, buf, size, flags, false/* !may_fault */ ); + err =__bpf_get_stack_pe (trace, buf, size, flags); } -return err; clear: -memset (buf,0 , size); +if (err <0 ) +memset (buf,0 , size);return err; }
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH bpf-next 8/9] bpf: Remove trace_in argument from __bpf_get_stack
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] bpf_get_stack_pe permanently truncates the shared perf event callchain for user stacks. --
--- Patch [8]: [PATCH bpf-next 8/9] bpf: Remove trace_in argument from __bpf_get_stack --- commit 42ed70b803080e71468d115e2948bdba98a800f5
Author: Jiri Olsa <[email protected]>
bpf: Remove trace_in argument from __bpf_get_stack
This commit removes the trace_in argument from __bpf_get_stack and handles the trace_in case directly in a new __bpf_get_stack_pe function. This simplifies __bpf_get_stack for upcoming preemption fixes.
Quoted 4 lines
kernel/bpf/stackmap.c +0
-0
[ ... ]
kernel/bpf/stackmap.c BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx, · line 1005
Quoted 8 lines
kernel/bpf/stackmap.c +1
-2@@ -978,14 +1001,13 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx, skip += nr_kernel;if (skip >BPF_F_SKIP_FIELD_MASK ) goto clear; - flags = (flags & ~BPF_F_SKIP_FIELD_MASK ) | skip; - err =__bpf_get_stack (regs,NULL , trace, buf, size, flags, false/* !may_fault */ ); + err =__bpf_get_stack_pe (trace, buf, size, flags);
[Severity: High]
This is a pre-existing issue, but does this permanently truncate the shared perf event callchain for user stacks?
In bpf_get_stack_pe(), the kernel stack branch explicitly saves and restores trace->nr around the call to __bpf_get_stack_pe().
However, the user stack branch directly calls __bpf_get_stack_pe() without saving the original trace->nr value. Inside __bpf_get_stack_pe(), the trace size is modified in place:
static int __bpf_get_stack_pe(...)
{
...
max_depth = stack_map_calculate_max_depth(size, elem_size, flags);
trace->nr = min_t(u32, trace->nr, max_depth);
...
}If a BPF program attached to a perf event requests a user stack trace and provides a buffer smaller than the actual callchain length, trace->nr is truncated.
When the perf subsystem later processes this event and outputs it to the perf ring buffer, could this result in incomplete or corrupted profiling data due to the truncated user space callchain length?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
Re: [PATCH bpf-next 8/9] bpf: Remove trace_in argument from __bpf_get_stack
Quoted 49 lines · On Mon, Jul 20, 2026 at 09:09:45AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High]bpf_get_stack_pepermanently truncates the shared perf event callchain for user stacks. ----- Patch [8]: [PATCH bpf-next 8/9] bpf: Remove
trace_inargument from__bpf_get_stack--- commit42ed70b803080e71468d115e2948bdba98a800f5
Author: Jiri Olsa <[email protected]>bpf: Remove
trace_inargument from__bpf_get_stackThis commit removes the
trace_inargument from__bpf_get_stackand handles thetrace_incase directly in a new__bpf_get_stack_pefunction. This simplifies__bpf_get_stackfor upcoming preemption fixes.Quoted 4 lines
kernel/bpf/stackmap.c +0
-0[ ... ]
kernel/bpf/stackmap.c BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx, · line 1005
Quoted 8 lines
kernel/bpf/stackmap.c +1
-2@@ -978,14 +1001,13 @@ BPF_CALL_4(bpf_get_stack_pe, struct bpf_perf_event_data_kern *, ctx, skip += nr_kernel;if (skip >BPF_F_SKIP_FIELD_MASK ) goto clear; - flags = (flags & ~BPF_F_SKIP_FIELD_MASK ) | skip; - err =__bpf_get_stack (regs,NULL , trace, buf, size, flags, false/* !may_fault */ ); + err =__bpf_get_stack_pe (trace, buf, size, flags);[Severity: High]
This is a pre-existing issue, but does this permanently truncate the shared perf event callchain for user stacks?In
bpf_get_stack_pe(), the kernel stack branch explicitly saves and restorestrace->nraround the call to__bpf_get_stack_pe().However, the user stack branch directly calls
__bpf_get_stack_pe()without saving the originaltrace->nrvalue. Inside__bpf_get_stack_pe(), the trace size is modified in place:static int
__bpf_get_stack_pe(...){ ... max_depth = stack_map_calculate_max_depth(size, elem_size, flags); trace->nr = min_t(u32, trace->nr, max_depth); ... }
Quoted 7 lines
If a BPF program attached to a perf event requests a user stack trace and provides a buffer smaller than the actual callchain length,
trace->nris truncated.When the perf subsystem later processes this event and outputs it to the perf ring buffer, could this result in incomplete or corrupted profiling data due to the truncated user space callchain length?
right, will fix that in next version
jirka
Jiri Olsa
reviewerPATCH 9/9 bpf: Disable preemption in __bpf_get_stack
In reply to Jiri OlsaFrom: Daniel Borkmann <[email protected]>
get_perf_callchain() returns a per-CPU perf_callchain_entry buffer and releases its recursion slot via put_callchain_entry() before returning, so nothing keeps the entry reserved while __bpf_get_stack() consumes it below.
A preemptible BPF program (e.g. a non-sleepable raw tracepoint program on a PREEMPT kernel, which runs under migrate_disable() but not preempt_disable()) can be scheduled out between obtaining the entry and the copy. Another task scheduled on the same CPU then reuses the same per-CPU buffer and overwrites trace->nr with a larger value. copy_len is then computed from the inflated trace->nr and can exceed the caller's buffer, causing an out-of-bounds write in the memcpy() and in the build_id path.
The rcu_read_lock() previously taken here does not prevent this. It is only taken on the may_fault path, and under CONFIG_PREEMPT_RCU it does not disable preemption; it merely keeps perf's callchain buffer array alive (freed via call_rcu()) and does nothing to stop another task from reusing the entry.
Disable preemption around obtaining the callchain entry and copying it into the caller's buffer, so the entry cannot be reused underneath us and trace->nr stays bounded by max_depth. Build ID resolution may fault and is therefore deferred until after preemption is re-enabled; by then the instruction pointers have already been copied into buf, so it operates only on that private copy. Note, preempt_disable() also subsumes the buffer-lifetime guarantee the rcu_read_lock() provided, since a preempt-disabled section is an RCU read-side critical section for the callchain buffers' call_rcu() reclaim.
--- kernel/bpf/stackmap.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
kernel/bpf/stackmap.c +3 -6
@@ -819,8 +819,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, max_depth =stack_map_calculate_max_depth (size, elem_size, flags); -if (may_fault) -rcu_read_lock ();/* need RCU for perf's callchain below */ +preempt_disable ();if (kernel && task) { trace =get_callchain_entry_for_task (task, max_depth);
@@ -830,16 +829,14 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, }if (unlikely (!trace) || trace->nr < skip) { -if (may_fault) -rcu_read_unlock (); +preempt_enable (); goto err_fault; } trace_nr =callchain_store (trace, buf, size, elem_size, flags);/* trace should not be dereferenced after this point */ -if (may_fault) -rcu_read_unlock (); +preempt_enable ();return callchain_finalize (buf, size, trace_nr, elem_size, user_build_id, user, may_fault);
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH bpf-next 9/9] bpf: Disable preemption in __bpf_get_stack
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Replacing rcu_read_lock() with preempt_disable() triggers a lockdep warning ('suspicious rcu_dereference_check() usage') when __bpf_get_stack() is called from sleepable BPF programs.
Pre-existing issues:
- [Critical] Synchronous re-entrancy from BPF trampolines (fentry/fexit) enables a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability leading to an out-of-bounds memory write.
- [High] bpf_get_stack_pe permanently truncates the shared perf callchain length for user stacks.
--
--- Patch [9]: [PATCH bpf-next 9/9] bpf: Disable preemption in __bpf_get_stack --- commit d1da66040bb1a7fc97bf16d7cb97a0712702bc93
Author: Daniel Borkmann <[email protected]>
bpf: Disable preemption in __bpf_get_stack
This patch replaces rcu_read_lock() with preempt_disable() in __bpf_get_stack() to prevent another task from reusing the per-CPU perf_callchain_entry buffer. It ensures the callchain entry cannot be reused and overwritten underneath the copy operation. Build ID resolution is deferred until after preemption is re-enabled.
kernel/bpf/stackmap.c static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, · line 822
Quoted 11 lines
kernel/bpf/stackmap.c +1
-2@@ -819,8 +819,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, max_depth =stack_map_calculate_max_depth (size, elem_size, flags); -if (may_fault) -rcu_read_lock ();/* need RCU for perf's callchain below */ +preempt_disable ();
[Severity: High]
Does replacing rcu_read_lock() with preempt_disable() trigger a lockdep warning when __bpf_get_stack() is called from sleepable BPF programs?
Sleepable programs run under rcu_read_lock_trace(), not rcu_read_lock(). With preempt_disable() replacing rcu_read_lock(), the lockdep rcu_lock_map is not acquired.
When get_perf_callchain() calls get_callchain_entry(), it accesses callchain_cpus_entries using rcu_dereference(). Since rcu_dereference() explicitly checks for rcu_read_lock_held(), will this evaluate to false and trigger a suspicious rcu_dereference_check() warning?
kernel/bpf/stackmap.c static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, · line 829
Quoted 5 lines
kernel/bpf/stackmap.c +0
-0if (kernel && task) { trace =get_callchain_entry_for_task (task, max_depth);@@ -830,16 +829,14 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, }
[Severity: Critical]
This is a pre-existing issue, but can synchronous re-entrancy from BPF trampolines enable a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability?
get_perf_callchain() acquires the shared per-CPU perf_callchain_entry buffer but drops the recursion lock via put_callchain_entry() right before returning.
If a BPF fexit program is attached to get_perf_callchain(), it will run synchronously before __bpf_get_stack() resumes. The fexit program can recursively call bpf_get_stack() with a much larger size. Since the lock was dropped, it re-acquires the identical buffer and inflates trace->nr.
Upon returning, __bpf_get_stack() passes the inflated trace to callchain_store(). Because callchain_store() blindly trusts trace->nr without re-validating it against max_depth, will this result in an out-of-bounds memcpy()?
Quoted 6 lines
kernel/bpf/stackmap.c +1
-2if (unlikely (!trace) || trace->nr < skip) { -if (may_fault) -rcu_read_unlock (); +preempt_enable (); goto err_fault;
kernel/bpf/stackmap.c static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, · line 841
Quoted 10 lines
kernel/bpf/stackmap.c +1
-2} trace_nr =callchain_store (trace, buf, size, elem_size, flags);/* trace should not be dereferenced after this point */ -if (may_fault) -rcu_read_unlock (); +preempt_enable ();return callchain_finalize (buf, size, trace_nr, elem_size, user_build_id, user, may_fault);
[Severity: High]
This is a pre-existing issue, but does bpf_get_stack_pe() permanently truncate the shared perf callchain length for user stacks?
When bpf_get_stack_pe() is called with BPF_F_USER_STACK, it routes to __bpf_get_stack_pe(), passing the shared perf event buffer:
kernel/bpf/stackmap.c:__bpf_get_stack_pe() {
...
trace->nr = min_t(u32, trace->nr, max_depth);
...
}Unlike the kernel branch which explicitly saves and restores trace->nr, the user branch fails to restore the original length before returning in bpf_get_stack_pe().
Can this inadvertently or maliciously corrupt the event record for any subsequent users of the perf subsystem?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
Ihor Solodrai
reviewerRe: [PATCH bpf-next 9/9] bpf: Disable preemption in __bpf_get_stack
In reply to Jiri OlsaQuoted 49 lines · Jiri Olsa
From: Daniel Borkmann <[email protected]>
get_perf_callchain()returns a per-CPUperf_callchain_entrybuffer and releases its recursion slot viaput_callchain_entry()before returning, so nothing keeps the entry reserved while__bpf_get_stack()consumes it below.A preemptible BPF program (e.g. a non-sleepable raw tracepoint program on a PREEMPT kernel, which runs under
migrate_disable()but notpreempt_disable()) can be scheduled out between obtaining the entry and the copy. Another task scheduled on the same CPU then reuses the same per-CPU buffer and overwritestrace->nrwith a larger value.copy_lenis then computed from the inflatedtrace->nrand can exceed the caller's buffer, causing an out-of-bounds write in thememcpy()and in thebuild_idpath.The
rcu_read_lock()previously taken here does not prevent this. It is only taken on themay_faultpath, and underCONFIG_PREEMPT_RCUit does not disable preemption; it merely keeps perf's callchain buffer array alive (freed viacall_rcu()) and does nothing to stop another task from reusing the entry.Disable preemption around obtaining the callchain entry and copying it into the caller's buffer, so the entry cannot be reused underneath us and
trace->nrstays bounded bymax_depth. Build ID resolution may fault and is therefore deferred until after preemption is re-enabled; by then the instruction pointers have already been copied into buf, so it operates only on that private copy. Note,preempt_disable()also subsumes the buffer-lifetime guarantee thercu_read_lock()provided, since a preempt-disabled section is an RCU read-side critical section for the callchain buffers'call_rcu()reclaim.Fixes c195651e565a ("bpf: add bpf_get_stack helper")Reported-by Tao Chen <[email protected]>Reported-by STAR Labs SG <[email protected]>Signed-off-by Daniel Borkmann <[email protected]>[ changed Fixes: commit ]Signed-off-by Jiri Olsa <[email protected]>--- kernel/bpf/stackmap.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)kernel/bpf/stackmap.c +0
-0
kernel/bpf/stackmap.c static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, · line 822
Quoted 6 lines
kernel/bpf/stackmap.c +1
-2@@ -819,8 +819,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, max_depth =stack_map_calculate_max_depth (size, elem_size, flags); -if (may_fault) -rcu_read_lock ();/* need RCU for perf's callchain below */ +preempt_disable ();
With the series applied on bpf-ci-like kconfig I get a "suspicious RCU usage" splat on BPF selftests, pasted at the bottom.
AFAIU, the rcu_read_lock() that's removed here was not only controlling the lifetime (which is now covered by preempt_disable), but also putting the RCU read-side annotation for the
entries = rcu_dereference(callchain_cpus_entries);
in get_callchain_entry() (callchain.c:163).
preempt_disable() never takes rcu_lock_map, and a sleepable program holds only rcu_read_lock_trace() (rcu_tasks_trace_srcu_struct), which is a different lockmap. So on PREEMPT_RCU + PROVE_RCU the rcu_dereference_check() there now fails (see the splat).
The non-sleepable path is fine because it enters with rcu_read_lock() held via rcu_read_lock_dont_migrate().
I think to fix this we have to keep if (may_fault) rcu_read_lock(); alongside preempt_disable(). Something like this:
kernel/bpf/stackmap.c +6 -0
@@ -820,6 +820,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, max_depth =stack_map_calculate_max_depth (size, elem_size, flags);preempt_disable (); +if (may_fault) +rcu_read_lock ();if (kernel && task) { trace =get_callchain_entry_for_task (task, max_depth);
@@ -829,6 +831,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, }if (unlikely (!trace) || trace->nr < skip) { +if (may_fault) +rcu_read_unlock ();preempt_enable (); goto err_fault; }
@@ -836,6 +840,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, trace_nr =callchain_store (trace, buf, size, elem_size, flags);/* trace should not be dereferenced after this point */ +if (may_fault) +rcu_read_unlock ();preempt_enable (); return callchain_finalize(buf, size, trace_nr, elem_size,
user_build_id, user, may_fault);
I confirmed this diff fixes the splat below.
I guess teaching the perf side to accept rcu_read_lock_sched_held() would also work, but that's out of scope for a bpf fix and may be no less tricky.
[ 9.083612] =============================
[ 9.083795] WARNING: suspicious RCU usage
[ 9.085691] 7.2.0-rc3-gd7530927491c #2 Tainted: G OE
[ 9.086212] -----------------------------
[ 9.086394] kernel/events/callchain.c:163 suspicious rcu_dereference_check() usage!
[ 9.086736]
[ 9.086736] other info that might help us debug this:
[ 9.086736]
[ 9.087108]
[ 9.087108] rcu_scheduler_active = 2, debug_locks = 1
[ 9.087394] 2 locks held by uprobe_multi/237:
[ 9.087612] #0: ffffffff85479e38 (rcu_tasks_trace_srcu_struct){....}-{0:0}, at: uprobe_notify_resume+0x2af/0xe40
[ 9.088091] #1: ffffffff85479e38 (rcu_tasks_trace_srcu_struct){....}-{0:0}, at: uprobe_prog_run+0x5e3/0xb20
[ 9.088553]
[ 9.088553] stack backtrace:
[ 9.088752] CPU: 0 UID: 0 PID: 237 Comm: uprobe_multi Tainted: G
OE 7.2.0-rc3-gd7530927491c #2 PREEMPT(full)
[ 9.088760] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[ 9.088762] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-5.el9 11/05/2023
[ 9.088766] Call Trace:
[ 9.088769] <TASK>
[ 9.088773] dump_stack_lvl+0x83/0xa0
[ 9.088783] lockdep_rcu_suspicious+0x151/0x1c0
[ 9.088798] get_callchain_entry+0x19b/0x260
[ 9.088808] get_perf_callchain+0x352/0x7f0
[ 9.088818] ? __pfx_get_perf_callchain+0x10/0x10
[ 9.088822] ? unwind_get_return_address+0x5e/0xa0
[ 9.088829] ? __pfx_stack_trace_consume_entry+0x10/0x10
[ 9.088835] ? srso_return_thunk+0x5/0x5f
[ 9.088851] ? srso_return_thunk+0x5/0x5f
[ 9.088855] ? __lock_acquire+0x41f/0x2520
[ 9.088870] __bpf_get_stack+0x2f1/0x420
[ 9.088886] ? __pfx___bpf_get_stack+0x10/0x10
[ 9.088900] ? srso_return_thunk+0x5/0x5f
[ 9.088913] bpf_prog_51bb4c0f31ae471b_uprobe_sleepable+0x2d/0x42
[ 9.088921] uprobe_prog_run+0x405/0xb20
[ 9.088929] ? srso_return_thunk+0x5/0x5f
[ 9.088937] ? __pfx_uprobe_prog_run+0x10/0x10
[ 9.088947] ? lock_acquire+0x176/0x2e0
[ 9.088953] ? srso_return_thunk+0x5/0x5f
[ 9.088971] ? uprobe_multi_link_handler+0x4c/0xb0
[ 9.088977] uprobe_multi_link_handler+0x4c/0xb0
[ 9.088987] handler_chain+0x1bb/0x1530
[ 9.089002] ? lock_acquire+0x176/0x2e0
[ 9.089007] ? __kmalloc_cache_noprof+0xd4/0x610
[ 9.089016] ? __pfx_handler_chain+0x10/0x10
[ 9.089023] ? srso_return_thunk+0x5/0x5f
[ 9.089030] ? srso_return_thunk+0x5/0x5f
[ 9.089034] ? kasan_save_track+0x14/0x30
[ 9.089040] ? srso_return_thunk+0x5/0x5f
[ 9.089045] ? __kasan_kmalloc+0x7f/0x90
[ 9.089053] ? srso_return_thunk+0x5/0x5f
[ 9.089057] ? srso_return_thunk+0x5/0x5f
[ 9.089062] ? lockdep_init_map_type+0x4b/0x220
[ 9.089069] ? srso_return_thunk+0x5/0x5f
[ 9.089073] ? timer_init_key+0xfc/0x240
[ 9.089081] ? srso_return_thunk+0x5/0x5f
[ 9.089085] ? lockdep_init_map_type+0x4b/0x220
[ 9.089098] uprobe_notify_resume+0x53d/0xe40
[ 9.089102] ? srso_return_thunk+0x5/0x5f
[ 9.089107] ? srso_return_thunk+0x5/0x5f
[ 9.089111] ? find_held_lock+0x2b/0x80
[ 9.089118] ? atomic_notifier_call_chain+0x7a/0x110
[ 9.089127] ? __pfx_uprobe_notify_resume+0x10/0x10
[ 9.089139] ? srso_return_thunk+0x5/0x5f
[ 9.089143] ? atomic_notifier_call_chain+0x84/0x110
[ 9.089150] ? srso_return_thunk+0x5/0x5f
[ 9.089158] ? notify_die+0x8b/0x110
[ 9.089174] exit_to_user_mode_loop+0x9a/0x430
[ 9.089192] exc_int3+0x295/0x2c0
[ 9.089204] asm_exc_int3+0x39/0x40
[ 9.089210] RIP: 0033:0x6c4176
[ 9.089216] Code: 13 55 48 89 e5 e8 7a ff ff ff c6 05 0b 6f 2d 00 01 5d c3 90 c3 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 f3 0f 1e fa eb 8a <cc> 48 89 e5 b8 00 00 00 00 5d c3 55 48 89 e5 b8 00 00 00 00 5d c3
[ 9.089219] RSP: 002b:00007fff8e88c458 EFLAGS: 00000246
[ 9.089223] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f4babb9404b
[ 9.089226] RDX: 00007fff8e88c47f RSI: 0000000000001000 RDI: 00000000007a0000
[ 9.089229] RBP: 00007fff8e88c490 R08: 00007f4babc88330 R09: 00007f4babce40c0
[ 9.089231] R10: 00007f4baba90d98 R11: 0000000000000202 R12: 00007fff8e88c5c8
[ 9.089233] R13: 0000000000793ac1 R14: 000000000099ade8 R15: 00007f4babd17000
[ 9.089259] </TASK>pw-bot: cr
Quoted 22 lines
kernel/bpf/stackmap.c +2
-4if (kernel && task) { trace =get_callchain_entry_for_task (task, max_depth);@@ -830,16 +829,14 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, }if (unlikely (!trace) || trace->nr < skip) { -if (may_fault) -rcu_read_unlock (); +preempt_enable (); goto err_fault; } trace_nr =callchain_store (trace, buf, size, elem_size, flags);/* trace should not be dereferenced after this point */ -if (may_fault) -rcu_read_unlock (); +preempt_enable ();return callchain_finalize (buf, size, trace_nr, elem_size, user_build_id, user, may_fault);
Re: [PATCH bpf-next 9/9] bpf: Disable preemption in __bpf_get_stack
Quoted 114 lines · Ihor Solodrai
Quoted 49 lines · Jiri Olsa
From: Daniel Borkmann <[email protected]>
get_perf_callchain()returns a per-CPUperf_callchain_entrybuffer and releases its recursion slot viaput_callchain_entry()before returning, so nothing keeps the entry reserved while__bpf_get_stack()consumes it below.A preemptible BPF program (e.g. a non-sleepable raw tracepoint program on a PREEMPT kernel, which runs under
migrate_disable()but notpreempt_disable()) can be scheduled out between obtaining the entry and the copy. Another task scheduled on the same CPU then reuses the same per-CPU buffer and overwritestrace->nrwith a larger value.copy_lenis then computed from the inflatedtrace->nrand can exceed the caller's buffer, causing an out-of-bounds write in thememcpy()and in thebuild_idpath.The
rcu_read_lock()previously taken here does not prevent this. It is only taken on themay_faultpath, and underCONFIG_PREEMPT_RCUit does not disable preemption; it merely keeps perf's callchain buffer array alive (freed viacall_rcu()) and does nothing to stop another task from reusing the entry.Disable preemption around obtaining the callchain entry and copying it into the caller's buffer, so the entry cannot be reused underneath us and
trace->nrstays bounded bymax_depth. Build ID resolution may fault and is therefore deferred until after preemption is re-enabled; by then the instruction pointers have already been copied into buf, so it operates only on that private copy. Note,preempt_disable()also subsumes the buffer-lifetime guarantee thercu_read_lock()provided, since a preempt-disabled section is an RCU read-side critical section for the callchain buffers'call_rcu()reclaim.Fixes c195651e565a ("bpf: add bpf_get_stack helper")Reported-by Tao Chen <[email protected]>Reported-by STAR Labs SG <[email protected]>Signed-off-by Daniel Borkmann <[email protected]>[ changed Fixes: commit ]Signed-off-by Jiri Olsa <[email protected]>--- kernel/bpf/stackmap.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)kernel/bpf/stackmap.c +0
-0kernel/bpf/stackmap.c static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, · line 820
Quoted 4 lines
kernel/bpf/stackmap.c +1
-2@@ -819,8 +819,7 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, max_depth =stack_map_calculate_max_depth (size, elem_size, flags); -if (may_fault) -rcu_read_lock ();/* need RCU for perf's callchain below */ +preempt_disable ();With the series applied on bpf-ci-like kconfig I get a "suspicious RCU usage" splat on BPF selftests, pasted at the bottom.
AFAIU, the
rcu_read_lock()that's removed here was not only controlling the lifetime (which is now covered bypreempt_disable), but also putting the RCU read-side annotation for theentries = rcu_dereference(callchain_cpus_entries);in
get_callchain_entry()(callchain.c:163).
preempt_disable()never takesrcu_lock_map, and a sleepable program holds onlyrcu_read_lock_trace()(rcu_tasks_trace_srcu_struct), which is a different lockmap. So onPREEMPT_RCU+PROVE_RCUthercu_dereference_check()there now fails (see the splat).The non-sleepable path is fine because it enters with
rcu_read_lock()held viarcu_read_lock_dont_migrate().I think to fix this we have to keep if (
may_fault)rcu_read_lock(); alongsidepreempt_disable(). Something like this:kernel/bpf/stackmap.c +2
-0@@ -820,6 +820,8 @@ static long __bpf_get_stack(struct pt_regs *regs, structtask_struct *task, max_depth =stack_map_calculate_max_depth (size, elem_size, flags);preempt_disable (); +if (may_fault) +rcu_read_lock ();if (kernel && task) {trace = get_callchain_entry_for_task(task, max_depth);kernel/bpf/stackmap.c +2
-0@@ -829,6 +831,8 @@ static long __bpf_get_stack(struct pt_regs *regs, struct task_struct *task, }if (unlikely (!trace) || trace->nr < skip) { +if (may_fault) +rcu_read_unlock ();preempt_enable (); goto err_fault;}kernel/bpf/stackmap.c +2
-0@@ -836,6 +840,8 @@ static long __bpf_get_stack(struct pt_regs *regs, structtask_struct *task, trace_nr =callchain_store (trace, buf, size, elem_size, flags);/* trace should not be dereferenced after this point */ +if (may_fault) +rcu_read_unlock ();preempt_enable ();return callchain_finalize(buf, size,
trace_nr,elem_size,user_build_id, user,may_fault);
Quoted 1 line
I confirmed this diff fixes the splat below.
yes, I managed to reproduce it as well.. basically just to add the preemption_disable/enable hunks and keey rcu locks in place
Quoted 4 lines
I guess teaching the perf side to accept
rcu_read_lock_sched_held()would also work, but that's out of scope for a bpf fix and may be no less tricky.
will check, but you might be right it'd be tricky
thanks,
jirka
Ihor Solodrai
reviewerQuoted 20 lines · Jiri Olsa
hi,
we need to disable preemption forget_perf_callchainand keep it disabled as long as we are accessing its returned trace entries buffer.This patchset refactors both
bpf_get_stackandbpf_get_stackidhelpers as suggested by Andrii [1] before applying the actual preemption fix.Note the initial fix was sent by Tao Chen [2], but there was no follow up on this since February, hence this post.
thanks,
jirka[1] https://lore.kernel.org/bpf/CAEf4BzZwvAUgLwz-M0Y_NJLTmedyY9U6s7LrSmn751hQdTP4Uw@mail.gmail.com/
[2] https://lore.kernel.org/bpf/[email protected]/
---
Daniel Borkmann (1):
bpf: Disable preemption in__bpf_get_stack
Quoted 9 lines
Jiri Olsa (8):
bpf: Factorstackid_initfunction from__bpf_get_stackid
bpf: Factorstackid_fastpathfunction from__bpf_get_stackid
bpf: Factorstackid_new_bucketfrom__bpf_get_stackid
bpf: Use stack id functions instead of__bpf_get_stackid
bpf: Disable preemption inbpf_get_stackid
bpf: Factorcallchain_storefunction from__bpf_get_stack
bpf: Factorcallchain_finalizefunction from__bpf_get_stack
bpf: Removetrace_inargument from__bpf_get_stack
Hi Jiri.
It took me some effort to wrap my head around the problem, but with that the refactoring and the fix make sense overall.
I think there is one blocker which was flagged by sashiko and should be simple to fix. Sending details on patch #9 in a bit.
Thank you for picking this up!
Quoted 3 lines
kernel/bpf/stackmap.c | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------- 1 file changed, 198 insertions(+), 94 deletions(-)