Messages in this thread | | | Subject | Re: [PATCH v3] [net]: Fix skb->csum update in inet_proto_csum_replace16(). | From | Daniel Borkmann <> | Date | Thu, 23 Jan 2020 15:20:39 +0100 |
| |
On 1/23/20 9:21 AM, Florian Westphal wrote: > Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 1/22/20 12:43 PM, Florian Westphal wrote: >>> Daniel Borkmann <daniel@iogearbox.net> 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. :/ > > 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
| |