On 10/21/24 6:34 PM, Alexei Starovoitov wrote:
On Sun, Oct 20, 2024 at 12:16 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:To identify whether a st_ops program requests private stack or not, the st_ops stub function is checked. If the stub function has the following name <st_ops_name>__<member_name>__priv_stack then the corresponding st_ops member func requests to use private stack. The information that the private stack is requested or not is encoded in struct bpf_struct_ops_func_info which will later be used by verifier. Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx> --- include/linux/bpf.h | 2 ++ kernel/bpf/bpf_struct_ops.c | 35 +++++++++++++++++++++++++---------- kernel/bpf/verifier.c | 8 +++++++- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index f3884ce2603d..376e43fc72b9 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1491,6 +1491,7 @@ struct bpf_prog_aux { bool exception_boundary; bool is_extended; /* true if extended by freplace program */ bool priv_stack_eligible; + bool priv_stack_always; u64 prog_array_member_cnt; /* counts how many times as member of prog_array */ struct mutex ext_mutex; /* mutex for is_extended and prog_array_member_cnt */ struct bpf_arena *arena; @@ -1776,6 +1777,7 @@ struct bpf_struct_ops { struct bpf_struct_ops_func_info { struct bpf_ctx_arg_aux *info; u32 cnt; + bool priv_stack_always; }; struct bpf_struct_ops_desc { diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 8279b5a57798..2cd4bd086c7a 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -145,33 +145,44 @@ void bpf_struct_ops_image_free(void *image) } #define MAYBE_NULL_SUFFIX "__nullable" -#define MAX_STUB_NAME 128 +#define MAX_STUB_NAME 140 /* Return the type info of a stub function, if it exists. * - * The name of a stub function is made up of the name of the struct_ops and - * the name of the function pointer member, separated by "__". For example, - * if the struct_ops type is named "foo_ops" and the function pointer - * member is named "bar", the stub function name would be "foo_ops__bar". + * The name of a stub function is made up of the name of the struct_ops, + * the name of the function pointer member and optionally "priv_stack" + * suffix, separated by "__". For example, if the struct_ops type is named + * "foo_ops" and the function pointer member is named "bar", the stub + * function name would be "foo_ops__bar". If a suffix "priv_stack" exists, + * the stub function name would be "foo_ops__bar__priv_stack". */ static const struct btf_type * find_stub_func_proto(const struct btf *btf, const char *st_op_name, - const char *member_name) + const char *member_name, bool *priv_stack_always) { char stub_func_name[MAX_STUB_NAME]; const struct btf_type *func_type; s32 btf_id; int cp; - cp = snprintf(stub_func_name, MAX_STUB_NAME, "%s__%s", + cp = snprintf(stub_func_name, MAX_STUB_NAME, "%s__%s__priv_stack", st_op_name, member_name);I don't think this approach fits. pw-bot: cr Also looking at original commit 1611603537a4 ("bpf: Create argument information for nullable arguments.") that added this %s__%s notation I'm not sure why we went with that approach. Just to avoid adding __nullable suffix in the actual callback and using cfi stub callback names with such suffixes as a "proxy" for the real callback? Did we ever use this functionality for anything other than bpf_testmod_ops__test_maybe_null selftest ? Martin ?
The __nullable is to tag an argument of an ops. The member in the struct (e.g. tcp_congestion_ops) is a pointer to FUNC_PROTO and its argument does not have an argument name to tag. Hence, we went with tagging the actual FUNC in the cfi object.
The __nullable argument tagging request was originally from sched_ext but I also don't see its usage in-tree for now.
For the priv_stack tagging, I also don't think it is a good way of doing it. It is like adding __nullable to flag the ops may return NULL pointer which I also tried to avoid in the bpf-qdisc patch set.
