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.
Signed-off-by: Praveen Chaudhary <[email protected]>
Signed-off-by: Zhenggen Xu <[email protected]>
Signed-off-by: Andy Stracner <[email protected]>
Reviewed-by: Florian Westphal <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Zhenggen Xu <[email protected]>
> Signed-off-by: Andy Stracner <[email protected]>
> Reviewed-by: Florian Westphal <[email protected]>
> ---
> 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.