| Message ID | 20241104193505.3242662-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Support private stack for bpf progs | expand |
On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@linux.dev> 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. pw-bot: cr > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- > include/linux/bpf_verifier.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 4513372c5bc8..ad887c68d3e1 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -889,9 +889,8 @@ static inline bool bpf_prog_check_recur(const struct bpf_prog *prog) > return prog->expected_attach_type != BPF_TRACE_ITER; > case BPF_PROG_TYPE_STRUCT_OPS: > case BPF_PROG_TYPE_LSM: > - return false; > default: > - return true; > + return false; > } > } > > -- > 2.43.5 >
On 11/4/24 5:21 PM, Alexei Starovoitov wrote: > On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@linux.dev> 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? > > pw-bot: cr > >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> >> --- >> include/linux/bpf_verifier.h | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h >> index 4513372c5bc8..ad887c68d3e1 100644 >> --- a/include/linux/bpf_verifier.h >> +++ b/include/linux/bpf_verifier.h >> @@ -889,9 +889,8 @@ static inline bool bpf_prog_check_recur(const struct bpf_prog *prog) >> return prog->expected_attach_type != BPF_TRACE_ITER; >> case BPF_PROG_TYPE_STRUCT_OPS: >> case BPF_PROG_TYPE_LSM: >> - return false; >> default: >> - return true; >> + return false; >> } >> } >> >> -- >> 2.43.5 >>
On Mon, Nov 4, 2024 at 5:35 PM Yonghong Song <yonghong.song@linux.dev> wrote: > > > On 11/4/24 5:21 PM, Alexei Starovoitov wrote: > > On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@linux.dev> 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.
On 11/4/24 5:55 PM, Alexei Starovoitov wrote: > On Mon, Nov 4, 2024 at 5:35 PM Yonghong Song <yonghong.song@linux.dev> wrote: >> >> On 11/4/24 5:21 PM, Alexei Starovoitov wrote: >>> On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song <yonghong.song@linux.dev> 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().
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@linux.dev> wrote: >>> >>> On 11/4/24 5:21 PM, Alexei Starovoitov wrote: >>>> On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song >>>> <yonghong.song@linux.dev> 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().
On Mon, Nov 4, 2024 at 7:50 PM Yonghong Song <yonghong.song@linux.dev> 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@linux.dev> wrote: > >>> > >>> On 11/4/24 5:21 PM, Alexei Starovoitov wrote: > >>>> On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song > >>>> <yonghong.song@linux.dev> 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. Unless struct_ops will explicit request priv stack via bool flag. Then we will also add recursion protection in trampoline.
On 11/4/24 8:28 PM, Alexei Starovoitov wrote: > On Mon, Nov 4, 2024 at 7:50 PM Yonghong Song <yonghong.song@linux.dev> 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@linux.dev> wrote: >>>>> On 11/4/24 5:21 PM, Alexei Starovoitov wrote: >>>>>> On Mon, Nov 4, 2024 at 11:35 AM Yonghong Song >>>>>> <yonghong.song@linux.dev> 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.
On Mon, Nov 4, 2024 at 10:02 PM Yonghong Song <yonghong.song@linux.dev> 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; }
On 11/5/24 7:50 AM, Alexei Starovoitov wrote: > On Mon, Nov 4, 2024 at 10:02 PM Yonghong Song <yonghong.song@linux.dev> 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; else return PRIV_STACK_ADAPTIVE; } /* fallthrough */ default: return NO_PRIV_STACK; }
On Tue, Nov 5, 2024 at 8:33 AM Yonghong Song <yonghong.song@linux.dev> wrote: > > > > > On 11/5/24 7:50 AM, Alexei Starovoitov wrote: > > On Mon, Nov 4, 2024 at 10:02 PM Yonghong Song <yonghong.song@linux.dev> 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
On 11/5/24 8:38 AM, Alexei Starovoitov wrote: > On Tue, Nov 5, 2024 at 8:33 AM Yonghong Song <yonghong.song@linux.dev> wrote: >> >> >> >> On 11/5/24 7:50 AM, Alexei Starovoitov wrote: >>> On Mon, Nov 4, 2024 at 10:02 PM Yonghong Song <yonghong.song@linux.dev> 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;
On Tue, Nov 5, 2024 at 8:48 AM Yonghong Song <yonghong.song@linux.dev> wrote: > > > > > On 11/5/24 8:38 AM, Alexei Starovoitov wrote: > > On Tue, Nov 5, 2024 at 8:33 AM Yonghong Song <yonghong.song@linux.dev> wrote: > >> > >> > >> > >> On 11/5/24 7:50 AM, Alexei Starovoitov wrote: > >>> On Mon, Nov 4, 2024 at 10:02 PM Yonghong Song <yonghong.song@linux.dev> 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.
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 4513372c5bc8..ad887c68d3e1 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -889,9 +889,8 @@ static inline bool bpf_prog_check_recur(const struct bpf_prog *prog) return prog->expected_attach_type != BPF_TRACE_ITER; case BPF_PROG_TYPE_STRUCT_OPS: case BPF_PROG_TYPE_LSM: - return false; default: - return true; + return false; } }
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. Signed-off-by: Yonghong Song <yonghong.song@linux.dev> --- include/linux/bpf_verifier.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)