* [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active
@ 2025-08-04 12:16 Tao Chen
2025-08-04 13:02 ` Jiri Olsa
0 siblings, 1 reply; 7+ messages in thread
From: Tao Chen @ 2025-08-04 12:16 UTC (permalink / raw)
To: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, mattbobrowski,
rostedt, mhiramat, mathieu.desnoyers
Cc: bpf, linux-kernel, linux-trace-kernel, Tao Chen
The syscall link_create not protected by bpf_disable_instrumentation,
accessing percpu data bpf_prog_active should use cpu local_lock when
kprobe_multi program attach.
Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by: Tao Chen <chen.dylane@linux.dev>
---
kernel/trace/bpf_trace.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3ae52978cae..f6762552e8e 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2728,23 +2728,23 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
struct pt_regs *regs;
int err;
+ migrate_disable();
if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
bpf_prog_inc_misses_counter(link->link.prog);
err = 1;
goto out;
}
- migrate_disable();
rcu_read_lock();
regs = ftrace_partial_regs(fregs, bpf_kprobe_multi_pt_regs_ptr());
old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx);
err = bpf_prog_run(link->link.prog, regs);
bpf_reset_run_ctx(old_run_ctx);
rcu_read_unlock();
- migrate_enable();
out:
__this_cpu_dec(bpf_prog_active);
+ migrate_enable();
return err;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active 2025-08-04 12:16 [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active Tao Chen @ 2025-08-04 13:02 ` Jiri Olsa 2025-08-04 14:15 ` Tao Chen 2025-08-05 4:05 ` Yonghong Song 0 siblings, 2 replies; 7+ messages in thread From: Jiri Olsa @ 2025-08-04 13:02 UTC (permalink / raw) To: Tao Chen Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song, john.fastabend, kpsingh, sdf, haoluo, mattbobrowski, rostedt, mhiramat, mathieu.desnoyers, bpf, linux-kernel, linux-trace-kernel On Mon, Aug 04, 2025 at 08:16:15PM +0800, Tao Chen wrote: > The syscall link_create not protected by bpf_disable_instrumentation, > accessing percpu data bpf_prog_active should use cpu local_lock when > kprobe_multi program attach. > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") > Signed-off-by: Tao Chen <chen.dylane@linux.dev> > --- > kernel/trace/bpf_trace.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 3ae52978cae..f6762552e8e 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2728,23 +2728,23 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link, > struct pt_regs *regs; > int err; > > + migrate_disable(); > if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) { this is called all the way from graph tracer, which disables preemption in function_graph_enter_regs, so I think we can safely use __this_cpu_inc_return > bpf_prog_inc_misses_counter(link->link.prog); > err = 1; > goto out; > } > > - migrate_disable(); hum, but now I'm not sure why we disable migration in here then jirka > rcu_read_lock(); > regs = ftrace_partial_regs(fregs, bpf_kprobe_multi_pt_regs_ptr()); > old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx); > err = bpf_prog_run(link->link.prog, regs); > bpf_reset_run_ctx(old_run_ctx); > rcu_read_unlock(); > - migrate_enable(); > > out: > __this_cpu_dec(bpf_prog_active); > + migrate_enable(); > return err; > } > > -- > 2.48.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active 2025-08-04 13:02 ` Jiri Olsa @ 2025-08-04 14:15 ` Tao Chen 2025-08-05 9:07 ` Jiri Olsa 2025-08-05 4:05 ` Yonghong Song 1 sibling, 1 reply; 7+ messages in thread From: Tao Chen @ 2025-08-04 14:15 UTC (permalink / raw) To: Jiri Olsa Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song, john.fastabend, kpsingh, sdf, haoluo, mattbobrowski, rostedt, mhiramat, mathieu.desnoyers, bpf, linux-kernel, linux-trace-kernel 在 2025/8/4 21:02, Jiri Olsa 写道: > On Mon, Aug 04, 2025 at 08:16:15PM +0800, Tao Chen wrote: >> The syscall link_create not protected by bpf_disable_instrumentation, >> accessing percpu data bpf_prog_active should use cpu local_lock when >> kprobe_multi program attach. >> >> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") >> Signed-off-by: Tao Chen <chen.dylane@linux.dev> >> --- >> kernel/trace/bpf_trace.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index 3ae52978cae..f6762552e8e 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -2728,23 +2728,23 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link, >> struct pt_regs *regs; >> int err; >> >> + migrate_disable(); >> if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) { > > this is called all the way from graph tracer, which disables preemption in > function_graph_enter_regs, so I think we can safely use __this_cpu_inc_return > > >> bpf_prog_inc_misses_counter(link->link.prog); >> err = 1; >> goto out; >> } >> >> - migrate_disable(); > > hum, but now I'm not sure why we disable migration in here then > It seems that there is a cant_migrate() check in bpf_prog_run, so it should be disabled before run. > jirka > >> rcu_read_lock(); >> regs = ftrace_partial_regs(fregs, bpf_kprobe_multi_pt_regs_ptr()); >> old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx); >> err = bpf_prog_run(link->link.prog, regs); >> bpf_reset_run_ctx(old_run_ctx); >> rcu_read_unlock(); >> - migrate_enable(); >> >> out: >> __this_cpu_dec(bpf_prog_active); >> + migrate_enable(); >> return err; >> } >> >> -- >> 2.48.1 >> -- Best Regards Tao Chen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active 2025-08-04 14:15 ` Tao Chen @ 2025-08-05 9:07 ` Jiri Olsa 2025-08-05 9:27 ` Tao Chen 0 siblings, 1 reply; 7+ messages in thread From: Jiri Olsa @ 2025-08-05 9:07 UTC (permalink / raw) To: Tao Chen Cc: Jiri Olsa, ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song, john.fastabend, kpsingh, sdf, haoluo, mattbobrowski, rostedt, mhiramat, mathieu.desnoyers, bpf, linux-kernel, linux-trace-kernel On Mon, Aug 04, 2025 at 10:15:46PM +0800, Tao Chen wrote: > 在 2025/8/4 21:02, Jiri Olsa 写道: > > On Mon, Aug 04, 2025 at 08:16:15PM +0800, Tao Chen wrote: > > > The syscall link_create not protected by bpf_disable_instrumentation, > > > accessing percpu data bpf_prog_active should use cpu local_lock when > > > kprobe_multi program attach. > > > > > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") > > > Signed-off-by: Tao Chen <chen.dylane@linux.dev> > > > --- > > > kernel/trace/bpf_trace.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > > index 3ae52978cae..f6762552e8e 100644 > > > --- a/kernel/trace/bpf_trace.c > > > +++ b/kernel/trace/bpf_trace.c > > > @@ -2728,23 +2728,23 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link, > > > struct pt_regs *regs; > > > int err; > > > + migrate_disable(); > > > if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) { > > > > this is called all the way from graph tracer, which disables preemption in > > function_graph_enter_regs, so I think we can safely use __this_cpu_inc_return > > > > > > > bpf_prog_inc_misses_counter(link->link.prog); > > > err = 1; > > > goto out; > > > } > > > - migrate_disable(); > > > > hum, but now I'm not sure why we disable migration in here then > > > > It seems that there is a cant_migrate() check in bpf_prog_run, so it should > be disabled before run. yes, but disabled preemption will take care of that I think we can do change below plus some comment that Yonghong is suggesting in the other reply jirka --- diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 3ae52978cae6..74e8d9543c6d 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -2734,14 +2734,12 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link, goto out; } - migrate_disable(); rcu_read_lock(); regs = ftrace_partial_regs(fregs, bpf_kprobe_multi_pt_regs_ptr()); old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx); err = bpf_prog_run(link->link.prog, regs); bpf_reset_run_ctx(old_run_ctx); rcu_read_unlock(); - migrate_enable(); out: __this_cpu_dec(bpf_prog_active); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active 2025-08-05 9:07 ` Jiri Olsa @ 2025-08-05 9:27 ` Tao Chen 0 siblings, 0 replies; 7+ messages in thread From: Tao Chen @ 2025-08-05 9:27 UTC (permalink / raw) To: Jiri Olsa Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song, john.fastabend, kpsingh, sdf, haoluo, mattbobrowski, rostedt, mhiramat, mathieu.desnoyers, bpf, linux-kernel, linux-trace-kernel 在 2025/8/5 17:07, Jiri Olsa 写道: > On Mon, Aug 04, 2025 at 10:15:46PM +0800, Tao Chen wrote: >> 在 2025/8/4 21:02, Jiri Olsa 写道: >>> On Mon, Aug 04, 2025 at 08:16:15PM +0800, Tao Chen wrote: >>>> The syscall link_create not protected by bpf_disable_instrumentation, >>>> accessing percpu data bpf_prog_active should use cpu local_lock when >>>> kprobe_multi program attach. >>>> >>>> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") >>>> Signed-off-by: Tao Chen <chen.dylane@linux.dev> >>>> --- >>>> kernel/trace/bpf_trace.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>>> index 3ae52978cae..f6762552e8e 100644 >>>> --- a/kernel/trace/bpf_trace.c >>>> +++ b/kernel/trace/bpf_trace.c >>>> @@ -2728,23 +2728,23 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link, >>>> struct pt_regs *regs; >>>> int err; >>>> + migrate_disable(); >>>> if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) { >>> >>> this is called all the way from graph tracer, which disables preemption in >>> function_graph_enter_regs, so I think we can safely use __this_cpu_inc_return >>> >>> >>>> bpf_prog_inc_misses_counter(link->link.prog); >>>> err = 1; >>>> goto out; >>>> } >>>> - migrate_disable(); >>> >>> hum, but now I'm not sure why we disable migration in here then >>> >> >> It seems that there is a cant_migrate() check in bpf_prog_run, so it should >> be disabled before run. > > yes, but disabled preemption will take care of that > I see, you are right, preempt will pass the check, thanks. void __cant_migrate(const char *file, int line) { static unsigned long prev_jiffy; if (irqs_disabled()) return; if (is_migration_disabled(current)) return; if (!IS_ENABLED(CONFIG_PREEMPT_COUNT)) return; if (preempt_count() > 0) return; ... > I think we can do change below plus some comment that Yonghong > is suggesting in the other reply > Yes, i will remove the migrate_disable and add some comment as you and Yonghong suggested. > jirka > > > --- > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 3ae52978cae6..74e8d9543c6d 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -2734,14 +2734,12 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link, > goto out; > } > > - migrate_disable(); > rcu_read_lock(); > regs = ftrace_partial_regs(fregs, bpf_kprobe_multi_pt_regs_ptr()); > old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx); > err = bpf_prog_run(link->link.prog, regs); > bpf_reset_run_ctx(old_run_ctx); > rcu_read_unlock(); > - migrate_enable(); > > out: > __this_cpu_dec(bpf_prog_active); -- Best Regards Tao Chen ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active 2025-08-04 13:02 ` Jiri Olsa 2025-08-04 14:15 ` Tao Chen @ 2025-08-05 4:05 ` Yonghong Song 2025-08-05 12:28 ` Tao Chen 1 sibling, 1 reply; 7+ messages in thread From: Yonghong Song @ 2025-08-05 4:05 UTC (permalink / raw) To: Jiri Olsa, Tao Chen Cc: ast, daniel, andrii, martin.lau, eddyz87, song, john.fastabend, kpsingh, sdf, haoluo, mattbobrowski, rostedt, mhiramat, mathieu.desnoyers, bpf, linux-kernel, linux-trace-kernel On 8/4/25 6:02 AM, Jiri Olsa wrote: > On Mon, Aug 04, 2025 at 08:16:15PM +0800, Tao Chen wrote: >> The syscall link_create not protected by bpf_disable_instrumentation, >> accessing percpu data bpf_prog_active should use cpu local_lock when >> kprobe_multi program attach. >> >> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") >> Signed-off-by: Tao Chen <chen.dylane@linux.dev> >> --- >> kernel/trace/bpf_trace.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index 3ae52978cae..f6762552e8e 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -2728,23 +2728,23 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link, >> struct pt_regs *regs; >> int err; >> >> + migrate_disable(); >> if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) { > this is called all the way from graph tracer, which disables preemption in > function_graph_enter_regs, so I think we can safely use __this_cpu_inc_return Agree. migrate_disable() is not needed here. But it would be great to add some comments here since for most other prog_run, they typically have migrate_disable/enable. > > >> bpf_prog_inc_misses_counter(link->link.prog); >> err = 1; >> goto out; >> } >> >> - migrate_disable(); > hum, but now I'm not sure why we disable migration in here then Probably a oversight. > > jirka > >> rcu_read_lock(); >> regs = ftrace_partial_regs(fregs, bpf_kprobe_multi_pt_regs_ptr()); >> old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx); >> err = bpf_prog_run(link->link.prog, regs); >> bpf_reset_run_ctx(old_run_ctx); >> rcu_read_unlock(); >> - migrate_enable(); >> >> out: >> __this_cpu_dec(bpf_prog_active); >> + migrate_enable(); >> return err; >> } >> >> -- >> 2.48.1 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active 2025-08-05 4:05 ` Yonghong Song @ 2025-08-05 12:28 ` Tao Chen 0 siblings, 0 replies; 7+ messages in thread From: Tao Chen @ 2025-08-05 12:28 UTC (permalink / raw) To: Yonghong Song, Jiri Olsa Cc: ast, daniel, andrii, martin.lau, eddyz87, song, john.fastabend, kpsingh, sdf, haoluo, mattbobrowski, rostedt, mhiramat, mathieu.desnoyers, bpf, linux-kernel, linux-trace-kernel 在 2025/8/5 12:05, Yonghong Song 写道: > > > On 8/4/25 6:02 AM, Jiri Olsa wrote: >> On Mon, Aug 04, 2025 at 08:16:15PM +0800, Tao Chen wrote: >>> The syscall link_create not protected by bpf_disable_instrumentation, >>> accessing percpu data bpf_prog_active should use cpu local_lock when >>> kprobe_multi program attach. >>> >>> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link") >>> Signed-off-by: Tao Chen <chen.dylane@linux.dev> >>> --- >>> kernel/trace/bpf_trace.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>> index 3ae52978cae..f6762552e8e 100644 >>> --- a/kernel/trace/bpf_trace.c >>> +++ b/kernel/trace/bpf_trace.c >>> @@ -2728,23 +2728,23 @@ kprobe_multi_link_prog_run(struct >>> bpf_kprobe_multi_link *link, >>> struct pt_regs *regs; >>> int err; >>> + migrate_disable(); >>> if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) { >> this is called all the way from graph tracer, which disables >> preemption in >> function_graph_enter_regs, so I think we can safely use >> __this_cpu_inc_return > > Agree. migrate_disable() is not needed here. But it would be great to > add some > comments here since for most other prog_run, they typically have > migrate_disable/enable. > >> >> >>> bpf_prog_inc_misses_counter(link->link.prog); >>> err = 1; >>> goto out; >>> } >>> - migrate_disable(); >> hum, but now I'm not sure why we disable migration in here then > > Probably a oversight. > >> >> jirka >> >>> rcu_read_lock(); >>> regs = ftrace_partial_regs(fregs, bpf_kprobe_multi_pt_regs_ptr()); >>> old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx); >>> err = bpf_prog_run(link->link.prog, regs); >>> bpf_reset_run_ctx(old_run_ctx); >>> rcu_read_unlock(); >>> - migrate_enable(); >>> out: >>> __this_cpu_dec(bpf_prog_active); >>> + migrate_enable(); >>> return err; >>> } >>> -- >>> 2.48.1 >>> > Hi Jiri, Yonghong, I send another patch as you suggested, pls review it, thanks. https://lore.kernel.org/bpf/20250805122312.1890951-1-chen.dylane@linux.dev -- Best Regards Tao Chen ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-08-05 12:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-04 12:16 [PATCH bpf-next] bpf: Disable migrate when kprobe_multi attach to access bpf_prog_active Tao Chen 2025-08-04 13:02 ` Jiri Olsa 2025-08-04 14:15 ` Tao Chen 2025-08-05 9:07 ` Jiri Olsa 2025-08-05 9:27 ` Tao Chen 2025-08-05 4:05 ` Yonghong Song 2025-08-05 12:28 ` Tao Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).