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.
