Re: [PATCH bpf-next v6 3/9] bpf: Support private stack for struct ops programs

[Martin KaFai Lau <martin.lau@linux.dev>]

On 10/21/24 6:34 PM, Alexei Starovoitov wrote:
> On Sun, Oct 20, 2024 at 12:16 PM Yonghong Song <yonghong.song@linux.dev> 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@linux.dev>
>> ---
>>   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.