| Message ID | 1572368351-3156-2-git-send-email-pchaudhary@linkedin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [v2,netfilter] : Fix skb->csum calculation when netfilter manipulation for NF_NAT_MANIP_SRC\DST is done on IPV6 packet. | expand |
Hi Praveen, On Tue, Oct 29, 2019 at 09:59:11AM -0700, Praveen Chaudhary wrote: > No need to update skb->csum in function inet_proto_csum_replace16(), > even if skb->ip_summed == CHECKSUM_COMPLETE, because change in L4 > header checksum field and change in IPV6 header cancels each other > for skb->csum calculation. Two comestic issues with this patch: * Patch subject is a bit long, could you rewrite it? Probably: net: Fix skb->csum update on inet_proto_csum_replace16() And describe in the patch description that you trigger this from netfilter IPv6 and NF_NAT_MANIP_SRC\DST. * Regarding the comment on top of the function, could you make it fit into the 80-chars per column, it is shrinked to less than 70-chars per column for some reason. Probably you can just fully document this function while including this description, as it happens with other functions in this file (this last sentence is a suggestion, not a dealbreaker). BTW, in your description you refer to < 3.16 though, and I I think the problem manifests since ce25d66ad5f8d92, correct? Thanks. > Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com> > Signed-off-by: Zhenggen Xu <zxu@linkedin.com> > Signed-off-by: Andy Stracner <astracner@linkedin.com> > Reviewed-by: Florian Westphal <fw@strlen.de> > --- > Changes in V2. > 1.) Updating diff as per email discussion with Florian Westphal. > Since inet_proto_csum_replace16() does incorrect calculation > for skb->csum in all cases. > 2.) Change in Commmit logs. > --- > --- > net/core/utils.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/core/utils.c b/net/core/utils.c > index 6b6e51d..cec9924 100644 > --- a/net/core/utils.c > +++ b/net/core/utils.c > @@ -438,6 +438,12 @@ void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb, > } > EXPORT_SYMBOL(inet_proto_csum_replace4); > > +/** > + * No need to update skb->csum in this function, even if > + * skb->ip_summed == CHECKSUM_COMPLETE, because change in > + * L4 header checksum field and change in IPV6 header > + * cancels each other for skb->csum calculation. > + */ > void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb, > const __be32 *from, const __be32 *to, > bool pseudohdr) > @@ -449,9 +455,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); > } else if (pseudohdr) > *sum = ~csum_fold(csum_partial(diff, sizeof(diff), > csum_unfold(*sum))); > -- > 2.7.4 >
Thanks Pablo for review, Sure, I will update a.) patch subject, b.) description and c.) the full document of function. I feel, the bug was exposed with e6afc8ace6dd5cef5e812f26c72579da8806f5ac i.e with introduction of udp_csum_pull_header function.
diff --git a/net/core/utils.c b/net/core/utils.c index 6b6e51d..cec9924 100644 --- a/net/core/utils.c +++ b/net/core/utils.c @@ -438,6 +438,12 @@ void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb, } EXPORT_SYMBOL(inet_proto_csum_replace4); +/** + * No need to update skb->csum in this function, even if + * skb->ip_summed == CHECKSUM_COMPLETE, because change in + * L4 header checksum field and change in IPV6 header + * cancels each other for skb->csum calculation. + */ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb, const __be32 *from, const __be32 *to, bool pseudohdr) @@ -449,9 +455,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); } else if (pseudohdr) *sum = ~csum_fold(csum_partial(diff, sizeof(diff), csum_unfold(*sum)));