| Message ID | 20230903151448.61696-2-hffilwlqm@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf, x64: Fix tailcall infinite loop | expand |
On Sun, Sep 03, 2023 at 11:14:45PM +0800, Leon Hwang wrote: > Without understanding emit_prologue(), it is really hard to figure out > where does tail_call_cnt come from, even though searching tail_call_cnt > in the whole kernel repo. > > By adding these comments, it is a little bit easy to understand s/easy/easier > tail_call_cnt initialisation. > > Signed-off-by: Leon Hwang <hffilwlqm@gmail.com> I was rather thinking about dropping these comments altogether. We should be trying to provide fixes as small as possible IMHO. Having this as a separate commit also breaks logistics of this set as the fix + selftest should be routed towards bpf tree, whereas this particular commit is rather a bpf-next candidate. PTAL at https://www.kernel.org/doc/html/latest/bpf/bpf_devel_QA.html > --- > arch/x86/net/bpf_jit_comp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index a5930042139d3..bcca1c9b9a027 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf, > prog += X86_PATCH_SIZE; > if (!ebpf_from_cbpf) { > if (tail_call_reachable && !is_subprog) > + /* When it's the entry of the whole tailcall context, > + * zeroing rax means initialising tail_call_cnt. > + */ > EMIT2(0x31, 0xC0); /* xor eax, eax */ > else > + /* Keep the same instruction layout. */ > EMIT2(0x66, 0x90); /* nop2 */ > } > EMIT1(0x55); /* push rbp */ > -- > 2.41.0 >
On 6/9/23 03:26, Maciej Fijalkowski wrote: > On Sun, Sep 03, 2023 at 11:14:45PM +0800, Leon Hwang wrote: >> Without understanding emit_prologue(), it is really hard to figure out >> where does tail_call_cnt come from, even though searching tail_call_cnt >> in the whole kernel repo. >> >> By adding these comments, it is a little bit easy to understand > > s/easy/easier Got it. > >> tail_call_cnt initialisation. >> >> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com> > > I was rather thinking about dropping these comments altogether. We should > be trying to provide fixes as small as possible IMHO. > > Having this as a separate commit also breaks logistics of this set as > the fix + selftest should be routed towards bpf tree, whereas this > particular commit is rather a bpf-next candidate. > > PTAL at https://www.kernel.org/doc/html/latest/bpf/bpf_devel_QA.html Thanks for your review. I'll keep this commit as one of this set. Thanks, Leon > >> --- >> arch/x86/net/bpf_jit_comp.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c >> index a5930042139d3..bcca1c9b9a027 100644 >> --- a/arch/x86/net/bpf_jit_comp.c >> +++ b/arch/x86/net/bpf_jit_comp.c >> @@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf, >> prog += X86_PATCH_SIZE; >> if (!ebpf_from_cbpf) { >> if (tail_call_reachable && !is_subprog) >> + /* When it's the entry of the whole tailcall context, >> + * zeroing rax means initialising tail_call_cnt. >> + */ >> EMIT2(0x31, 0xC0); /* xor eax, eax */ >> else >> + /* Keep the same instruction layout. */ >> EMIT2(0x66, 0x90); /* nop2 */ >> } >> EMIT1(0x55); /* push rbp */ >> -- >> 2.41.0 >>
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index a5930042139d3..bcca1c9b9a027 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf, prog += X86_PATCH_SIZE; if (!ebpf_from_cbpf) { if (tail_call_reachable && !is_subprog) + /* When it's the entry of the whole tailcall context, + * zeroing rax means initialising tail_call_cnt. + */ EMIT2(0x31, 0xC0); /* xor eax, eax */ else + /* Keep the same instruction layout. */ EMIT2(0x66, 0x90); /* nop2 */ } EMIT1(0x55); /* push rbp */
Without understanding emit_prologue(), it is really hard to figure out where does tail_call_cnt come from, even though searching tail_call_cnt in the whole kernel repo. By adding these comments, it is a little bit easy to understand tail_call_cnt initialisation. Signed-off-by: Leon Hwang <hffilwlqm@gmail.com> --- arch/x86/net/bpf_jit_comp.c | 4 ++++ 1 file changed, 4 insertions(+)