Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
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
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
> 

  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-29 16:59 Praveen Chaudhary
2019-10-29 16:59 ` Praveen Chaudhary
2019-11-06  9:33   ` Pablo Neira Ayuso [this message]
2019-11-06 20:48     ` Praveen Chaudhary

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191106093347.inrzhrlrle6g6naf@salvia \
    --to=pablo@netfilter.org \
    --cc=astracner@linkedin.com \
    --cc=davem@davemloft.net \
    --cc=fw@strlen.de \
    --cc=kadlec@netfilter.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=praveen5582@gmail.com \
    --cc=zxu@linkedin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git