[PATCH bpf-next v3 0/4] bpf: cgroup skb improvements for bpf_prog_test_run

[PATCH bpf-next v3 0/4] bpf: cgroup skb improvements for bpf_prog_test_run

[Dmitry Yakunin]

This patchset contains some improvements for testing cgroup/skb programs
through BPF_PROG_TEST_RUN command.

v2:
  - fix build without CONFIG_CGROUP_BPF (kernel test robot <lkp@intel.com>)

v3:
  - fix build without CONFIG_IPV6 (kernel test robot <lkp@intel.com>)

Dmitry Yakunin (4):
  bpf: setup socket family and addresses in bpf_prog_test_run_skb
  bpf: allow to specify ifindex for skb in bpf_prog_test_run_skb
  bpf: export some cgroup storages allocation helpers for reusing
  bpf: try to use existing cgroup storage in bpf_prog_test_run_skb

 include/linux/bpf-cgroup.h                         |  36 +++++++
 kernel/bpf/cgroup.c                                |  25 -----
 net/bpf/test_run.c                                 | 113 ++++++++++++++++++---
 .../selftests/bpf/prog_tests/cgroup_skb_prog_run.c |  78 ++++++++++++++
 tools/testing/selftests/bpf/prog_tests/skb_ctx.c   |   5 +
 5 files changed, 217 insertions(+), 40 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c

-- 
2.7.4

[PATCH bpf-next v3 1/4] bpf: setup socket family and addresses in bpf_prog_test_run_skb

[Dmitry Yakunin]

Now it's impossible to test all branches of cgroup_skb bpf program which
accesses skb->family and skb->{local,remote}_ip{4,6} fields because they
are zeroed during socket allocation. This commit fills socket family and
addresses from related fields in constructed skb.

v2:
  - fix build without CONFIG_IPV6 (kernel test robot <lkp@intel.com>)

Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
---
 net/bpf/test_run.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index bfd4ccd..0c3283d 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -432,6 +432,23 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	skb->protocol = eth_type_trans(skb, current->nsproxy->net_ns->loopback_dev);
 	skb_reset_network_header(skb);
 
+	switch (skb->protocol) {
+	case htons(ETH_P_IP):
+		sk->sk_family = AF_INET;
+		sk->sk_rcv_saddr = ip_hdr(skb)->saddr;
+		sk->sk_daddr = ip_hdr(skb)->daddr;
+		break;
+#if IS_ENABLED(CONFIG_IPV6)
+	case htons(ETH_P_IPV6):
+		sk->sk_family = AF_INET6;
+		sk->sk_v6_rcv_saddr = ipv6_hdr(skb)->saddr;
+		sk->sk_v6_daddr = ipv6_hdr(skb)->daddr;
+		break;
+#endif
+	default:
+		break;
+	}
+
 	if (is_l2)
 		__skb_push(skb, hh_len);
 	if (is_direct_pkt_access)
-- 
2.7.4

[PATCH bpf-next v3 2/4] bpf: allow to specify ifindex for skb in bpf_prog_test_run_skb

[Dmitry Yakunin]

Now skb->dev is unconditionally set to the loopback device in current net
namespace. But if we want to test bpf program which contains code branch
based on ifindex condition (eg filters out localhost packets) it is useful
to allow specifying of ifindex from userspace. This patch adds such option
through ctx_in (__sk_buff) parameter.

Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
---
 net/bpf/test_run.c                               | 22 ++++++++++++++++++++--
 tools/testing/selftests/bpf/prog_tests/skb_ctx.c |  5 +++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 0c3283d..0e92973 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -310,6 +310,12 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
 	/* priority is allowed */
 
 	if (!range_is_zero(__skb, offsetofend(struct __sk_buff, priority),
+			   offsetof(struct __sk_buff, ifindex)))
+		return -EINVAL;
+
+	/* ifindex is allowed */
+
+	if (!range_is_zero(__skb, offsetofend(struct __sk_buff, ifindex),
 			   offsetof(struct __sk_buff, cb)))
 		return -EINVAL;
 
@@ -364,6 +370,7 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
 
 	__skb->mark = skb->mark;
 	__skb->priority = skb->priority;
+	__skb->ifindex = skb->dev->ifindex;
 	__skb->tstamp = skb->tstamp;
 	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
 	__skb->wire_len = cb->pkt_len;
@@ -374,6 +381,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 			  union bpf_attr __user *uattr)
 {
 	bool is_l2 = false, is_direct_pkt_access = false;
+	struct net *net = current->nsproxy->net_ns;
+	struct net_device *dev = net->loopback_dev;
 	u32 size = kattr->test.data_size_in;
 	u32 repeat = kattr->test.repeat;
 	struct __sk_buff *ctx = NULL;
@@ -415,7 +424,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 		kfree(ctx);
 		return -ENOMEM;
 	}
-	sock_net_set(sk, current->nsproxy->net_ns);
+	sock_net_set(sk, net);
 	sock_init_data(NULL, sk);
 
 	skb = build_skb(data, 0);
@@ -429,7 +438,14 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 
 	skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
 	__skb_put(skb, size);
-	skb->protocol = eth_type_trans(skb, current->nsproxy->net_ns->loopback_dev);
+	if (ctx && ctx->ifindex > 1) {
+		dev = dev_get_by_index(net, ctx->ifindex);
+		if (!dev) {
+			ret = -ENODEV;
+			goto out;
+		}
+	}
+	skb->protocol = eth_type_trans(skb, dev);
 	skb_reset_network_header(skb);
 
 	switch (skb->protocol) {
@@ -481,6 +497,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 		ret = bpf_ctx_finish(kattr, uattr, ctx,
 				     sizeof(struct __sk_buff));
 out:
+	if (dev && dev != net->loopback_dev)
+		dev_put(dev);
 	kfree_skb(skb);
 	bpf_sk_storage_free(sk);
 	kfree(sk);
diff --git a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
index 7021b92..25de86a 100644
--- a/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
+++ b/tools/testing/selftests/bpf/prog_tests/skb_ctx.c
@@ -11,6 +11,7 @@ void test_skb_ctx(void)
 		.cb[3] = 4,
 		.cb[4] = 5,
 		.priority = 6,
+		.ifindex = 1,
 		.tstamp = 7,
 		.wire_len = 100,
 		.gso_segs = 8,
@@ -92,6 +93,10 @@ void test_skb_ctx(void)
 		   "ctx_out_priority",
 		   "skb->priority == %d, expected %d\n",
 		   skb.priority, 7);
+	CHECK_ATTR(skb.ifindex != 1,
+		   "ctx_out_ifindex",
+		   "skb->ifindex == %d, expected %d\n",
+		   skb.ifindex, 1);
 	CHECK_ATTR(skb.tstamp != 8,
 		   "ctx_out_tstamp",
 		   "skb->tstamp == %lld, expected %d\n",
-- 
2.7.4

Re: [PATCH bpf-next v3 2/4] bpf: allow to specify ifindex for skb in bpf_prog_test_run_skb

[Daniel Borkmann]

On 7/15/20 9:51 PM, Dmitry Yakunin wrote:
> Now skb->dev is unconditionally set to the loopback device in current net
> namespace. But if we want to test bpf program which contains code branch
> based on ifindex condition (eg filters out localhost packets) it is useful
> to allow specifying of ifindex from userspace. This patch adds such option
> through ctx_in (__sk_buff) parameter.
> 
> Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
> ---
>   net/bpf/test_run.c                               | 22 ++++++++++++++++++++--
>   tools/testing/selftests/bpf/prog_tests/skb_ctx.c |  5 +++++
>   2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 0c3283d..0e92973 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -310,6 +310,12 @@ static int convert___skb_to_skb(struct sk_buff *skb, struct __sk_buff *__skb)
>   	/* priority is allowed */
>   
>   	if (!range_is_zero(__skb, offsetofend(struct __sk_buff, priority),
> +			   offsetof(struct __sk_buff, ifindex)))
> +		return -EINVAL;
> +
> +	/* ifindex is allowed */
> +
> +	if (!range_is_zero(__skb, offsetofend(struct __sk_buff, ifindex),
>   			   offsetof(struct __sk_buff, cb)))
>   		return -EINVAL;
>   
> @@ -364,6 +370,7 @@ static void convert_skb_to___skb(struct sk_buff *skb, struct __sk_buff *__skb)
>   
>   	__skb->mark = skb->mark;
>   	__skb->priority = skb->priority;
> +	__skb->ifindex = skb->dev->ifindex;
>   	__skb->tstamp = skb->tstamp;
>   	memcpy(__skb->cb, &cb->data, QDISC_CB_PRIV_LEN);
>   	__skb->wire_len = cb->pkt_len;
> @@ -374,6 +381,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>   			  union bpf_attr __user *uattr)
>   {
>   	bool is_l2 = false, is_direct_pkt_access = false;
> +	struct net *net = current->nsproxy->net_ns;
> +	struct net_device *dev = net->loopback_dev;
>   	u32 size = kattr->test.data_size_in;
>   	u32 repeat = kattr->test.repeat;
>   	struct __sk_buff *ctx = NULL;
> @@ -415,7 +424,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>   		kfree(ctx);
>   		return -ENOMEM;
>   	}
> -	sock_net_set(sk, current->nsproxy->net_ns);
> +	sock_net_set(sk, net);
>   	sock_init_data(NULL, sk);
>   
>   	skb = build_skb(data, 0);
> @@ -429,7 +438,14 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>   
>   	skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
>   	__skb_put(skb, size);
> -	skb->protocol = eth_type_trans(skb, current->nsproxy->net_ns->loopback_dev);
> +	if (ctx && ctx->ifindex > 1) {
> +		dev = dev_get_by_index(net, ctx->ifindex);
> +		if (!dev) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +	}
> +	skb->protocol = eth_type_trans(skb, dev);
>   	skb_reset_network_header(skb);
>   
>   	switch (skb->protocol) {
> @@ -481,6 +497,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
>   		ret = bpf_ctx_finish(kattr, uattr, ctx,
>   				     sizeof(struct __sk_buff));
>   out:

Overall this looks good. One small note is that dev_get_by_index() will hold the device
for the entire test duration preventing to release it from user side, but I think in this
context it's an acceptable trade-off.

> +	if (dev && dev != net->loopback_dev)
> +		dev_put(dev);
>   	kfree_skb(skb);
>   	bpf_sk_storage_free(sk);
>   	kfree(sk);

[PATCH bpf-next v3 3/4] bpf: export some cgroup storages allocation helpers for reusing

[Dmitry Yakunin]

This patch exports bpf_cgroup_storages_alloc and bpf_cgroup_storages_free
helpers to the header file and reuses them in bpf_test_run.

v2:
  - fix build without CONFIG_CGROUP_BPF (kernel test robot <lkp@intel.com>)

Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
---
 include/linux/bpf-cgroup.h | 36 ++++++++++++++++++++++++++++++++++++
 kernel/bpf/cgroup.c        | 25 -------------------------
 net/bpf/test_run.c         | 16 ++++------------
 3 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index 2c6f266..5c10fe6 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -175,6 +175,33 @@ int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
 int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
 				     void *value, u64 flags);
 
+static inline void bpf_cgroup_storages_free(struct bpf_cgroup_storage
+					    *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
+{
+	enum bpf_cgroup_storage_type stype;
+
+	for_each_cgroup_storage_type(stype)
+		bpf_cgroup_storage_free(storage[stype]);
+}
+
+static inline int bpf_cgroup_storages_alloc(struct bpf_cgroup_storage
+					    *storage[MAX_BPF_CGROUP_STORAGE_TYPE],
+					    struct bpf_prog *prog)
+{
+	enum bpf_cgroup_storage_type stype;
+
+	for_each_cgroup_storage_type(stype) {
+		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
+		if (IS_ERR(storage[stype])) {
+			storage[stype] = NULL;
+			bpf_cgroup_storages_free(storage);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
 #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb)			      \
 ({									      \
@@ -398,6 +425,15 @@ static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
 	return 0;
 }
 
+static inline void bpf_cgroup_storages_free(
+	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE]) {}
+
+static inline int bpf_cgroup_storages_alloc(
+	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE],
+	struct bpf_prog *prog) {
+	return 0;
+}
+
 #define cgroup_bpf_enabled (0)
 #define BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, type, t_ctx) ({ 0; })
 #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index ac53102..e4c2792 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -28,31 +28,6 @@ void cgroup_bpf_offline(struct cgroup *cgrp)
 	percpu_ref_kill(&cgrp->bpf.refcnt);
 }
 
-static void bpf_cgroup_storages_free(struct bpf_cgroup_storage *storages[])
-{
-	enum bpf_cgroup_storage_type stype;
-
-	for_each_cgroup_storage_type(stype)
-		bpf_cgroup_storage_free(storages[stype]);
-}
-
-static int bpf_cgroup_storages_alloc(struct bpf_cgroup_storage *storages[],
-				     struct bpf_prog *prog)
-{
-	enum bpf_cgroup_storage_type stype;
-
-	for_each_cgroup_storage_type(stype) {
-		storages[stype] = bpf_cgroup_storage_alloc(prog, stype);
-		if (IS_ERR(storages[stype])) {
-			storages[stype] = NULL;
-			bpf_cgroup_storages_free(storages);
-			return -ENOMEM;
-		}
-	}
-
-	return 0;
-}
-
 static void bpf_cgroup_storages_assign(struct bpf_cgroup_storage *dst[],
 				       struct bpf_cgroup_storage *src[])
 {
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 0e92973..050390d 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -19,20 +19,13 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 			u32 *retval, u32 *time, bool xdp)
 {
 	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
-	enum bpf_cgroup_storage_type stype;
 	u64 time_start, time_spent = 0;
 	int ret = 0;
 	u32 i;
 
-	for_each_cgroup_storage_type(stype) {
-		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
-		if (IS_ERR(storage[stype])) {
-			storage[stype] = NULL;
-			for_each_cgroup_storage_type(stype)
-				bpf_cgroup_storage_free(storage[stype]);
-			return -ENOMEM;
-		}
-	}
+	ret = bpf_cgroup_storages_alloc(storage, prog);
+	if (ret)
+		return ret;
 
 	if (!repeat)
 		repeat = 1;
@@ -72,8 +65,7 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 	do_div(time_spent, repeat);
 	*time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
 
-	for_each_cgroup_storage_type(stype)
-		bpf_cgroup_storage_free(storage[stype]);
+	bpf_cgroup_storages_free(storage);
 
 	return ret;
 }
-- 
2.7.4

Re: [PATCH bpf-next v3 3/4] bpf: export some cgroup storages allocation helpers for reusing

[Daniel Borkmann]

On 7/15/20 9:51 PM, Dmitry Yakunin wrote:
> This patch exports bpf_cgroup_storages_alloc and bpf_cgroup_storages_free
> helpers to the header file and reuses them in bpf_test_run.
> 
> v2:
>    - fix build without CONFIG_CGROUP_BPF (kernel test robot <lkp@intel.com>)
> 
> Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
> ---
>   include/linux/bpf-cgroup.h | 36 ++++++++++++++++++++++++++++++++++++
>   kernel/bpf/cgroup.c        | 25 -------------------------
>   net/bpf/test_run.c         | 16 ++++------------
>   3 files changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 2c6f266..5c10fe6 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -175,6 +175,33 @@ int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
>   int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
>   				     void *value, u64 flags);
>   
> +static inline void bpf_cgroup_storages_free(struct bpf_cgroup_storage
> +					    *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
> +{
> +	enum bpf_cgroup_storage_type stype;
> +
> +	for_each_cgroup_storage_type(stype)
> +		bpf_cgroup_storage_free(storage[stype]);
> +}
> +
> +static inline int bpf_cgroup_storages_alloc(struct bpf_cgroup_storage
> +					    *storage[MAX_BPF_CGROUP_STORAGE_TYPE],
> +					    struct bpf_prog *prog)
> +{
> +	enum bpf_cgroup_storage_type stype;
> +
> +	for_each_cgroup_storage_type(stype) {
> +		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> +		if (IS_ERR(storage[stype])) {
> +			storage[stype] = NULL;
> +			bpf_cgroup_storages_free(storage);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
>   #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb)			      \
>   ({									      \
> @@ -398,6 +425,15 @@ static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
>   	return 0;
>   }
>   
> +static inline void bpf_cgroup_storages_free(
> +	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE]) {}
> +
> +static inline int bpf_cgroup_storages_alloc(
> +	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE],
> +	struct bpf_prog *prog) {
> +	return 0;
> +}
> +
>   #define cgroup_bpf_enabled (0)
>   #define BPF_CGROUP_RUN_SA_PROG_LOCK(sk, uaddr, type, t_ctx) ({ 0; })
>   #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index ac53102..e4c2792 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -28,31 +28,6 @@ void cgroup_bpf_offline(struct cgroup *cgrp)
>   	percpu_ref_kill(&cgrp->bpf.refcnt);
>   }
>   
> -static void bpf_cgroup_storages_free(struct bpf_cgroup_storage *storages[])
> -{
> -	enum bpf_cgroup_storage_type stype;
> -
> -	for_each_cgroup_storage_type(stype)
> -		bpf_cgroup_storage_free(storages[stype]);
> -}
> -
> -static int bpf_cgroup_storages_alloc(struct bpf_cgroup_storage *storages[],
> -				     struct bpf_prog *prog)
> -{
> -	enum bpf_cgroup_storage_type stype;
> -
> -	for_each_cgroup_storage_type(stype) {
> -		storages[stype] = bpf_cgroup_storage_alloc(prog, stype);
> -		if (IS_ERR(storages[stype])) {
> -			storages[stype] = NULL;
> -			bpf_cgroup_storages_free(storages);
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	return 0;
> -}
> -

nit: Can't we just export them from here instead of inlining? Given this is for
test_run.c anyway, I don't think it's worth the extra churn.

>   static void bpf_cgroup_storages_assign(struct bpf_cgroup_storage *dst[],
>   				       struct bpf_cgroup_storage *src[])
>   {
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 0e92973..050390d 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -19,20 +19,13 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>   			u32 *retval, u32 *time, bool xdp)
>   {
>   	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
> -	enum bpf_cgroup_storage_type stype;
>   	u64 time_start, time_spent = 0;
>   	int ret = 0;
>   	u32 i;
>   
> -	for_each_cgroup_storage_type(stype) {
> -		storage[stype] = bpf_cgroup_storage_alloc(prog, stype);
> -		if (IS_ERR(storage[stype])) {
> -			storage[stype] = NULL;
> -			for_each_cgroup_storage_type(stype)
> -				bpf_cgroup_storage_free(storage[stype]);
> -			return -ENOMEM;
> -		}
> -	}
> +	ret = bpf_cgroup_storages_alloc(storage, prog);
> +	if (ret)
> +		return ret;
>   
>   	if (!repeat)
>   		repeat = 1;
> @@ -72,8 +65,7 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>   	do_div(time_spent, repeat);
>   	*time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
>   
> -	for_each_cgroup_storage_type(stype)
> -		bpf_cgroup_storage_free(storage[stype]);
> +	bpf_cgroup_storages_free(storage);
>   
>   	return ret;
>   }
> 

[PATCH bpf-next v3 4/4] bpf: try to use existing cgroup storage in bpf_prog_test_run_skb

[Dmitry Yakunin]

Now we cannot check results in cgroup storage after running
BPF_PROG_TEST_RUN command because it allocates dummy cgroup storage
during test. This patch implements simple logic for searching already
allocated cgroup storage through iterating effective programs of current
cgroup and finding the first match. If match is not found fallback to
temporary storage is happened.

v2:
  - fix build without CONFIG_CGROUP_BPF (kernel test robot <lkp@intel.com>)

Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
---
 net/bpf/test_run.c                                 | 64 +++++++++++++++++-
 .../selftests/bpf/prog_tests/cgroup_skb_prog_run.c | 78 ++++++++++++++++++++++
 2 files changed, 139 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 050390d..7382b22 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -15,15 +15,67 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/bpf_test_run.h>
 
+#ifdef CONFIG_CGROUP_BPF
+
+static struct bpf_prog_array_item *bpf_prog_find_active(struct bpf_prog *prog,
+							struct bpf_prog_array *effective)
+{
+	struct bpf_prog_array_item *item;
+	struct bpf_prog_array *array;
+	struct bpf_prog *p;
+
+	array = rcu_dereference(effective);
+	if (!array)
+		return NULL;
+
+	item = &array->items[0];
+	while ((p = READ_ONCE(item->prog))) {
+		if (p == prog)
+			return item;
+		item++;
+	}
+
+	return NULL;
+}
+
+static struct bpf_cgroup_storage **bpf_prog_find_active_storage(struct bpf_prog *prog)
+{
+	struct bpf_prog_array_item *item;
+	struct cgroup *cgrp;
+
+	if (prog->type != BPF_PROG_TYPE_CGROUP_SKB)
+		return NULL;
+
+	cgrp = task_dfl_cgroup(current);
+
+	item = bpf_prog_find_active(prog,
+				    cgrp->bpf.effective[BPF_CGROUP_INET_INGRESS]);
+	if (!item)
+		item = bpf_prog_find_active(prog,
+					    cgrp->bpf.effective[BPF_CGROUP_INET_EGRESS]);
+
+	return item ? item->cgroup_storage : NULL;
+}
+
+#else
+
+static struct bpf_cgroup_storage **bpf_prog_find_active_storage(struct bpf_prog *prog)
+{
+	return NULL;
+}
+
+#endif
+
 static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 			u32 *retval, u32 *time, bool xdp)
 {
-	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
+	struct bpf_cgroup_storage *dummy_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
+	struct bpf_cgroup_storage **storage = dummy_storage;
 	u64 time_start, time_spent = 0;
 	int ret = 0;
 	u32 i;
 
-	ret = bpf_cgroup_storages_alloc(storage, prog);
+	ret = bpf_cgroup_storages_alloc(dummy_storage, prog);
 	if (ret)
 		return ret;
 
@@ -31,6 +83,9 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 		repeat = 1;
 
 	rcu_read_lock();
+	storage = bpf_prog_find_active_storage(prog);
+	if (!storage)
+		storage = dummy_storage;
 	migrate_disable();
 	time_start = ktime_get_ns();
 	for (i = 0; i < repeat; i++) {
@@ -54,6 +109,9 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 			cond_resched();
 
 			rcu_read_lock();
+			storage = bpf_prog_find_active_storage(prog);
+			if (!storage)
+				storage = dummy_storage;
 			migrate_disable();
 			time_start = ktime_get_ns();
 		}
@@ -65,7 +123,7 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 	do_div(time_spent, repeat);
 	*time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
 
-	bpf_cgroup_storages_free(storage);
+	bpf_cgroup_storages_free(dummy_storage);
 
 	return ret;
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c b/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
new file mode 100644
index 0000000..12ca881
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#include "cgroup_helpers.h"
+#include "network_helpers.h"
+
+static char bpf_log_buf[BPF_LOG_BUF_SIZE];
+
+void test_cgroup_skb_prog_run(void)
+{
+	struct bpf_insn prog[] = {
+		BPF_LD_MAP_FD(BPF_REG_1, 0), /* map fd */
+		BPF_MOV64_IMM(BPF_REG_2, 0), /* flags, not used */
+		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage),
+		BPF_MOV64_IMM(BPF_REG_1, 1),
+		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_W, BPF_REG_0, BPF_REG_1, 0, 0),
+
+		BPF_MOV64_IMM(BPF_REG_0, 1), /* r0 = 1 */
+		BPF_EXIT_INSN(),
+	};
+	size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn);
+	int storage_fd = -1, prog_fd = -1, cg_fd = -1;
+	struct bpf_cgroup_storage_key key;
+	__u32 duration, retval, size;
+	char buf[128];
+	__u64 value;
+	int err;
+
+	storage_fd = bpf_create_map(BPF_MAP_TYPE_CGROUP_STORAGE,
+				    sizeof(struct bpf_cgroup_storage_key),
+				    8, 0, 0);
+	if (CHECK(storage_fd < 0, "create_map", "%s\n", strerror(errno)))
+		goto out;
+
+	prog[0].imm = storage_fd;
+
+	prog_fd = bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB,
+				   prog, insns_cnt, "GPL", 0,
+				   bpf_log_buf, BPF_LOG_BUF_SIZE);
+	if (CHECK(prog_fd < 0, "prog_load",
+		  "verifier output:\n%s\n-------\n", bpf_log_buf))
+		goto out;
+
+	if (CHECK_FAIL(setup_cgroup_environment()))
+		goto out;
+
+	cg_fd = create_and_get_cgroup("/cg");
+	if (CHECK_FAIL(cg_fd < 0))
+		goto out;
+
+	if (CHECK_FAIL(join_cgroup("/cg")))
+		goto out;
+
+	if (CHECK(bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_EGRESS, 0),
+		  "prog_attach", "%s\n", strerror(errno)))
+		goto out;
+
+	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
+				buf, &size, &retval, &duration);
+	CHECK(err || retval != 1, "prog_test_run",
+	      "err %d errno %d retval %d\n", err, errno, retval);
+
+	/* check that cgroup storage results are available after test run */
+
+	err = bpf_map_get_next_key(storage_fd, NULL, &key);
+	CHECK(err, "map_get_next_key", "%s\n", strerror(errno));
+
+	err = bpf_map_lookup_elem(storage_fd, &key, &value);
+	CHECK(err || value != NUM_ITER,
+	      "map_lookup_elem",
+	      "err %d errno %d cnt %lld(%d)\n", err, errno, value, NUM_ITER);
+out:
+	close(storage_fd);
+	close(prog_fd);
+	close(cg_fd);
+	cleanup_cgroup_environment();
+}
-- 
2.7.4

Re: [PATCH bpf-next v3 4/4] bpf: try to use existing cgroup storage in bpf_prog_test_run_skb

[Daniel Borkmann]

On 7/15/20 9:51 PM, Dmitry Yakunin wrote:
> Now we cannot check results in cgroup storage after running
> BPF_PROG_TEST_RUN command because it allocates dummy cgroup storage
> during test. This patch implements simple logic for searching already
> allocated cgroup storage through iterating effective programs of current
> cgroup and finding the first match. If match is not found fallback to
> temporary storage is happened.
> 
> v2:
>    - fix build without CONFIG_CGROUP_BPF (kernel test robot <lkp@intel.com>)
> 
> Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
> ---
>   net/bpf/test_run.c                                 | 64 +++++++++++++++++-
>   .../selftests/bpf/prog_tests/cgroup_skb_prog_run.c | 78 ++++++++++++++++++++++
>   2 files changed, 139 insertions(+), 3 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
> 
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 050390d..7382b22 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -15,15 +15,67 @@
>   #define CREATE_TRACE_POINTS
>   #include <trace/events/bpf_test_run.h>
>   
> +#ifdef CONFIG_CGROUP_BPF
> +
> +static struct bpf_prog_array_item *bpf_prog_find_active(struct bpf_prog *prog,
> +							struct bpf_prog_array *effective)
> +{
> +	struct bpf_prog_array_item *item;
> +	struct bpf_prog_array *array;
> +	struct bpf_prog *p;
> +
> +	array = rcu_dereference(effective);
> +	if (!array)
> +		return NULL;
> +
> +	item = &array->items[0];
> +	while ((p = READ_ONCE(item->prog))) {
> +		if (p == prog)
> +			return item;
> +		item++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct bpf_cgroup_storage **bpf_prog_find_active_storage(struct bpf_prog *prog)
> +{
> +	struct bpf_prog_array_item *item;
> +	struct cgroup *cgrp;
> +
> +	if (prog->type != BPF_PROG_TYPE_CGROUP_SKB)
> +		return NULL;
> +
> +	cgrp = task_dfl_cgroup(current);
> +
> +	item = bpf_prog_find_active(prog,
> +				    cgrp->bpf.effective[BPF_CGROUP_INET_INGRESS]);
> +	if (!item)
> +		item = bpf_prog_find_active(prog,
> +					    cgrp->bpf.effective[BPF_CGROUP_INET_EGRESS]);
> +
> +	return item ? item->cgroup_storage : NULL;
> +}
> +
> +#else
> +
> +static struct bpf_cgroup_storage **bpf_prog_find_active_storage(struct bpf_prog *prog)
> +{
> +	return NULL;
> +}
> +
> +#endif
> +
>   static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>   			u32 *retval, u32 *time, bool xdp)
>   {
> -	struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
> +	struct bpf_cgroup_storage *dummy_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
> +	struct bpf_cgroup_storage **storage = dummy_storage;
>   	u64 time_start, time_spent = 0;
>   	int ret = 0;
>   	u32 i;
>   
> -	ret = bpf_cgroup_storages_alloc(storage, prog);
> +	ret = bpf_cgroup_storages_alloc(dummy_storage, prog);
>   	if (ret)
>   		return ret;
>   
> @@ -31,6 +83,9 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>   		repeat = 1;
>   
>   	rcu_read_lock();
> +	storage = bpf_prog_find_active_storage(prog);
> +	if (!storage)
> +		storage = dummy_storage;
>   	migrate_disable();
>   	time_start = ktime_get_ns();
>   	for (i = 0; i < repeat; i++) {
> @@ -54,6 +109,9 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>   			cond_resched();
>   
>   			rcu_read_lock();
> +			storage = bpf_prog_find_active_storage(prog);
> +			if (!storage)
> +				storage = dummy_storage;
>   			migrate_disable();
>   			time_start = ktime_get_ns();
>   		}
> @@ -65,7 +123,7 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>   	do_div(time_spent, repeat);
>   	*time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
>   
> -	bpf_cgroup_storages_free(storage);
> +	bpf_cgroup_storages_free(dummy_storage);
>   
>   	return ret;
>   }
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c b/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
> new file mode 100644
> index 0000000..12ca881
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +
> +#include "cgroup_helpers.h"
> +#include "network_helpers.h"
> +
> +static char bpf_log_buf[BPF_LOG_BUF_SIZE];
> +
> +void test_cgroup_skb_prog_run(void)
> +{
> +	struct bpf_insn prog[] = {
> +		BPF_LD_MAP_FD(BPF_REG_1, 0), /* map fd */
> +		BPF_MOV64_IMM(BPF_REG_2, 0), /* flags, not used */
> +		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage),
> +		BPF_MOV64_IMM(BPF_REG_1, 1),
> +		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_W, BPF_REG_0, BPF_REG_1, 0, 0),
> +
> +		BPF_MOV64_IMM(BPF_REG_0, 1), /* r0 = 1 */
> +		BPF_EXIT_INSN(),
> +	};
> +	size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn);
> +	int storage_fd = -1, prog_fd = -1, cg_fd = -1;
> +	struct bpf_cgroup_storage_key key;
> +	__u32 duration, retval, size;
> +	char buf[128];
> +	__u64 value;
> +	int err;
> +
> +	storage_fd = bpf_create_map(BPF_MAP_TYPE_CGROUP_STORAGE,
> +				    sizeof(struct bpf_cgroup_storage_key),
> +				    8, 0, 0);
> +	if (CHECK(storage_fd < 0, "create_map", "%s\n", strerror(errno)))
> +		goto out;
> +
> +	prog[0].imm = storage_fd;
> +
> +	prog_fd = bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB,
> +				   prog, insns_cnt, "GPL", 0,
> +				   bpf_log_buf, BPF_LOG_BUF_SIZE);
> +	if (CHECK(prog_fd < 0, "prog_load",
> +		  "verifier output:\n%s\n-------\n", bpf_log_buf))
> +		goto out;
> +
> +	if (CHECK_FAIL(setup_cgroup_environment()))
> +		goto out;
> +
> +	cg_fd = create_and_get_cgroup("/cg");
> +	if (CHECK_FAIL(cg_fd < 0))
> +		goto out;
> +
> +	if (CHECK_FAIL(join_cgroup("/cg")))
> +		goto out;
> +
> +	if (CHECK(bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_EGRESS, 0),
> +		  "prog_attach", "%s\n", strerror(errno)))
> +		goto out;
> +
> +	err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
> +				buf, &size, &retval, &duration);

Hm, I think this approach is rather suboptimal, meaning, you need to load & even
actively attach the test program also to the cgroup aside from pushing this via
BPF prog test infra. So any other potential background traffic egressing from the
application will also go through the test program via BPF_CGROUP_INET_EGRESS.
Can't we instead extend the test infra to prepopulate and fetch the content from
the temp storage instead so this does not have any other side-effects?

> +	CHECK(err || retval != 1, "prog_test_run",
> +	      "err %d errno %d retval %d\n", err, errno, retval);
> +
> +	/* check that cgroup storage results are available after test run */
> +
> +	err = bpf_map_get_next_key(storage_fd, NULL, &key);
> +	CHECK(err, "map_get_next_key", "%s\n", strerror(errno));
> +
> +	err = bpf_map_lookup_elem(storage_fd, &key, &value);
> +	CHECK(err || value != NUM_ITER,
> +	      "map_lookup_elem",
> +	      "err %d errno %d cnt %lld(%d)\n", err, errno, value, NUM_ITER);
> +out:
> +	close(storage_fd);
> +	close(prog_fd);
> +	close(cg_fd);
> +	cleanup_cgroup_environment();
> +}
> 

Re: [PATCH bpf-next v3 4/4] bpf: try to use existing cgroup storage in bpf_prog_test_run_skb

[Dmitry Yakunin]

16.07.2020, 23:19, "Daniel Borkmann" <daniel@iogearbox.net>:
> On 7/15/20 9:51 PM, Dmitry Yakunin wrote:
>>  Now we cannot check results in cgroup storage after running
>>  BPF_PROG_TEST_RUN command because it allocates dummy cgroup storage
>>  during test. This patch implements simple logic for searching already
>>  allocated cgroup storage through iterating effective programs of current
>>  cgroup and finding the first match. If match is not found fallback to
>>  temporary storage is happened.
>>
>>  v2:
>>     - fix build without CONFIG_CGROUP_BPF (kernel test robot <lkp@intel.com>)
>>
>>  Signed-off-by: Dmitry Yakunin <zeil@yandex-team.ru>
>>  ---
>>    net/bpf/test_run.c | 64 +++++++++++++++++-
>>    .../selftests/bpf/prog_tests/cgroup_skb_prog_run.c | 78 ++++++++++++++++++++++
>>    2 files changed, 139 insertions(+), 3 deletions(-)
>>    create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
>>
>>  diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
>>  index 050390d..7382b22 100644
>>  --- a/net/bpf/test_run.c
>>  +++ b/net/bpf/test_run.c
>>  @@ -15,15 +15,67 @@
>>    #define CREATE_TRACE_POINTS
>>    #include <trace/events/bpf_test_run.h>
>>
>>  +#ifdef CONFIG_CGROUP_BPF
>>  +
>>  +static struct bpf_prog_array_item *bpf_prog_find_active(struct bpf_prog *prog,
>>  + struct bpf_prog_array *effective)
>>  +{
>>  + struct bpf_prog_array_item *item;
>>  + struct bpf_prog_array *array;
>>  + struct bpf_prog *p;
>>  +
>>  + array = rcu_dereference(effective);
>>  + if (!array)
>>  + return NULL;
>>  +
>>  + item = &array->items[0];
>>  + while ((p = READ_ONCE(item->prog))) {
>>  + if (p == prog)
>>  + return item;
>>  + item++;
>>  + }
>>  +
>>  + return NULL;
>>  +}
>>  +
>>  +static struct bpf_cgroup_storage **bpf_prog_find_active_storage(struct bpf_prog *prog)
>>  +{
>>  + struct bpf_prog_array_item *item;
>>  + struct cgroup *cgrp;
>>  +
>>  + if (prog->type != BPF_PROG_TYPE_CGROUP_SKB)
>>  + return NULL;
>>  +
>>  + cgrp = task_dfl_cgroup(current);
>>  +
>>  + item = bpf_prog_find_active(prog,
>>  + cgrp->bpf.effective[BPF_CGROUP_INET_INGRESS]);
>>  + if (!item)
>>  + item = bpf_prog_find_active(prog,
>>  + cgrp->bpf.effective[BPF_CGROUP_INET_EGRESS]);
>>  +
>>  + return item ? item->cgroup_storage : NULL;
>>  +}
>>  +
>>  +#else
>>  +
>>  +static struct bpf_cgroup_storage **bpf_prog_find_active_storage(struct bpf_prog *prog)
>>  +{
>>  + return NULL;
>>  +}
>>  +
>>  +#endif
>>  +
>>    static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>>                            u32 *retval, u32 *time, bool xdp)
>>    {
>>  - struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
>>  + struct bpf_cgroup_storage *dummy_storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { NULL };
>>  + struct bpf_cgroup_storage **storage = dummy_storage;
>>            u64 time_start, time_spent = 0;
>>            int ret = 0;
>>            u32 i;
>>
>>  - ret = bpf_cgroup_storages_alloc(storage, prog);
>>  + ret = bpf_cgroup_storages_alloc(dummy_storage, prog);
>>            if (ret)
>>                    return ret;
>>
>>  @@ -31,6 +83,9 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>>                    repeat = 1;
>>
>>            rcu_read_lock();
>>  + storage = bpf_prog_find_active_storage(prog);
>>  + if (!storage)
>>  + storage = dummy_storage;
>>            migrate_disable();
>>            time_start = ktime_get_ns();
>>            for (i = 0; i < repeat; i++) {
>>  @@ -54,6 +109,9 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>>                            cond_resched();
>>
>>                            rcu_read_lock();
>>  + storage = bpf_prog_find_active_storage(prog);
>>  + if (!storage)
>>  + storage = dummy_storage;
>>                            migrate_disable();
>>                            time_start = ktime_get_ns();
>>                    }
>>  @@ -65,7 +123,7 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
>>            do_div(time_spent, repeat);
>>            *time = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
>>
>>  - bpf_cgroup_storages_free(storage);
>>  + bpf_cgroup_storages_free(dummy_storage);
>>
>>            return ret;
>>    }
>>  diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c b/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
>>  new file mode 100644
>>  index 0000000..12ca881
>>  --- /dev/null
>>  +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_skb_prog_run.c
>>  @@ -0,0 +1,78 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +
>>  +#include <test_progs.h>
>>  +
>>  +#include "cgroup_helpers.h"
>>  +#include "network_helpers.h"
>>  +
>>  +static char bpf_log_buf[BPF_LOG_BUF_SIZE];
>>  +
>>  +void test_cgroup_skb_prog_run(void)
>>  +{
>>  + struct bpf_insn prog[] = {
>>  + BPF_LD_MAP_FD(BPF_REG_1, 0), /* map fd */
>>  + BPF_MOV64_IMM(BPF_REG_2, 0), /* flags, not used */
>>  + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_get_local_storage),
>>  + BPF_MOV64_IMM(BPF_REG_1, 1),
>>  + BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_W, BPF_REG_0, BPF_REG_1, 0, 0),
>>  +
>>  + BPF_MOV64_IMM(BPF_REG_0, 1), /* r0 = 1 */
>>  + BPF_EXIT_INSN(),
>>  + };
>>  + size_t insns_cnt = sizeof(prog) / sizeof(struct bpf_insn);
>>  + int storage_fd = -1, prog_fd = -1, cg_fd = -1;
>>  + struct bpf_cgroup_storage_key key;
>>  + __u32 duration, retval, size;
>>  + char buf[128];
>>  + __u64 value;
>>  + int err;
>>  +
>>  + storage_fd = bpf_create_map(BPF_MAP_TYPE_CGROUP_STORAGE,
>>  + sizeof(struct bpf_cgroup_storage_key),
>>  + 8, 0, 0);
>>  + if (CHECK(storage_fd < 0, "create_map", "%s\n", strerror(errno)))
>>  + goto out;
>>  +
>>  + prog[0].imm = storage_fd;
>>  +
>>  + prog_fd = bpf_load_program(BPF_PROG_TYPE_CGROUP_SKB,
>>  + prog, insns_cnt, "GPL", 0,
>>  + bpf_log_buf, BPF_LOG_BUF_SIZE);
>>  + if (CHECK(prog_fd < 0, "prog_load",
>>  + "verifier output:\n%s\n-------\n", bpf_log_buf))
>>  + goto out;
>>  +
>>  + if (CHECK_FAIL(setup_cgroup_environment()))
>>  + goto out;
>>  +
>>  + cg_fd = create_and_get_cgroup("/cg");
>>  + if (CHECK_FAIL(cg_fd < 0))
>>  + goto out;
>>  +
>>  + if (CHECK_FAIL(join_cgroup("/cg")))
>>  + goto out;
>>  +
>>  + if (CHECK(bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_INET_EGRESS, 0),
>>  + "prog_attach", "%s\n", strerror(errno)))
>>  + goto out;
>>  +
>>  + err = bpf_prog_test_run(prog_fd, NUM_ITER, &pkt_v4, sizeof(pkt_v4),
>>  + buf, &size, &retval, &duration);
>
> Hm, I think this approach is rather suboptimal, meaning, you need to load & even
> actively attach the test program also to the cgroup aside from pushing this via
> BPF prog test infra. So any other potential background traffic egressing from the
> application will also go through the test program via BPF_CGROUP_INET_EGRESS.
> Can't we instead extend the test infra to prepopulate and fetch the content from
> the temp storage instead so this does not have any other side-effects?

Thanks for you response, Daniel! Yes, I forgot to mention that this change can affect existing storage values if we run PROG_TEST_RUN command on the online program. But I thought that the case of testing bpf programs on production environments is uncommon and such solution is acceptable trade-off. I see potential rework of this patch through extending bpf_attr for PROG_TEST_RUN with user pointer to memory for cgroup storage contents and dumping cgroup storage with lookup_batch callback after test ends. Does this solution sounds good for you?

>>  + CHECK(err || retval != 1, "prog_test_run",
>>  + "err %d errno %d retval %d\n", err, errno, retval);
>>  +
>>  + /* check that cgroup storage results are available after test run */
>>  +
>>  + err = bpf_map_get_next_key(storage_fd, NULL, &key);
>>  + CHECK(err, "map_get_next_key", "%s\n", strerror(errno));
>>  +
>>  + err = bpf_map_lookup_elem(storage_fd, &key, &value);
>>  + CHECK(err || value != NUM_ITER,
>>  + "map_lookup_elem",
>>  + "err %d errno %d cnt %lld(%d)\n", err, errno, value, NUM_ITER);
>>  +out:
>>  + close(storage_fd);
>>  + close(prog_fd);
>>  + close(cg_fd);
>>  + cleanup_cgroup_environment();
>>  +}