* [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 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-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-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).