On Thu, Jan 05, 2023 at 08:36:07AM IST, Alexei Starovoitov wrote: > On Sun, Jan 01, 2023 at 02:03:57PM +0530, Kumar Kartikeya Dwivedi wrote: > > Currently, while reads are disallowed for dynptr stack slots, writes are > > not. Reads don't work from both direct access and helpers, while writes > > do work in both cases, but have the effect of overwriting the slot_type. > > Unrelated to this patch set, but disallowing reads from dynptr slots > seems like unnecessary restriction. > We allow reads from spilled slots and conceptually dynptr slots should > fall in is_spilled_reg() category in check_stack_read_*(). > > We already can do: > d = bpf_rdonly_cast(dynptr, bpf_core_type_id_kernel(struct bpf_dynptr_kern)) > d->size; Not sure this cast is required, it can just be reads from the stack and clang will generate CO-RE relocatable accesses when casted to the right struct with preserve_access_index attribute set? Or did I miss something? > and there is really no need to add bpf_dynptr* accessors either as helpers or as kfuncs. > All accessors can simply be 'static inline' pure bpf functions in bpf_helpers.h. > Automatic inlining and zero kernel side maintenance. Yeah, it could be made to work (perhaps even portably using CO-RE and relocatable enum values which check for set bits etc.). But in the end how do you define such an interface, will it be UAPI like xdp_md or __sk_buff, or unstable like kfunc, or just best-effort stable as long as user is making use of CO-RE relocs? > > With verifier allowing reads into dynptr we can also enable bpf_cast_to_kern_ctx() > to convert struct bpf_dynptr to struct bpf_dynptr_kern and enable > even faster reads. I think rdonly_cast is unnecessary, just enabling reads into stack will be enough to enable this.
