Update skb->csum, when netfilter code updates IPV6 SRC\DST address in IPV6 HEADER due to iptable rule.
Signed-off-by: Praveen Chaudhary <[email protected]>
Signed-off-by: Zhenggen Xu <[email protected]>
Signed-off-by: Andy Stracner <[email protected]>
---
include/net/checksum.h | 2 ++
net/core/utils.c | 13 +++++++++++++
net/netfilter/nf_nat_proto.c | 2 ++
3 files changed, 17 insertions(+)
diff --git a/include/net/checksum.h b/include/net/checksum.h
index 97bf488..d7d28b7 100644
--- a/include/net/checksum.h
+++ b/include/net/checksum.h
@@ -145,6 +145,8 @@ void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
const __be32 *from, const __be32 *to,
bool pseudohdr);
+void inet_proto_skb_csum_replace16(struct sk_buff *skb,
+ const __be32 *from, const __be32 *to);
void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
__wsum diff, bool pseudohdr);
diff --git a/net/core/utils.c b/net/core/utils.c
index 6b6e51d..ab3284b 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -458,6 +458,19 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
}
EXPORT_SYMBOL(inet_proto_csum_replace16);
+void inet_proto_skb_csum_replace16(struct sk_buff *skb,
+ const __be32 *from, const __be32 *to)
+{
+ __be32 diff[] = {
+ ~from[0], ~from[1], ~from[2], ~from[3],
+ to[0], to[1], to[2], to[3],
+ };
+ if (skb->ip_summed == CHECKSUM_COMPLETE)
+ skb->csum = csum_partial(diff, sizeof(diff),
+ skb->csum);
+}
+EXPORT_SYMBOL(inet_proto_skb_csum_replace16);
+
void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
__wsum diff, bool pseudohdr)
{
diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
index 0a59c14..de94590 100644
--- a/net/netfilter/nf_nat_proto.c
+++ b/net/netfilter/nf_nat_proto.c
@@ -467,6 +467,8 @@ static void nf_nat_ipv6_csum_update(struct sk_buff *skb,
}
inet_proto_csum_replace16(check, skb, oldip->s6_addr32,
newip->s6_addr32, true);
+ inet_proto_skb_csum_replace16(skb, oldip->s6_addr32,
+ newip->s6_addr32);
#endif
}
--
2.7.4
Praveen Chaudhary <[email protected]> wrote:
> Update skb->csum, when netfilter code updates IPV6 SRC\DST address in IPV6 HEADER due to iptable rule.
>
> Signed-off-by: Praveen Chaudhary <[email protected]>
> Signed-off-by: Zhenggen Xu <[email protected]>
> Signed-off-by: Andy Stracner <[email protected]>
> ---
> include/net/checksum.h | 2 ++
> net/core/utils.c | 13 +++++++++++++
> net/netfilter/nf_nat_proto.c | 2 ++
> 3 files changed, 17 insertions(+)
>
> diff --git a/include/net/checksum.h b/include/net/checksum.h
> index 97bf488..d7d28b7 100644
> --- a/include/net/checksum.h
> +++ b/include/net/checksum.h
> @@ -145,6 +145,8 @@ void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
> void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
> const __be32 *from, const __be32 *to,
> bool pseudohdr);
> +void inet_proto_skb_csum_replace16(struct sk_buff *skb,
> + const __be32 *from, const __be32 *to);
> void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
> __wsum diff, bool pseudohdr);
>
> diff --git a/net/core/utils.c b/net/core/utils.c
> index 6b6e51d..ab3284b 100644
> --- a/net/core/utils.c
> +++ b/net/core/utils.c
> @@ -458,6 +458,19 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
> }
> EXPORT_SYMBOL(inet_proto_csum_replace16);
>
> +void inet_proto_skb_csum_replace16(struct sk_buff *skb,
> + const __be32 *from, const __be32 *to)
> +{
> + __be32 diff[] = {
> + ~from[0], ~from[1], ~from[2], ~from[3],
> + to[0], to[1], to[2], to[3],
> + };
> + if (skb->ip_summed == CHECKSUM_COMPLETE)
> + skb->csum = csum_partial(diff, sizeof(diff),
> + skb->csum);
> +}
> +EXPORT_SYMBOL(inet_proto_skb_csum_replace16);
> +
> void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
> __wsum diff, bool pseudohdr)
> {
> diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
> index 0a59c14..de94590 100644
> --- a/net/netfilter/nf_nat_proto.c
> +++ b/net/netfilter/nf_nat_proto.c
> @@ -467,6 +467,8 @@ static void nf_nat_ipv6_csum_update(struct sk_buff *skb,
> }
> inet_proto_csum_replace16(check, skb, oldip->s6_addr32,
> newip->s6_addr32, true);
> + inet_proto_skb_csum_replace16(skb, oldip->s6_addr32,
> + newip->s6_addr32);
This is confusing.
You're saying that inet_proto_csum_replace16() is producing a wrong
skb->csum. So why are you adding a new function to do the
csum calculation instead of fixing inet_proto_csum_replace16() to do
the right thing?
praveen chaudhary <[email protected]> wrote:
> inet_proto_csum_replace16 is called from many places, whereas this fix is
> applicable only for nf_nat_ipv6_csum_update. where we need to update
> skb->csum for ipv6 src/dst address change.
Under which circumstances does inet_proto_csum_replace16 upate
skb->csum correctly?
> So, I added a new function. Basically, I used a safe approach to fix it,
> without impacting other cases. Let me know other options, I am open to
> suggestions.
You seem to imply inet_proto_csum_replace16 is fine and only broken for ipv6
nat.
Hi Florian
Thanks for giving time for review. This fix is pretty important for SONiC OS (https://azure.github.io/SONiC/). So I really appreciate it.
>> inet_proto_csum_replace16 is called from many places, whereas this fix is
>> applicable only for nf_nat_ipv6_csum_update. where we need to update
>> skb->csum for ipv6 src/dst address change.
>
>Under which circumstances does inet_proto_csum_replace16 upate
>skb->csum correctly?
inet_proto_csum_replace16 calculates skb->csum correctly if skb->csum does not include 16-bit sum of IPv6 Header i.e skb->data points to UDP\TCP\ICMPv6 header while calling __skb_checksum_complete() on packet. Function inet_proto_csum_replace16 is called from nf_nat_ipv6_csum_update(), nf_flow_nat_ipv6_tcp(), nf_flow_nat_ipv6_udp() and update_ipv6_checksum(). For all nf_*() functions, inet_proto_csum_replace16() will not update skb->csum correctly\completely.
But I am not sure about update_ipv6_checksum() (in net/openvswitch/actions.c). This is where I seek help from experts. If even for update_ipv6_checksum(), skb->csum includes 16-bit sum of IPv6 Header then inet_proto_csum_replace16() does updates skb->csum correctly. Then our fix will be to remove below line from this function. Because change in UDP\TCP\ICMPv6 header checksum field and change in IPv6 SRC\DST address cancels each other for checksum calculation, i.e. no update to skb->csum is needed.
```
if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
skb->csum = ~csum_partial(diff, sizeof(diff),
~skb->csum);
```
>
>> So, I added a new function. Basically, I used a safe approach to fix it,
>> without impacting other cases. Let me know other options, I am open to
>> suggestions.
>
>You seem to imply inet_proto_csum_replace16 is fine and only broken for ipv6
>nat.
Yeah, as mentioned above, I took safe approach to fix only nf_nat_ipv6_csum_update() part, where I am sure that skb->csum is broken. But I am not sure if (net/openvswitch/actions.c) needs this fix or not. Consider this my lack of expertise, So kindly suggest whether net/openvswitch/actions.c needs this fix or not. Note: I may not be able to test this part. After your suggestion, I will change my patch. Again Thanks for your time.