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?
> /* 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
>
