Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs

[Martin KaFai Lau <martin.lau@linux.dev>]

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@linux.dev> 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.