BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf 1/2] bpf: Set mac_len in bpf_skb_change_head
@ 2021-04-27 13:55 Jussi Maki
  2021-04-27 13:55 ` [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head Jussi Maki
  0 siblings, 1 reply; 6+ messages in thread
From: Jussi Maki @ 2021-04-27 13:55 UTC (permalink / raw)
  To: bpf; +Cc: daniel, Jussi Maki

The skb_change_head() helper did not set "skb->mac_len", which is problematic
when it's used in combination with skb_redirect_peer(). Without it, redirecting
a packet from a L3 device such as wireguard to the veth peer device will cause
skb->data to point to the middle of the IP header on entry to tcp_v4_rcv() since
the L2 header is not pulled correctly due to mac_len=0.

Signed-off-by: Jussi Maki <joamaki@gmail.com>
---
 net/core/filter.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index cae56d08a670..65ab4e21c087 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3784,6 +3784,7 @@ static inline int __bpf_skb_change_head(struct sk_buff *skb, u32 head_room,
 		__skb_push(skb, head_room);
 		memset(skb->data, 0, head_room);
 		skb_reset_mac_header(skb);
+		skb_reset_mac_len(skb);
 	}
 
 	return ret;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head
  2021-04-27 13:55 [PATCH bpf 1/2] bpf: Set mac_len in bpf_skb_change_head Jussi Maki
@ 2021-04-27 13:55 ` Jussi Maki
  2021-04-27 21:41   ` Andrii Nakryiko
  0 siblings, 1 reply; 6+ messages in thread
From: Jussi Maki @ 2021-04-27 13:55 UTC (permalink / raw)
  To: bpf; +Cc: daniel, Jussi Maki

Adds test to check that bpf_skb_change_head can be used
in combination with bpf_redirect_peer to redirect a packet
from L3 device to veth.

Fixes: a426d97e970d ("bpf: Set mac_len in bpf_skb_change_head")
Signed-off-by: Jussi Maki <joamaki@gmail.com>
---
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   2 +-
 .../selftests/bpf/progs/test_tc_peer.c        |  24 ++++
 .../testing/selftests/bpf/test_tc_peer_user.c | 127 ++++++++++++++++++
 .../testing/selftests/bpf/test_tc_redirect.sh |  95 ++++++++++---
 5 files changed, 233 insertions(+), 16 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/test_tc_peer_user.c

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 4866f6a21901..49f3f050be4d 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -27,6 +27,7 @@ test_tcpnotify_user
 test_libbpf
 test_tcp_check_syncookie_user
 test_sysctl
+test_tc_peer_user
 xdping
 test_cpp
 *.skel.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 283e5ad8385e..0e05fefe2333 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -84,7 +84,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
 TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
-	xdpxceiver
+	xdpxceiver test_tc_peer_user
 
 TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
 
diff --git a/tools/testing/selftests/bpf/progs/test_tc_peer.c b/tools/testing/selftests/bpf/progs/test_tc_peer.c
index fc84a7685aa2..49f0eefd58e7 100644
--- a/tools/testing/selftests/bpf/progs/test_tc_peer.c
+++ b/tools/testing/selftests/bpf/progs/test_tc_peer.c
@@ -5,8 +5,11 @@
 #include <linux/bpf.h>
 #include <linux/stddef.h>
 #include <linux/pkt_cls.h>
+#include <linux/if_ether.h>
+#include <linux/ip.h>
 
 #include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
 
 enum {
 	dev_src,
@@ -42,4 +45,25 @@ SEC("src_ingress") int tc_src(struct __sk_buff *skb)
 	return bpf_redirect_peer(get_dev_ifindex(dev_dst), 0);
 }
 
+SEC("src_ingress_l3") int tc_src_l3(struct __sk_buff *skb)
+{
+	__u16 proto = skb->protocol;
+
+	if (bpf_skb_change_head(skb, ETH_HLEN, 0) != 0)
+		return TC_ACT_SHOT;
+
+	__u8 src_mac[] = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55};
+	if (bpf_skb_store_bytes(skb, 0, &src_mac, ETH_ALEN, 0) != 0)
+		return TC_ACT_SHOT;
+
+	__u8 dst_mac[] = {0x00, 0x22, 0x33, 0x44, 0x55, 0x66};
+	if (bpf_skb_store_bytes(skb, ETH_ALEN, &dst_mac, ETH_ALEN, 0) != 0)
+		return TC_ACT_SHOT;
+
+	if (bpf_skb_store_bytes(skb, ETH_ALEN + ETH_ALEN, &proto, sizeof(__u16), 0) != 0)
+		return TC_ACT_SHOT;
+
+	return bpf_redirect_peer(get_dev_ifindex(dev_dst), 0);
+}
+
 char __license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_tc_peer_user.c b/tools/testing/selftests/bpf/test_tc_peer_user.c
new file mode 100644
index 000000000000..bdecf2bd1cc1
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tc_peer_user.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Networking across two network namespaces based on TUN/TAP.
+ * Like veth, but slow and L3. Used for testing BPF redirect_peer
+ * from L3 to L2 veth device.
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/if.h>
+#include <linux/if_tun.h>
+#include <linux/limits.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+int tun_alloc(char *name) {
+	struct ifreq ifr;
+	int fd, err;
+	char cmd[512];
+
+	if ((fd = open("/dev/net/tun", O_RDWR)) < 0)
+		return -1;
+
+	memset(&ifr, 0, sizeof(ifr));
+
+	ifr.ifr_flags = IFF_TUN | IFF_NO_PI;
+	if (*name)
+		strncpy(ifr.ifr_name, name, IFNAMSIZ);
+
+	if ((err = ioctl(fd, TUNSETIFF, (void *) &ifr)) < 0) {
+		close(fd);
+		return err;
+	}
+
+	snprintf(cmd, sizeof(cmd), "ip link set dev %s up", name);
+	system(cmd);
+
+	return fd;
+}
+
+#define MAX(a, b) ((a) > (b) ? (a) : (b))
+
+enum {
+	SRC_TO_TARGET = 0,
+	TARGET_TO_SRC = 1,
+};
+
+void setns_by_name(char *name) {
+	int nsfd;
+	char nspath[PATH_MAX];
+
+        snprintf(nspath, sizeof(nspath), "%s/%s", "/var/run/netns", name);
+        nsfd = open(nspath, O_RDONLY | O_CLOEXEC);
+        if (nsfd < 0) {
+		fprintf(stderr, "failed to open net namespace %s: %s\n", name, strerror(errno));
+		exit(1);
+        }
+	setns(nsfd, CLONE_NEWNET);
+	close(nsfd);
+}
+
+int main(int argc, char **argv) {
+	char *src_ns, *src_tun, *target_ns, *target_tun;
+	int srcfd, targetfd;
+
+	if (argc != 5) {
+		fprintf(stderr, "usage: %s <source namespace> <source tun device name> <target namespace> <target tun device name>\n", argv[0]);
+		return 1;
+	}
+
+	src_ns = argv[1];
+	src_tun = argv[2];
+	target_ns = argv[3];
+	target_tun = argv[4];
+
+	setns_by_name(src_ns);
+	srcfd = tun_alloc(src_tun);
+	if (srcfd < 0) {
+		fprintf(stderr, "failed to allocate tun device\n");
+		return 1;
+	}
+
+	setns_by_name(target_ns); 
+	targetfd = tun_alloc(target_tun);
+	if (srcfd < 0) {
+		fprintf(stderr, "failed to allocate tun device\n");
+		return 1;
+	}
+
+	fd_set rfds, wfds;
+	FD_ZERO(&rfds);
+	FD_ZERO(&wfds);
+
+	for (;;) {
+	        char buf[4096];
+	        int direction, nread, nwrite;
+		FD_SET(srcfd, &rfds);
+		FD_SET(targetfd, &rfds);
+
+		if (select(1 + MAX(srcfd, targetfd), &rfds, NULL, NULL, NULL) < 0) {
+		       fprintf(stderr, "select failed: %s\n", strerror(errno));
+		       return 1;
+	        }
+
+	        direction = FD_ISSET(srcfd, &rfds) ? SRC_TO_TARGET : TARGET_TO_SRC;
+
+	        nread = read(direction == SRC_TO_TARGET ? srcfd : targetfd, buf, sizeof(buf));
+	        if (nread < 0) {
+		       fprintf(stderr, "read failed: %s\n", strerror(errno));
+		       return 1;
+	        }
+
+	        nwrite = write(direction == SRC_TO_TARGET ? targetfd : srcfd, buf, nread);
+	        if (nwrite != nread) {
+		       fprintf(stderr, "write failed: %s\n", strerror(errno));
+		       return 1;
+	        }
+	}
+}
diff --git a/tools/testing/selftests/bpf/test_tc_redirect.sh b/tools/testing/selftests/bpf/test_tc_redirect.sh
index 8868aa1ca902..469e1dc13f03 100755
--- a/tools/testing/selftests/bpf/test_tc_redirect.sh
+++ b/tools/testing/selftests/bpf/test_tc_redirect.sh
@@ -50,15 +50,32 @@ readonly IP4_DST="172.16.2.100"
 readonly IP6_SRC="::1:dead:beef:cafe"
 readonly IP6_DST="::2:dead:beef:cafe"
 
+readonly IP4_TUN_SRC="172.17.1.100"
+readonly IP4_TUN_FWD="172.17.1.200"
+
+readonly IP6_TUN_SRC="1::dead:beef:0"
+readonly IP6_TUN_FWD="1::dead:beef:1"
+
 readonly IP4_SLL="169.254.0.1"
 readonly IP4_DLL="169.254.0.2"
 readonly IP4_NET="169.254.0.0"
 
+readonly MAC_DST_FWD="00:11:22:33:44:55"
+readonly MAC_DST="00:22:33:44:55:66"
+
+TEST_TC_PEER_USER_PID=""
+NC4_PID=""
+NC6_PID=""
+
 netns_cleanup()
 {
 	ip netns del ${NS_SRC}
 	ip netns del ${NS_FWD}
 	ip netns del ${NS_DST}
+
+	[ -n "$TEST_TC_PEER_USER_PID" ] && kill ${TEST_TC_PEER_USER_PID} || true
+	[ -n "${NC4_PID}" ] && kill ${NC4_PID} || true
+	[ -n "${NC6_PID}" ] && kill ${NC6_PID} || true
 }
 
 netns_setup()
@@ -70,6 +87,9 @@ netns_setup()
 	ip link add veth_src type veth peer name veth_src_fwd
 	ip link add veth_dst type veth peer name veth_dst_fwd
 
+	ip link set veth_dst_fwd address ${MAC_DST_FWD}
+	ip link set veth_dst address ${MAC_DST}
+
 	ip link set veth_src netns ${NS_SRC}
 	ip link set veth_src_fwd netns ${NS_FWD}
 
@@ -117,14 +137,20 @@ netns_setup()
 
 	ip -netns ${NS_SRC} neigh add ${IP6_DST} dev veth_src lladdr $fmac_src
 	ip -netns ${NS_DST} neigh add ${IP6_SRC} dev veth_dst lladdr $fmac_dst
+
+	ip -netns ${NS_DST} neigh add ${IP4_TUN_SRC} dev veth_dst lladdr $fmac_dst
+	ip -netns ${NS_DST} neigh add ${IP6_TUN_SRC} dev veth_dst lladdr $fmac_dst
+	ip -netns ${NS_DST} neigh add ${IP6_TUN_FWD} dev veth_dst lladdr $fmac_dst
 }
 
 netns_test_connectivity()
 {
 	set +e
 
-	ip netns exec ${NS_DST} bash -c "nc -4 -l -p 9004 &"
-	ip netns exec ${NS_DST} bash -c "nc -6 -l -p 9006 &"
+	ip netns exec ${NS_DST} nc -4 -l -p 9004 &
+	NC4_PID=$!
+	ip netns exec ${NS_DST} nc -6 -l -p 9006 &
+	NC6_PID=$!
 
 	TEST="TCPv4 connectivity test"
 	ip netns exec ${NS_SRC} bash -c "timeout ${TIMEOUT} dd if=/dev/zero bs=1000 count=100 > /dev/tcp/${IP4_DST}/9004"
@@ -170,14 +196,52 @@ netns_setup_bpf()
 {
 	local obj=$1
 	local use_forwarding=${2:-0}
+	local use_tuntap=${3:-0}
+
+	if [ "$use_tuntap" -eq "1" ]; then
+		# Set up tuntap based tunnel between src and fwd namespaces.
+		./test_tc_peer_user ${NS_SRC} tun_src ${NS_FWD} tun_fwd &
+		TEST_TC_PEER_USER_PID=$!
+
+		while ! ip -netns ${NS_SRC} link show tun_src; do echo "Waiting for tun_src to appear..."; sleep 0.5; done
+		while ! ip -netns ${NS_FWD} link show tun_fwd; do echo "Waiting for tun_fwd to appoar..."; sleep 0.5; done
+
+		ip -netns ${NS_SRC} addr add dev tun_src ${IP4_TUN_SRC}/24
+		ip -netns ${NS_FWD} addr add dev tun_fwd ${IP4_TUN_FWD}/24
 
-	ip netns exec ${NS_FWD} tc qdisc add dev veth_src_fwd clsact
-	ip netns exec ${NS_FWD} tc filter add dev veth_src_fwd ingress bpf da obj $obj sec src_ingress
-	ip netns exec ${NS_FWD} tc filter add dev veth_src_fwd egress  bpf da obj $obj sec chk_egress
+		ip -netns ${NS_SRC} addr add dev tun_src ${IP6_TUN_SRC}/64 nodad
+		ip -netns ${NS_FWD} addr add dev tun_fwd ${IP6_TUN_FWD}/64 nodad
 
-	ip netns exec ${NS_FWD} tc qdisc add dev veth_dst_fwd clsact
-	ip netns exec ${NS_FWD} tc filter add dev veth_dst_fwd ingress bpf da obj $obj sec dst_ingress
-	ip netns exec ${NS_FWD} tc filter add dev veth_dst_fwd egress  bpf da obj $obj sec chk_egress
+		ip -netns ${NS_SRC} route del ${IP4_DST}/32 dev veth_src scope global
+		ip -netns ${NS_SRC} route add ${IP4_DST}/32 via ${IP4_TUN_FWD} dev tun_src scope global
+		ip -netns ${NS_DST} route add ${IP4_TUN_SRC}/32 dev veth_dst scope global
+
+		ip -netns ${NS_SRC} route del ${IP6_DST}/128 dev veth_src scope global
+		ip -netns ${NS_SRC} route add ${IP6_DST}/128 via ${IP6_TUN_FWD} dev tun_src scope global
+		ip -netns ${NS_DST} route add ${IP6_TUN_SRC}/128 dev veth_dst scope global
+		ip -netns ${NS_DST} route add ${IP6_TUN_FWD}/128 dev veth_dst scope global
+
+		ip netns exec ${NS_FWD} tc qdisc add dev tun_fwd clsact
+		ip netns exec ${NS_FWD} tc filter add dev tun_fwd ingress bpf da obj $obj sec src_ingress_l3
+
+		# Enable forwarding back towards src, but not for packets coming from the tunnel.
+		ip netns exec ${NS_FWD} sysctl -w net.ipv4.ip_forward=1
+		ip netns exec ${NS_FWD} sysctl -w net.ipv6.conf.all.forwarding=1
+		ip netns exec ${NS_FWD} sysctl -w net.ipv4.conf.veth_dst_fwd.forwarding=1
+		ip netns exec ${NS_FWD} sysctl -w net.ipv4.conf.veth_src_fwd.forwarding=0
+		ip netns exec ${NS_FWD} sysctl -w net.ipv4.conf.tun_fwd.forwarding=0
+		ip netns exec ${NS_FWD} sysctl -w net.ipv6.conf.veth_src_fwd.forwarding=0
+		ip netns exec ${NS_FWD} sysctl -w net.ipv6.conf.veth_dst_fwd.forwarding=1
+		ip netns exec ${NS_FWD} sysctl -w net.ipv6.conf.tun_fwd.forwarding=0
+	else
+		ip netns exec ${NS_FWD} tc qdisc add dev veth_src_fwd clsact
+		ip netns exec ${NS_FWD} tc filter add dev veth_src_fwd ingress bpf da obj $obj sec src_ingress
+		ip netns exec ${NS_FWD} tc filter add dev veth_src_fwd egress  bpf da obj $obj sec chk_egress
+
+		ip netns exec ${NS_FWD} tc qdisc add dev veth_dst_fwd clsact
+		ip netns exec ${NS_FWD} tc filter add dev veth_dst_fwd ingress bpf da obj $obj sec dst_ingress
+		ip netns exec ${NS_FWD} tc filter add dev veth_dst_fwd egress  bpf da obj $obj sec chk_egress
+	fi
 
 	if [ "$use_forwarding" -eq "1" ]; then
 		# bpf_fib_lookup() checks if forwarding is enabled
@@ -190,13 +254,10 @@ netns_setup_bpf()
 	veth_src=$(ip netns exec ${NS_FWD} cat /sys/class/net/veth_src_fwd/ifindex)
 	veth_dst=$(ip netns exec ${NS_FWD} cat /sys/class/net/veth_dst_fwd/ifindex)
 
-	progs=$(ip netns exec ${NS_FWD} bpftool net --json | jq -r '.[] | .tc | map(.id) | .[]')
-	for prog in $progs; do
-		map=$(bpftool prog show id $prog --json | jq -r '.map_ids | .? | .[]')
-		if [ ! -z "$map" ]; then
-			bpftool map update id $map key hex $(hex_mem_str 0) value hex $(hex_mem_str $veth_src)
-			bpftool map update id $map key hex $(hex_mem_str 1) value hex $(hex_mem_str $veth_dst)
-		fi
+ 	maps=$(bpftool map list --json | jq -r '.[] | select(.name == "ifindex_map") | .id')
+	for map in $maps; do
+		bpftool map update id $map key hex $(hex_mem_str 0) value hex $(hex_mem_str $veth_src)
+		bpftool map update id $map key hex $(hex_mem_str 1) value hex $(hex_mem_str $veth_dst)
 	done
 }
 
@@ -214,3 +275,7 @@ netns_cleanup
 netns_setup
 netns_setup_bpf test_tc_peer.o
 netns_test_connectivity
+netns_cleanup
+netns_setup
+netns_setup_bpf test_tc_peer.o 0 1
+netns_test_connectivity
-- 
2.30.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head
  2021-04-27 13:55 ` [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head Jussi Maki
@ 2021-04-27 21:41   ` Andrii Nakryiko
  2021-04-28 10:39     ` Jussi Maki
  0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2021-04-27 21:41 UTC (permalink / raw)
  To: Jussi Maki; +Cc: bpf, Daniel Borkmann

On Tue, Apr 27, 2021 at 6:56 AM Jussi Maki <joamaki@gmail.com> wrote:
>
> Adds test to check that bpf_skb_change_head can be used
> in combination with bpf_redirect_peer to redirect a packet
> from L3 device to veth.
>
> Fixes: a426d97e970d ("bpf: Set mac_len in bpf_skb_change_head")
> Signed-off-by: Jussi Maki <joamaki@gmail.com>
> ---
>  tools/testing/selftests/bpf/.gitignore        |   1 +
>  tools/testing/selftests/bpf/Makefile          |   2 +-
>  .../selftests/bpf/progs/test_tc_peer.c        |  24 ++++
>  .../testing/selftests/bpf/test_tc_peer_user.c | 127 ++++++++++++++++++
>  .../testing/selftests/bpf/test_tc_redirect.sh |  95 ++++++++++---
>  5 files changed, 233 insertions(+), 16 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/test_tc_peer_user.c
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 4866f6a21901..49f3f050be4d 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -27,6 +27,7 @@ test_tcpnotify_user
>  test_libbpf
>  test_tcp_check_syncookie_user
>  test_sysctl
> +test_tc_peer_user

can we make it into a reasonable test inside test_progs? that way it
will be executed regularly

>  xdping
>  test_cpp
>  *.skel.h
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 283e5ad8385e..0e05fefe2333 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -84,7 +84,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
>  TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
>         flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
>         test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
> -       xdpxceiver
> +       xdpxceiver test_tc_peer_user
>
>  TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
>
> diff --git a/tools/testing/selftests/bpf/progs/test_tc_peer.c b/tools/testing/selftests/bpf/progs/test_tc_peer.c
> index fc84a7685aa2..49f0eefd58e7 100644
> --- a/tools/testing/selftests/bpf/progs/test_tc_peer.c
> +++ b/tools/testing/selftests/bpf/progs/test_tc_peer.c
> @@ -5,8 +5,11 @@
>  #include <linux/bpf.h>
>  #include <linux/stddef.h>
>  #include <linux/pkt_cls.h>
> +#include <linux/if_ether.h>
> +#include <linux/ip.h>
>
>  #include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
>
>  enum {
>         dev_src,
> @@ -42,4 +45,25 @@ SEC("src_ingress") int tc_src(struct __sk_buff *skb)
>         return bpf_redirect_peer(get_dev_ifindex(dev_dst), 0);
>  }
>
> +SEC("src_ingress_l3") int tc_src_l3(struct __sk_buff *skb)

please keep SEC() on separate line

also, is this a BPF_PROG_TYPE_SCHED_CLS program? Can you usee libbpf's
"classifier/src_ingress_l3" naming convention?

> +{
> +       __u16 proto = skb->protocol;
> +
> +       if (bpf_skb_change_head(skb, ETH_HLEN, 0) != 0)
> +               return TC_ACT_SHOT;
> +
> +       __u8 src_mac[] = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55};

we try to keep BPF code compliant with C89, so please move all the
variable declaration into a single block at the top of a function

> +       if (bpf_skb_store_bytes(skb, 0, &src_mac, ETH_ALEN, 0) != 0)
> +               return TC_ACT_SHOT;
> +
> +       __u8 dst_mac[] = {0x00, 0x22, 0x33, 0x44, 0x55, 0x66};
> +       if (bpf_skb_store_bytes(skb, ETH_ALEN, &dst_mac, ETH_ALEN, 0) != 0)
> +               return TC_ACT_SHOT;
> +
> +       if (bpf_skb_store_bytes(skb, ETH_ALEN + ETH_ALEN, &proto, sizeof(__u16), 0) != 0)
> +               return TC_ACT_SHOT;
> +
> +       return bpf_redirect_peer(get_dev_ifindex(dev_dst), 0);
> +}
> +

[...]

> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> +
> +enum {
> +       SRC_TO_TARGET = 0,
> +       TARGET_TO_SRC = 1,
> +};
> +
> +void setns_by_name(char *name) {


{ should be on a new line

please run scripts/checkpatch.pl on these files, it will point out
trivial issues like this

> +       int nsfd;
> +       char nspath[PATH_MAX];
> +
> +        snprintf(nspath, sizeof(nspath), "%s/%s", "/var/run/netns", name);
> +        nsfd = open(nspath, O_RDONLY | O_CLOEXEC);
> +        if (nsfd < 0) {
> +               fprintf(stderr, "failed to open net namespace %s: %s\n", name, strerror(errno));
> +               exit(1);
> +        }

here seems to be a mix of tabs and spaces

> +       setns(nsfd, CLONE_NEWNET);
> +       close(nsfd);
> +}
> +

[...]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head
  2021-04-27 21:41   ` Andrii Nakryiko
@ 2021-04-28 10:39     ` Jussi Maki
  2021-04-28 10:49       ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: Jussi Maki @ 2021-04-28 10:39 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, Daniel Borkmann

On Tue, Apr 27, 2021 at 11:41 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> > ...
> > +test_tc_peer_user
>
> can we make it into a reasonable test inside test_progs? that way it
> will be executed regularly

There doesn't seem to be any tests yet for redirect_peer in test_progs and I'd
like this test to live next to them. Would it make sense to rework
test_tc_redirect.sh
into the test_progs framework?

What's your thoughts on this Daniel?

> >
> > +SEC("src_ingress_l3") int tc_src_l3(struct __sk_buff *skb)
>
> please keep SEC() on separate line
>
> also, is this a BPF_PROG_TYPE_SCHED_CLS program? Can you usee libbpf's
> "classifier/src_ingress_l3" naming convention?

I'm following the conventions in that file. Should they all be
changed? Happy to do that.

> > +{
> > +       __u16 proto = skb->protocol;
> > +
> > +       if (bpf_skb_change_head(skb, ETH_HLEN, 0) != 0)
> > +               return TC_ACT_SHOT;
> > +
> > +       __u8 src_mac[] = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55};
>
> we try to keep BPF code compliant with C89, so please move all the
> variable declaration into a single block at the top of a function

Will fix, thanks.

> > +void setns_by_name(char *name) {
>
>
> { should be on a new line
>
> please run scripts/checkpatch.pl on these files, it will point out
> trivial issues like this

Sorry about that. Will make sure I'll run it on all future submissions.

> > +       int nsfd;
> > +       char nspath[PATH_MAX];
> > +
> > +        snprintf(nspath, sizeof(nspath), "%s/%s", "/var/run/netns", name);
> > +        nsfd = open(nspath, O_RDONLY | O_CLOEXEC);
> > +        if (nsfd < 0) {
> > +               fprintf(stderr, "failed to open net namespace %s: %s\n", name, strerror(errno));
> > +               exit(1);
> > +        }
>
> here seems to be a mix of tabs and spaces

Thanks, will fix it and my editor's settings ;-).

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head
  2021-04-28 10:39     ` Jussi Maki
@ 2021-04-28 10:49       ` Daniel Borkmann
  2021-04-28 13:39         ` Jussi Maki
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2021-04-28 10:49 UTC (permalink / raw)
  To: Jussi Maki, Andrii Nakryiko; +Cc: bpf

On 4/28/21 12:39 PM, Jussi Maki wrote:
> On Tue, Apr 27, 2021 at 11:41 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>> ...
>>> +test_tc_peer_user
>>
>> can we make it into a reasonable test inside test_progs? that way it
>> will be executed regularly
> 
> There doesn't seem to be any tests yet for redirect_peer in test_progs and I'd
> like this test to live next to them. Would it make sense to rework
> test_tc_redirect.sh
> into the test_progs framework?
> 
> What's your thoughts on this Daniel?

Yes, please, that would be awesome to have test_tc_redirect.sh reworked to
run as part of test_progs! :) Then it will be covered by CI [0].

Thanks,
Daniel

   [0] https://travis-ci.com/github/kernel-patches/bpf/pull_requests

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head
  2021-04-28 10:49       ` Daniel Borkmann
@ 2021-04-28 13:39         ` Jussi Maki
  0 siblings, 0 replies; 6+ messages in thread
From: Jussi Maki @ 2021-04-28 13:39 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: Andrii Nakryiko, bpf

On Wed, Apr 28, 2021 at 12:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Yes, please, that would be awesome to have test_tc_redirect.sh reworked to
> run as part of test_progs! :) Then it will be covered by CI [0].

Sure thing. I'll rework it and will follow up with a separate patch
for the rework and then v2
of this change.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-27 13:55 [PATCH bpf 1/2] bpf: Set mac_len in bpf_skb_change_head Jussi Maki
2021-04-27 13:55 ` [PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_head Jussi Maki
2021-04-27 21:41   ` Andrii Nakryiko
2021-04-28 10:39     ` Jussi Maki
2021-04-28 10:49       ` Daniel Borkmann
2021-04-28 13:39         ` Jussi Maki

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git