* Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs
[not found] ` <20220907183129.745846-2-joannelkoong@gmail.com>
@ 2022-09-09 23:12 ` Martin KaFai Lau
2022-10-19 20:22 ` Joanne Koong
0 siblings, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2022-09-09 23:12 UTC (permalink / raw)
To: Joanne Koong
Cc: andrii, daniel, ast, martin.lau, kuba, memxor, toke, netdev,
kernel-team, bpf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs
2022-09-09 23:12 ` [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs Martin KaFai Lau
@ 2022-10-19 20:22 ` Joanne Koong
2022-10-20 6:34 ` Martin KaFai Lau
0 siblings, 1 reply; 5+ messages in thread
From: Joanne Koong @ 2022-10-19 20:22 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: andrii, daniel, ast, martin.lau, kuba, memxor, toke, netdev,
kernel-team, bpf
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!
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs
2022-10-19 20:22 ` Joanne Koong
@ 2022-10-20 6:34 ` Martin KaFai Lau
2022-10-20 6:40 ` Martin KaFai Lau
0 siblings, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2022-10-20 6:34 UTC (permalink / raw)
To: Joanne Koong
Cc: andrii, daniel, ast, martin.lau, kuba, memxor, toke, netdev,
kernel-team, bpf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs
2022-10-20 6:34 ` Martin KaFai Lau
@ 2022-10-20 6:40 ` Martin KaFai Lau
2022-10-20 19:30 ` Joanne Koong
0 siblings, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2022-10-20 6:40 UTC (permalink / raw)
To: Joanne Koong
Cc: andrii, daniel, ast, martin.lau, kuba, memxor, toke, netdev,
kernel-team, bpf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs
2022-10-20 6:40 ` Martin KaFai Lau
@ 2022-10-20 19:30 ` Joanne Koong
0 siblings, 0 replies; 5+ messages in thread
From: Joanne Koong @ 2022-10-20 19:30 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: andrii, daniel, ast, martin.lau, kuba, memxor, toke, netdev,
kernel-team, bpf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-20 19:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220907183129.745846-1-joannelkoong@gmail.com>
[not found] ` <20220907183129.745846-2-joannelkoong@gmail.com>
2022-09-09 23:12 ` [PATCH bpf-next v6 1/3] bpf: Add skb dynptrs Martin KaFai Lau
2022-10-19 20:22 ` Joanne Koong
2022-10-20 6:34 ` Martin KaFai Lau
2022-10-20 6:40 ` Martin KaFai Lau
2022-10-20 19:30 ` Joanne Koong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).