On 1/26/24 15:21, Andrii Nakryiko wrote:
On Mon, Jan 22, 2024 at 1:22 PM <thinker.li@xxxxxxxxx> wrote:From: Kui-Feng Lee <thinker.li@xxxxxxxxx> 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@xxxxxxxxxx/
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. ---[...]
