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

On 1/23/20 9:21 AM, Florian Westphal wrote:

Daniel Borkmann [off-list ref] wrote:

quoted

On 1/22/20 12:43 PM, Florian Westphal wrote:

quoted

Daniel Borkmann [off-list ref] wrote:

quoted

quoted

@@ -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. :/

I'm reasonably sure removing the skb->csum update from the other
helpers will also break ipv4 nat :)

So, AFAIU from what you're saying above the patch seems fine as-is and
just needs a more verbose commit message explaining why replace16()
doesn't update skb->csum while all the other ones do.

Is that correct?

Probably better a comment in the code to avoid confusion on why it's not done in
inet_proto_csum_replace16() but all the other cases; mainly to avoid some folks
in future sending random cleanup patches w/ removal attempts.

Thanks,
Daniel