On Wed, Mar 11, 2026 at 2:22 AM Kumar Kartikeya Dwivedi <[email protected]> wrote: > > On Wed, 11 Mar 2026 at 07:08, Leon Hwang <[email protected]> wrote: > > > > On 11/3/26 01:23, Kumar Kartikeya Dwivedi wrote: > > > On Tue, 3 Mar 2026 at 17:04, <[email protected]> wrote: > > >> > > >>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c > > >>> --- a/kernel/bpf/core.c > > >>> +++ b/kernel/bpf/core.c > > >> > > >> [ ... ] > > >> > > >>> + switch (match) { > > >>> + case BPF_MAP_OWNER_MATCH_FOR_INIT: > > >>> + if (owner->kprobe_write_ctx != aux->kprobe_write_ctx) > > >>> + return false; > > >>> + break; > > >>> + > > >>> + case BPF_MAP_OWNER_MATCH_FOR_UPDATE: > > >>> + if (!owner->kprobe_write_ctx && aux->kprobe_write_ctx) > > >>> + return false; > > >>> + break; > > >>> + } > > >> > > >> In the v1 review, Alexei Starovoitov asked for strict conformance > > >> both ways rather than one-directional validation: > > >> > > >> "Don't be fancy. Require strict conformance both ways > > >> in *all* patches." > > >> > > >> https://lore.kernel.org/bpf/caadnvq+-v0-1i8_+cypk7g0cnv-2n8e9szv43ym3az197el...@mail.gmail.com/ > > >> > > >> The BPF_MAP_OWNER_MATCH_FOR_UPDATE case still uses one-directional > > >> matching, rejecting only !owner->kprobe_write_ctx && > > >> aux->kprobe_write_ctx > > >> but allowing the reverse. Was this an intentional design choice, and > > >> if so, should the changelog note the disagreement? > > >> > > > > > > Let's follow the approach Alexei outlined, while the changes look ok > > > to me, let's remove the one way check and just do > > > owner->kprobe_write_ctx != aux->kprobe_write_ctx, same for the other > > > two checks. With this we can also get rid of this INIT vs UPDATE > > > distinction. > > > > > > Other than that I think patches are good, please also test both > > > directions in the selftest in next respin. > > > > > > > Hi Kumar, > > > > Thanks for your review. > > > > I agreed that both-ways check could simplify the code. But, the UX could > > not be great. > > > > Let me explain it with an example. > > > > struct { > > __uint(type, BPF_MAP_TYPE_PROG_ARRAY); > > __uint(max_entries, 1); > > __uint(key_size, sizeof(__u32)); > > __uint(value_size, sizeof(__u32)); > > } jmp_table SEC(".maps"); > > > > SEC("?kprobe") > > int prog_a(struct pt_regs *regs) > > { > > regs->ax = 0; > > bpf_tail_call_static(regs, &jmp_table, 0); > > return 0; > > } > > > > SEC("?kprobe") > > int prog_b(struct pt_regs *regs) > > { > > return 0; > > } > > > > prog_a is kprobe_write_ctx. > > prog_b is !kprobe_write_ctx. And, it will be added to jmp_table. > > > > With both-ways check, prog_b is required to modify regs. I think it's > > too restrictive for users to use prog_b as tail callee. > > > > With one-way-check of this patch, prog_b can be used as tail callee to > > keep regs as-is. > > > > This was what I was concerned about, and the reason why I did not follow > > Alexei's approach. > > I agree but the main question is whether such a case is realistic, are > we going to have write_ctx programs tail calling this way? > Tail calls are already pretty rare, thinking more about it extension > programs are probably also broken wrt checks in this set. > bpf_check_attach_target is doing none of these things for > prog_extension = true. Nobody reported a problem, so I doubt anyone is > hitting this. > It probably also needs to be fixed. > Since you noticed it, we should close the gap conservatively for now, > and wait for a real use case to pop up before enabling this one-way.
+1 tail_calls in general hopefully will be deprecated soon. As soon as we have support for indirect calls there won't be any reason for tail calls to exist. (other than not breaking current users). We definitely don't want to open it up further. So the simplest fix.
