On Wed, Oct 19, 2022 at 11:40 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> 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.'
>
Great! I'll change this in v7 to have bpf_dynptr_data() be able to
return both read-writable and read-only data slices, where the rd-only
property is enforced by the verifier.
