On Fri, Jan 26, 2024 at 4:15 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote: > > > > 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. See [0], seems like you can stick to __nullable, and I'll update my patch set with __arg_nullable. User-facing naming will be consistent. Verifier internally will keep using PTR_MAYBE_NULL flag, of course. [0] https://lore.kernel.org/bpf/CAADnVQKx3RK8pK4xpNEPQKYGUemO0VjdRePdr34fJwHZs6Urag@xxxxxxxxxxxxxx/ > > > > >> { > >> 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. > >> > >> --- > >> > > > > [...]
