Date:   Wed, 19 Oct 2022 23:34:43 -0700
From:   Martin KaFai Lau <martin.lau@...ux.dev>
To:     Joanne Koong <joannelkoong@...il.com>
Cc:     andrii@...nel.org, daniel@...earbox.net, ast@...nel.org,
        martin.lau@...nel.org, kuba@...nel.org, memxor@...il.com,
        toke@...hat.com, netdev@...r.kernel.org, kernel-team@...com,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs

On 10/19/22 1:22 PM, Joanne Koong wrote:
> On Fri, Sep 9, 2022 at 4:12 PM Martin KaFai Lau <martin.lau@...ux.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 
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.

Powered by blists - more mailing lists