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/ > { > 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. > > --- > [...]
