On Tue, Nov 5, 2024 at 8:48 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
>
>
> 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;
The last version makes sense to me.
bpf_prog_check_recur() should also return true when
prog->aux->priv_stack_requested to make sure trampoline adds a
run-time check.
