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:21, Andrii Nakryiko wrote:
> On Mon, Jan 22, 2024 at 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.
>>
>>    static int bpf_testmod_ops_stub_test_maybe_null(int dummy, struct
>>                  task_struct *task__nullable)
> 
> let's keep this consistent with __arg_nullable/__arg_maybe_null? ([0])
> I'd very much prefer __arg_nullable and __nullable vs
> __arg_maybe_null/__maybe_null, but Alexei didn't like the naming when
> I posted v1.
> 
> But in any case, I think it helps to keep similar concepts named
> similarly, right?
> 
>    [0] https://patchwork.kernel.org/project/netdevbpf/patch/20240125205510.3642094-6-andrii@kernel.org/

Let me paraphrase it. "__arg_maybe_null" is prefered for the case here.

> 
>>    {
>>            return 0;
>>    }
>>
>> This means that the argument 1 (2nd) of bpf_testmod_ops->test_maybe_null,
>> which is a function pointer that can be null. With this annotation, the
>> verifier will understand how to check programs using this arguments.  A BPF
>> program that implement test_maybe_null should check the pointer to make
>> sure it is not null before using it. For example,
>>
>>    if (task__nullable)
>>        save_tgid = task__nullable->tgid
>>
>> Without the check, the verifier will reject the program.
>>
>> Since we already has stub functions for kCFI, we just reuse these stub
>> functions with the naming convention mentioned earlier. These stub
>> functions with the naming convention is only required if there are nullable
>> arguments to annotate. For functions without nullable arguments, stub
>> functions are not necessary for the purpose of this patch.
>>
>> ---
>>
> 
> [...]