On Wed, Feb 1, 2023 at 5:21 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Tue, Jan 31, 2023 at 4:40 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Tue, Jan 31, 2023 at 04:11:47PM -0800, Andrii Nakryiko wrote: > > > > > > > > When prog is just parsing the packet it doesn't need to finalize with bpf_dynptr_write. > > > > The prog can always write into the pointer followed by if (p == buf) bpf_dynptr_write. > > > > No need for rdonly flag, but extra copy is there in case of cloned which > > > > could have been avoided with extra rd_only flag. > > > > > > Yep, given we are designing bpf_dynptr_slice for performance, extra > > > copy on reads is unfortunate. ro/rw flag or have separate > > > bpf_dynptr_slice_rw vs bpf_dynptr_slice_ro? > > > > Either flag or two kfuncs sound good to me. > > Would it make sense to make bpf_dynptr_slice() as read-only variant, > and bpf_dynptr_slice_rw() for read/write? I think the common case is > read-only, right? And if users mistakenly use bpf_dynptr_slice() for > r/w case, they will get a verifier error when trying to write into the > returned pointer. While if we make bpf_dynptr_slice() as read-write, > users won't realize they are paying a performance penalty for > something that they don't actually need. Makes sense and it matches skb_header_pointer() usage in the kernel which is read-only. Since there is no verifier the read-only-ness is not enforced, but we can do it. Looks like we've converged on bpf_dynptr_slice() and bpf_dynptr_slice_rw(). The question remains what to do with bpf_dynptr_data() backed by skb/xdp. Should we return EINVAL to discourage its usage? Of course, we can come up with sensible behavior for bpf_dynptr_data(), but it will have quirks that will be not easy to document. Even with extensive docs the users might be surprised by the behavior.
