[PATCH bpf 1/2] bpf: pull before calling skb_postpull_rcsum()

[PATCH bpf 1/2] bpf: pull before calling skb_postpull_rcsum()

[Jakub Kicinski]

Anand hit a BUG() when pulling off headers on egress to a SW tunnel.
We get to skb_checksum_help() with an invalid checksum offset
(commit d7ea0d9df2a6 ("net: remove two BUG() from skb_checksum_help()")
converted those BUGs to WARN_ONs()).
He points out oddness in how skb_postpull_rcsum() gets used.
Indeed looks like we should pull before "postpull", otherwise
the CHECKSUM_PARTIAL fixup from skb_postpull_rcsum() will not
be able to do its job:

	if (skb->ip_summed == CHECKSUM_PARTIAL &&
	    skb_checksum_start_offset(skb) < 0)
		skb->ip_summed = CHECKSUM_NONE;

Reported-by: Anand Parthasarathy <anpartha@meta.com>
Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: daniel@iogearbox.net
CC: martin.lau@linux.dev
CC: song@kernel.org
CC: john.fastabend@gmail.com
CC: sdf@google.com
CC: bpf@vger.kernel.org
---
 net/core/filter.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 929358677183..43cc1fe58a2c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3180,15 +3180,18 @@ static int bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
 
 static int bpf_skb_generic_pop(struct sk_buff *skb, u32 off, u32 len)
 {
+	void *old_data;
+
 	/* skb_ensure_writable() is not needed here, as we're
 	 * already working on an uncloned skb.
 	 */
 	if (unlikely(!pskb_may_pull(skb, off + len)))
 		return -ENOMEM;
 
-	skb_postpull_rcsum(skb, skb->data + off, len);
-	memmove(skb->data + len, skb->data, off);
+	old_data = skb->data;
 	__skb_pull(skb, len);
+	skb_postpull_rcsum(skb, old_data + off, len);
+	memmove(skb->data, old_data, off);
 
 	return 0;
 }
-- 
2.38.1

[PATCH bpf 2/2] selftests/bpf: tunnel: add sanity test for checksums

[Jakub Kicinski]

Simple netdevsim based test. Netdevsim will validate xmit'ed
packets, in particular we care about checksum sanity (along
the lines of checks inside skb_checksum_help()). Triggering
skb_checksum_help() directly would require the right HW device
or a crypto device setup, netdevsim is much simpler.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/netdev.c                |  5 ++++
 tools/testing/selftests/bpf/test_tc_tunnel.sh | 27 +++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 6db6a75ff9b9..e4808a6d37a4 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -33,6 +33,11 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!nsim_ipsec_tx(ns, skb))
 		goto out;
 
+	/* Validate the packet */
+	if (skb->ip_summed == CHECKSUM_PARTIAL)
+		WARN_ON_ONCE((unsigned int)skb_checksum_start_offset(skb) >=
+			     skb_headlen(skb));
+
 	u64_stats_update_begin(&ns->syncp);
 	ns->tx_packets++;
 	ns->tx_bytes += skb->len;
diff --git a/tools/testing/selftests/bpf/test_tc_tunnel.sh b/tools/testing/selftests/bpf/test_tc_tunnel.sh
index 334bdfeab940..4dac87f6a6fa 100755
--- a/tools/testing/selftests/bpf/test_tc_tunnel.sh
+++ b/tools/testing/selftests/bpf/test_tc_tunnel.sh
@@ -15,6 +15,7 @@ readonly ns1_v4=192.168.1.1
 readonly ns2_v4=192.168.1.2
 readonly ns1_v6=fd::1
 readonly ns2_v6=fd::2
+readonly nsim_v4=192.168.2.1
 
 # Must match port used by bpf program
 readonly udpport=5555
@@ -67,6 +68,10 @@ cleanup() {
 	if [[ -n $server_pid ]]; then
 		kill $server_pid 2> /dev/null
 	fi
+
+	if [ -e /sys/bus/netdevsim/devices/netdevsim1 ]; then
+	    echo 1 > /sys/bus/netdevsim/del_device
+	fi
 }
 
 server_listen() {
@@ -93,6 +98,25 @@ verify_data() {
 	fi
 }
 
+decap_sanity() {
+    echo "test decap sanity"
+    modprobe netdevsim
+    echo 1 1 > /sys/bus/netdevsim/new_device
+    udevadm settle
+    nsim=$(ls /sys/bus/netdevsim/devices/netdevsim1/net/)
+    ip link set dev $nsim up
+    ip addr add dev $nsim $nsim_v4/24
+
+    tc qdisc add dev $nsim clsact
+    tc filter add dev $nsim egress \
+       bpf direct-action object-file ${BPF_FILE} section decap
+
+    echo abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz | \
+	nc -u 192.168.2.2 7777
+
+    echo 1 > /sys/bus/netdevsim/del_device
+}
+
 set -e
 
 # no arguments: automated test, run all
@@ -138,6 +162,9 @@ if [[ "$#" -eq "0" ]]; then
 		$0 ipv6 ip6udp $mac 2000
 	done
 
+	echo "decap sanity check"
+	decap_sanity
+
 	echo "OK. All tests passed"
 	exit 0
 fi
-- 
2.38.1

[PATCH bpf] selftests/bpf: Test bpf_skb_adjust_room on CHECKSUM_PARTIAL

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

When the bpf_skb_adjust_room() shrinks the skb such that
its csum_start is invalid, the skb->ip_summed should
be reset from CHECKSUM_PARTIAL to CHECKSUM_NONE.

This patch adds a test to ensure the skb->ip_summed changed
from CHECKSUM_PARTIAL to CHECKSUM_NONE after bpf_skb_adjust_room().

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
 .../selftests/bpf/prog_tests/decap_sanity.c   | 83 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_tracing_net.h     |  6 ++
 .../selftests/bpf/progs/decap_sanity.c        | 68 +++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/decap_sanity.c
 create mode 100644 tools/testing/selftests/bpf/progs/decap_sanity.c

diff --git a/tools/testing/selftests/bpf/prog_tests/decap_sanity.c b/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
new file mode 100644
index 000000000000..2fbb3017b740
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <net/if.h>
+#include <linux/in6.h>
+
+#include "test_progs.h"
+#include "network_helpers.h"
+#include "decap_sanity.skel.h"
+
+#define SYS(fmt, ...)						\
+	({							\
+		char cmd[1024];					\
+		snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__);	\
+		if (!ASSERT_OK(system(cmd), cmd))		\
+			goto fail;				\
+	})
+
+#define NS_TEST "decap_sanity_ns"
+#define IPV6_IFACE_ADDR "face::1"
+#define UDP_TEST_PORT 7777
+
+void test_decap_sanity(void)
+{
+	LIBBPF_OPTS(bpf_tc_hook, qdisc_hook, .attach_point = BPF_TC_EGRESS);
+	LIBBPF_OPTS(bpf_tc_opts, tc_attach);
+	struct nstoken *nstoken = NULL;
+	struct decap_sanity *skel;
+	struct sockaddr_in6 addr;
+	socklen_t addrlen;
+	char buf[128] = {};
+	int sockfd, err;
+
+	skel = decap_sanity__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel open_and_load"))
+		return;
+
+	SYS("ip netns add %s", NS_TEST);
+	SYS("ip -net %s -6 addr add %s/128 dev lo nodad", NS_TEST, IPV6_IFACE_ADDR);
+	SYS("ip -net %s link set dev lo up", NS_TEST);
+
+	nstoken = open_netns(NS_TEST);
+	if (!ASSERT_OK_PTR(nstoken, "open_netns"))
+		goto fail;
+
+	qdisc_hook.ifindex = if_nametoindex("lo");
+	if (!ASSERT_GT(qdisc_hook.ifindex, 0, "if_nametoindex lo"))
+		goto fail;
+
+	err = bpf_tc_hook_create(&qdisc_hook);
+	if (!ASSERT_OK(err, "create qdisc hook"))
+		goto fail;
+
+	tc_attach.prog_fd = bpf_program__fd(skel->progs.decap_sanity);
+	err = bpf_tc_attach(&qdisc_hook, &tc_attach);
+	if (!ASSERT_OK(err, "attach filter"))
+		goto fail;
+
+	addrlen = sizeof(addr);
+	err = make_sockaddr(AF_INET6, IPV6_IFACE_ADDR, UDP_TEST_PORT,
+			    (void *)&addr, &addrlen);
+	if (!ASSERT_OK(err, "make_sockaddr"))
+		goto fail;
+	sockfd = socket(AF_INET6, SOCK_DGRAM, 0);
+	if (!ASSERT_NEQ(sockfd, -1, "socket"))
+		goto fail;
+	err = sendto(sockfd, buf, sizeof(buf), 0, (void *)&addr, addrlen);
+	close(sockfd);
+	if (!ASSERT_EQ(err, sizeof(buf), "send"))
+		goto fail;
+
+	ASSERT_EQ(skel->bss->init_csum_partial, true, "init_csum_partial");
+	ASSERT_EQ(skel->bss->final_csum_none, true, "final_csum_none");
+	ASSERT_EQ(skel->bss->broken_csum_start, false, "broken_csum_start");
+
+fail:
+	if (nstoken)
+		close_netns(nstoken);
+	system("ip netns del " NS_TEST " >& /dev/null");
+	decap_sanity__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
index b394817126cf..cfed4df490f3 100644
--- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
+++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
@@ -50,6 +50,12 @@
 #define ICSK_TIME_LOSS_PROBE	5
 #define ICSK_TIME_REO_TIMEOUT	6
 
+#define ETH_HLEN		14
+#define ETH_P_IPV6		0x86DD
+
+#define CHECKSUM_NONE		0
+#define CHECKSUM_PARTIAL	3
+
 #define IFNAMSIZ		16
 
 #define RTF_GATEWAY		0x0002
diff --git a/tools/testing/selftests/bpf/progs/decap_sanity.c b/tools/testing/selftests/bpf/progs/decap_sanity.c
new file mode 100644
index 000000000000..b85113554cbf
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/decap_sanity.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#include "vmlinux.h"
+#include "bpf_tracing_net.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+#define UDP_TEST_PORT 7777
+
+void *bpf_cast_to_kern_ctx(void *) __ksym;
+bool init_csum_partial = false;
+bool final_csum_none = false;
+bool broken_csum_start = false;
+
+static inline unsigned int skb_headlen(const struct sk_buff *skb)
+{
+	return skb->len - skb->data_len;
+}
+
+static unsigned int skb_headroom(const struct sk_buff *skb)
+{
+	return skb->data - skb->head;
+}
+
+static int skb_checksum_start_offset(const struct sk_buff *skb)
+{
+	return skb->csum_start - skb_headroom(skb);
+}
+
+SEC("tc")
+int decap_sanity(struct __sk_buff *skb)
+{
+	struct sk_buff *kskb;
+	struct ipv6hdr ip6h;
+	struct udphdr udph;
+	int err;
+
+	if (skb->protocol != __bpf_constant_htons(ETH_P_IPV6))
+		return TC_ACT_SHOT;
+
+	if (bpf_skb_load_bytes(skb, ETH_HLEN, &ip6h, sizeof(ip6h)))
+		return TC_ACT_SHOT;
+
+	if (ip6h.nexthdr != IPPROTO_UDP)
+		return TC_ACT_SHOT;
+
+	if (bpf_skb_load_bytes(skb, ETH_HLEN + sizeof(ip6h), &udph, sizeof(udph)))
+		return TC_ACT_SHOT;
+
+	if (udph.dest != __bpf_constant_htons(UDP_TEST_PORT))
+		return TC_ACT_SHOT;
+
+	kskb = bpf_cast_to_kern_ctx(skb);
+	init_csum_partial = (kskb->ip_summed == CHECKSUM_PARTIAL);
+	err = bpf_skb_adjust_room(skb, -(s32)(ETH_HLEN + sizeof(ip6h) + sizeof(udph)),
+				  1, BPF_F_ADJ_ROOM_FIXED_GSO);
+	if (err)
+		return TC_ACT_SHOT;
+	final_csum_none = (kskb->ip_summed == CHECKSUM_NONE);
+	if (kskb->ip_summed == CHECKSUM_PARTIAL &&
+	    (unsigned int)skb_checksum_start_offset(kskb) >= skb_headlen(kskb))
+		broken_csum_start = true;
+
+	return TC_ACT_SHOT;
+}
+
+char __license[] SEC("license") = "GPL";
-- 
2.30.2

Re: [PATCH bpf 2/2] selftests/bpf: tunnel: add sanity test for checksums

[Martin KaFai Lau]

On 12/19/22 4:47 PM, Jakub Kicinski wrote:
> Simple netdevsim based test. Netdevsim will validate xmit'ed
> packets, in particular we care about checksum sanity (along
> the lines of checks inside skb_checksum_help()). Triggering
> skb_checksum_help() directly would require the right HW device
> or a crypto device setup, netdevsim is much simpler.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   drivers/net/netdevsim/netdev.c                |  5 ++++
>   tools/testing/selftests/bpf/test_tc_tunnel.sh | 27 +++++++++++++++++++
>   2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 6db6a75ff9b9..e4808a6d37a4 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -33,6 +33,11 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	if (!nsim_ipsec_tx(ns, skb))
>   		goto out;
>   
> +	/* Validate the packet */
> +	if (skb->ip_summed == CHECKSUM_PARTIAL)
> +		WARN_ON_ONCE((unsigned int)skb_checksum_start_offset(skb) >=
> +			     skb_headlen(skb));
> +
>   	u64_stats_update_begin(&ns->syncp);
>   	ns->tx_packets++;
>   	ns->tx_bytes += skb->len;
> diff --git a/tools/testing/selftests/bpf/test_tc_tunnel.sh b/tools/testing/selftests/bpf/test_tc_tunnel.sh
> index 334bdfeab940..4dac87f6a6fa 100755
> --- a/tools/testing/selftests/bpf/test_tc_tunnel.sh
> +++ b/tools/testing/selftests/bpf/test_tc_tunnel.sh
> @@ -15,6 +15,7 @@ readonly ns1_v4=192.168.1.1
>   readonly ns2_v4=192.168.1.2
>   readonly ns1_v6=fd::1
>   readonly ns2_v6=fd::2
> +readonly nsim_v4=192.168.2.1
>   
>   # Must match port used by bpf program
>   readonly udpport=5555
> @@ -67,6 +68,10 @@ cleanup() {
>   	if [[ -n $server_pid ]]; then
>   		kill $server_pid 2> /dev/null
>   	fi
> +
> +	if [ -e /sys/bus/netdevsim/devices/netdevsim1 ]; then
> +	    echo 1 > /sys/bus/netdevsim/del_device
> +	fi
>   }
>   
>   server_listen() {
> @@ -93,6 +98,25 @@ verify_data() {
>   	fi
>   }
>   
> +decap_sanity() {
> +    echo "test decap sanity"
> +    modprobe netdevsim
> +    echo 1 1 > /sys/bus/netdevsim/new_device
> +    udevadm settle
> +    nsim=$(ls /sys/bus/netdevsim/devices/netdevsim1/net/)
> +    ip link set dev $nsim up
> +    ip addr add dev $nsim $nsim_v4/24
> +
> +    tc qdisc add dev $nsim clsact
> +    tc filter add dev $nsim egress \
> +       bpf direct-action object-file ${BPF_FILE} section decap
> +
> +    echo abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz | \
> +	nc -u 192.168.2.2 7777

Thanks for the fix and the idea on how to test it.

I have posted a patch to translate this test to a test for test_progs that can 
finish and exit such that it can be run continuously in CI.  The test attaches a 
tc-bpf at lo and the bpf prog directly checks for the skb->ip_summed == 
CHECKSUM_NONE and the broken csum_start condition.

If the test_progs patch looks good, patch 1 can be landed first and then land 
the test_progs patch.  wdyt?

Re: [PATCH bpf 2/2] selftests/bpf: tunnel: add sanity test for checksums

[Jakub Kicinski]

On Tue, 20 Dec 2022 15:21:08 -0800 Martin KaFai Lau wrote:
> Thanks for the fix and the idea on how to test it.
> 
> I have posted a patch to translate this test to a test for test_progs that can 
> finish and exit such that it can be run continuously in CI.  The test attaches a 
> tc-bpf at lo and the bpf prog directly checks for the skb->ip_summed == 
> CHECKSUM_NONE and the broken csum_start condition.
> 
> If the test_progs patch looks good, patch 1 can be landed first and then land 
> the test_progs patch.  wdyt?

I'm not attached to the test, your patch looks good!

Re: [PATCH bpf 1/2] bpf: pull before calling skb_postpull_rcsum()

[Stanislav Fomichev]

On Mon, Dec 19, 2022 at 4:47 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Anand hit a BUG() when pulling off headers on egress to a SW tunnel.
> We get to skb_checksum_help() with an invalid checksum offset
> (commit d7ea0d9df2a6 ("net: remove two BUG() from skb_checksum_help()")
> converted those BUGs to WARN_ONs()).
> He points out oddness in how skb_postpull_rcsum() gets used.
> Indeed looks like we should pull before "postpull", otherwise
> the CHECKSUM_PARTIAL fixup from skb_postpull_rcsum() will not
> be able to do its job:
>
>         if (skb->ip_summed == CHECKSUM_PARTIAL &&
>             skb_checksum_start_offset(skb) < 0)
>                 skb->ip_summed = CHECKSUM_NONE;
>
> Reported-by: Anand Parthasarathy <anpartha@meta.com>
> Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: daniel@iogearbox.net
> CC: martin.lau@linux.dev
> CC: song@kernel.org
> CC: john.fastabend@gmail.com
> CC: sdf@google.com
> CC: bpf@vger.kernel.org
> ---
>  net/core/filter.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 929358677183..43cc1fe58a2c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3180,15 +3180,18 @@ static int bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
>
>  static int bpf_skb_generic_pop(struct sk_buff *skb, u32 off, u32 len)
>  {
> +       void *old_data;
> +
>         /* skb_ensure_writable() is not needed here, as we're
>          * already working on an uncloned skb.
>          */
>         if (unlikely(!pskb_may_pull(skb, off + len)))
>                 return -ENOMEM;
>
> -       skb_postpull_rcsum(skb, skb->data + off, len);
> -       memmove(skb->data + len, skb->data, off);
> +       old_data = skb->data;
>         __skb_pull(skb, len);

[..]

> +       skb_postpull_rcsum(skb, old_data + off, len);

Are you sure about the 'old_data + off' part here (for
CHECKSUM_COMPLETE)? Shouldn't it be old_data?
I'm assuming we need to negate the old parts that we've pulled?

Maybe safer/more correct to do the following?

skb_pull_rcsum(skb, off);
memmove(skb->data, skb->data-off, off);


> +       memmove(skb->data, old_data, off);
>
>         return 0;
>  }
> --
> 2.38.1
>

Re: [PATCH bpf 1/2] bpf: pull before calling skb_postpull_rcsum()

[Jakub Kicinski]

On Mon, 19 Dec 2022 17:21:27 -0800 Stanislav Fomichev wrote:
> > -       skb_postpull_rcsum(skb, skb->data + off, len);
> > -       memmove(skb->data + len, skb->data, off);
> > +       old_data = skb->data;
> >         __skb_pull(skb, len);  
> 
> [..]

Very counter-productively trimmed ;)

> > +       skb_postpull_rcsum(skb, old_data + off, len);  
> 
> Are you sure about the 'old_data + off' part here (for
> CHECKSUM_COMPLETE)? Shouldn't it be old_data?

AFAIU before:

      old_data (aka skb->data before)
     /
    / off  len 
   V-----><--->
[ .=======xxxxx... buffer ...... ]
          ^
           \
            the xxx part is what we delete

after:
          skb->data (after)
         /
        v
[ .yyyyy=======... buffer ...... ]
   <---><----->
    len   off  
   ^
    \
     the yyy part is technically the old values of === but now "outside"
     of the valid packet data

> I'm assuming we need to negate the old parts that we've pulled?

Yes.

> Maybe safer/more correct to do the following?
> 
> skb_pull_rcsum(skb, off);

This just pulls from the front, we need to skip over various L2/L3
headers thanks to off. Hopefully the diagrams help, LMK if they are
wrong.

> memmove(skb->data, skb->data-off, off);

Re: [PATCH bpf 1/2] bpf: pull before calling skb_postpull_rcsum()

[Stanislav Fomichev]

On Mon, Dec 19, 2022 at 5:45 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 19 Dec 2022 17:21:27 -0800 Stanislav Fomichev wrote:
> > > -       skb_postpull_rcsum(skb, skb->data + off, len);
> > > -       memmove(skb->data + len, skb->data, off);
> > > +       old_data = skb->data;
> > >         __skb_pull(skb, len);
> >
> > [..]
>
> Very counter-productively trimmed ;)
>
> > > +       skb_postpull_rcsum(skb, old_data + off, len);
> >
> > Are you sure about the 'old_data + off' part here (for
> > CHECKSUM_COMPLETE)? Shouldn't it be old_data?
>
> AFAIU before:
>
>       old_data (aka skb->data before)
>      /
>     / off  len
>    V-----><--->
> [ .=======xxxxx... buffer ...... ]
>           ^
>            \
>             the xxx part is what we delete
>
> after:
>           skb->data (after)
>          /
>         v
> [ .yyyyy=======... buffer ...... ]
>    <---><----->
>     len   off
>    ^
>     \
>      the yyy part is technically the old values of === but now "outside"
>      of the valid packet data
>
> > I'm assuming we need to negate the old parts that we've pulled?
>
> Yes.
>
> > Maybe safer/more correct to do the following?
> >
> > skb_pull_rcsum(skb, off);
>
> This just pulls from the front, we need to skip over various L2/L3
> headers thanks to off. Hopefully the diagrams help, LMK if they are
> wrong.

Ah, yeah, you're totally correct, thanks for the pics :-) Not sure how
I mixed up off/len in skb_pull_rcsum..

Acked-by: Stanislav Fomichev <sdf@google.com>

> > memmove(skb->data, skb->data-off, off);

Re: [PATCH bpf 1/2] bpf: pull before calling skb_postpull_rcsum()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Mon, 19 Dec 2022 16:47:00 -0800 you wrote:
> Anand hit a BUG() when pulling off headers on egress to a SW tunnel.
> We get to skb_checksum_help() with an invalid checksum offset
> (commit d7ea0d9df2a6 ("net: remove two BUG() from skb_checksum_help()")
> converted those BUGs to WARN_ONs()).
> He points out oddness in how skb_postpull_rcsum() gets used.
> Indeed looks like we should pull before "postpull", otherwise
> the CHECKSUM_PARTIAL fixup from skb_postpull_rcsum() will not
> be able to do its job:
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: pull before calling skb_postpull_rcsum()
    https://git.kernel.org/bpf/bpf/c/54c3f1a81421
  - [bpf,2/2] selftests/bpf: tunnel: add sanity test for checksums
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html