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