On 10/19/22 11:34 PM, Martin KaFai Lau wrote:
On 10/19/22 1:22 PM, Joanne Koong wrote:
On Fri, Sep 9, 2022 at 4:12 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
On 9/7/22 11:31 AM, Joanne Koong wrote:
For bpf prog types that don't support writes on skb data, the dynptr is
read-only (bpf_dynptr_write() will return an error and bpf_dynptr_data()
will return NULL; for a read-only data slice, there will be a separate
API bpf_dynptr_data_rdonly(), which will be added in the near future).
I just caught up on the v4 discussion about loadtime-vs-runtime error on
write. From a user perspective, I am not concerned on which error.
Either way, I will quickly find out the packet header is not changed.
For the dynptr init helper bpf_dynptr_from_skb(), the user does not need
to know its skb is read-only or not and uses the same helper. The
verifier in this case uses its knowledge on the skb context and uses
bpf_dynptr_from_skb_rdonly_proto or bpf_dynptr_from_skb_rdwr_proto
accordingly.
Now for the slice helper, the user needs to remember its skb is read
only (or not) and uses bpf_dynptr_data() vs bpf_dynptr_data_rdonly()
accordingly. Yes, if it only needs to read, the user can always stay
with bpf_dynptr_data_rdonly (which is not the initially supported one
though). However, it is still unnecessary burden and surprise to user.
It is likely it will silently turn everything into bpf_dynptr_read()
against the user intention. eg:
if (bpf_dynptr_from_skb(skb, 0, &dynptr))
return 0;
ip6h = bpf_dynptr_data(&dynptr, 0, sizeof(*ip6h));
if (!ip6h) {
/* Unlikely case, in non-linear section, just bpf_dynptr_read()
* Oops...actually bpf_dynptr_data_rdonly() should be used.
*/
bpf_dynptr_read(buf, sizeof(*ip6h), &dynptr, 0, 0);
ip6h = buf;
}
I see your point. I agree that it'd be best if we could prevent this
burden on the user, but I think the trade-off would be that if we have
bpf_dynptr_data return data slices that are read-only and data slices
that are writable (where rd-only vs. writable is tracked by verifier),
then in the future we won't be able to support dynptrs that are
dynamically read-only (since to reject at load time, the verifier must
know statically whether the dynptr is read-only or not). I'm not sure
how likely it is that we'd run into a case where we'll need dynamic
read-only dynptrs though. What are your thoughts on this?
Out of all dynptr helpers, bpf_dynptr_data() is pretty much the only important
function for header parsing because of the runtime offset. This offset is good
to be tracked in runtime to avoid smart compiler getting in the way. imo,
making this helper less usage surprise is important. If the verifier can help,
then static checking is useful here.
It is hard to comment without a real use case on when we want to flip a dynptr
to rdonly in a dynamic/runtime way. Thus, comparing with the example like the
skb here, my preference is pretty obvious :)
Beside, a quick thought is doing this static checking now should now stop the
typo: should *not* stop the... :(
dynamic rdonly flip later. I imagine it will be a helper call like
bpf_dynptr_set_rdonly(). The verifier should be able to track this helper call.
