Re: [PATCH v3] [net]: Fix skb->csum update in inet_proto_csum_replace16().

[Daniel Borkmann]

On 1/22/20 12:43 PM, Florian Westphal wrote:
> Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> > > @@ -449,9 +464,6 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
> > >    	if (skb->ip_summed != CHECKSUM_PARTIAL) {
> > >    		*sum = csum_fold(csum_partial(diff, sizeof(diff),
> > >    				 ~csum_unfold(*sum)));
> > > -		if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
> > > -			skb->csum = ~csum_partial(diff, sizeof(diff),
> > > -						  ~skb->csum);
> >
> > What is the technical rationale in removing this here but not in any of the
> > other inet_proto_csum_replace*() functions? You changelog has zero analysis
> > on why here but not elsewhere this change would be needed?
>
> Right, I think it could be dropped everywhere BUT there is a major caveat:
>
> At least for the nf_nat case ipv4 header manipulation (which uses the other
> helpers froum utils.c) will eventually also update iph->checksum field
> to account for the changed ip addresses.
>
> And that update doesn't touch skb->csum.
>
> So in a way the update of skb->csum in the other helpers indirectly account
> for later ip header checksum update.
>
> At least that was my conclusion when reviewing the earlier incarnation
> of the patch.

Mainly asking because not inet_proto_csum_replace16() but the other ones are
exposed via BPF and they are all in no way fundamentally different to each
other, but my concern is that depending on how the BPF prog updates the csums
things could start to break. :/