Re: [PATCH v2] [netfilter]: Fix skb->csum calculation when netfilter manipulation for NF_NAT_MANIP_SRC\DST is done on IPV6 packet.

From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Praveen Chaudhary <praveen5582@gmail.com>
Cc: fw@strlen.de, davem@davemloft.net, kadlec@netfilter.org,
	netfilter-devel@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, Zhenggen Xu <zxu@linkedin.com>,
	Andy Stracner <astracner@linkedin.com>
Subject: Re: [PATCH v2] [netfilter]: Fix skb->csum calculation when netfilter manipulation for NF_NAT_MANIP_SRC\DST is done on IPV6 packet.
Date: Wed, 6 Nov 2019 10:33:47 +0100	[thread overview]
Message-ID: <20191106093347.inrzhrlrle6g6naf@salvia> (raw)
In-Reply-To: <1572368351-3156-2-git-send-email-pchaudhary@linkedin.com>

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
> 