Re: [PATCHv2 bpf-next 05/11] bpf: Disable preemption in bpf_get_stackid

On Thu, Jul 30, 2026 at 03:52:14PM -0700, Andrii Nakryiko wrote:
> On Thu, Jul 30, 2026 at 6:43 AM Leon Hwang <[email protected]> wrote:
> >
> > On 2026/7/30 19:38, Jiri Olsa wrote:
> > > On Wed, Jul 29, 2026 at 06:30:26PM +0800, Leon Hwang wrote:
> > >> On 29/7/26 16:38, Jiri Olsa wrote:
> > >>> The 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.
> > >>>
> > >>> Cc: [email protected]
> > >>> Fixes: d5a3b1f69186 ("bpf: introduce BPF_MAP_TYPE_STACK_TRACE")
> > >>> Reported-by: Tao Chen <[email protected]>
> > >>> Closes: https://lore.kernel.org/bpf/[email protected]/
> > >>> Signed-off-by: Jiri Olsa <[email protected]>
> > >>> ---
> > >>>  kernel/bpf/stackmap.c | 19 +++++++++++++------
> > >>>  1 file changed, 13 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > >>> index 43b1e5436047..e48ddb4edd47 100644
> > >>> --- a/kernel/bpf/stackmap.c
> > >>> +++ b/kernel/bpf/stackmap.c
> > >>> @@ -624,30 +624,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();
> > >>
> > >> Better to use the guard-style guard(preempt)() in this series.
> > >
> > > we prefer to call stackid_install with preemption enabled
> >
> >
> > Understand.
> >
> > I wonder if we could avoid the "goto out".
> >
> > scoped_guard(preempt) { ... } might work here.
> 
> +1, came here to say the same, I think it should be
> 
> scoped_guard(preempt) {
>    ... do all the things with direct return ...
> }
> 
> /* we got new bucket here */
> 
> return stackid_install(...);

looks good, will check on that

thanks,
jirka