On Fri, Jan 26, 2024 at 4:01 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Fri, Jan 26, 2024 at 3:21 PM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> 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. > > fwiw I'm aware that _Nullable is a standard and it's supported by clang,etc. > If folks insist on such suffix, I can live with that. > But I absolutely don't want that to be a reason to rename > PTR_MAYBE_NULL in the verifier. > My preference is for consistency in the verifier and suffixes. > Hence __maybe_null. > But I'm ok if we do __nullable and keep PTR_MAYBE_NULL. +1 for that. That is the internal name, and we also have an internal xxx_OR_NULL convention. We don't need to change any of that, IMO.
