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

[Paul Chaignon <paul.chaignon@gmail.com>]

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.