Re: [RFC bpf-next] bpf, selftests/bpf: Support PTR_MAYBE_NULL for struct_ops arguments.

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

On 1/10/24 5:50 PM, Kui-Feng Lee wrote:
> 
> 
> On 1/10/24 15:44, Martin KaFai Lau wrote:
>> On 1/10/24 2:17 PM, thinker.li@gmail.com wrote:
>>> The proposed solution here is to add PTR_MAYBE_NULL annotations to
>>> arguments
>>
>> [ ... ]
>>
>>> == Future Work ==
>>>
>>> We require an improved method for annotating arguments. Initially, we
>>> anticipated annotating arguments by appending a suffix to argument names,
>>> such as arg1__maybe_null. However, this approach does not function for
>>> function pointers due to compiler limitations. Nevertheless, it does work
>>> for functions. To resolve this, we need compiler support to enable the
>>> inclusion of argument names in the DWARF for function pointer types.
>>
>> After reading the high level of the patch,
>> while it needs compiler work to support decl tagging (or arg name) in a 
>> struct_ops's func_proto, changing the info->reg_type of a struct_ops's 
>> argument have been doable in the ".is_valid_access" without new kernel code 
>> change in verifier/btf.c.
> 
> btf_ctx_access() mentioned in the original message is a help function
> called by the implementation of .is_valid_access. So, just like you
> said, they definitely can be handled by .is_valid_access it-self.
> 
> Do you prefer to let developers to handle it by themself instead of
> handling by the helpers?

I would prefer one way to do the same thing. ".is_valid_access" should be more 
flexible and straightforward. e.g. "bpf_tcp_ca_is_valid_access" can promote all 
"struct sock" pointers to "struct tcp_sock" without needing to specify them func 
by func.

It would be nice to eventually have both compilers support tagging in the 
struct_ops's func_proto. I was trying to say ".is_valid_access" can already add 
PTR_MAYBE_NULL now while waiting for the compiler support.

If the sched_ext adds PTR_MAYBE_NULL in its ".is_valid_access", what else is 
missing in the verifier.c and btf.c? I saw the patch has the following changes 
in verifier.c. Is it needed?

 > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
 > index 60f08f468399..190735f3eaf5 100644
 > --- a/kernel/bpf/verifier.c
 > +++ b/kernel/bpf/verifier.c
 > @@ -8200,6 +8200,7 @@ static int check_reg_type(struct bpf_verifier_env *env, 
u32 regno,
 >   	case PTR_TO_BTF_ID | PTR_TRUSTED:
 >   	case PTR_TO_BTF_ID | MEM_RCU:
 >   	case PTR_TO_BTF_ID | PTR_MAYBE_NULL:
 > +	case PTR_TO_BTF_ID | PTR_MAYBE_NULL | PTR_TRUSTED:
 >   	case PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_RCU:
 >   	{
 >   		/* For bpf_sk_release, it needs to match against first member