Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Praveen Chaudhary <praveen5582@gmail.com>
To:     fw@strlen.de
Cc:     astracner@linkedin.com, davem@davemloft.net, kadlec@netfilter.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        pablo@netfilter.org, praveen5582@gmail.com, zxu@linkedin.com
Subject: RE: [netfilter]: Fix skb->csum calculation when netfilter
Date:   Wed, 23 Oct 2019 22:19:33 -0700
Message-Id: <1571894373-11188-1-git-send-email-pchaudhary@linkedin.com>
In-Reply-To: <20191024011218.GT25052@breakpoint.cc>
References: <20191024011218.GT25052@breakpoint.cc>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.