[PATCH bpf 1/2] bpf: Fix bpf_skb_change_tail wrt csum partial skbs

[PATCH bpf 1/2] bpf: Fix bpf_skb_change_tail wrt csum partial skbs

[Daniel Borkmann]

Cilium generates ICMP "frag needed" replies from BPF when a LB DSR
packet exceeds the egress MTU. The reply is built by first trimming the
packet down to target size via bpf_skb_change_tail(), and then pushing
the ICMP error headers in front of it.

The trim is rejected for skbs which carry a checksum offload, e.g. TCP
packets aggregated by GRO on ingress where tcp_gro_complete() leaves
the skb as CHECKSUM_PARTIAL. __bpf_skb_min_len() raises the minimum
length to the end of the L4 checksum field, so a trim to 42 bytes bails
out with -EINVAL given a min_len of 52 in this case, and due to that
the ICMP generator fails. This is not the case if GRO is turned off.

Fix this bpf_skb_change_tail() restriction and drop the checksum offload
when the new length no longer covers the checksum field. The BPF program
rewrites the skb into an ICMP error and computes the checksum itself
anyway.

Fixes: 5293efe62df8 ("bpf: add bpf_skb_change_tail helper")
Reported-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Reported-by: Yusuke Suzuki <yusuke.suzuki@isovalent.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 net/core/filter.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 61940e753552..8513167a858a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3961,12 +3961,6 @@ static u32 __bpf_skb_min_len(const struct sk_buff *skb)
 		if (offset > 0)
 			min_len = offset;
 	}
-	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		offset = skb_checksum_start_offset(skb) +
-			 skb->csum_offset + sizeof(__sum16);
-		if (offset > 0)
-			min_len = offset;
-	}
 	return min_len;
 }
 
@@ -3983,6 +3977,11 @@ static int bpf_skb_grow_rcsum(struct sk_buff *skb, unsigned int new_len)
 
 static int bpf_skb_trim_rcsum(struct sk_buff *skb, unsigned int new_len)
 {
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    new_len < skb_checksum_start_offset(skb) + skb->csum_offset +
+		      sizeof(__sum16))
+		skb->ip_summed = CHECKSUM_NONE;
+
 	return __skb_trim_rcsum(skb, new_len);
 }
 
-- 
2.43.0

[PATCH bpf 2/2] selftests/bpf: Add test for bpf_skb_change_tail on csum partial skbs

[Daniel Borkmann]

Add a test which builds an ICMP error out of a TCP segment. A tcx prog
on the client's egress side trims the first data segment down to the
target size and pushes the ICMP error headers in front of it to then
reflect the packet back to the sender.

  # LDLIBS=-static PKG_CONFIG='pkg-config --static' ./vmtest.sh -- ./test_progs -t tc_change_tail
  [...]
  #509     tc_change_tail:OK
  #510     tc_change_tail_pmtu:OK
  Summary: 2/0 PASSED, 0 SKIPPED, 0/0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 .../bpf/prog_tests/tc_change_tail_pmtu.c      | 125 +++++++++++++++++
 .../bpf/progs/test_tc_change_tail_pmtu.c      | 129 ++++++++++++++++++
 2 files changed, 254 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tc_change_tail_pmtu.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_tc_change_tail_pmtu.c

diff --git a/tools/testing/selftests/bpf/prog_tests/tc_change_tail_pmtu.c b/tools/testing/selftests/bpf/prog_tests/tc_change_tail_pmtu.c
new file mode 100644
index 000000000000..7acdbd5757a9
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/tc_change_tail_pmtu.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <netinet/tcp.h>
+
+#include "test_progs.h"
+#include "network_helpers.h"
+#include "test_tc_change_tail_pmtu.skel.h"
+
+#define CLIENT_NS	"tc-change-tail-cli-ns"
+#define SERVER_NS	"tc-change-tail-srv-ns"
+#define CLIENT_IP	"192.168.1.1"
+#define SERVER_IP	"192.168.1.2"
+
+#define TEST_PMTU	1000
+#define TEST_MSS_MAX	(TEST_PMTU - 20 - 20)
+#define TIMEOUT_MS	3000
+#define XFER_BYTES	8192
+
+void test_tc_change_tail_pmtu(void)
+{
+	LIBBPF_OPTS(bpf_tcx_opts, tcx_opts);
+	int mss_before = 0, mss_after = 0, ifindex, port;
+	int srv_fd = -1, srv_conn_fd = -1, cli_fd = -1;
+	struct test_tc_change_tail_pmtu *skel = NULL;
+	struct nstoken *nstoken = NULL;
+	static char buf[XFER_BYTES];
+	socklen_t optlen;
+	ssize_t bytes;
+	size_t total;
+
+	if (!ASSERT_OK(make_netns(CLIENT_NS), "make client ns"))
+		return;
+	if (!ASSERT_OK(make_netns(SERVER_NS), "make server ns"))
+		goto out_client_ns;
+
+	nstoken = open_netns(CLIENT_NS);
+	if (!ASSERT_OK_PTR(nstoken, "open client ns"))
+		goto out;
+	SYS(out, "ip link add veth1 type veth peer name veth2 netns " SERVER_NS);
+	SYS(out, "ip -4 addr add " CLIENT_IP "/24 dev veth1");
+	SYS(out, "ip link set veth1 up");
+	ifindex = if_nametoindex("veth1");
+	if (!ASSERT_NEQ(ifindex, 0, "if_nametoindex"))
+		goto out;
+	close_netns(nstoken);
+	nstoken = NULL;
+
+	nstoken = open_netns(SERVER_NS);
+	if (!ASSERT_OK_PTR(nstoken, "open server ns"))
+		goto out;
+	SYS(out, "ip -4 addr add " SERVER_IP "/24 dev veth2");
+	SYS(out, "ip link set veth2 up");
+	srv_fd = start_server(AF_INET, SOCK_STREAM, SERVER_IP, 0, TIMEOUT_MS);
+	if (!ASSERT_OK_FD(srv_fd, "start server"))
+		goto out;
+	close_netns(nstoken);
+	nstoken = NULL;
+
+	skel = test_tc_change_tail_pmtu__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open and load skeleton"))
+		goto out;
+
+	port = get_socket_local_port(srv_fd);
+	if (!ASSERT_GE(port, 0, "get server port"))
+		goto out;
+
+	skel->bss->server_port = port;
+	skel->bss->pmtu = TEST_PMTU;
+
+	nstoken = open_netns(CLIENT_NS);
+	if (!ASSERT_OK_PTR(nstoken, "open client ns"))
+		goto out;
+
+	skel->links.change_tail_icmp =
+		bpf_program__attach_tcx(skel->progs.change_tail_icmp, ifindex,
+					&tcx_opts);
+	if (!ASSERT_OK_PTR(skel->links.change_tail_icmp, "attach tcx"))
+		goto out;
+
+	cli_fd = connect_to_fd(srv_fd, TIMEOUT_MS);
+	if (!ASSERT_OK_FD(cli_fd, "connect to server"))
+		goto out;
+	srv_conn_fd = accept(srv_fd, NULL, NULL);
+	if (!ASSERT_OK_FD(srv_conn_fd, "accept connection"))
+		goto out;
+	if (!ASSERT_OK(settimeo(srv_conn_fd, TIMEOUT_MS), "set server timeout"))
+		goto out;
+
+	optlen = sizeof(mss_before);
+	if (!ASSERT_OK(getsockopt(cli_fd, IPPROTO_TCP, TCP_MAXSEG, &mss_before,
+				  &optlen), "get mss before"))
+		goto out;
+
+	bytes = send(cli_fd, buf, sizeof(buf), 0);
+	if (!ASSERT_EQ(bytes, (ssize_t)sizeof(buf), "send data"))
+		goto out;
+
+	for (total = 0; total < sizeof(buf); total += bytes) {
+		bytes = recv(srv_conn_fd, buf, sizeof(buf), 0);
+		if (bytes <= 0)
+			break;
+	}
+
+	ASSERT_EQ(total, sizeof(buf), "receive data");
+	ASSERT_OK(skel->data->change_tail_ret, "change tail");
+	ASSERT_OK(skel->bss->adjust_room_ret, "adjust room");
+	ASSERT_TRUE(skel->bss->icmp_sent, "icmp sent");
+
+	optlen = sizeof(mss_after);
+	if (!ASSERT_OK(getsockopt(cli_fd, IPPROTO_TCP, TCP_MAXSEG, &mss_after,
+				  &optlen), "get mss after"))
+		goto out;
+
+	ASSERT_LT(mss_after, mss_before, "mss reduced");
+	ASSERT_LE(mss_after, TEST_MSS_MAX, "mss below pmtu");
+out:
+	close(srv_conn_fd);
+	close(cli_fd);
+	close(srv_fd);
+	test_tc_change_tail_pmtu__destroy(skel);
+	close_netns(nstoken);
+	remove_netns(SERVER_NS);
+out_client_ns:
+	remove_netns(CLIENT_NS);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_tc_change_tail_pmtu.c b/tools/testing/selftests/bpf/progs/test_tc_change_tail_pmtu.c
new file mode 100644
index 000000000000..5c4c07545bc9
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_tc_change_tail_pmtu.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdbool.h>
+#include <stddef.h>
+
+#include <linux/bpf.h>
+#include <linux/icmp.h>
+#include <linux/if_ether.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <linux/tcp.h>
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+#define ICMP_SAMPLE_LEN	(sizeof(struct iphdr) + 8)
+#define ICMP_HDRS_LEN	(sizeof(struct iphdr) + sizeof(struct icmphdr))
+
+__be16 server_port = 0;
+__u16 pmtu = 0;
+
+long change_tail_ret = 1;
+long adjust_room_ret = 0;
+bool icmp_sent = false;
+bool icmp_err = false;
+
+static __always_inline __sum16 csum_fold(__wsum csum)
+{
+	csum = (csum & 0xffff) + (csum >> 16);
+	csum = (csum & 0xffff) + (csum >> 16);
+
+	return (__sum16)~csum;
+}
+
+SEC("tc/egress")
+int change_tail_icmp(struct __sk_buff *skb)
+{
+	__u8 smac[ETH_ALEN], dmac[ETH_ALEN];
+	void *data, *data_end;
+	struct icmphdr *icmp;
+	struct ethhdr *eth;
+	struct tcphdr *tcp;
+	__be32 saddr, daddr;
+	struct iphdr *ip;
+	__wsum csum;
+
+	if (icmp_sent || icmp_err)
+		return TCX_PASS;
+
+	data = (void *)(long)skb->data;
+	data_end = (void *)(long)skb->data_end;
+
+	eth = data;
+	if ((void *)(eth + 1) > data_end)
+		return TCX_PASS;
+	if (eth->h_proto != bpf_htons(ETH_P_IP))
+		return TCX_PASS;
+
+	ip = (void *)(eth + 1);
+	if ((void *)(ip + 1) > data_end)
+		return TCX_PASS;
+	if (ip->ihl != 5 || ip->protocol != IPPROTO_TCP)
+		return TCX_PASS;
+
+	tcp = (void *)(ip + 1);
+	if ((void *)(tcp + 1) > data_end)
+		return TCX_PASS;
+	if (tcp->dest != server_port)
+		return TCX_PASS;
+	if (bpf_ntohs(ip->tot_len) <= sizeof(*ip) + tcp->doff * 4)
+		return TCX_PASS;
+
+	__builtin_memcpy(smac, eth->h_source, ETH_ALEN);
+	__builtin_memcpy(dmac, eth->h_dest, ETH_ALEN);
+	saddr = ip->saddr;
+	daddr = ip->daddr;
+
+	change_tail_ret = bpf_skb_change_tail(skb, ETH_HLEN + ICMP_SAMPLE_LEN, 0);
+	if (change_tail_ret) {
+		icmp_err = true;
+		return TCX_PASS;
+	}
+
+	adjust_room_ret = bpf_skb_adjust_room(skb, ICMP_HDRS_LEN,
+					      BPF_ADJ_ROOM_MAC,
+					      BPF_F_ADJ_ROOM_NO_CSUM_RESET);
+	if (adjust_room_ret) {
+		icmp_err = true;
+		return TCX_DROP;
+	}
+
+	data = (void *)(long)skb->data;
+	data_end = (void *)(long)skb->data_end;
+
+	eth = data;
+	ip = (void *)(eth + 1);
+	icmp = (void *)(ip + 1);
+	if ((void *)icmp + sizeof(*icmp) + ICMP_SAMPLE_LEN > data_end) {
+		icmp_err = true;
+		return TCX_DROP;
+	}
+
+	__builtin_memcpy(eth->h_dest, smac, ETH_ALEN);
+	__builtin_memcpy(eth->h_source, dmac, ETH_ALEN);
+
+	__builtin_memset(icmp, 0, sizeof(*icmp));
+	icmp->type = ICMP_DEST_UNREACH;
+	icmp->code = ICMP_FRAG_NEEDED;
+	icmp->un.frag.mtu = bpf_htons(pmtu);
+
+	__builtin_memset(ip, 0, sizeof(*ip));
+	ip->version = 4;
+	ip->ihl = 5;
+	ip->ttl = 64;
+	ip->protocol = IPPROTO_ICMP;
+	ip->tot_len = bpf_htons(ICMP_HDRS_LEN + ICMP_SAMPLE_LEN);
+	ip->saddr = daddr;
+	ip->daddr = saddr;
+
+	csum = bpf_csum_diff(NULL, 0, (__be32 *)icmp,
+			     sizeof(*icmp) + ICMP_SAMPLE_LEN, 0);
+	icmp->checksum = csum_fold(csum);
+	csum = bpf_csum_diff(NULL, 0, (__be32 *)ip, sizeof(*ip), 0);
+	ip->check = csum_fold(csum);
+	icmp_sent = true;
+	return bpf_redirect(skb->ifindex, BPF_F_INGRESS);
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.43.0

Re: [PATCH bpf 1/2] bpf: Fix bpf_skb_change_tail wrt csum partial skbs

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon,  7 Sep 2026 14:10:24 +0200 you wrote:
> Cilium generates ICMP "frag needed" replies from BPF when a LB DSR
> packet exceeds the egress MTU. The reply is built by first trimming the
> packet down to target size via bpf_skb_change_tail(), and then pushing
> the ICMP error headers in front of it.
> 
> The trim is rejected for skbs which carry a checksum offload, e.g. TCP
> packets aggregated by GRO on ingress where tcp_gro_complete() leaves
> the skb as CHECKSUM_PARTIAL. __bpf_skb_min_len() raises the minimum
> length to the end of the L4 checksum field, so a trim to 42 bytes bails
> out with -EINVAL given a min_len of 52 in this case, and due to that
> the ICMP generator fails. This is not the case if GRO is turned off.
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: Fix bpf_skb_change_tail wrt csum partial skbs
    https://git.kernel.org/bpf/bpf/c/3b55f350c68a
  - [bpf,2/2] selftests/bpf: Add test for bpf_skb_change_tail on csum partial skbs
    https://git.kernel.org/bpf/bpf/c/15e2565f1c43

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