On Thu, Jul 28, 2022 at 10:45 AM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> Hi, Joanne,
>
> 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(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 20c26aed7896..7fbd4324c848 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -407,11 +407,14 @@ enum bpf_type_flag {
> > /* Size is known at compile time. */
> > MEM_FIXED_SIZE = BIT(10 + BPF_BASE_TYPE_BITS),
> >
> > + /* DYNPTR points to sk_buff */
> > + DYNPTR_TYPE_SKB = BIT(11 + BPF_BASE_TYPE_BITS),
> > +
> > __BPF_TYPE_FLAG_MAX,
> > __BPF_TYPE_LAST_FLAG = __BPF_TYPE_FLAG_MAX - 1,
> > };
> >
> > -#define DYNPTR_TYPE_FLAG_MASK (DYNPTR_TYPE_LOCAL | DYNPTR_TYPE_RINGBUF)
> > +#define DYNPTR_TYPE_FLAG_MASK (DYNPTR_TYPE_LOCAL | DYNPTR_TYPE_RINGBUF | DYNPTR_TYPE_SKB)
> >
>
> I wonder if we could maximize the use of these flags by combining them
> with other base types, not just DYNPTR. For example, does TYPE_LOCAL
> indicate memory is on stack? If so, can we apply LOCAL on PTR_TO_MEM?
> If we have PTR_TO_MEM + LOCAL, can it be used to replace PTR_TO_STACK
> in some scenarios?
>
> WDYT?
Hi Hao. I love the idea but unfortunately I don't think it applies
neatly in this case. "local" in the context of dynptrs means memory
that is local to the bpf program (eg includes not just memory on the
stack).
>
> > /* Max number of base types. */
> > #define BPF_BASE_TYPE_LIMIT (1UL << BPF_BASE_TYPE_BITS)
> > @@ -2556,12 +2559,15 @@ enum bpf_dynptr_type {
> > BPF_DYNPTR_TYPE_LOCAL,
> > /* Underlying data is a ringbuf record */
> > BPF_DYNPTR_TYPE_RINGBUF,
> > + /* Underlying data is a sk_buff */
> > + BPF_DYNPTR_TYPE_SKB,
> > };
> >
> <...>
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > --
> > 2.30.2
> >
