[PATCH bpf-next 00/13] Extend skb dynptr for metadata access from TC

[PATCH bpf-next 00/13] Extend skb dynptr for metadata access from TC

[Jakub Sitnicki]

TL;DR
-----

This is the first step in an effort which aims to enable skb metadata
access for all BPF programs which operate on an skb context.

By skb metadata we mean the custom metadata area which can be allocated
from an XDP program with the bpf_xdp_adjust_meta helper. Network stack code
accesses it using the skb_metadata_* helpers.

Overview
--------

Today, the skb metadata is accessible only by the BPF TC ingress programs
through the __sk_buff->data_meta pointer. We propose a three step plan to
make skb metadata available to all other BPF programs which operate on skb
objects:

 1) Extend skb dynptr for metadata access from TC (this patch set)

    This is a preparatory step, but it also stands on its own. Here we
    enable access to the skb metadata through a bpf_dynptr, the same way we
    can already access the skb payload today.

    In the next step (2) we plan to relocate the metadata as skb travels
    through the network stack. That will require a safe way to access the
    metadata area irrespective of its location.

    The checks relying on pointer arithmetic - __sk_buff->data_meta and
    ->data - were not built for that. They require the metadata to be
    located right in front of the payload. Otherwise their guarantees break
    down.

    This is where the dynptr [1] comes into play. It solves exactly that
    problem. The dynptr to skb metadata can be backed by a memory area that
    resides in a different location depending on code path.

 2) Persist skb metadata past the TC hook (future)

    Keeping the metadata in front of the packet headers as the skb travels
    through the network stack is problematic - see the discussion of
    alternative approaches below. Hence, we plan to relocate as necessary
    after the TC hook.

    Where to? We don't know yet. There are a couple of options: (i) move it
    to the top of skb headroom, or (ii) allocate dedicated memory for it.
    They are not mutually exclusive. The right solution might be a mix.

    When? That is also an open question. It could be done on device to
    protocol handover or lazily when headers get pushed or headroom gets
    resized.

 3) skb dynptr for sockops, sk_lookup, etc. (future)

    There are BPF program types which don't get an __sk_buff as a context,
    but they either have, or could have in some cases, access to the skb
    itself. As a final touch, we want to provide a way to create an skb
    dynptr from these special contexts.

TIMTOWDI
--------

Alternative approaches which we considered:

* Keep the metadata always in front of skb->data

We think it is a bad idea for two reasons, outlined below. Nevertheless we
are open to it if necessary.

 1) Performance concerns

    It would require the network stack to move the metadata on each header
    pull/push (see skb_reorder_vlan_header() for an example). While doable,
    there is an expected performance overhead.

 2) Potential for bugs

    In addition to updating skb_push/pull and pskp_expand_head, we would
    need to audit any code paths which operate on skb->data pointer
    directly without going through the helper. This creates a "known
    unknown" risk.

* Design a new custom metadata area from scratch

We have tried that in Arthur's patch set [2]. One of the outcomes of the
discussion there was that we don't want to have two places to store custom
metadata. Hence the change of approach.

-jkbs

PS. This series is not as long as it looks. I kept the more granular commit
split to "show the work". I can squash some together if needed.

[1] https://docs.ebpf.io/linux/concepts/dynptrs/
[2] https://lore.kernel.org/all/20250422-afabre-traits-010-rfc2-v2-0-92bcc6b146c9@arthurfabre.com/

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
Jakub Sitnicki (13):
      bpf: Ignore dynptr offset in skb data access
      bpf: Helpers for skb dynptr read/write/slice
      bpf: Add new variant of skb dynptr for the metadata area
      bpf: Enable read access to skb metadata with bpf_dynptr_read
      bpf: Enable write access to skb metadata with bpf_dynptr_write
      bpf: Enable read-write access to skb metadata with dynptr slice
      net: Clear skb metadata on handover from device to protocol
      selftests/bpf: Pass just bpf_map to xdp_context_test helper
      selftests/bpf: Parametrize test_xdp_context_tuntap
      selftests/bpf: Cover read access to skb metadata via dynptr
      selftests/bpf: Cover write access to skb metadata via dynptr
      selftests/bpf: Cover lack of access to skb metadata at ip layer
      selftests/bpf: Count successful bpf program runs

 include/linux/filter.h                             |  25 ++-
 include/uapi/linux/bpf.h                           |   9 +
 kernel/bpf/helpers.c                               |  10 +-
 net/core/dev.c                                     |   1 +
 net/core/filter.c                                  | 104 +++++++++--
 tools/include/uapi/linux/bpf.h                     |   9 +
 .../bpf/prog_tests/xdp_context_test_run.c          | 194 +++++++++++++++++----
 tools/testing/selftests/bpf/progs/test_xdp_meta.c  | 171 ++++++++++++++++--
 tools/testing/selftests/bpf/test_progs.h           |   1 +
 9 files changed, 446 insertions(+), 78 deletions(-)

[PATCH bpf-next 01/13] bpf: Ignore dynptr offset in skb data access

[Jakub Sitnicki]

Prepare to use (struct bpf_dynptr)->offset to distinguish between an skb
dynptr for the payload vs the metadata area.

ptr->offset is always set to zero by bpf_dynptr_from_skb(). We don't need
to account for it on access.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 kernel/bpf/helpers.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index f48fa3fe8dec..40c18b37ab05 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1776,7 +1776,7 @@ static int __bpf_dynptr_read(void *dst, u32 len, const struct bpf_dynptr_kern *s
 		memmove(dst, src->data + src->offset + offset, len);
 		return 0;
 	case BPF_DYNPTR_TYPE_SKB:
-		return __bpf_skb_load_bytes(src->data, src->offset + offset, dst, len);
+		return __bpf_skb_load_bytes(src->data, offset, dst, len);
 	case BPF_DYNPTR_TYPE_XDP:
 		return __bpf_xdp_load_bytes(src->data, src->offset + offset, dst, len);
 	default:
@@ -1829,8 +1829,7 @@ int __bpf_dynptr_write(const struct bpf_dynptr_kern *dst, u32 offset, void *src,
 		memmove(dst->data + dst->offset + offset, src, len);
 		return 0;
 	case BPF_DYNPTR_TYPE_SKB:
-		return __bpf_skb_store_bytes(dst->data, dst->offset + offset, src, len,
-					     flags);
+		return __bpf_skb_store_bytes(dst->data, offset, src, len, flags);
 	case BPF_DYNPTR_TYPE_XDP:
 		if (flags)
 			return -EINVAL;
@@ -2695,9 +2694,9 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr *p, u32 offset,
 		return ptr->data + ptr->offset + offset;
 	case BPF_DYNPTR_TYPE_SKB:
 		if (buffer__opt)
-			return skb_header_pointer(ptr->data, ptr->offset + offset, len, buffer__opt);
+			return skb_header_pointer(ptr->data, offset, len, buffer__opt);
 		else
-			return skb_pointer_if_linear(ptr->data, ptr->offset + offset, len);
+			return skb_pointer_if_linear(ptr->data, offset, len);
 	case BPF_DYNPTR_TYPE_XDP:
 	{
 		void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);

-- 
2.43.0

[PATCH bpf-next 02/13] bpf: Helpers for skb dynptr read/write/slice

[Jakub Sitnicki]

Prepare to handle skb dynptrs for accessing the metadata area.

Code move. No observable changes.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 include/linux/filter.h | 25 ++++++++++++++++++-------
 kernel/bpf/helpers.c   |  9 +++------
 net/core/filter.c      | 38 +++++++++++++++++++++++++++-----------
 3 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index eca229752cbe..468d83604241 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1764,27 +1764,38 @@ static __always_inline long __bpf_xdp_redirect_map(struct bpf_map *map, u64 inde
 }
 
 #ifdef CONFIG_NET
-int __bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset, void *to, u32 len);
-int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
-			  u32 len, u64 flags);
+int bpf_dynptr_skb_read(const struct bpf_dynptr_kern *src, u32 offset,
+			void *dst, u32 len);
+int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst, u32 offset,
+			 const void *src, u32 len, u64 flags);
+void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
+			   void *buf, u32 len);
 int __bpf_xdp_load_bytes(struct xdp_buff *xdp, u32 offset, void *buf, u32 len);
 int __bpf_xdp_store_bytes(struct xdp_buff *xdp, u32 offset, void *buf, u32 len);
 void *bpf_xdp_pointer(struct xdp_buff *xdp, u32 offset, u32 len);
 void bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off,
 		      void *buf, unsigned long len, bool flush);
 #else /* CONFIG_NET */
-static inline int __bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset,
-				       void *to, u32 len)
+static inline int bpf_dynptr_skb_read(const struct bpf_dynptr_kern *src,
+				      u32 offset, void *dst, u32 len)
 {
 	return -EOPNOTSUPP;
 }
 
-static inline int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset,
-					const void *from, u32 len, u64 flags)
+static inline int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst,
+				       u32 offset, const void *src, u32 len,
+				       u64 flags);
 {
 	return -EOPNOTSUPP;
 }
 
+static inline void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr,
+					 u32 offset, void *buf, u32 len);
+
+{
+	return NULL;
+}
+
 static inline int __bpf_xdp_load_bytes(struct xdp_buff *xdp, u32 offset,
 				       void *buf, u32 len)
 {
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 40c18b37ab05..da9c6ccd7cd7 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1776,7 +1776,7 @@ static int __bpf_dynptr_read(void *dst, u32 len, const struct bpf_dynptr_kern *s
 		memmove(dst, src->data + src->offset + offset, len);
 		return 0;
 	case BPF_DYNPTR_TYPE_SKB:
-		return __bpf_skb_load_bytes(src->data, offset, dst, len);
+		return bpf_dynptr_skb_read(src, offset, dst, len);
 	case BPF_DYNPTR_TYPE_XDP:
 		return __bpf_xdp_load_bytes(src->data, src->offset + offset, dst, len);
 	default:
@@ -1829,7 +1829,7 @@ int __bpf_dynptr_write(const struct bpf_dynptr_kern *dst, u32 offset, void *src,
 		memmove(dst->data + dst->offset + offset, src, len);
 		return 0;
 	case BPF_DYNPTR_TYPE_SKB:
-		return __bpf_skb_store_bytes(dst->data, offset, src, len, flags);
+		return bpf_dynptr_skb_write(dst, offset, src, len, flags);
 	case BPF_DYNPTR_TYPE_XDP:
 		if (flags)
 			return -EINVAL;
@@ -2693,10 +2693,7 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr *p, u32 offset,
 	case BPF_DYNPTR_TYPE_RINGBUF:
 		return ptr->data + ptr->offset + offset;
 	case BPF_DYNPTR_TYPE_SKB:
-		if (buffer__opt)
-			return skb_header_pointer(ptr->data, offset, len, buffer__opt);
-		else
-			return skb_pointer_if_linear(ptr->data, offset, len);
+		return bpf_dynptr_skb_slice(ptr, offset, buffer__opt, len);
 	case BPF_DYNPTR_TYPE_XDP:
 	{
 		void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset + offset, len);
diff --git a/net/core/filter.c b/net/core/filter.c
index 7a72f766aacf..1fee51b72220 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1736,12 +1736,6 @@ static const struct bpf_func_proto bpf_skb_store_bytes_proto = {
 	.arg5_type	= ARG_ANYTHING,
 };
 
-int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
-			  u32 len, u64 flags)
-{
-	return ____bpf_skb_store_bytes(skb, offset, from, len, flags);
-}
-
 BPF_CALL_4(bpf_skb_load_bytes, const struct sk_buff *, skb, u32, offset,
 	   void *, to, u32, len)
 {
@@ -1772,11 +1766,6 @@ static const struct bpf_func_proto bpf_skb_load_bytes_proto = {
 	.arg4_type	= ARG_CONST_SIZE,
 };
 
-int __bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset, void *to, u32 len)
-{
-	return ____bpf_skb_load_bytes(skb, offset, to, len);
-}
-
 BPF_CALL_4(bpf_flow_dissector_load_bytes,
 	   const struct bpf_flow_dissector *, ctx, u32, offset,
 	   void *, to, u32, len)
@@ -11978,6 +11967,33 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	return func;
 }
 
+int bpf_dynptr_skb_read(const struct bpf_dynptr_kern *src, u32 offset,
+			void *dst, u32 len)
+{
+	const struct sk_buff *skb = src->data;
+
+	return ____bpf_skb_load_bytes(skb, offset, dst, len);
+}
+
+int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst, u32 offset,
+			 const void *src, u32 len, u64 flags)
+{
+	struct sk_buff *skb = dst->data;
+
+	return ____bpf_skb_store_bytes(skb, offset, src, len, flags);
+}
+
+void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
+			   void *buf, u32 len)
+{
+	const struct sk_buff *skb = ptr->data;
+
+	if (buf)
+		return skb_header_pointer(skb, offset, len, buf);
+	else
+		return skb_pointer_if_linear(skb, offset, len);
+}
+
 __bpf_kfunc_start_defs();
 __bpf_kfunc int bpf_dynptr_from_skb(struct __sk_buff *s, u64 flags,
 				    struct bpf_dynptr *ptr__uninit)

-- 
2.43.0

[PATCH bpf-next 03/13] bpf: Add new variant of skb dynptr for the metadata area

[Jakub Sitnicki]

Add a new flag for the bpf_dynptr_from_skb helper to let users to create
dynptrs to skb metadata area. Access paths are stubbed out. Implemented by
the following changes.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 include/uapi/linux/bpf.h |  9 ++++++++
 net/core/filter.c        | 60 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 719ba230032f..ab5730d2fb29 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7591,4 +7591,13 @@ enum bpf_kfunc_flags {
 	BPF_F_PAD_ZEROS = (1ULL << 0),
 };
 
+/**
+ * enum bpf_dynptr_from_skb_flags - Flags for bpf_dynptr_from_skb()
+ *
+ * @BPF_DYNPTR_F_SKB_METADATA: Create dynptr to the SKB metadata area
+ */
+enum bpf_dynptr_from_skb_flags {
+	BPF_DYNPTR_F_SKB_METADATA = (1ULL << 0),
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/net/core/filter.c b/net/core/filter.c
index 1fee51b72220..3c2948517838 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11967,12 +11967,27 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	return func;
 }
 
+enum skb_dynptr_offset {
+	SKB_DYNPTR_METADATA	= -1,
+	SKB_DYNPTR_PAYLOAD	= 0,
+};
+
 int bpf_dynptr_skb_read(const struct bpf_dynptr_kern *src, u32 offset,
 			void *dst, u32 len)
 {
 	const struct sk_buff *skb = src->data;
 
-	return ____bpf_skb_load_bytes(skb, offset, dst, len);
+	switch (src->offset) {
+	case SKB_DYNPTR_PAYLOAD:
+		return ____bpf_skb_load_bytes(skb, offset, dst, len);
+
+	case SKB_DYNPTR_METADATA:
+		return -EOPNOTSUPP; /* not implemented */
+
+	default:
+		WARN_ONCE(true, "%s: unknown skb dynptr offset %d\n", __func__, src->offset);
+		return -EFAULT;
+	}
 }
 
 int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst, u32 offset,
@@ -11980,7 +11995,17 @@ int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst, u32 offset,
 {
 	struct sk_buff *skb = dst->data;
 
-	return ____bpf_skb_store_bytes(skb, offset, src, len, flags);
+	switch (dst->offset) {
+	case SKB_DYNPTR_PAYLOAD:
+		return ____bpf_skb_store_bytes(skb, offset, src, len, flags);
+
+	case SKB_DYNPTR_METADATA:
+		return -EOPNOTSUPP; /* not implemented */
+
+	default:
+		WARN_ONCE(true, "%s: unknown skb dynptr offset %d\n", __func__, dst->offset);
+		return -EFAULT;
+	}
 }
 
 void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
@@ -11988,10 +12013,20 @@ void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
 {
 	const struct sk_buff *skb = ptr->data;
 
-	if (buf)
-		return skb_header_pointer(skb, offset, len, buf);
-	else
-		return skb_pointer_if_linear(skb, offset, len);
+	switch (ptr->offset) {
+	case SKB_DYNPTR_PAYLOAD:
+		if (buf)
+			return skb_header_pointer(skb, offset, len, buf);
+		else
+			return skb_pointer_if_linear(skb, offset, len);
+
+	case SKB_DYNPTR_METADATA:
+		return NULL;	/* not implemented */
+
+	default:
+		WARN_ONCE(true, "%s: unknown skb dynptr offset %d\n", __func__, ptr->offset);
+		return NULL;
+	}
 }
 
 __bpf_kfunc_start_defs();
@@ -12000,13 +12035,22 @@ __bpf_kfunc int bpf_dynptr_from_skb(struct __sk_buff *s, u64 flags,
 {
 	struct bpf_dynptr_kern *ptr = (struct bpf_dynptr_kern *)ptr__uninit;
 	struct sk_buff *skb = (struct sk_buff *)s;
+	u32 offset, size;
 
-	if (flags) {
+	if (unlikely(flags & ~BPF_DYNPTR_F_SKB_METADATA)) {
 		bpf_dynptr_set_null(ptr);
 		return -EINVAL;
 	}
 
-	bpf_dynptr_init(ptr, skb, BPF_DYNPTR_TYPE_SKB, 0, skb->len);
+	if (flags & BPF_DYNPTR_F_SKB_METADATA) {
+		offset = SKB_DYNPTR_METADATA;
+		size = skb_metadata_len(skb);
+	} else {
+		offset = SKB_DYNPTR_PAYLOAD;
+		size = skb->len;
+	}
+
+	bpf_dynptr_init(ptr, skb, BPF_DYNPTR_TYPE_SKB, offset, size);
 
 	return 0;
 }

-- 
2.43.0

[PATCH bpf-next 04/13] bpf: Enable read access to skb metadata with bpf_dynptr_read

[Jakub Sitnicki]

Make it possible to read from skb metadata area using the
bpf_dynptr_read() BPF helper.

This prepares ground for access to skb metadata from all BPF hooks
which operate on __sk_buff context.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/filter.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 3c2948517838..f71b4b6b09fb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11981,9 +11981,15 @@ int bpf_dynptr_skb_read(const struct bpf_dynptr_kern *src, u32 offset,
 	case SKB_DYNPTR_PAYLOAD:
 		return ____bpf_skb_load_bytes(skb, offset, dst, len);
 
-	case SKB_DYNPTR_METADATA:
-		return -EOPNOTSUPP; /* not implemented */
+	case SKB_DYNPTR_METADATA: {
+		u32 meta_len = skb_metadata_len(skb);
+
+		if (len > meta_len || offset > meta_len - len)
+			return -E2BIG; /* out of bounds */
 
+		memmove(dst, skb_metadata_end(skb) - meta_len + offset, len);
+		return 0;
+	}
 	default:
 		WARN_ONCE(true, "%s: unknown skb dynptr offset %d\n", __func__, src->offset);
 		return -EFAULT;

-- 
2.43.0

[PATCH bpf-next 05/13] bpf: Enable write access to skb metadata with bpf_dynptr_write

[Jakub Sitnicki]

Make it possible to write to skb metadata area using the
bpf_dynptr_write() BPF helper.

This prepares ground for access to skb metadata from all BPF hooks
which operate on __sk_buff context.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/filter.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index f71b4b6b09fb..ab6599f42bb7 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12005,8 +12005,15 @@ int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst, u32 offset,
 	case SKB_DYNPTR_PAYLOAD:
 		return ____bpf_skb_store_bytes(skb, offset, src, len, flags);
 
-	case SKB_DYNPTR_METADATA:
-		return -EOPNOTSUPP; /* not implemented */
+	case SKB_DYNPTR_METADATA: {
+		u32 meta_len = skb_metadata_len(skb);
+
+		if (len > meta_len || offset > meta_len - len)
+			return -E2BIG; /* out of bounds */
+
+		memmove(skb_metadata_end(skb) - meta_len + offset, src, len);
+		return 0;
+	}
 
 	default:
 		WARN_ONCE(true, "%s: unknown skb dynptr offset %d\n", __func__, dst->offset);

-- 
2.43.0

[PATCH bpf-next 06/13] bpf: Enable read-write access to skb metadata with dynptr slice

[Jakub Sitnicki]

Make it possible to read from or write to skb metadata area using the
dynptr slices creates with bpf_dynptr_slice() or bpf_dynptr_slice_rdwr().

This prepares ground for access to skb metadata from all BPF hooks
which operate on __sk_buff context.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/filter.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index ab6599f42bb7..020da46f93a7 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12033,9 +12033,14 @@ void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
 		else
 			return skb_pointer_if_linear(skb, offset, len);
 
-	case SKB_DYNPTR_METADATA:
-		return NULL;	/* not implemented */
+	case SKB_DYNPTR_METADATA: {
+		u32 meta_len = skb_metadata_len(skb);
 
+		if (len > meta_len || offset > meta_len - len)
+			return NULL; /* out of bounds */
+
+		return skb_metadata_end(skb) - meta_len + offset;
+	}
 	default:
 		WARN_ONCE(true, "%s: unknown skb dynptr offset %d\n", __func__, ptr->offset);
 		return NULL;

-- 
2.43.0

[PATCH bpf-next 07/13] net: Clear skb metadata on handover from device to protocol

[Jakub Sitnicki]

With the extension of bpf_dynptr_from_skb(BPF_DYNPTR_F_SKB_METADATA), all
BPF programs authorized to call this kfunc now have access to the skb
metadata area.

These programs can read up to skb_shinfo(skb)->meta_len bytes located just
before skb_mac_header(skb), regardless of what data is currently there.

However, as the network stack processes the skb, headers may be added or
removed. Hence, we cannot assume that skb_mac_header() always marks the end
of the metadata area.

To avoid potential pitfalls, reset the skb metadata length to zero before
passing the skb to the protocol layers. This is a temporary measure until
we can make metadata persist through protocol processing.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index be97c440ecd5..4a2389997535 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5839,6 +5839,7 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
 	}
 #endif
 	skb_reset_redirect(skb);
+	skb_metadata_clear(skb);
 skip_classify:
 	if (pfmemalloc && !skb_pfmemalloc_protocol(skb))
 		goto drop;

-- 
2.43.0

[PATCH bpf-next 08/13] selftests/bpf: Pass just bpf_map to xdp_context_test helper

[Jakub Sitnicki]

Prepare for parametrizing the xdp_context tests. The assert_test_result
helper doesn't need the whole skeleton. Pass just what it needs.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
index b9d9f0a502ce..0134651d94ab 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -156,15 +156,14 @@ static int send_test_packet(int ifindex)
 	return -1;
 }
 
-static void assert_test_result(struct test_xdp_meta *skel)
+static void assert_test_result(const struct bpf_map *result_map)
 {
 	int err;
 	__u32 map_key = 0;
 	__u8 map_value[TEST_PAYLOAD_LEN];
 
-	err = bpf_map__lookup_elem(skel->maps.test_result, &map_key,
-				   sizeof(map_key), &map_value,
-				   TEST_PAYLOAD_LEN, BPF_ANY);
+	err = bpf_map__lookup_elem(result_map, &map_key, sizeof(map_key),
+				   &map_value, TEST_PAYLOAD_LEN, BPF_ANY);
 	if (!ASSERT_OK(err, "lookup test_result"))
 		return;
 
@@ -248,7 +247,7 @@ void test_xdp_context_veth(void)
 	if (!ASSERT_OK(ret, "send_test_packet"))
 		goto close;
 
-	assert_test_result(skel);
+	assert_test_result(skel->maps.test_result);
 
 close:
 	close_netns(nstoken);
@@ -313,7 +312,7 @@ void test_xdp_context_tuntap(void)
 	if (!ASSERT_EQ(ret, sizeof(packet), "write packet"))
 		goto close;
 
-	assert_test_result(skel);
+	assert_test_result(skel->maps.test_result);
 
 close:
 	if (tap_fd >= 0)

-- 
2.43.0

[PATCH bpf-next 09/13] selftests/bpf: Parametrize test_xdp_context_tuntap

[Jakub Sitnicki]

We want to add more test cases to cover different ways to access the
metadata area. Prepare for it. Pull up the skeleton management.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../bpf/prog_tests/xdp_context_test_run.c          | 31 +++++++++++++++-------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
index 0134651d94ab..6c66e27e5bc7 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -256,12 +256,13 @@ void test_xdp_context_veth(void)
 	netns_free(tx_ns);
 }
 
-void test_xdp_context_tuntap(void)
+static void test_tuntap(struct bpf_program *xdp_prog,
+			struct bpf_program *tc_prog,
+			struct bpf_map *result_map)
 {
 	LIBBPF_OPTS(bpf_tc_hook, tc_hook, .attach_point = BPF_TC_INGRESS);
 	LIBBPF_OPTS(bpf_tc_opts, tc_opts, .handle = 1, .priority = 1);
 	struct netns_obj *ns = NULL;
-	struct test_xdp_meta *skel = NULL;
 	__u8 packet[sizeof(struct ethhdr) + TEST_PAYLOAD_LEN];
 	int tap_fd = -1;
 	int tap_ifindex;
@@ -277,10 +278,6 @@ void test_xdp_context_tuntap(void)
 
 	SYS(close, "ip link set dev " TAP_NAME " up");
 
-	skel = test_xdp_meta__open_and_load();
-	if (!ASSERT_OK_PTR(skel, "open and load skeleton"))
-		goto close;
-
 	tap_ifindex = if_nametoindex(TAP_NAME);
 	if (!ASSERT_GE(tap_ifindex, 0, "if_nametoindex"))
 		goto close;
@@ -290,12 +287,12 @@ void test_xdp_context_tuntap(void)
 	if (!ASSERT_OK(ret, "bpf_tc_hook_create"))
 		goto close;
 
-	tc_opts.prog_fd = bpf_program__fd(skel->progs.ing_cls);
+	tc_opts.prog_fd = bpf_program__fd(tc_prog);
 	ret = bpf_tc_attach(&tc_hook, &tc_opts);
 	if (!ASSERT_OK(ret, "bpf_tc_attach"))
 		goto close;
 
-	ret = bpf_xdp_attach(tap_ifindex, bpf_program__fd(skel->progs.ing_xdp),
+	ret = bpf_xdp_attach(tap_ifindex, bpf_program__fd(xdp_prog),
 			     0, NULL);
 	if (!ASSERT_GE(ret, 0, "bpf_xdp_attach"))
 		goto close;
@@ -312,11 +309,25 @@ void test_xdp_context_tuntap(void)
 	if (!ASSERT_EQ(ret, sizeof(packet), "write packet"))
 		goto close;
 
-	assert_test_result(skel->maps.test_result);
+	assert_test_result(result_map);
 
 close:
 	if (tap_fd >= 0)
 		close(tap_fd);
-	test_xdp_meta__destroy(skel);
 	netns_free(ns);
 }
+
+void test_xdp_context_tuntap(void)
+{
+	struct test_xdp_meta *skel = NULL;
+
+	skel = test_xdp_meta__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open and load skeleton"))
+		return;
+
+	if (test__start_subtest("data_meta"))
+		test_tuntap(skel->progs.ing_xdp, skel->progs.ing_cls,
+			    skel->maps.test_result);
+
+	test_xdp_meta__destroy(skel);
+}

-- 
2.43.0

[PATCH bpf-next 10/13] selftests/bpf: Cover read access to skb metadata via dynptr

[Jakub Sitnicki]

Exercise reading from SKB metadata area in two new ways:
1. indirectly, with bpf_dynptr_read(), and
2. directly, with bpf_dynptr_slice().

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/include/uapi/linux/bpf.h                     |  9 +++++
 .../bpf/prog_tests/xdp_context_test_run.c          | 21 +++++++++++
 tools/testing/selftests/bpf/progs/test_xdp_meta.c  | 42 ++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 719ba230032f..ab5730d2fb29 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -7591,4 +7591,13 @@ enum bpf_kfunc_flags {
 	BPF_F_PAD_ZEROS = (1ULL << 0),
 };
 
+/**
+ * enum bpf_dynptr_from_skb_flags - Flags for bpf_dynptr_from_skb()
+ *
+ * @BPF_DYNPTR_F_SKB_METADATA: Create dynptr to the SKB metadata area
+ */
+enum bpf_dynptr_from_skb_flags {
+	BPF_DYNPTR_F_SKB_METADATA = (1ULL << 0),
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
index 6c66e27e5bc7..7e4526461a4c 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -171,6 +171,18 @@ static void assert_test_result(const struct bpf_map *result_map)
 		     "test_result map contains test payload");
 }
 
+static bool clear_test_result(struct bpf_map *result_map)
+{
+	const __u8 v[sizeof(test_payload)] = {};
+	const __u32 k = 0;
+	int err;
+
+	err = bpf_map__update_elem(result_map, &k, sizeof(k), v, sizeof(v), BPF_ANY);
+	ASSERT_OK(err, "update test_result");
+
+	return err == 0;
+}
+
 void test_xdp_context_veth(void)
 {
 	LIBBPF_OPTS(bpf_tc_hook, tc_hook, .attach_point = BPF_TC_INGRESS);
@@ -268,6 +280,9 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 	int tap_ifindex;
 	int ret;
 
+	if (!clear_test_result(result_map))
+		return;
+
 	ns = netns_new(TAP_NETNS, true);
 	if (!ASSERT_OK_PTR(ns, "create and open ns"))
 		return;
@@ -328,6 +343,12 @@ void test_xdp_context_tuntap(void)
 	if (test__start_subtest("data_meta"))
 		test_tuntap(skel->progs.ing_xdp, skel->progs.ing_cls,
 			    skel->maps.test_result);
+	if (test__start_subtest("dynptr_read"))
+		test_tuntap(skel->progs.ing_xdp, skel->progs.ing_cls_dynptr_read,
+			    skel->maps.test_result);
+	if (test__start_subtest("dynptr_slice"))
+		test_tuntap(skel->progs.ing_xdp, skel->progs.ing_cls_dynptr_slice,
+			    skel->maps.test_result);
 
 	test_xdp_meta__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_meta.c b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
index fcf6ca14f2ea..6b11134520be 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_meta.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
@@ -1,8 +1,10 @@
+#include <stdbool.h>
 #include <linux/bpf.h>
 #include <linux/if_ether.h>
 #include <linux/pkt_cls.h>
 
 #include <bpf/bpf_helpers.h>
+#include "bpf_kfuncs.h"
 
 #define META_SIZE 32
 
@@ -40,6 +42,46 @@ int ing_cls(struct __sk_buff *ctx)
 	return TC_ACT_SHOT;
 }
 
+/* Read from metadata using bpf_dynptr_read helper */
+SEC("tc")
+int ing_cls_dynptr_read(struct __sk_buff *ctx)
+{
+	struct bpf_dynptr meta;
+	const __u32 zero = 0;
+	__u8 *dst;
+
+	dst = bpf_map_lookup_elem(&test_result, &zero);
+	if (!dst)
+		return TC_ACT_SHOT;
+
+	bpf_dynptr_from_skb(ctx, BPF_DYNPTR_F_SKB_METADATA, &meta);
+	bpf_dynptr_read(dst, META_SIZE, &meta, 0, 0);
+
+	return TC_ACT_SHOT;
+}
+
+/* Read from metadata using read-only dynptr slice */
+SEC("tc")
+int ing_cls_dynptr_slice(struct __sk_buff *ctx)
+{
+	struct bpf_dynptr meta;
+	const __u32 zero = 0;
+	__u8 *dst, *src;
+
+	dst = bpf_map_lookup_elem(&test_result, &zero);
+	if (!dst)
+		return TC_ACT_SHOT;
+
+	bpf_dynptr_from_skb(ctx, BPF_DYNPTR_F_SKB_METADATA, &meta);
+	src = bpf_dynptr_slice(&meta, 0, NULL, META_SIZE);
+	if (!src)
+		return TC_ACT_SHOT;
+
+	__builtin_memcpy(dst, src, META_SIZE);
+
+	return TC_ACT_SHOT;
+}
+
 SEC("xdp")
 int ing_xdp(struct xdp_md *ctx)
 {

-- 
2.43.0

[PATCH bpf-next 11/13] selftests/bpf: Cover write access to skb metadata via dynptr

[Jakub Sitnicki]

Add tests what exercise writes to skb metadata in two ways:
1. indirectly, using bpf_dynptr_write helper,
2. directly, using a read-write dynptr slice.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../bpf/prog_tests/xdp_context_test_run.c          | 36 ++++++++++--
 tools/testing/selftests/bpf/progs/test_xdp_meta.c  | 67 ++++++++++++++++++++++
 2 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
index 7e4526461a4c..79c4c58276e6 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -269,7 +269,8 @@ void test_xdp_context_veth(void)
 }
 
 static void test_tuntap(struct bpf_program *xdp_prog,
-			struct bpf_program *tc_prog,
+			struct bpf_program *tc_prio_1_prog,
+			struct bpf_program *tc_prio_2_prog,
 			struct bpf_map *result_map)
 {
 	LIBBPF_OPTS(bpf_tc_hook, tc_hook, .attach_point = BPF_TC_INGRESS);
@@ -302,11 +303,20 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 	if (!ASSERT_OK(ret, "bpf_tc_hook_create"))
 		goto close;
 
-	tc_opts.prog_fd = bpf_program__fd(tc_prog);
+	tc_opts.prog_fd = bpf_program__fd(tc_prio_1_prog);
 	ret = bpf_tc_attach(&tc_hook, &tc_opts);
 	if (!ASSERT_OK(ret, "bpf_tc_attach"))
 		goto close;
 
+	if (tc_prio_2_prog) {
+		LIBBPF_OPTS(bpf_tc_opts, tc_opts, .handle = 1, .priority = 2,
+			    .prog_fd = bpf_program__fd(tc_prio_2_prog));
+
+		ret = bpf_tc_attach(&tc_hook, &tc_opts);
+		if (!ASSERT_OK(ret, "bpf_tc_attach"))
+			goto close;
+	}
+
 	ret = bpf_xdp_attach(tap_ifindex, bpf_program__fd(xdp_prog),
 			     0, NULL);
 	if (!ASSERT_GE(ret, 0, "bpf_xdp_attach"))
@@ -341,13 +351,29 @@ void test_xdp_context_tuntap(void)
 		return;
 
 	if (test__start_subtest("data_meta"))
-		test_tuntap(skel->progs.ing_xdp, skel->progs.ing_cls,
+		test_tuntap(skel->progs.ing_xdp,
+			    skel->progs.ing_cls,
+			    NULL, /* tc prio 2 */
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_read"))
-		test_tuntap(skel->progs.ing_xdp, skel->progs.ing_cls_dynptr_read,
+		test_tuntap(skel->progs.ing_xdp,
+			    skel->progs.ing_cls_dynptr_read,
+			    NULL, /* tc prio 2 */
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_slice"))
-		test_tuntap(skel->progs.ing_xdp, skel->progs.ing_cls_dynptr_slice,
+		test_tuntap(skel->progs.ing_xdp,
+			    skel->progs.ing_cls_dynptr_slice,
+			    NULL, /* tc prio 2 */
+			    skel->maps.test_result);
+	if (test__start_subtest("dynptr_write"))
+		test_tuntap(skel->progs.ing_xdp_zalloc_meta,
+			    skel->progs.ing_cls_dynptr_write,
+			    skel->progs.ing_cls_dynptr_read,
+			    skel->maps.test_result);
+	if (test__start_subtest("dynptr_slice_rdwr"))
+		test_tuntap(skel->progs.ing_xdp_zalloc_meta,
+			    skel->progs.ing_cls_dynptr_slice_rdwr,
+			    skel->progs.ing_cls_dynptr_slice,
 			    skel->maps.test_result);
 
 	test_xdp_meta__destroy(skel);
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_meta.c b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
index 6b11134520be..b6fed72b1005 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_meta.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
@@ -60,6 +60,24 @@ int ing_cls_dynptr_read(struct __sk_buff *ctx)
 	return TC_ACT_SHOT;
 }
 
+/* Write to metadata using bpf_dynptr_write helper */
+SEC("tc")
+int ing_cls_dynptr_write(struct __sk_buff *ctx)
+{
+	struct bpf_dynptr data, meta;
+	__u8 *src;
+
+	bpf_dynptr_from_skb(ctx, 0, &data);
+	src = bpf_dynptr_slice(&data, sizeof(struct ethhdr), NULL, META_SIZE);
+	if (!src)
+		return TC_ACT_SHOT;
+
+	bpf_dynptr_from_skb(ctx, BPF_DYNPTR_F_SKB_METADATA, &meta);
+	bpf_dynptr_write(&meta, 0, src, META_SIZE, 0);
+
+	return TC_ACT_UNSPEC; /* pass */
+}
+
 /* Read from metadata using read-only dynptr slice */
 SEC("tc")
 int ing_cls_dynptr_slice(struct __sk_buff *ctx)
@@ -82,6 +100,55 @@ int ing_cls_dynptr_slice(struct __sk_buff *ctx)
 	return TC_ACT_SHOT;
 }
 
+/* Write to metadata using writeable dynptr slice */
+SEC("tc")
+int ing_cls_dynptr_slice_rdwr(struct __sk_buff *ctx)
+{
+	struct bpf_dynptr data, meta;
+	__u8 *src, *dst;
+
+	bpf_dynptr_from_skb(ctx, 0, &data);
+	src = bpf_dynptr_slice(&data, sizeof(struct ethhdr), NULL, META_SIZE);
+	if (!src)
+		return TC_ACT_SHOT;
+
+	bpf_dynptr_from_skb(ctx, BPF_DYNPTR_F_SKB_METADATA, &meta);
+	dst = bpf_dynptr_slice_rdwr(&meta, 0, NULL, META_SIZE);
+	if (!dst)
+		return TC_ACT_SHOT;
+
+	__builtin_memcpy(dst, src, META_SIZE);
+
+	return TC_ACT_UNSPEC; /* pass */
+}
+
+/* Reserve and clear space for metadata but don't populate it */
+SEC("xdp")
+int ing_xdp_zalloc_meta(struct xdp_md *ctx)
+{
+	struct ethhdr *eth = ctx_ptr(ctx, data);
+	__u8 *meta;
+	int ret;
+
+	/* Drop any non-test packets */
+	if (eth + 1 > ctx_ptr(ctx, data_end))
+		return XDP_DROP;
+	if (eth->h_proto != 0)
+		return XDP_DROP;
+
+	ret = bpf_xdp_adjust_meta(ctx, -META_SIZE);
+	if (ret < 0)
+		return XDP_DROP;
+
+	meta = ctx_ptr(ctx, data_meta);
+	if (meta + META_SIZE > ctx_ptr(ctx, data))
+		return XDP_DROP;
+
+	__builtin_memset(meta, 0, META_SIZE);
+
+	return XDP_PASS;
+}
+
 SEC("xdp")
 int ing_xdp(struct xdp_md *ctx)
 {

-- 
2.43.0

[PATCH bpf-next 12/13] selftests/bpf: Cover lack of access to skb metadata at ip layer

[Jakub Sitnicki]

Currently we don't expect skb metadata to persist beyond the device hooks.
Extend the test run BPF program from the Netfilter pre-routing hook to
verify this behavior.

Note, that the added test has no observable side-effect yet. This will be
addressed by the next change.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../bpf/prog_tests/xdp_context_test_run.c          | 94 ++++++++++++++++------
 tools/testing/selftests/bpf/progs/test_xdp_meta.c  | 62 +++++++++-----
 tools/testing/selftests/bpf/test_progs.h           |  1 +
 3 files changed, 115 insertions(+), 42 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
index 79c4c58276e6..4cf8e009a054 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -19,6 +19,9 @@ static const __u8 test_payload[TEST_PAYLOAD_LEN] = {
 	0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38,
 };
 
+#define PACKET_LEN \
+	(sizeof(struct ethhdr) + sizeof(struct iphdr) + TEST_PAYLOAD_LEN)
+
 void test_xdp_context_error(int prog_fd, struct bpf_test_run_opts opts,
 			    __u32 data_meta, __u32 data, __u32 data_end,
 			    __u32 ingress_ifindex, __u32 rx_queue_index,
@@ -120,18 +123,38 @@ void test_xdp_context_test_run(void)
 	test_xdp_context_test_run__destroy(skel);
 }
 
+static void init_test_packet(__u8 *pkt)
+{
+	struct ethhdr *eth = &(struct ethhdr){
+		.h_dest = { 0x02, 0x00, 0x00, 0x00, 0x00, 0x01 },
+		.h_source = { 0x02, 0x00, 0x00, 0x00, 0x00, 0x02 },
+		.h_proto = htons(ETH_P_IP),
+	};
+	struct iphdr *iph = &(struct iphdr){
+		.ihl = 5,
+		.version = IPVERSION,
+		.ttl = IPDEFTTL,
+		.protocol = 61, /* host internal protocol */
+		.saddr = inet_addr("10.0.0.2"),
+		.daddr = inet_addr("10.0.0.1"),
+	};
+
+	eth = memcpy(pkt, eth, sizeof(*eth));
+	pkt += sizeof(*eth);
+	iph = memcpy(pkt, iph, sizeof(*iph));
+	pkt += sizeof(*iph);
+	memcpy(pkt, test_payload, sizeof(test_payload));
+
+	iph->tot_len = htons(sizeof(*iph) + sizeof(test_payload));
+	iph->check = build_ip_csum(iph);
+}
+
 static int send_test_packet(int ifindex)
 {
+	__u8 packet[PACKET_LEN];
 	int n, sock = -1;
-	__u8 packet[sizeof(struct ethhdr) + TEST_PAYLOAD_LEN];
-
-	/* The ethernet header is not relevant for this test and doesn't need to
-	 * be meaningful.
-	 */
-	struct ethhdr eth = { 0 };
 
-	memcpy(packet, &eth, sizeof(eth));
-	memcpy(packet + sizeof(eth), test_payload, TEST_PAYLOAD_LEN);
+	init_test_packet(packet);
 
 	sock = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
 	if (!ASSERT_GE(sock, 0, "socket"))
@@ -271,17 +294,18 @@ void test_xdp_context_veth(void)
 static void test_tuntap(struct bpf_program *xdp_prog,
 			struct bpf_program *tc_prio_1_prog,
 			struct bpf_program *tc_prio_2_prog,
+			struct bpf_program *nf_prog,
 			struct bpf_map *result_map)
 {
 	LIBBPF_OPTS(bpf_tc_hook, tc_hook, .attach_point = BPF_TC_INGRESS);
-	LIBBPF_OPTS(bpf_tc_opts, tc_opts, .handle = 1, .priority = 1);
+	struct bpf_link *nf_link = NULL;
 	struct netns_obj *ns = NULL;
-	__u8 packet[sizeof(struct ethhdr) + TEST_PAYLOAD_LEN];
+	__u8 packet[PACKET_LEN];
 	int tap_fd = -1;
 	int tap_ifindex;
 	int ret;
 
-	if (!clear_test_result(result_map))
+	if (result_map && !clear_test_result(result_map))
 		return;
 
 	ns = netns_new(TAP_NETNS, true);
@@ -292,6 +316,8 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 	if (!ASSERT_GE(tap_fd, 0, "open_tuntap"))
 		goto close;
 
+	SYS(close, "ip link set dev " TAP_NAME " addr 02:00:00:00:00:01");
+	SYS(close, "ip addr add dev " TAP_NAME " 10.0.0.1/24");
 	SYS(close, "ip link set dev " TAP_NAME " up");
 
 	tap_ifindex = if_nametoindex(TAP_NAME);
@@ -303,10 +329,14 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 	if (!ASSERT_OK(ret, "bpf_tc_hook_create"))
 		goto close;
 
-	tc_opts.prog_fd = bpf_program__fd(tc_prio_1_prog);
-	ret = bpf_tc_attach(&tc_hook, &tc_opts);
-	if (!ASSERT_OK(ret, "bpf_tc_attach"))
-		goto close;
+	if (tc_prio_1_prog) {
+		LIBBPF_OPTS(bpf_tc_opts, tc_opts, .handle = 1, .priority = 1,
+			    .prog_fd = bpf_program__fd(tc_prio_1_prog));
+
+		ret = bpf_tc_attach(&tc_hook, &tc_opts);
+		if (!ASSERT_OK(ret, "bpf_tc_attach"))
+			goto close;
+	}
 
 	if (tc_prio_2_prog) {
 		LIBBPF_OPTS(bpf_tc_opts, tc_opts, .handle = 1, .priority = 2,
@@ -317,28 +347,33 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 			goto close;
 	}
 
+	if (nf_prog) {
+		LIBBPF_OPTS(bpf_netfilter_opts, nf_opts,
+			    .pf = NFPROTO_IPV4, .hooknum = NF_INET_PRE_ROUTING);
+
+		nf_link = bpf_program__attach_netfilter(nf_prog, &nf_opts);
+		if (!ASSERT_OK_PTR(nf_link, "attach_netfilter"))
+			goto close;
+	}
+
 	ret = bpf_xdp_attach(tap_ifindex, bpf_program__fd(xdp_prog),
 			     0, NULL);
 	if (!ASSERT_GE(ret, 0, "bpf_xdp_attach"))
 		goto close;
 
-	/* The ethernet header is not relevant for this test and doesn't need to
-	 * be meaningful.
-	 */
-	struct ethhdr eth = { 0 };
-
-	memcpy(packet, &eth, sizeof(eth));
-	memcpy(packet + sizeof(eth), test_payload, TEST_PAYLOAD_LEN);
-
+	init_test_packet(packet);
 	ret = write(tap_fd, packet, sizeof(packet));
 	if (!ASSERT_EQ(ret, sizeof(packet), "write packet"))
 		goto close;
 
-	assert_test_result(result_map);
+	if (result_map)
+		assert_test_result(result_map);
 
 close:
 	if (tap_fd >= 0)
 		close(tap_fd);
+	if (nf_link)
+		bpf_link__destroy(nf_link);
 	netns_free(ns);
 }
 
@@ -354,27 +389,38 @@ void test_xdp_context_tuntap(void)
 		test_tuntap(skel->progs.ing_xdp,
 			    skel->progs.ing_cls,
 			    NULL, /* tc prio 2 */
+			    NULL, /* netfilter */
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_read"))
 		test_tuntap(skel->progs.ing_xdp,
 			    skel->progs.ing_cls_dynptr_read,
 			    NULL, /* tc prio 2 */
+			    NULL, /* netfilter */
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_slice"))
 		test_tuntap(skel->progs.ing_xdp,
 			    skel->progs.ing_cls_dynptr_slice,
 			    NULL, /* tc prio 2 */
+			    NULL, /* netfilter */
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_write"))
 		test_tuntap(skel->progs.ing_xdp_zalloc_meta,
 			    skel->progs.ing_cls_dynptr_write,
 			    skel->progs.ing_cls_dynptr_read,
+			    NULL, /* netfilter */
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_slice_rdwr"))
 		test_tuntap(skel->progs.ing_xdp_zalloc_meta,
 			    skel->progs.ing_cls_dynptr_slice_rdwr,
 			    skel->progs.ing_cls_dynptr_slice,
+			    NULL, /* netfilter */
 			    skel->maps.test_result);
+	if (test__start_subtest("dynptr_nf_hook"))
+		test_tuntap(skel->progs.ing_xdp,
+			    NULL, /* tc prio 1 */
+			    NULL, /* tc prio 2 */
+			    skel->progs.ing_nf,
+			    NULL /* ignore result for now */);
 
 	test_xdp_meta__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_meta.c b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
index b6fed72b1005..41411d164190 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_meta.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
@@ -1,15 +1,25 @@
 #include <stdbool.h>
 #include <linux/bpf.h>
 #include <linux/if_ether.h>
+#include <linux/ip.h>
 #include <linux/pkt_cls.h>
 
+#include <bpf/bpf_endian.h>
 #include <bpf/bpf_helpers.h>
 #include "bpf_kfuncs.h"
 
+#define META_OFFSET (sizeof(struct ethhdr) + sizeof(struct iphdr))
 #define META_SIZE 32
 
+#define NF_DROP 0
+#define NF_ACCEPT 1
+
 #define ctx_ptr(ctx, mem) (void *)(unsigned long)ctx->mem
 
+struct bpf_nf_ctx {
+	struct sk_buff *skb;
+} __attribute__((preserve_access_index));
+
 /* Demonstrates how metadata can be passed from an XDP program to a TC program
  * using bpf_xdp_adjust_meta.
  * For the sake of testing the metadata support in drivers, the XDP program uses
@@ -60,6 +70,20 @@ int ing_cls_dynptr_read(struct __sk_buff *ctx)
 	return TC_ACT_SHOT;
 }
 
+/* Check that we can't get a dynptr slice to skb metadata yet */
+SEC("netfilter")
+int ing_nf(struct bpf_nf_ctx *ctx)
+{
+	struct __sk_buff *skb = (struct __sk_buff *)ctx->skb;
+	struct bpf_dynptr meta;
+
+	bpf_dynptr_from_skb(skb, BPF_DYNPTR_F_SKB_METADATA, &meta);
+	if (bpf_dynptr_size(&meta) != 0)
+		return NF_DROP;
+
+	return NF_ACCEPT;
+}
+
 /* Write to metadata using bpf_dynptr_write helper */
 SEC("tc")
 int ing_cls_dynptr_write(struct __sk_buff *ctx)
@@ -68,7 +92,7 @@ int ing_cls_dynptr_write(struct __sk_buff *ctx)
 	__u8 *src;
 
 	bpf_dynptr_from_skb(ctx, 0, &data);
-	src = bpf_dynptr_slice(&data, sizeof(struct ethhdr), NULL, META_SIZE);
+	src = bpf_dynptr_slice(&data, META_OFFSET, NULL, META_SIZE);
 	if (!src)
 		return TC_ACT_SHOT;
 
@@ -108,7 +132,7 @@ int ing_cls_dynptr_slice_rdwr(struct __sk_buff *ctx)
 	__u8 *src, *dst;
 
 	bpf_dynptr_from_skb(ctx, 0, &data);
-	src = bpf_dynptr_slice(&data, sizeof(struct ethhdr), NULL, META_SIZE);
+	src = bpf_dynptr_slice(&data, META_OFFSET, NULL, META_SIZE);
 	if (!src)
 		return TC_ACT_SHOT;
 
@@ -126,14 +150,18 @@ int ing_cls_dynptr_slice_rdwr(struct __sk_buff *ctx)
 SEC("xdp")
 int ing_xdp_zalloc_meta(struct xdp_md *ctx)
 {
-	struct ethhdr *eth = ctx_ptr(ctx, data);
+	const void *data_end = ctx_ptr(ctx, data_end);
+	const struct ethhdr *eth;
+	const struct iphdr *iph;
 	__u8 *meta;
 	int ret;
 
-	/* Drop any non-test packets */
-	if (eth + 1 > ctx_ptr(ctx, data_end))
+	/* Expect Eth | IPv4 (proto=61) | ... */
+	eth = ctx_ptr(ctx, data);
+	if (eth + 1 > data_end || eth->h_proto != bpf_htons(ETH_P_IP))
 		return XDP_DROP;
-	if (eth->h_proto != 0)
+	iph = (void *)(eth + 1);
+	if (iph + 1 > data_end || iph->protocol != 61)
 		return XDP_DROP;
 
 	ret = bpf_xdp_adjust_meta(ctx, -META_SIZE);
@@ -153,7 +181,8 @@ SEC("xdp")
 int ing_xdp(struct xdp_md *ctx)
 {
 	__u8 *data, *data_meta, *data_end, *payload;
-	struct ethhdr *eth;
+	const struct ethhdr *eth;
+	const struct iphdr *iph;
 	int ret;
 
 	ret = bpf_xdp_adjust_meta(ctx, -META_SIZE);
@@ -164,18 +193,15 @@ int ing_xdp(struct xdp_md *ctx)
 	data_end  = ctx_ptr(ctx, data_end);
 	data      = ctx_ptr(ctx, data);
 
-	eth = (struct ethhdr *)data;
-	payload = data + sizeof(struct ethhdr);
-
-	if (payload + META_SIZE > data_end ||
-	    data_meta + META_SIZE > data)
+	/* Expect Eth | IPv4 (proto=61) | meta blob */
+	eth = (void *)data;
+	if (eth + 1 > data_end || eth->h_proto != bpf_htons(ETH_P_IP))
 		return XDP_DROP;
-
-	/* The Linux networking stack may send other packets on the test
-	 * interface that interfere with the test. Just drop them.
-	 * The test packets can be recognized by their ethertype of zero.
-	 */
-	if (eth->h_proto != 0)
+	iph = (void *)(eth + 1);
+	if (iph + 1 > data_end || iph->protocol != 61)
+		return XDP_DROP;
+	payload = (void *)(iph + 1);
+	if (payload + META_SIZE > data_end || data_meta + META_SIZE > data)
 		return XDP_DROP;
 
 	__builtin_memcpy(data_meta, payload, META_SIZE);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index df2222a1806f..204f54cdaab1 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -20,6 +20,7 @@ typedef __u16 __sum16;
 #include <linux/ip.h>
 #include <linux/ipv6.h>
 #include <linux/filter.h>
+#include <linux/netfilter.h>
 #include <linux/perf_event.h>
 #include <linux/socket.h>
 #include <linux/unistd.h>

-- 
2.43.0

[PATCH bpf-next 13/13] selftests/bpf: Count successful bpf program runs

[Jakub Sitnicki]

The skb metadata tests for BPF programs which don't have metadata access
yet have no observable side-effects. Hence, we can't detect breakage.

Count each successful BPF program pass, when taking the expected path, as a
side-effect to test for.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../bpf/prog_tests/xdp_context_test_run.c          | 19 ++++++++++++++++-
 tools/testing/selftests/bpf/progs/test_xdp_meta.c  | 24 ++++++++++++++--------
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
index 4cf8e009a054..b9c9f874f1b4 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -294,13 +294,15 @@ void test_xdp_context_veth(void)
 static void test_tuntap(struct bpf_program *xdp_prog,
 			struct bpf_program *tc_prio_1_prog,
 			struct bpf_program *tc_prio_2_prog,
-			struct bpf_program *nf_prog,
+			struct bpf_program *nf_prog, __u32 *prog_run_cnt,
 			struct bpf_map *result_map)
 {
 	LIBBPF_OPTS(bpf_tc_hook, tc_hook, .attach_point = BPF_TC_INGRESS);
 	struct bpf_link *nf_link = NULL;
 	struct netns_obj *ns = NULL;
 	__u8 packet[PACKET_LEN];
+	__u32 want_run_cnt = 0;
+	__u32 have_run_cnt;
 	int tap_fd = -1;
 	int tap_ifindex;
 	int ret;
@@ -336,6 +338,7 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 		ret = bpf_tc_attach(&tc_hook, &tc_opts);
 		if (!ASSERT_OK(ret, "bpf_tc_attach"))
 			goto close;
+		want_run_cnt++;
 	}
 
 	if (tc_prio_2_prog) {
@@ -345,6 +348,7 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 		ret = bpf_tc_attach(&tc_hook, &tc_opts);
 		if (!ASSERT_OK(ret, "bpf_tc_attach"))
 			goto close;
+		want_run_cnt++;
 	}
 
 	if (nf_prog) {
@@ -354,18 +358,25 @@ static void test_tuntap(struct bpf_program *xdp_prog,
 		nf_link = bpf_program__attach_netfilter(nf_prog, &nf_opts);
 		if (!ASSERT_OK_PTR(nf_link, "attach_netfilter"))
 			goto close;
+		want_run_cnt++;
 	}
 
 	ret = bpf_xdp_attach(tap_ifindex, bpf_program__fd(xdp_prog),
 			     0, NULL);
 	if (!ASSERT_GE(ret, 0, "bpf_xdp_attach"))
 		goto close;
+	want_run_cnt++;
 
 	init_test_packet(packet);
 	ret = write(tap_fd, packet, sizeof(packet));
 	if (!ASSERT_EQ(ret, sizeof(packet), "write packet"))
 		goto close;
 
+	have_run_cnt = __atomic_exchange_n(prog_run_cnt, 0, __ATOMIC_SEQ_CST);
+	if (!ASSERT_EQ(have_run_cnt, want_run_cnt,
+		       "unexpected bpf prog runs"))
+		goto close;
+
 	if (result_map)
 		assert_test_result(result_map);
 
@@ -390,36 +401,42 @@ void test_xdp_context_tuntap(void)
 			    skel->progs.ing_cls,
 			    NULL, /* tc prio 2 */
 			    NULL, /* netfilter */
+			    &skel->bss->prog_run_cnt,
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_read"))
 		test_tuntap(skel->progs.ing_xdp,
 			    skel->progs.ing_cls_dynptr_read,
 			    NULL, /* tc prio 2 */
 			    NULL, /* netfilter */
+			    &skel->bss->prog_run_cnt,
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_slice"))
 		test_tuntap(skel->progs.ing_xdp,
 			    skel->progs.ing_cls_dynptr_slice,
 			    NULL, /* tc prio 2 */
 			    NULL, /* netfilter */
+			    &skel->bss->prog_run_cnt,
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_write"))
 		test_tuntap(skel->progs.ing_xdp_zalloc_meta,
 			    skel->progs.ing_cls_dynptr_write,
 			    skel->progs.ing_cls_dynptr_read,
 			    NULL, /* netfilter */
+			    &skel->bss->prog_run_cnt,
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_slice_rdwr"))
 		test_tuntap(skel->progs.ing_xdp_zalloc_meta,
 			    skel->progs.ing_cls_dynptr_slice_rdwr,
 			    skel->progs.ing_cls_dynptr_slice,
 			    NULL, /* netfilter */
+			    &skel->bss->prog_run_cnt,
 			    skel->maps.test_result);
 	if (test__start_subtest("dynptr_nf_hook"))
 		test_tuntap(skel->progs.ing_xdp,
 			    NULL, /* tc prio 1 */
 			    NULL, /* tc prio 2 */
 			    skel->progs.ing_nf,
+			    &skel->bss->prog_run_cnt,
 			    NULL /* ignore result for now */);
 
 	test_xdp_meta__destroy(skel);
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_meta.c b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
index 41411d164190..ae149e45cf0c 100644
--- a/tools/testing/selftests/bpf/progs/test_xdp_meta.c
+++ b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
@@ -35,6 +35,14 @@ struct {
 	__uint(value_size, META_SIZE);
 } test_result SEC(".maps");
 
+__u32 prog_run_cnt = 0;
+
+static __always_inline int run_ok(int retval)
+{
+	__sync_fetch_and_add(&prog_run_cnt, 1);
+	return retval;
+}
+
 SEC("tc")
 int ing_cls(struct __sk_buff *ctx)
 {
@@ -49,7 +57,7 @@ int ing_cls(struct __sk_buff *ctx)
 
 	bpf_map_update_elem(&test_result, &key, data_meta, BPF_ANY);
 
-	return TC_ACT_SHOT;
+	return run_ok(TC_ACT_SHOT);
 }
 
 /* Read from metadata using bpf_dynptr_read helper */
@@ -67,7 +75,7 @@ int ing_cls_dynptr_read(struct __sk_buff *ctx)
 	bpf_dynptr_from_skb(ctx, BPF_DYNPTR_F_SKB_METADATA, &meta);
 	bpf_dynptr_read(dst, META_SIZE, &meta, 0, 0);
 
-	return TC_ACT_SHOT;
+	return run_ok(TC_ACT_SHOT);
 }
 
 /* Check that we can't get a dynptr slice to skb metadata yet */
@@ -81,7 +89,7 @@ int ing_nf(struct bpf_nf_ctx *ctx)
 	if (bpf_dynptr_size(&meta) != 0)
 		return NF_DROP;
 
-	return NF_ACCEPT;
+	return run_ok(NF_ACCEPT);
 }
 
 /* Write to metadata using bpf_dynptr_write helper */
@@ -99,7 +107,7 @@ int ing_cls_dynptr_write(struct __sk_buff *ctx)
 	bpf_dynptr_from_skb(ctx, BPF_DYNPTR_F_SKB_METADATA, &meta);
 	bpf_dynptr_write(&meta, 0, src, META_SIZE, 0);
 
-	return TC_ACT_UNSPEC; /* pass */
+	return run_ok(TC_ACT_UNSPEC); /* pass */
 }
 
 /* Read from metadata using read-only dynptr slice */
@@ -121,7 +129,7 @@ int ing_cls_dynptr_slice(struct __sk_buff *ctx)
 
 	__builtin_memcpy(dst, src, META_SIZE);
 
-	return TC_ACT_SHOT;
+	return run_ok(TC_ACT_SHOT);
 }
 
 /* Write to metadata using writeable dynptr slice */
@@ -143,7 +151,7 @@ int ing_cls_dynptr_slice_rdwr(struct __sk_buff *ctx)
 
 	__builtin_memcpy(dst, src, META_SIZE);
 
-	return TC_ACT_UNSPEC; /* pass */
+	return run_ok(TC_ACT_UNSPEC); /* pass */
 }
 
 /* Reserve and clear space for metadata but don't populate it */
@@ -174,7 +182,7 @@ int ing_xdp_zalloc_meta(struct xdp_md *ctx)
 
 	__builtin_memset(meta, 0, META_SIZE);
 
-	return XDP_PASS;
+	return run_ok(XDP_PASS);
 }
 
 SEC("xdp")
@@ -205,7 +213,7 @@ int ing_xdp(struct xdp_md *ctx)
 		return XDP_DROP;
 
 	__builtin_memcpy(data_meta, payload, META_SIZE);
-	return XDP_PASS;
+	return run_ok(XDP_PASS);
 }
 
 char _license[] SEC("license") = "GPL";

-- 
2.43.0

Re: [PATCH bpf-next 07/13] net: Clear skb metadata on handover from device to protocol

[Stanislav Fomichev]

On 06/30, Jakub Sitnicki wrote:
> With the extension of bpf_dynptr_from_skb(BPF_DYNPTR_F_SKB_METADATA), all
> BPF programs authorized to call this kfunc now have access to the skb
> metadata area.
> 
> These programs can read up to skb_shinfo(skb)->meta_len bytes located just
> before skb_mac_header(skb), regardless of what data is currently there.
> 
> However, as the network stack processes the skb, headers may be added or
> removed. Hence, we cannot assume that skb_mac_header() always marks the end
> of the metadata area.
> 
> To avoid potential pitfalls, reset the skb metadata length to zero before
> passing the skb to the protocol layers. This is a temporary measure until
> we can make metadata persist through protocol processing.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  net/core/dev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index be97c440ecd5..4a2389997535 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5839,6 +5839,7 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>  	}
>  #endif
>  	skb_reset_redirect(skb);
> +	skb_metadata_clear(skb);

And the assumption that it's not gonna break the existing cases is
because there is currently no way to read that metadata out afterwards?

Re: [PATCH bpf-next 03/13] bpf: Add new variant of skb dynptr for the metadata area

[Stanislav Fomichev]

On 06/30, Jakub Sitnicki wrote:
> Add a new flag for the bpf_dynptr_from_skb helper to let users to create
> dynptrs to skb metadata area. Access paths are stubbed out. Implemented by
> the following changes.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  include/uapi/linux/bpf.h |  9 ++++++++
>  net/core/filter.c        | 60 +++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 719ba230032f..ab5730d2fb29 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7591,4 +7591,13 @@ enum bpf_kfunc_flags {
>  	BPF_F_PAD_ZEROS = (1ULL << 0),
>  };
>  
> +/**
> + * enum bpf_dynptr_from_skb_flags - Flags for bpf_dynptr_from_skb()
> + *
> + * @BPF_DYNPTR_F_SKB_METADATA: Create dynptr to the SKB metadata area
> + */
> +enum bpf_dynptr_from_skb_flags {
> +	BPF_DYNPTR_F_SKB_METADATA = (1ULL << 0),
> +};
> +
>  #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 1fee51b72220..3c2948517838 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -11967,12 +11967,27 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  	return func;
>  }
>  
> +enum skb_dynptr_offset {
> +	SKB_DYNPTR_METADATA	= -1,

nit: any reason not do make it 1? The offset is u32, so that -1 reads a bit
intentional and I don't get the intention :-)

Re: [PATCH bpf-next 07/13] net: Clear skb metadata on handover from device to protocol

[Jakub Sitnicki]

On Mon, Jun 30, 2025 at 09:25 AM -07, Stanislav Fomichev wrote:
> On 06/30, Jakub Sitnicki wrote:
>> With the extension of bpf_dynptr_from_skb(BPF_DYNPTR_F_SKB_METADATA), all
>> BPF programs authorized to call this kfunc now have access to the skb
>> metadata area.
>> 
>> These programs can read up to skb_shinfo(skb)->meta_len bytes located just
>> before skb_mac_header(skb), regardless of what data is currently there.
>> 
>> However, as the network stack processes the skb, headers may be added or
>> removed. Hence, we cannot assume that skb_mac_header() always marks the end
>> of the metadata area.
>> 
>> To avoid potential pitfalls, reset the skb metadata length to zero before
>> passing the skb to the protocol layers. This is a temporary measure until
>> we can make metadata persist through protocol processing.
>> 
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>>  net/core/dev.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index be97c440ecd5..4a2389997535 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5839,6 +5839,7 @@ static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>>  	}
>>  #endif
>>  	skb_reset_redirect(skb);
>> +	skb_metadata_clear(skb);
>
> And the assumption that it's not gonna break the existing cases is
> because there is currently no way to read that metadata out afterwards?

Correct, only tc_cls_act_is_valid_access tags the register as
PTR_TO_PACKET_META when loading __skb->data_meta, which allows access.

Something worth adding to the description. Thanks.

Re: [PATCH bpf-next 03/13] bpf: Add new variant of skb dynptr for the metadata area

[Jakub Sitnicki]

On Mon, Jun 30, 2025 at 09:27 AM -07, Stanislav Fomichev wrote:
> On 06/30, Jakub Sitnicki wrote:
>> Add a new flag for the bpf_dynptr_from_skb helper to let users to create
>> dynptrs to skb metadata area. Access paths are stubbed out. Implemented by
>> the following changes.
>> 
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>>  include/uapi/linux/bpf.h |  9 ++++++++
>>  net/core/filter.c        | 60 +++++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 61 insertions(+), 8 deletions(-)
>> 
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 719ba230032f..ab5730d2fb29 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -7591,4 +7591,13 @@ enum bpf_kfunc_flags {
>>  	BPF_F_PAD_ZEROS = (1ULL << 0),
>>  };
>>  
>> +/**
>> + * enum bpf_dynptr_from_skb_flags - Flags for bpf_dynptr_from_skb()
>> + *
>> + * @BPF_DYNPTR_F_SKB_METADATA: Create dynptr to the SKB metadata area
>> + */
>> +enum bpf_dynptr_from_skb_flags {
>> +	BPF_DYNPTR_F_SKB_METADATA = (1ULL << 0),
>> +};
>> +
>>  #endif /* _UAPI__LINUX_BPF_H__ */
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 1fee51b72220..3c2948517838 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -11967,12 +11967,27 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>>  	return func;
>>  }
>>  
>> +enum skb_dynptr_offset {
>> +	SKB_DYNPTR_METADATA	= -1,
>
> nit: any reason not do make it 1? The offset is u32, so that -1 reads a bit
> intentional and I don't get the intention :-)

Since we're abusing the "offset" field to serve as an enum tag, I
figured seeing 0xffffffff in a memory dump will be an clear indication
that this is not an offset.

Also, metadata comes before payload, like -1 does before 0...

JK of course. No preference here. Went with my gut.

Re: [PATCH bpf-next 02/13] bpf: Helpers for skb dynptr read/write/slice

[kernel test robot]

Hi Jakub,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Jakub-Sitnicki/bpf-Ignore-dynptr-offset-in-skb-data-access/20250630-225941
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20250630-skb-metadata-thru-dynptr-v1-2-f17da13625d8%40cloudflare.com
patch subject: [PATCH bpf-next 02/13] bpf: Helpers for skb dynptr read/write/slice
config: microblaze-allnoconfig (https://download.01.org/0day-ci/archive/20250701/202507010904.MkxDYPdY-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250701/202507010904.MkxDYPdY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507010904.MkxDYPdY-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from kernel/sysctl.c:29:
>> include/linux/filter.h:1788:1: error: expected identifier or '(' before '{' token
    1788 | {
         | ^
   include/linux/filter.h:1795:1: error: expected identifier or '(' before '{' token
    1795 | {
         | ^
>> include/linux/filter.h:1785:19: warning: 'bpf_dynptr_skb_write' declared 'static' but never defined [-Wunused-function]
    1785 | static inline int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst,
         |                   ^~~~~~~~~~~~~~~~~~~~
>> include/linux/filter.h:1792:21: warning: 'bpf_dynptr_skb_slice' declared 'static' but never defined [-Wunused-function]
    1792 | static inline void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr,
         |                     ^~~~~~~~~~~~~~~~~~~~


vim +1788 include/linux/filter.h

b5964b968ac64c Joanne Koong   2023-03-01  1784  
e8b34e67737d71 Jakub Sitnicki 2025-06-30 @1785  static inline int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst,
e8b34e67737d71 Jakub Sitnicki 2025-06-30  1786  				       u32 offset, const void *src, u32 len,
e8b34e67737d71 Jakub Sitnicki 2025-06-30  1787  				       u64 flags);
b5964b968ac64c Joanne Koong   2023-03-01 @1788  {
b5964b968ac64c Joanne Koong   2023-03-01  1789  	return -EOPNOTSUPP;
b5964b968ac64c Joanne Koong   2023-03-01  1790  }
05421aecd4ed65 Joanne Koong   2023-03-01  1791  
e8b34e67737d71 Jakub Sitnicki 2025-06-30 @1792  static inline void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr,
e8b34e67737d71 Jakub Sitnicki 2025-06-30  1793  					 u32 offset, void *buf, u32 len);
e8b34e67737d71 Jakub Sitnicki 2025-06-30  1794  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH bpf-next 02/13] bpf: Helpers for skb dynptr read/write/slice

[kernel test robot]

Hi Jakub,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Jakub-Sitnicki/bpf-Ignore-dynptr-offset-in-skb-data-access/20250630-225941
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20250630-skb-metadata-thru-dynptr-v1-2-f17da13625d8%40cloudflare.com
patch subject: [PATCH bpf-next 02/13] bpf: Helpers for skb dynptr read/write/slice
config: i386-buildonly-randconfig-002-20250701 (https://download.01.org/0day-ci/archive/20250701/202507011044.vjYugeUq-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250701/202507011044.vjYugeUq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507011044.vjYugeUq-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from kernel/bpf/helpers.c:15:
   include/linux/filter.h:1788:1: error: expected identifier or '(' before '{' token
    1788 | {
         | ^
   include/linux/filter.h:1795:1: error: expected identifier or '(' before '{' token
    1795 | {
         | ^
   kernel/bpf/helpers.c: In function '____bpf_snprintf':
   kernel/bpf/helpers.c:1068:9: warning: function '____bpf_snprintf' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1068 |         err = bstr_printf(str, str_size, fmt, data.bin_args);
         |         ^~~
   include/linux/filter.h: At top level:
>> include/linux/filter.h:1785:19: warning: 'bpf_dynptr_skb_write' used but never defined
    1785 | static inline int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst,
         |                   ^~~~~~~~~~~~~~~~~~~~
>> include/linux/filter.h:1792:21: warning: 'bpf_dynptr_skb_slice' used but never defined
    1792 | static inline void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr,
         |                     ^~~~~~~~~~~~~~~~~~~~


vim +/bpf_dynptr_skb_write +1785 include/linux/filter.h

  1784	
> 1785	static inline int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst,
  1786					       u32 offset, const void *src, u32 len,
  1787					       u64 flags);
  1788	{
  1789		return -EOPNOTSUPP;
  1790	}
  1791	
> 1792	static inline void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr,
  1793						 u32 offset, void *buf, u32 len);
  1794	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH bpf-next 02/13] bpf: Helpers for skb dynptr read/write/slice

[Jakub Sitnicki]

On Tue, Jul 01, 2025 at 10:03 AM +08, kernel test robot wrote:
> Hi Jakub,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on bpf-next/master]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jakub-Sitnicki/bpf-Ignore-dynptr-offset-in-skb-data-access/20250630-225941
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> patch link:    https://lore.kernel.org/r/20250630-skb-metadata-thru-dynptr-v1-2-f17da13625d8%40cloudflare.com
> patch subject: [PATCH bpf-next 02/13] bpf: Helpers for skb dynptr read/write/slice
> config: microblaze-allnoconfig (https://download.01.org/0day-ci/archive/20250701/202507010904.MkxDYPdY-lkp@intel.com/config)
> compiler: microblaze-linux-gcc (GCC) 15.1.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250701/202507010904.MkxDYPdY-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202507010904.MkxDYPdY-lkp@intel.com/
>
> All error/warnings (new ones prefixed by >>):
>
>    In file included from kernel/sysctl.c:29:
>>> include/linux/filter.h:1788:1: error: expected identifier or '(' before '{' token
>     1788 | {
>          | ^
>    include/linux/filter.h:1795:1: error: expected identifier or '(' before '{' token
>     1795 | {
>          | ^
>>> include/linux/filter.h:1785:19: warning: 'bpf_dynptr_skb_write' declared 'static' but never defined [-Wunused-function]
>     1785 | static inline int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst,
>          |                   ^~~~~~~~~~~~~~~~~~~~
>>> include/linux/filter.h:1792:21: warning: 'bpf_dynptr_skb_slice' declared 'static' but never defined [-Wunused-function]
>     1792 | static inline void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr,
>          |                     ^~~~~~~~~~~~~~~~~~~~
>
>
> vim +1788 include/linux/filter.h
>
> b5964b968ac64c Joanne Koong   2023-03-01  1784  
> e8b34e67737d71 Jakub Sitnicki 2025-06-30 @1785  static inline int bpf_dynptr_skb_write(const struct bpf_dynptr_kern *dst,
> e8b34e67737d71 Jakub Sitnicki 2025-06-30  1786  				       u32 offset, const void *src, u32 len,
> e8b34e67737d71 Jakub Sitnicki 2025-06-30  1787  				       u64 flags);
> b5964b968ac64c Joanne Koong   2023-03-01 @1788  {
> b5964b968ac64c Joanne Koong   2023-03-01  1789  	return -EOPNOTSUPP;
> b5964b968ac64c Joanne Koong   2023-03-01  1790  }
> 05421aecd4ed65 Joanne Koong   2023-03-01  1791  
> e8b34e67737d71 Jakub Sitnicki 2025-06-30 @1792  static inline void *bpf_dynptr_skb_slice(const struct bpf_dynptr_kern *ptr,
> e8b34e67737d71 Jakub Sitnicki 2025-06-30  1793  					 u32 offset, void *buf, u32 len);
> e8b34e67737d71 Jakub Sitnicki 2025-06-30  1794  

Copy-paste mistake - extra ';' in the stub definition when
CONFIG_NET=n. My bad.

Will fix and respin once people had more time to review.

Re: [PATCH bpf-next 01/13] bpf: Ignore dynptr offset in skb data access

[Andrii Nakryiko]

On Mon, Jun 30, 2025 at 8:23 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> Prepare to use (struct bpf_dynptr)->offset to distinguish between an skb
> dynptr for the payload vs the metadata area.
>
> ptr->offset is always set to zero by bpf_dynptr_from_skb(). We don't need
> to account for it on access.

Huh?.. What about bpf_dynptr_adjust()? This is a wrong approach to
have some magical offset values.

More general question about your patch set: is there ever a need to
work with both metadata and data as one area of memory (i.e., copying
both metadata and data in the same single operation, or setting it as
one thing?). If not, why not have two different dynptrs, one for data
(what we have today) and one exclusively for packet's metadata?

>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  kernel/bpf/helpers.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

[...]

Re: [PATCH bpf-next 03/13] bpf: Add new variant of skb dynptr for the metadata area

[Andrii Nakryiko]

On Mon, Jun 30, 2025 at 7:56 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> Add a new flag for the bpf_dynptr_from_skb helper to let users to create
> dynptrs to skb metadata area. Access paths are stubbed out. Implemented by
> the following changes.
>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  include/uapi/linux/bpf.h |  9 ++++++++
>  net/core/filter.c        | 60 +++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 719ba230032f..ab5730d2fb29 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7591,4 +7591,13 @@ enum bpf_kfunc_flags {
>         BPF_F_PAD_ZEROS = (1ULL << 0),
>  };
>
> +/**
> + * enum bpf_dynptr_from_skb_flags - Flags for bpf_dynptr_from_skb()
> + *
> + * @BPF_DYNPTR_F_SKB_METADATA: Create dynptr to the SKB metadata area
> + */
> +enum bpf_dynptr_from_skb_flags {
> +       BPF_DYNPTR_F_SKB_METADATA = (1ULL << 0),
> +};
> +
>  #endif /* _UAPI__LINUX_BPF_H__ */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 1fee51b72220..3c2948517838 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -11967,12 +11967,27 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>         return func;
>  }
>
> +enum skb_dynptr_offset {
> +       SKB_DYNPTR_METADATA     = -1,
> +       SKB_DYNPTR_PAYLOAD      = 0,
> +};

I'm missing why you need to do it in this hacky way instead of just
having both bpf_dynptr_from_skb() and bpf_dynptr_from_skb_metadata()
(or whatever we bikeshed it into), which will create
BPF_DYNPTR_TYPE_SKB or new BPF_DYNPTR_TYPE_SKB_META dynptr kind,
respectively. Why so complicated?

[...]

Re: [PATCH bpf-next 01/13] bpf: Ignore dynptr offset in skb data access

[Jakub Sitnicki]

On Tue, Jul 01, 2025 at 01:55 PM -07, Andrii Nakryiko wrote:
> On Mon, Jun 30, 2025 at 8:23 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> Prepare to use (struct bpf_dynptr)->offset to distinguish between an skb
>> dynptr for the payload vs the metadata area.
>>
>> ptr->offset is always set to zero by bpf_dynptr_from_skb(). We don't need
>> to account for it on access.
>
> Huh?.. What about bpf_dynptr_adjust()? This is a wrong approach to
> have some magical offset values.

Crap. I'm not gonna lie. I totally missed that.

You're right. It completely breaks down.

I was hoping I could piggyback on skb dynptr, but doesn't look like it.

> More general question about your patch set: is there ever a need to
> work with both metadata and data as one area of memory (i.e., copying
> both metadata and data in the same single operation, or setting it as
> one thing?). If not, why not have two different dynptrs, one for data
> (what we have today) and one exclusively for packet's metadata?

Having two dynptr kinds, one for payload, one for metadata, sounds like
a much better direction. I will pivot to that.

Metadata and payload are logically separate, AFAIK. It just so happens
that the metadata is currently located in front of the payload.

I asked around to find out why is it so - it seems that the decision was
made to place the metadata like that becase it saves you one additional
pointer load. Otherwise you'd need something like
__sk_buff->data_meta_end to marks the end of metadata.

Re: [PATCH bpf-next 03/13] bpf: Add new variant of skb dynptr for the metadata area

[Jakub Sitnicki]

On Tue, Jul 01, 2025 at 01:59 PM -07, Andrii Nakryiko wrote:
> On Mon, Jun 30, 2025 at 7:56 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> Add a new flag for the bpf_dynptr_from_skb helper to let users to create
>> dynptrs to skb metadata area. Access paths are stubbed out. Implemented by
>> the following changes.
>>
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>>  include/uapi/linux/bpf.h |  9 ++++++++
>>  net/core/filter.c        | 60 +++++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 61 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 719ba230032f..ab5730d2fb29 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -7591,4 +7591,13 @@ enum bpf_kfunc_flags {
>>         BPF_F_PAD_ZEROS = (1ULL << 0),
>>  };
>>
>> +/**
>> + * enum bpf_dynptr_from_skb_flags - Flags for bpf_dynptr_from_skb()
>> + *
>> + * @BPF_DYNPTR_F_SKB_METADATA: Create dynptr to the SKB metadata area
>> + */
>> +enum bpf_dynptr_from_skb_flags {
>> +       BPF_DYNPTR_F_SKB_METADATA = (1ULL << 0),
>> +};
>> +
>>  #endif /* _UAPI__LINUX_BPF_H__ */
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 1fee51b72220..3c2948517838 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -11967,12 +11967,27 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>>         return func;
>>  }
>>
>> +enum skb_dynptr_offset {
>> +       SKB_DYNPTR_METADATA     = -1,
>> +       SKB_DYNPTR_PAYLOAD      = 0,
>> +};
>
> I'm missing why you need to do it in this hacky way instead of just
> having both bpf_dynptr_from_skb() and bpf_dynptr_from_skb_metadata()
> (or whatever we bikeshed it into), which will create
> BPF_DYNPTR_TYPE_SKB or new BPF_DYNPTR_TYPE_SKB_META dynptr kind,
> respectively. Why so complicated?
>
> [...]

Agree. Let's keep things simple.

This piggybacking on the skb dynptr was a bad idea.