Re: [PATCH bpf-next v3 3/6] bpf: Disallow !kprobe_write_ctx progs tail-calling kprobe_write_ctx progs

[Kumar Kartikeya Dwivedi]

On Wed, 11 Mar 2026 at 07:08, Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
> On 11/3/26 01:23, Kumar Kartikeya Dwivedi wrote:
> > On Tue, 3 Mar 2026 at 17:04, <bot+bpf-ci@xxxxxxxxxx> 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_0A@xxxxxxxxxxxxxx/
> >>
> >> 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
>