* [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled
@ 2024-12-16 10:46 Andrea Righi
2024-12-16 16:16 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Andrea Righi @ 2024-12-16 10:46 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: John Fastabend, Andrii Nakryiko, Martin KaFai Lau,
Eduard Zingerman, Song Liu, KP Singh, Stanislav Fomichev, Hao Luo,
Jiri Olsa, bpf, linux-kernel
Calling bpf_get_smp_processor_id() in a kernel with CONFIG_SMP disabled
can trigger the following bug, as pcpu_hot is unavailable:
[ 8.471774] BUG: unable to handle page fault for address: 00000000936a290c
[ 8.471849] #PF: supervisor read access in kernel mode
[ 8.471881] #PF: error_code(0x0000) - not-present page
Fix by preventing the inlining of bpf_get_smp_processor_id() when
CONFIG_SMP disabled.
Fixes: 1ae6921009e5 ("bpf: inline bpf_get_smp_processor_id() helper")
Signed-off-by: Andrea Righi <arighi@nvidia.com>
---
kernel/bpf/verifier.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f7f892a52a37..d85413f1a784 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -21272,7 +21272,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
goto next_insn;
}
-#if defined(CONFIG_X86_64) && !defined(CONFIG_UML)
+#if defined(CONFIG_SMP) && defined(CONFIG_X86_64) && !defined(CONFIG_UML)
/* Implement bpf_get_smp_processor_id() inline. */
if (insn->imm == BPF_FUNC_get_smp_processor_id &&
verifier_inlines_helper_call(env, insn->imm)) {
--
2.47.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled 2024-12-16 10:46 [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled Andrea Righi @ 2024-12-16 16:16 ` Daniel Borkmann 2024-12-16 17:24 ` Andrea Righi 0 siblings, 1 reply; 6+ messages in thread From: Daniel Borkmann @ 2024-12-16 16:16 UTC (permalink / raw) To: Andrea Righi, Alexei Starovoitov Cc: John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel On 12/16/24 11:46 AM, Andrea Righi wrote: > Calling bpf_get_smp_processor_id() in a kernel with CONFIG_SMP disabled > can trigger the following bug, as pcpu_hot is unavailable: > > [ 8.471774] BUG: unable to handle page fault for address: 00000000936a290c > [ 8.471849] #PF: supervisor read access in kernel mode > [ 8.471881] #PF: error_code(0x0000) - not-present page > > Fix by preventing the inlining of bpf_get_smp_processor_id() when > CONFIG_SMP disabled. > > Fixes: 1ae6921009e5 ("bpf: inline bpf_get_smp_processor_id() helper") > Signed-off-by: Andrea Righi <arighi@nvidia.com> lgtm, but can't we instead do sth like this : diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index f7f892a52a37..761c70899754 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -21281,11 +21281,15 @@ static int do_misc_fixups(struct bpf_verifier_env *env) * changed in some incompatible and hard to support * way, it's fine to back out this inlining logic */ +#ifdef CONFIG_SMP insn_buf[0] = BPF_MOV32_IMM(BPF_REG_0, (u32)(unsigned long)&pcpu_hot.cpu_number); insn_buf[1] = BPF_MOV64_PERCPU_REG(BPF_REG_0, BPF_REG_0); insn_buf[2] = BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 0); cnt = 3; - +#else + BPF_ALU32_REG(BPF_XOR, BPF_REG_0, BPF_REG_0), + cnt = 1; +#endif new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); if (!new_prog) return -ENOMEM; Thanks, Daniel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled 2024-12-16 16:16 ` Daniel Borkmann @ 2024-12-16 17:24 ` Andrea Righi 2024-12-16 18:26 ` Eduard Zingerman 2024-12-16 20:28 ` Daniel Borkmann 0 siblings, 2 replies; 6+ messages in thread From: Andrea Righi @ 2024-12-16 17:24 UTC (permalink / raw) To: Daniel Borkmann Cc: Alexei Starovoitov, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel On Mon, Dec 16, 2024 at 05:16:33PM +0100, Daniel Borkmann wrote: > On 12/16/24 11:46 AM, Andrea Righi wrote: > > Calling bpf_get_smp_processor_id() in a kernel with CONFIG_SMP disabled > > can trigger the following bug, as pcpu_hot is unavailable: > > > > [ 8.471774] BUG: unable to handle page fault for address: 00000000936a290c > > [ 8.471849] #PF: supervisor read access in kernel mode > > [ 8.471881] #PF: error_code(0x0000) - not-present page > > > > Fix by preventing the inlining of bpf_get_smp_processor_id() when > > CONFIG_SMP disabled. > > > > Fixes: 1ae6921009e5 ("bpf: inline bpf_get_smp_processor_id() helper") > > Signed-off-by: Andrea Righi <arighi@nvidia.com> > > lgtm, but can't we instead do sth like this : > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index f7f892a52a37..761c70899754 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -21281,11 +21281,15 @@ static int do_misc_fixups(struct bpf_verifier_env *env) > * changed in some incompatible and hard to support > * way, it's fine to back out this inlining logic > */ > +#ifdef CONFIG_SMP > insn_buf[0] = BPF_MOV32_IMM(BPF_REG_0, (u32)(unsigned long)&pcpu_hot.cpu_number); > insn_buf[1] = BPF_MOV64_PERCPU_REG(BPF_REG_0, BPF_REG_0); > insn_buf[2] = BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 0); > cnt = 3; > - > +#else > + BPF_ALU32_REG(BPF_XOR, BPF_REG_0, BPF_REG_0), > + cnt = 1; > +#endif > new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); > if (!new_prog) > return -ENOMEM; That works as well (just tested) and it's probably better since we're basically inlining the return 0. Do you want me to send a v2 with this? Thanks, -Andrea ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled 2024-12-16 17:24 ` Andrea Righi @ 2024-12-16 18:26 ` Eduard Zingerman 2024-12-16 18:33 ` Alexei Starovoitov 2024-12-16 20:28 ` Daniel Borkmann 1 sibling, 1 reply; 6+ messages in thread From: Eduard Zingerman @ 2024-12-16 18:26 UTC (permalink / raw) To: Andrea Righi, Daniel Borkmann Cc: Alexei Starovoitov, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Song Liu, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel On Mon, 2024-12-16 at 18:24 +0100, Andrea Righi wrote: > On Mon, Dec 16, 2024 at 05:16:33PM +0100, Daniel Borkmann wrote: > > On 12/16/24 11:46 AM, Andrea Righi wrote: > > > Calling bpf_get_smp_processor_id() in a kernel with CONFIG_SMP disabled > > > can trigger the following bug, as pcpu_hot is unavailable: > > > > > > [ 8.471774] BUG: unable to handle page fault for address: 00000000936a290c > > > [ 8.471849] #PF: supervisor read access in kernel mode > > > [ 8.471881] #PF: error_code(0x0000) - not-present page > > > > > > Fix by preventing the inlining of bpf_get_smp_processor_id() when > > > CONFIG_SMP disabled. > > > > > > Fixes: 1ae6921009e5 ("bpf: inline bpf_get_smp_processor_id() helper") > > > Signed-off-by: Andrea Righi <arighi@nvidia.com> > > > > lgtm, but can't we instead do sth like this : > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index f7f892a52a37..761c70899754 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -21281,11 +21281,15 @@ static int do_misc_fixups(struct bpf_verifier_env *env) > > * changed in some incompatible and hard to support > > * way, it's fine to back out this inlining logic > > */ > > +#ifdef CONFIG_SMP > > insn_buf[0] = BPF_MOV32_IMM(BPF_REG_0, (u32)(unsigned long)&pcpu_hot.cpu_number); > > insn_buf[1] = BPF_MOV64_PERCPU_REG(BPF_REG_0, BPF_REG_0); > > insn_buf[2] = BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 0); > > cnt = 3; > > - > > +#else > > + BPF_ALU32_REG(BPF_XOR, BPF_REG_0, BPF_REG_0), > > + cnt = 1; > > +#endif > > new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); > > if (!new_prog) > > return -ENOMEM; > > That works as well (just tested) and it's probably better since we're > basically inlining the return 0. Do you want me to send a v2 with this? I think both Andrea's and Daniel's versions of the fix are good. Note, however, that I missed one more configuration variable when making bpf_get_smp_processor_id() inlinable: CONFIG_DEBUG_PREEMPT. Helper body: BPF_CALL_0(bpf_get_smp_processor_id) { return smp_processor_id(); } smp_processor_id definition: #ifdef CONFIG_DEBUG_PREEMPT extern unsigned int debug_smp_processor_id(void); # define smp_processor_id() debug_smp_processor_id() #else # define smp_processor_id() __smp_processor_id() #endif Thanks, Eduard. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled 2024-12-16 18:26 ` Eduard Zingerman @ 2024-12-16 18:33 ` Alexei Starovoitov 0 siblings, 0 replies; 6+ messages in thread From: Alexei Starovoitov @ 2024-12-16 18:33 UTC (permalink / raw) To: Eduard Zingerman Cc: Andrea Righi, Daniel Borkmann, Alexei Starovoitov, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Song Liu, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML On Mon, Dec 16, 2024 at 10:27 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Mon, 2024-12-16 at 18:24 +0100, Andrea Righi wrote: > > On Mon, Dec 16, 2024 at 05:16:33PM +0100, Daniel Borkmann wrote: > > > On 12/16/24 11:46 AM, Andrea Righi wrote: > > > > Calling bpf_get_smp_processor_id() in a kernel with CONFIG_SMP disabled > > > > can trigger the following bug, as pcpu_hot is unavailable: > > > > > > > > [ 8.471774] BUG: unable to handle page fault for address: 00000000936a290c > > > > [ 8.471849] #PF: supervisor read access in kernel mode > > > > [ 8.471881] #PF: error_code(0x0000) - not-present page > > > > > > > > Fix by preventing the inlining of bpf_get_smp_processor_id() when > > > > CONFIG_SMP disabled. > > > > > > > > Fixes: 1ae6921009e5 ("bpf: inline bpf_get_smp_processor_id() helper") > > > > Signed-off-by: Andrea Righi <arighi@nvidia.com> > > > > > > lgtm, but can't we instead do sth like this : > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index f7f892a52a37..761c70899754 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -21281,11 +21281,15 @@ static int do_misc_fixups(struct bpf_verifier_env *env) > > > * changed in some incompatible and hard to support > > > * way, it's fine to back out this inlining logic > > > */ > > > +#ifdef CONFIG_SMP > > > insn_buf[0] = BPF_MOV32_IMM(BPF_REG_0, (u32)(unsigned long)&pcpu_hot.cpu_number); > > > insn_buf[1] = BPF_MOV64_PERCPU_REG(BPF_REG_0, BPF_REG_0); > > > insn_buf[2] = BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 0); > > > cnt = 3; > > > - > > > +#else > > > + BPF_ALU32_REG(BPF_XOR, BPF_REG_0, BPF_REG_0), > > > + cnt = 1; > > > +#endif > > > new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); > > > if (!new_prog) > > > return -ENOMEM; > > > > That works as well (just tested) and it's probably better since we're > > basically inlining the return 0. Do you want me to send a v2 with this? > > I think both Andrea's and Daniel's versions of the fix are good. > Note, however, that I missed one more configuration variable when > making bpf_get_smp_processor_id() inlinable: CONFIG_DEBUG_PREEMPT. > > Helper body: > > BPF_CALL_0(bpf_get_smp_processor_id) > { > return smp_processor_id(); > } > > smp_processor_id definition: > > #ifdef CONFIG_DEBUG_PREEMPT > extern unsigned int debug_smp_processor_id(void); > # define smp_processor_id() debug_smp_processor_id() > #else > # define smp_processor_id() __smp_processor_id() > #endif No. It's fine as-is. We inline raw_smp_processor_id(). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled 2024-12-16 17:24 ` Andrea Righi 2024-12-16 18:26 ` Eduard Zingerman @ 2024-12-16 20:28 ` Daniel Borkmann 1 sibling, 0 replies; 6+ messages in thread From: Daniel Borkmann @ 2024-12-16 20:28 UTC (permalink / raw) To: Andrea Righi Cc: Alexei Starovoitov, John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, linux-kernel On 12/16/24 6:24 PM, Andrea Righi wrote: > On Mon, Dec 16, 2024 at 05:16:33PM +0100, Daniel Borkmann wrote: >> On 12/16/24 11:46 AM, Andrea Righi wrote: >>> Calling bpf_get_smp_processor_id() in a kernel with CONFIG_SMP disabled >>> can trigger the following bug, as pcpu_hot is unavailable: >>> >>> [ 8.471774] BUG: unable to handle page fault for address: 00000000936a290c >>> [ 8.471849] #PF: supervisor read access in kernel mode >>> [ 8.471881] #PF: error_code(0x0000) - not-present page >>> >>> Fix by preventing the inlining of bpf_get_smp_processor_id() when >>> CONFIG_SMP disabled. >>> >>> Fixes: 1ae6921009e5 ("bpf: inline bpf_get_smp_processor_id() helper") >>> Signed-off-by: Andrea Righi <arighi@nvidia.com> >> >> lgtm, but can't we instead do sth like this : >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index f7f892a52a37..761c70899754 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -21281,11 +21281,15 @@ static int do_misc_fixups(struct bpf_verifier_env *env) >> * changed in some incompatible and hard to support >> * way, it's fine to back out this inlining logic >> */ >> +#ifdef CONFIG_SMP >> insn_buf[0] = BPF_MOV32_IMM(BPF_REG_0, (u32)(unsigned long)&pcpu_hot.cpu_number); >> insn_buf[1] = BPF_MOV64_PERCPU_REG(BPF_REG_0, BPF_REG_0); >> insn_buf[2] = BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, 0); >> cnt = 3; >> - >> +#else >> + BPF_ALU32_REG(BPF_XOR, BPF_REG_0, BPF_REG_0), >> + cnt = 1; >> +#endif >> new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt); >> if (!new_prog) >> return -ENOMEM; > > That works as well (just tested) and it's probably better since we're > basically inlining the return 0. Do you want me to send a v2 with this? Yes, pls send v2, I think this is better than explicitly calling the helper under !CONFIG_SMP. Thanks, Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-16 20:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-16 10:46 [PATCH] bpf: do not inline bpf_get_smp_processor_id() with CONFIG_SMP disabled Andrea Righi 2024-12-16 16:16 ` Daniel Borkmann 2024-12-16 17:24 ` Andrea Righi 2024-12-16 18:26 ` Eduard Zingerman 2024-12-16 18:33 ` Alexei Starovoitov 2024-12-16 20:28 ` Daniel Borkmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.