[RFC PATCH bpf-next] bpf: Support L4 csum update for IPv6 address changes

[RFC PATCH bpf-next] bpf: Support L4 csum update for IPv6 address changes

[Paul Chaignon]

In Cilium, we use bpf_csum_diff + bpf_l4_csum_replace to, among other
things, update the L4 checksum after reverse SNATing IPv6 packets. That
use case is however not currently supported and leads to invalid
skb->csum values in some cases. This patch adds support for IPv6 address
changes in bpf_l4_csum_update via a new flag.

When calling bpf_l4_csum_replace in Cilium, it ends up calling
inet_proto_csum_replace_by_diff:

    1:  void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
    2:                                       __wsum diff, bool pseudohdr)
    3:  {
    4:      if (skb->ip_summed != CHECKSUM_PARTIAL) {
    5:          csum_replace_by_diff(sum, diff);
    6:          if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
    7:              skb->csum = ~csum_sub(diff, skb->csum);
    8:      } else if (pseudohdr) {
    9:          *sum = ~csum_fold(csum_add(diff, csum_unfold(*sum)));
    10:     }
    11: }

The bug happens when we're in the CHECKSUM_COMPLETE state. We've just
updated one of the IPv6 addresses. The helper now updates the L4 header
checksum on line 5. Next, it updates skb->csum on line 7. It shouldn't.

For an IPv6 packet, the updates of the IPv6 address and of the L4
checksum will cancel each other. The checksums are set such that
computing a checksum over the packet including its checksum will result
in a sum of 0. So the same is true here when we update the L4 checksum
on line 5. We'll update it as to cancel the previous IPv6 address
update. Hence skb->csum should remain untouched in this case.

The same bug doesn't affect IPv4 packets because, in that case, three
fields are updated: the IPv4 address, the IP checksum, and the L4
checksum. The change to the IPv4 address and one of the checksums still
cancel each other in skb->csum, but we're left with one checksum update
and should therefore update skb->csum accordingly. That's exactly what
inet_proto_csum_replace_by_diff does.

This special case for IPv6 L4 checksums is also described atop
inet_proto_csum_replace16, the function we should be using in this case.

This patch introduces a new bpf_l4_csum_replace flag, BPF_F_IPV6_ADDR,
to indicate we're updating the L4 checksum after an IPv6 address change.
When this flag is set, bpf_l4_csum_replace will call
inet_proto_csum_replace16 instead of inet_proto_csum_replace_by_diff.

I'm sending this as an RFC because I'm not convinced this is the ideal
solution, but also prefer to discuss this around concrete code rather
than a simple bug report. In particular, I would have preferred a
solution we can backport to stable. Since we will have to change the API
to provide additional information to bpf_l4_csum_replace, I'm unsure
such a solution exists.

Fixes: 7d672345ed295 ("bpf: add generic bpf_csum_diff helper")
Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
---
 include/uapi/linux/bpf.h       |  1 +
 net/core/filter.c              | 21 +++++++++++++--------
 tools/include/uapi/linux/bpf.h |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 16e95398c91c..ed2a8d223c86 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6069,6 +6069,7 @@ enum {
 	BPF_F_PSEUDO_HDR		= (1ULL << 4),
 	BPF_F_MARK_MANGLED_0		= (1ULL << 5),
 	BPF_F_MARK_ENFORCE		= (1ULL << 6),
+	BPF_F_IPV6_ADDR			= (1ULL << 7),
 };
 
 /* BPF_FUNC_skb_set_tunnel_key and BPF_FUNC_skb_get_tunnel_key flags. */
diff --git a/net/core/filter.c b/net/core/filter.c
index 30e7d3679088..777c9e467d07 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1965,13 +1965,15 @@ static const struct bpf_func_proto bpf_l3_csum_replace_proto = {
 BPF_CALL_5(bpf_l4_csum_replace, struct sk_buff *, skb, u32, offset,
 	   u64, from, u64, to, u64, flags)
 {
-	bool is_pseudo = flags & BPF_F_PSEUDO_HDR;
-	bool is_mmzero = flags & BPF_F_MARK_MANGLED_0;
-	bool do_mforce = flags & BPF_F_MARK_ENFORCE;
+	bool is_pseudo    = flags & BPF_F_PSEUDO_HDR;
+	bool is_mmzero    = flags & BPF_F_MARK_MANGLED_0;
+	bool do_mforce    = flags & BPF_F_MARK_ENFORCE;
+	bool is_ipv6_addr = flags & BPF_F_IPV6_ADDR;
 	__sum16 *ptr;
 
 	if (unlikely(flags & ~(BPF_F_MARK_MANGLED_0 | BPF_F_MARK_ENFORCE |
-			       BPF_F_PSEUDO_HDR | BPF_F_HDR_FIELD_MASK)))
+			       BPF_F_PSEUDO_HDR | BPF_F_HDR_FIELD_MASK |
+			       BPF_F_IPV6_ADDR)))
 		return -EINVAL;
 	if (unlikely(offset > 0xffff || offset & 1))
 		return -EFAULT;
@@ -1984,10 +1986,13 @@ BPF_CALL_5(bpf_l4_csum_replace, struct sk_buff *, skb, u32, offset,
 
 	switch (flags & BPF_F_HDR_FIELD_MASK) {
 	case 0:
-		if (unlikely(from != 0))
-			return -EINVAL;
-
-		inet_proto_csum_replace_by_diff(ptr, skb, to, is_pseudo);
+		if (is_ipv6_addr) {
+			inet_proto_csum_replace16(ptr, skb, (void *)from, (void *)to, is_pseudo);
+		} else {
+			if (unlikely(from != 0))
+				return -EINVAL;
+			inet_proto_csum_replace_by_diff(ptr, skb, to, is_pseudo);
+		}
 		break;
 	case 2:
 		inet_proto_csum_replace2(ptr, skb, from, to, is_pseudo);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 16e95398c91c..ed2a8d223c86 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6069,6 +6069,7 @@ enum {
 	BPF_F_PSEUDO_HDR		= (1ULL << 4),
 	BPF_F_MARK_MANGLED_0		= (1ULL << 5),
 	BPF_F_MARK_ENFORCE		= (1ULL << 6),
+	BPF_F_IPV6_ADDR			= (1ULL << 7),
 };
 
 /* BPF_FUNC_skb_set_tunnel_key and BPF_FUNC_skb_get_tunnel_key flags. */
-- 
2.43.0

Re: [RFC PATCH bpf-next] bpf: Support L4 csum update for IPv6 address changes

[Alexei Starovoitov]

On Tue, May 20, 2025 at 3:06 PM Paul Chaignon <paul.chaignon@gmail.com> wrote:
>
> In Cilium, we use bpf_csum_diff + bpf_l4_csum_replace to, among other
> things, update the L4 checksum after reverse SNATing IPv6 packets. That
> use case is however not currently supported and leads to invalid
> skb->csum values in some cases. This patch adds support for IPv6 address
> changes in bpf_l4_csum_update via a new flag.
>
> When calling bpf_l4_csum_replace in Cilium, it ends up calling
> inet_proto_csum_replace_by_diff:
>
>     1:  void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
>     2:                                       __wsum diff, bool pseudohdr)
>     3:  {
>     4:      if (skb->ip_summed != CHECKSUM_PARTIAL) {
>     5:          csum_replace_by_diff(sum, diff);
>     6:          if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
>     7:              skb->csum = ~csum_sub(diff, skb->csum);
>     8:      } else if (pseudohdr) {
>     9:          *sum = ~csum_fold(csum_add(diff, csum_unfold(*sum)));
>     10:     }
>     11: }
>
> The bug happens when we're in the CHECKSUM_COMPLETE state. We've just
> updated one of the IPv6 addresses. The helper now updates the L4 header
> checksum on line 5. Next, it updates skb->csum on line 7. It shouldn't.
>
> For an IPv6 packet, the updates of the IPv6 address and of the L4
> checksum will cancel each other. The checksums are set such that
> computing a checksum over the packet including its checksum will result
> in a sum of 0. So the same is true here when we update the L4 checksum
> on line 5. We'll update it as to cancel the previous IPv6 address
> update. Hence skb->csum should remain untouched in this case.

Is ILA broken then?
net/ipv6/ila/ila_common.c is using
inet_proto_csum_replace_by_diff()

or is it simply doing it differently?

Re: [RFC PATCH bpf-next] bpf: Support L4 csum update for IPv6 address changes

[kernel test robot]

Hi Paul,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Chaignon/bpf-Support-L4-csum-update-for-IPv6-address-changes/20250521-060705
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/aCz84JU60wd8etiT%40mail.gmail.com
patch subject: [RFC PATCH bpf-next] bpf: Support L4 csum update for IPv6 address changes
config: arc-randconfig-001-20250521 (https://download.01.org/0day-ci/archive/20250521/202505212155.GD0kA7Jj-lkp@intel.com/config)
compiler: arc-linux-gcc (GCC) 10.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250521/202505212155.GD0kA7Jj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505212155.GD0kA7Jj-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/core/filter.c: In function '____bpf_l4_csum_replace':
>> net/core/filter.c:1990:40: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1990 |    inet_proto_csum_replace16(ptr, skb, (void *)from, (void *)to, is_pseudo);
         |                                        ^
   net/core/filter.c:1990:54: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1990 |    inet_proto_csum_replace16(ptr, skb, (void *)from, (void *)to, is_pseudo);
         |                                                      ^


vim +1990 net/core/filter.c

  1964	
  1965	BPF_CALL_5(bpf_l4_csum_replace, struct sk_buff *, skb, u32, offset,
  1966		   u64, from, u64, to, u64, flags)
  1967	{
  1968		bool is_pseudo    = flags & BPF_F_PSEUDO_HDR;
  1969		bool is_mmzero    = flags & BPF_F_MARK_MANGLED_0;
  1970		bool do_mforce    = flags & BPF_F_MARK_ENFORCE;
  1971		bool is_ipv6_addr = flags & BPF_F_IPV6_ADDR;
  1972		__sum16 *ptr;
  1973	
  1974		if (unlikely(flags & ~(BPF_F_MARK_MANGLED_0 | BPF_F_MARK_ENFORCE |
  1975				       BPF_F_PSEUDO_HDR | BPF_F_HDR_FIELD_MASK |
  1976				       BPF_F_IPV6_ADDR)))
  1977			return -EINVAL;
  1978		if (unlikely(offset > 0xffff || offset & 1))
  1979			return -EFAULT;
  1980		if (unlikely(bpf_try_make_writable(skb, offset + sizeof(*ptr))))
  1981			return -EFAULT;
  1982	
  1983		ptr = (__sum16 *)(skb->data + offset);
  1984		if (is_mmzero && !do_mforce && !*ptr)
  1985			return 0;
  1986	
  1987		switch (flags & BPF_F_HDR_FIELD_MASK) {
  1988		case 0:
  1989			if (is_ipv6_addr) {
> 1990				inet_proto_csum_replace16(ptr, skb, (void *)from, (void *)to, is_pseudo);
  1991			} else {
  1992				if (unlikely(from != 0))
  1993					return -EINVAL;
  1994				inet_proto_csum_replace_by_diff(ptr, skb, to, is_pseudo);
  1995			}
  1996			break;
  1997		case 2:
  1998			inet_proto_csum_replace2(ptr, skb, from, to, is_pseudo);
  1999			break;
  2000		case 4:
  2001			inet_proto_csum_replace4(ptr, skb, from, to, is_pseudo);
  2002			break;
  2003		default:
  2004			return -EINVAL;
  2005		}
  2006	
  2007		if (is_mmzero && !*ptr)
  2008			*ptr = CSUM_MANGLED_0;
  2009		return 0;
  2010	}
  2011	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH bpf-next] bpf: Support L4 csum update for IPv6 address changes

[Paul Chaignon]

On Tue, May 20, 2025 at 06:07:15PM -0700, Alexei Starovoitov wrote:
> On Tue, May 20, 2025 at 3:06 PM Paul Chaignon <paul.chaignon@gmail.com> wrote:
> >
> > In Cilium, we use bpf_csum_diff + bpf_l4_csum_replace to, among other
> > things, update the L4 checksum after reverse SNATing IPv6 packets. That
> > use case is however not currently supported and leads to invalid
> > skb->csum values in some cases. This patch adds support for IPv6 address
> > changes in bpf_l4_csum_update via a new flag.
> >
> > When calling bpf_l4_csum_replace in Cilium, it ends up calling
> > inet_proto_csum_replace_by_diff:
> >
> >     1:  void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
> >     2:                                       __wsum diff, bool pseudohdr)
> >     3:  {
> >     4:      if (skb->ip_summed != CHECKSUM_PARTIAL) {
> >     5:          csum_replace_by_diff(sum, diff);
> >     6:          if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
> >     7:              skb->csum = ~csum_sub(diff, skb->csum);
> >     8:      } else if (pseudohdr) {
> >     9:          *sum = ~csum_fold(csum_add(diff, csum_unfold(*sum)));
> >     10:     }
> >     11: }
> >
> > The bug happens when we're in the CHECKSUM_COMPLETE state. We've just
> > updated one of the IPv6 addresses. The helper now updates the L4 header
> > checksum on line 5. Next, it updates skb->csum on line 7. It shouldn't.
> >
> > For an IPv6 packet, the updates of the IPv6 address and of the L4
> > checksum will cancel each other. The checksums are set such that
> > computing a checksum over the packet including its checksum will result
> > in a sum of 0. So the same is true here when we update the L4 checksum
> > on line 5. We'll update it as to cancel the previous IPv6 address
> > update. Hence skb->csum should remain untouched in this case.
> 
> Is ILA broken then?
> net/ipv6/ila/ila_common.c is using
> inet_proto_csum_replace_by_diff()
> 
> or is it simply doing it differently?

As far as I can tell, yes, it's affected by the same issue. Maybe nobody
noticed because 1) it only happens for CHECKSUM_COMPLETE and 2) only the
adj-transport checksum mode for ILA is affected. That mode doesn't look
covered in selftests and isn't the one recommended to users.

With ILA also affected, maybe passing the IPv6 flag to
inet_proto_csum_replace_by_diff is a better solution. That way we could
keep using csum_diff + l4_csum_replace.

Re: [RFC PATCH bpf-next] bpf: Support L4 csum update for IPv6 address changes

[Paul Chaignon]

On Wed, May 21, 2025 at 11:19:41PM +0200, Paul Chaignon wrote:
> On Tue, May 20, 2025 at 06:07:15PM -0700, Alexei Starovoitov wrote:
> > On Tue, May 20, 2025 at 3:06 PM Paul Chaignon <paul.chaignon@gmail.com> wrote:
> > >
> > > In Cilium, we use bpf_csum_diff + bpf_l4_csum_replace to, among other
> > > things, update the L4 checksum after reverse SNATing IPv6 packets. That
> > > use case is however not currently supported and leads to invalid
> > > skb->csum values in some cases. This patch adds support for IPv6 address
> > > changes in bpf_l4_csum_update via a new flag.
> > >
> > > When calling bpf_l4_csum_replace in Cilium, it ends up calling
> > > inet_proto_csum_replace_by_diff:
> > >
> > >     1:  void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
> > >     2:                                       __wsum diff, bool pseudohdr)
> > >     3:  {
> > >     4:      if (skb->ip_summed != CHECKSUM_PARTIAL) {
> > >     5:          csum_replace_by_diff(sum, diff);
> > >     6:          if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
> > >     7:              skb->csum = ~csum_sub(diff, skb->csum);
> > >     8:      } else if (pseudohdr) {
> > >     9:          *sum = ~csum_fold(csum_add(diff, csum_unfold(*sum)));
> > >     10:     }
> > >     11: }
> > >
> > > The bug happens when we're in the CHECKSUM_COMPLETE state. We've just
> > > updated one of the IPv6 addresses. The helper now updates the L4 header
> > > checksum on line 5. Next, it updates skb->csum on line 7. It shouldn't.
> > >
> > > For an IPv6 packet, the updates of the IPv6 address and of the L4
> > > checksum will cancel each other. The checksums are set such that
> > > computing a checksum over the packet including its checksum will result
> > > in a sum of 0. So the same is true here when we update the L4 checksum
> > > on line 5. We'll update it as to cancel the previous IPv6 address
> > > update. Hence skb->csum should remain untouched in this case.
> > 
> > Is ILA broken then?
> > net/ipv6/ila/ila_common.c is using
> > inet_proto_csum_replace_by_diff()
> > 
> > or is it simply doing it differently?
> 
> As far as I can tell, yes, it's affected by the same issue. Maybe nobody
> noticed because 1) it only happens for CHECKSUM_COMPLETE and 2) only the
> adj-transport checksum mode for ILA is affected. That mode doesn't look
> covered in selftests and isn't the one recommended to users.
> 
> With ILA also affected, maybe passing the IPv6 flag to
> inet_proto_csum_replace_by_diff is a better solution. That way we could
> keep using csum_diff + l4_csum_replace.

I was able to reproduce the bug locally on an ILA setup with
adj-transport checksum mode. The tricky part was ensuring
inet_proto_csum_replace_by_diff is called while we're in
CHECKSUM_COMPLETE state, but I imagine that's easier with a proper NIC
doing checksum offload on a server.

Configuration on the target:

$ ip a show dev eth0
14: eth0@if15: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 62:ae:35:9e:0f:8d brd ff:ff:ff:ff:ff:ff link-netnsid 0
    inet6 3333:0:0:1::e952/64 scope global 
       valid_lft forever preferred_lft forever
    inet6 fd00:10:244:2::e952/128 scope global nodad 
       valid_lft forever preferred_lft forever
    inet6 fe80::60ae:35ff:fe9e:f8d/64 scope link proto kernel_ll 
       valid_lft forever preferred_lft forever
$ ip ila add loc_match fd00:10:244:2 loc 3333:0:0:1 csum-mode adj-transport ident-type luid dev eth0

Then I hit [fd00:10:244:2::e952]:8000 with a server listening only on
[3333:0:0:1::e952]:8000. With the bug, the SYN packet is dropped with
SKB_DROP_REASON_TCP_CSUM after inet_proto_csum_replace_by_diff changed
skb->csum. pwru trace:

  IFACE   TUPLE                                                        FUNC
  eth0:9  [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp)  ipv6_rcv
  eth0:9  [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp)  ip6_rcv_core
  eth0:9  [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp)  nf_hook_slow
  eth0:9  [fd00:10:244:3::3d8]:51420->[fd00:10:244:1::c078]:8000(tcp)  inet_proto_csum_replace_by_diff
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     tcp_v6_early_demux
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     ip6_route_input
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     ip6_input
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     ip6_input_finish
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     ip6_protocol_deliver_rcu
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     raw6_local_deliver
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     ipv6_raw_deliver
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     tcp_v6_rcv
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     __skb_checksum_complete
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     kfree_skb_reason(SKB_DROP_REASON_TCP_CSUM)
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     skb_release_head_state
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     skb_release_data
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     skb_free_head
  eth0:9  [fd00:10:244:3::3d8]:51420->[3333:0:0:1::c078]:8000(tcp)     kfree_skbmem

With the fix, I can see inet_proto_csum_replace_by_diff is not touching
skb->csum and I get a SYN+ACK (after which it fails since I didn't
configure the whole ILA setup).

I'll send a patchset to fix both the ILA and BPF sides. Not sure which
tree I should send it to though?