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. > > Thanks, > Leon >
