On Mon, 1 Sept 2025 at 21:22, Puranjay Mohan <puranjay@xxxxxxxxxx> wrote:
>
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes:
>
> > On Mon, Sep 1, 2025 at 6:34 AM Puranjay Mohan <puranjay@xxxxxxxxxx> wrote:
> >>
> >> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> writes:
> >>
> >> > On Fri, Aug 29, 2025 at 3:30 AM Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:
> >> >>
> >> >> > +
> >> >> > +void bpf_prog_report_arena_violation(bool write, unsigned long addr)
> >> >> > +{
> >> >> > + struct bpf_stream_stage ss;
> >> >> > + struct bpf_prog *prog;
> >> >> > + u64 user_vm_start;
> >> >> > +
> >> >> > + prog = bpf_prog_find_from_stack();
> >> >>
> >> >> bpf_prog_find_from_stack depends on arch_bpf_stack_walk, which isn't available
> >> >> on all archs. How about switching to bpf_prog_ksym_find with the fault pc?
> >> >
> >> > Out of archs that support bpf arena only riscv doesn't
> >> > support arch_bpf_stack_walk(), which is probably fixable.
> >> > But I agree that direct bpf_prog_ksym_find() is cleaner here.
> >> > We need to make sure it works for subprogs, since streams[2] are
> >> > valid only for main prog.
> >> > I think we can add:
> >> > struct bpf_prog_aux {
> >> > ...
> >> > struct bpf_prog_aux *main_prog;
> >> > };
> >> > init it during jit_subprogs() and use it for stream access.
> >> > We can also remove skipping of subprogs in find_from_stack_cb() then.
> >> >
> >> > Kumar, wdyt?
> >>
> >> So, IIUC, after adding struct bpf_prog_aux *main_prog_aux in struct
> >> bpf_prog_aux,
> >>
> >> We can do in bpf_prog_alloc_no_stats():
> >> fp->aux->main_prog_aux = aux;
> >>
> >> and in jit_subprogs():
> >> func[i]->aux->main_prog_aux = prog->aux;
> >>
> >> and then all users of bpf_stream_get() can do
> >> bpf_stream_get(stream_id, prog->aux->main_prog_aux);
> >>
> >> with above we can allow find_from_stack_cb() to return subprogs.
> >> and bpf_prog_ksym_find() can be used in
> >> bpf_prog_report_arena_violation() without any other changes.
> >
> > Yes. That's exactly the proposal.
>
> I think we should go ahead with this approach but also divide
> bpf_prog_aux into two as you suggested. I will send a follow-up set for
> that.
find_from_stack_cb shouldn't be returning subprogs IMO, I think what
was meant by Alexei was to use the first found subprog to jump to its
main prog.
I think this will also make stream related logic work in cases where
we only have subprog of the program, and not the main prog (e.g. timer
callbacks, etc.) in the stack trace, which was probably an oversight
on my part. It would be nice to test this, let me know if you can fold
it in your set / send as a follow up, otherwise I will.
Additionally, I feel the extra pointer is unnecessary. Instead, the
logic to jump to the main prog from the subprog can be (if I didn't
miss anything):
prog->aux->func ? prog->aux->func[0]->aux ? prog-aux
When prog->aux->func is not set, that means there are no subprogs. I
would simply wrap this logic in bpf_main_prog_aux or something.
>
> Thanks,
> Puranjay
