On Mon, Aug 1, 2022 at 3:11 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Jul 26, 2022 at 11:48 AM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> >
> > Add skb dynptrs, which are dynptrs whose underlying pointer points
> > to a skb. The dynptr acts on skb data. skb dynptrs have two main
> > benefits. One is that they allow operations on sizes that are not
> > statically known at compile-time (eg variable-sized accesses).
> > Another is that parsing the packet data through dynptrs (instead of
> > through direct access of skb->data and skb->data_end) can be more
> > ergonomic and less brittle (eg does not need manual if checking for
> > being within bounds of data_end).
> >
> > For bpf prog types that don't support writes on skb data, the dynptr is
> > read-only (writes and data slices are not permitted). For reads on the
> > dynptr, this includes reading into data in the non-linear paged buffers
> > but for writes and data slices, if the data is in a paged buffer, the
> > user must first call bpf_skb_pull_data to pull the data into the linear
> > portion.
> >
> > Additionally, any helper calls that change the underlying packet buffer
> > (eg bpf_skb_pull_data) invalidates any data slices of the associated
> > dynptr.
> >
> > Right now, skb dynptrs can only be constructed from skbs that are
> > the bpf program context - as such, there does not need to be any
> > reference tracking or release on skb dynptrs.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > include/linux/bpf.h | 8 ++++-
> > include/linux/filter.h | 4 +++
> > include/uapi/linux/bpf.h | 42 ++++++++++++++++++++++++--
> > kernel/bpf/helpers.c | 54 +++++++++++++++++++++++++++++++++-
> > kernel/bpf/verifier.c | 43 +++++++++++++++++++++++----
> > net/core/filter.c | 53 ++++++++++++++++++++++++++++++---
> > tools/include/uapi/linux/bpf.h | 42 ++++++++++++++++++++++++--
> > 7 files changed, 229 insertions(+), 17 deletions(-)
> >
>
> [...]
>
> > + type = bpf_dynptr_get_type(dst);
> > +
> > + if (flags) {
> > + if (type == BPF_DYNPTR_TYPE_SKB) {
> > + if (flags & ~(BPF_F_RECOMPUTE_CSUM | BPF_F_INVALIDATE_HASH))
> > + return -EINVAL;
> > + } else {
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (type == BPF_DYNPTR_TYPE_SKB) {
> > + struct sk_buff *skb = dst->data;
> > +
> > + /* if the data is paged, the caller needs to pull it first */
> > + if (dst->offset + offset + len > skb->len - skb->data_len)
> > + return -EAGAIN;
> > +
> > + return __bpf_skb_store_bytes(skb, dst->offset + offset, src, len,
> > + flags);
> > + }
>
> It seems like it would be cleaner to have a switch per dynptr type and
> each case doing its extra error checking (like CSUM and HASH flags for
> TYPE_SKB) and then performing write operation.
>
>
> memcpy can be either a catch-all default case, or perhaps it's safer
> to explicitly list TYPE_LOCAL and TYPE_RINGBUF to do memcpy, and then
> default should WARN() and return error?
Sounds great, I will make these changes (and the one below) for v2
>
> > +
> > memcpy(dst->data + dst->offset + offset, src, len);
> >
> > return 0;
> > @@ -1555,6 +1594,7 @@ static const struct bpf_func_proto bpf_dynptr_write_proto = {
> >
> > BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len)
> > {
> > + enum bpf_dynptr_type type;
> > int err;
> >
> > if (!ptr->data)
> > @@ -1567,6 +1607,18 @@ BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len
> > if (bpf_dynptr_is_rdonly(ptr))
> > return 0;
> >
> > + type = bpf_dynptr_get_type(ptr);
> > +
> > + if (type == 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)
> > + return 0;
> > +
> > + return (unsigned long)(skb->data + ptr->offset + offset);
> > + }
> > +
> > return (unsigned long)(ptr->data + ptr->offset + offset);
>
> Similarly, all these dynptr helpers effectively dispatch different
> implementations based on dynptr type. I think switch is most
> appropriate for this.
>
> > }
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0d523741a543..0838653eeb4e 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -263,6 +263,7 @@ struct bpf_call_arg_meta {
> > u32 subprogno;
> > struct bpf_map_value_off_desc *kptr_off_desc;
> > u8 uninit_dynptr_regno;
> > + enum bpf_dynptr_type type;
> > };
> >
>
> [...]
