On Tue, Oct 22, 2024 at 10:27 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> 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.
Ahh. Right. That makes sense.
> The __nullable argument tagging request was originally from sched_ext but I also
> don't see its usage in-tree for now.
ok. Let's sync up with Tejun whether they have plans to use it.
> 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.
