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

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

[Martin KaFai Lau]

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;
}


> +	case BPF_DYNPTR_TYPE_SKB:
> +	{
> +		struct sk_buff *skb = ptr->data;
> +
> +		/* if the data is paged, the caller needs to pull it first */
> +		if (ptr->offset + offset + len > skb->len - skb->data_len)

nit. skb_headlen(skb)

The patches can't be applied cleanly also. Please remember to rebase. 
eg. commit afef88e65554 ("selftests/bpf: Store BPF object files with 
.bpf.o extension") has landed on Sep 2.

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

[Joanne Koong]

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?

>
> > +     case BPF_DYNPTR_TYPE_SKB:
> > +     {
> > +             struct sk_buff *skb = ptr->data;
> > +
> > +             /* if the data is paged, the caller needs to pull it first */
> > +             if (ptr->offset + offset + len > skb->len - skb->data_len)
>
> nit. skb_headlen(skb)
>
> The patches can't be applied cleanly also. Please remember to rebase.
> eg. commit afef88e65554 ("selftests/bpf: Store BPF object files with
> .bpf.o extension") has landed on Sep 2.

I will use skb_headlen(skb) and rebase for the next iteration :)
Thanks for reviewing this!
>
>

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

[Martin KaFai Lau]

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 
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.

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

[Martin KaFai Lau]

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.

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

[Joanne Koong]

On Wed, Oct 19, 2022 at 11:40 PM Martin KaFai Lau <martin.lau@linux.dev> 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@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.'
>

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.