Re: [PATCH bpf-next v9 02/10] bpf: Return false for bpf_prog_check_recur() default case

[Yonghong Song <yonghong.song@xxxxxxxxx>]

On 11/5/24 8:38 AM, Alexei Starovoitov wrote:
> On Tue, Nov 5, 2024 at 8:33 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> > On 11/5/24 7:50 AM, Alexei Starovoitov wrote:
> > > On Mon, Nov 4, 2024 at 10:02 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> > > > > I also don't understand the point of this patch 2.
> > > > > The patch 3 can still do:
> > > > >
> > > > > + switch (prog->type) {
> > > > > + case BPF_PROG_TYPE_KPROBE:
> > > > > + case BPF_PROG_TYPE_TRACEPOINT:
> > > > > + case BPF_PROG_TYPE_PERF_EVENT:
> > > > > + case BPF_PROG_TYPE_RAW_TRACEPOINT:
> > > > > +   return PRIV_STACK_ADAPTIVE;
> > > > > + default:
> > > > > +   break;
> > > > > + }
> > > > > +
> > > > > + if (!bpf_prog_check_recur(prog))
> > > > > +   return NO_PRIV_STACK;
> > > > >
> > > > > which would mean that iter, lsm, struct_ops will not be allowed
> > > > > to use priv stack.
> > > > One example is e.g. a TC prog. Since bpf_prog_check_recur(prog)
> > > > will return true (means supporting recursion), and private stack
> > > > does not really support TC prog, the logic will become more
> > > > complicated.
> > > >
> > > > I am totally okay with removing patch 2 and go back to my
> > > > previous approach to explicitly list prog types supporting
> > > > private stack.
> > > The point of reusing bpf_prog_check_recur() is that we don't
> > > need to duplicate the logic.
> > > We can still do something like:
> > > switch (prog->type) {
> > >    case BPF_PROG_TYPE_KPROBE:
> > >    case BPF_PROG_TYPE_TRACEPOINT:
> > >    case BPF_PROG_TYPE_PERF_EVENT:
> > >    case BPF_PROG_TYPE_RAW_TRACEPOINT:
> > >       return PRIV_STACK_ADAPTIVE;
> > >    case BPF_PROG_TYPE_TRACING:
> > >    case BPF_PROG_TYPE_LSM:
> > >    case BPF_PROG_TYPE_STRUCT_OPS:
> > >       if (bpf_prog_check_recur())
> > >         return PRIV_STACK_ADAPTIVE;
> > >       /* fallthrough */
> > >     default:
> > >       return NO_PRIV_STACK;
> > > }
> > Right. Listing trampoline related prog types explicitly
> > and using bpf_prog_check_recur() will be safe.
> >
> > One thing is for BPF_PROG_TYPE_STRUCT_OPS, PRIV_STACK_ALWAYS
> > will be returned. I will make adjustment like
> >
> > switch (prog->type) {
> >    case BPF_PROG_TYPE_KPROBE:
> >    case BPF_PROG_TYPE_TRACEPOINT:
> >    case BPF_PROG_TYPE_PERF_EVENT:
> >    case BPF_PROG_TYPE_RAW_TRACEPOINT:
> >       return PRIV_STACK_ADAPTIVE;
> >    case BPF_PROG_TYPE_TRACING:
> >    case BPF_PROG_TYPE_LSM:
> >    case BPF_PROG_TYPE_STRUCT_OPS:
> >       if (bpf_prog_check_recur()) {
> >         if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
> >             return PRIV_STACK_ALWAYS;
> hmm. definitely not unconditionally.
> Only when explicitly requested in callback.
>
> Something like this:
>     case BPF_PROG_TYPE_TRACING:
>     case BPF_PROG_TYPE_LSM:
>        if (bpf_prog_check_recur())
>           return PRIV_STACK_ADAPTIVE;
>     case BPF_PROG_TYPE_STRUCT_OPS:
>        if (prog->aux->priv_stack_requested)
>           return PRIV_STACK_ALWAYS;
>     default:
>        return NO_PRIV_STACK;
>
> and then we also change bpf_prog_check_recur()
>   to return true when prog->aux->priv_stack_requested

This works too. I had another thinking about
   case BPF_PROG_TYPE_LSM:
      if (bpf_prog_check_recur())
         return PRIV_STACK_ADAPTIVE;
   case BPF_PROG_TYPE_STRUCT_OPS:
      if (bpf_prog_check_recur())
         return PRIV_STACK_ALWAYS;

Note that in bpf_prog_check_recur(), for struct_ops,
will return prog->aux->priv_stack_request.
But think it is too verbose so didn't propose.

So explicitly using prog->aux->priv_stack_requested
is more visible. Maybe we can even do

   case BPF_PROG_TYPE_TRACING:
   case BPF_PROG_TYPE_LSM:
   case BPF_PROG_TYPE_STRUCT_OPS:
      if (prog->aux->priv_stack_requested)
         return PRIV_STACK_ALWYAS;
      else if (bpf_prog_check_recur())
         return PRIV_STACK_ADAPTIVE;
      /* fallthrough */
   default:
      return NO_PRIV_STACK;