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
