On 11/4/24 8:28 PM, Alexei Starovoitov wrote:
On Mon, Nov 4, 2024 at 7:50 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
On 11/4/24 6:53 PM, Yonghong Song wrote:
On 11/4/24 5:55 PM, Alexei Starovoitov wrote:
On Mon, Nov 4, 2024 at 5:35 PM Yonghong Song
<yonghong.song@xxxxxxxxx> wrote:
On 11/4/24 5:21 PM, Alexei Starovoitov wrote:
On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song
<yonghong.song@xxxxxxxxx> wrote:
The bpf_prog_check_recur() funciton is currently used by trampoline
and tracing programs (also using trampoline) to check whether a
particular prog supports recursion checking or not. The default case
(non-trampoline progs) return true in the current implementation.
Let us make the non-trampoline prog recursion check return false
instead. It does not impact any existing use cases and allows the
function to be used outside the trampoline context in the next patch.
Does not impact ?! But it does.
This patch removes recursion check from fentry progs.
This cannot be right.
The original bpf_prog_check_recur() implementation:
static inline bool bpf_prog_check_recur(const struct bpf_prog *prog)
{
switch (resolve_prog_type(prog)) {
case BPF_PROG_TYPE_TRACING:
return prog->expected_attach_type != BPF_TRACE_ITER;
case BPF_PROG_TYPE_STRUCT_OPS:
case BPF_PROG_TYPE_LSM:
return false;
default:
return true;
}
}
fentry prog is a TRACING prog, so it is covered. Did I miss anything?
I see. This is way too subtle.
You're correct that fentry is TYPE_TRACING,
so it could have "worked" if it was used to build trampolines only.
But this helper is called for other prog types:
case BPF_FUNC_task_storage_get:
if (bpf_prog_check_recur(prog))
return &bpf_task_storage_get_recur_proto;
return &bpf_task_storage_get_proto;
so it's still not correct, but for a different reason.
There are four uses for func bpf_prog_check_recur() in kernel based on
cscope: 0 kernel/bpf/trampoline.c bpf_trampoline_enter 1053 if
(bpf_prog_check_recur(prog)) 1 kernel/bpf/trampoline.c
bpf_trampoline_exit 1068 if (bpf_prog_check_recur(prog)) 2
kernel/trace/bpf_trace.c bpf_tracing_func_proto 1549 if
(bpf_prog_check_recur(prog)) 3 kernel/trace/bpf_trace.c
bpf_tracing_func_proto 1553 if (bpf_prog_check_recur(prog)) The 2nd
and 3rd ones are in bpf_trace.c. 1444 static const struct
bpf_func_proto * 1445 bpf_tracing_func_proto(enum bpf_func_id func_id,
const struct bpf_prog *prog) 1446 { 1447 switch (func_id) { ... 1548
case BPF_FUNC_task_storage_get: 1549 if (bpf_prog_check_recur(prog))
1550 return &bpf_task_storage_get_recur_proto; 1551 return
&bpf_task_storage_get_proto; 1552 case BPF_FUNC_task_storage_delete:
1553 if (bpf_prog_check_recur(prog)) 1554 return
&bpf_task_storage_delete_recur_proto; 1555 return
&bpf_task_storage_delete_proto; ... 1568 default: 1569 return
bpf_base_func_proto(func_id, prog); 1570 } 1571 } They are used for
tracing programs. So we should be safe here. But if you think that
changing bpf_proc_check_recur() and calling function
bpf_prog_check_recur() in bpf_enable_priv_stack() is too subtle, I can
go back to my original approach which makes all supported prog types
explicit in bpf_enable_priv_stack().
Sorry. Format issue again. The below is a better format:
There are four uses for func bpf_prog_check_recur() in kernel based on cscope:
0 kernel/bpf/trampoline.c bpf_trampoline_enter 1053 if (bpf_prog_check_recur(prog))
1 kernel/bpf/trampoline.c bpf_trampoline_exit 1068 if (bpf_prog_check_recur(prog))
2 kernel/trace/bpf_trace.c bpf_tracing_func_proto 1549 if (bpf_prog_check_recur(prog))
3 kernel/trace/bpf_trace.c bpf_tracing_func_proto 1553 if (bpf_prog_check_recur(prog))
The 2nd and 3rd ones are in bpf_trace.c.
1444 static const struct bpf_func_proto *
1445 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
1446 {
1447 switch (func_id) {
...
1548 case BPF_FUNC_task_storage_get:
1549 if (bpf_prog_check_recur(prog))
1550 return &bpf_task_storage_get_recur_proto;
1551 return &bpf_task_storage_get_proto;
1552 case BPF_FUNC_task_storage_delete:
1553 if (bpf_prog_check_recur(prog))
1554 return &bpf_task_storage_delete_recur_proto;
1555 return &bpf_task_storage_delete_proto;
...
1568 default:
1569 return bpf_base_func_proto(func_id, prog);
1570 }
1571 }
They are used for tracing programs. So we should be safe here. But if you think that
changing bpf_proc_check_recur() and calling function bpf_prog_check_recur()
in bpf_enable_priv_stack() is too subtle, I can go back to my original approach
which makes all supported prog types explicit in bpf_enable_priv_stack().
What do you mean 'it's safe' ?
If you change bpf_prog_check_recur() to return false like this patch does
then kprobe progs will not have recursion protection
calling task_storage_get() helper.
In the context of this helper it means that kprobe progs have to use:
nobusy = bpf_task_storage_trylock();
With this patch as-is there will be a deadlock in bpf_task_storage_lock()
when kprobe is using task storage.
So it looks broken to me.
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.
Unless struct_ops will explicit request priv stack via bool flag.
Then we will also add recursion protection in trampoline.
