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

[Kui-Feng Lee <sinquersw@gmail.com>]

On 1/26/24 15:05, Martin KaFai Lau wrote:
> On 1/22/24 1:22 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Allow passing a null pointer to the operators provided by a struct_ops
>> object. This is an RFC to collect feedbacks/opinions.
>>
>> The previous discussions against v1 came to the conclusion that the
>> developer should did it in ".is_valid_access". However, recently, kCFI 
>> for
>> struct_ops has been landed. We found it is possible to provide a generic
>> way to annotate arguments by adding a suffix after argument names of stub
>> functions. So, this RFC is resent to present the new idea.
>>
>> The function pointers that are passed to struct_ops operators (the 
>> function
>> pointers) are always considered reliable until now. They cannot be
>> null. However, in certain scenarios, it should be possible to pass null
>> pointers to these operators. For instance, sched_ext may pass a null
>> pointer in the struct task type to an operator that is provided by its
>> struct_ops objects.
>>
>> The proposed solution here is to add PTR_MAYBE_NULL annotations to
>> arguments and create instances of struct bpf_ctx_arg_aux (arg_info) for
>> these arguments. These arg_infos will be installed at
>> prog->aux->ctx_arg_info and will be checked by the BPF verifier when
>> loading the programs. When a struct_ops program accesses arguments in the
>> ctx, the verifier will call btf_ctx_access() (through
>> bpf_verifier_ops->is_valid_access) to verify the access. btf_ctx_access()
>> will check arg_info and use the information of the matched arg_info to
>> properly set reg_type.
> 
> 
> 
>>
>> For nullable arguments, this patch sets an arg_info to label them with
>> PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. This enforces the 
>> verifier to
>> check programs and ensure that they properly check the pointer. The
>> programs should check if the pointer is null before reading/writing the
>> pointed memory.
>>
>> The implementer of a struct_ops should annotate the arguments that can be
>> null. The implementer should define a stub function (empty) as a
>> placeholder for each defined operator. The name of a stub function should
>> be in the pattern "<st_op_type>_stub_<operator name>". For example, for
>> test_maybe_null of struct bpf_testmod_ops, it's stub function name should
>> be "bpf_testmod_ops_stub_test_maybe_null". You mark an argument 
>> nullable by
>> suffixing the argument name with "__nullable" at the stub function.  Here
>> is the example in bpf_testmod.c.
> 
> Neat idea to reuse the cfi stub. Some high level comments.
> 
> bpf_struct_ops_desc_init is also collecting the details of each 
> func_proto member. Check if this "__nullable" collection can be done in 
> the same loop.

It is a good idea.

> 
> Simplify the implementation of the member_arg_info allocations. There is 
> no need to compact everything in one continuous memory.
> 

Sure