bpf: pull before calling skb_postpull_rcsum()

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 <[email protected]>

---
  drivers/net/netdevsim/netdev.c                |  5 ++++
  tools/testing/selftests/bpf/test_tc_tunnel.sh | 27 +++++++++++++++++++
  2 files changed, 32 insertions(+)

drivers/net/netdevsim/netdev.c +5 -0

index 6db6a75ff9b9..e4808a6d37a4 100644
@@ -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;

tools/testing/selftests/bpf/test_tc_tunnel.sh +10 -0

index 334bdfeab940..4dac87f6a6fa 100755
@@ -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

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?

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 <[email protected]>

Fixes 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")

Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 net/core/filter.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

net/core/filter.c +3 -2

index 929358677183..43cc1fe58a2c 100644
@@ -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

Quoted 4 lines

net/core/filter.c +1 -2

-       skb_postpull_rcsum(skb, skb->data + off, len);
-       memmove(skb->data + len, skb->data, off);
+       old_data = skb->data;
        __skb_pull(skb, len);  

[..]

Quoted 1 line

+ 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);

Quoted 6 lines · On Mon, 19 Dec 2022 17:21:27 -0800 Stanislav Fomichev wrote:

Quoted 4 lines

net/core/filter.c +1 -2

-       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 ;)

Quoted 4 lines

Quoted 1 line

+ 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

Quoted 1 line

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

Yes.

Quoted 3 lines

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.

Quoted 1 line

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