[PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[Changli Gao]

after updating the value of the ICMP payload, inet_proto_csum_replace4() should
be called with zero pseudohdr.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 net/sched/act_nat.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 24e614c..59f05ee 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -246,7 +246,7 @@ static int tcf_nat(struct sk_buff *skb, struct tc_action *a,
 			iph->saddr = new_addr;
 
 		inet_proto_csum_replace4(&icmph->checksum, skb, addr, new_addr,
-					 1);
+					 0);
 		break;
 	}
 	default:

Re: [PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[Herbert Xu]

On Fri, Jul 30, 2010 at 08:04:18AM +0800, Changli Gao wrote:
> after updating the value of the ICMP payload, inet_proto_csum_replace4() should
> be called with zero pseudohdr.
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>

No, the code is correct as is.  We need to update the checksum
even if the checksum is partial, which is what the 1 is for.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[Changli Gao]

On Fri, Jul 30, 2010 at 5:09 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Jul 30, 2010 at 08:04:18AM +0800, Changli Gao wrote:
>> after updating the value of the ICMP payload, inet_proto_csum_replace4() should
>> be called with zero pseudohdr.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>
> No, the code is correct as is.  We need to update the checksum
> even if the checksum is partial, which is what the 1 is for.
>

Is it really necessary, and I have noticed that netfilter doesn't call
inet_proto_csum_replace4 in this way.

static bool
icmp_manip_pkt(struct sk_buff *skb,
               unsigned int iphdroff,
               const struct nf_conntrack_tuple *tuple,
               enum nf_nat_manip_type maniptype)
{
        const struct iphdr *iph = (struct iphdr *)(skb->data + iphdroff);
        struct icmphdr *hdr;
        unsigned int hdroff = iphdroff + iph->ihl*4;

        if (!skb_make_writable(skb, hdroff + sizeof(*hdr)))
                return false;

        hdr = (struct icmphdr *)(skb->data + hdroff);
        inet_proto_csum_replace2(&hdr->checksum, skb,
                                 hdr->un.echo.id, tuple->src.u.icmp.id, 0);
        hdr->un.echo.id = tuple->src.u.icmp.id;
        return true;
}

Thanks.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[Herbert Xu]

On Fri, Jul 30, 2010 at 06:10:19PM +0800, Changli Gao wrote:
>
> Is it really necessary, and I have noticed that netfilter doesn't call
> inet_proto_csum_replace4 in this way.

The checksum update is for the inner IP header.  netfilter does
of course update the checksum, it just doesn't do it here which is
for the outer IP header.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[Changli Gao]

On Fri, Jul 30, 2010 at 6:24 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> The checksum update is for the inner IP header.  netfilter does
> of course update the checksum, it just doesn't do it here which is
> for the outer IP header.
>

I know we need to update the ICMP checksum if we alter the payload(the
inner IP header here) of ICMP. But I doubt if the update is really
necessary if the checksum is partial, as the   checksum will be done
later(by ether skb_checksum_help() or NIC hardware). In fact, as there
isn't any pseudo header, the icmph->checksum should be always ZERO,
otherwise skb_checksum_help() or NIC will give the wrong checksums,
when the checksum is partial.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[Herbert Xu]

On Fri, Jul 30, 2010 at 10:16:05PM +0800, Changli Gao wrote:
> 
> I know we need to update the ICMP checksum if we alter the payload(the
> inner IP header here) of ICMP. But I doubt if the update is really
> necessary if the checksum is partial, as the   checksum will be done
> later(by ether skb_checksum_help() or NIC hardware). In fact, as there
> isn't any pseudo header, the icmph->checksum should be always ZERO,
> otherwise skb_checksum_help() or NIC will give the wrong checksums,
> when the checksum is partial.

Actually you are right.  I suppose the only reason this has never
shown up is because CHEKSUM_PARTIAL doesn't usually occur with
forwarded packets.

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] act_nat: the checksum of ICMP doesn't have pseudo header

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 30 Jul 2010 22:30:16 +0800

> On Fri, Jul 30, 2010 at 10:16:05PM +0800, Changli Gao wrote:
>> 
>> I know we need to update the ICMP checksum if we alter the payload(the
>> inner IP header here) of ICMP. But I doubt if the update is really
>> necessary if the checksum is partial, as the   checksum will be done
>> later(by ether skb_checksum_help() or NIC hardware). In fact, as there
>> isn't any pseudo header, the icmph->checksum should be always ZERO,
>> otherwise skb_checksum_help() or NIC will give the wrong checksums,
>> when the checksum is partial.
> 
> Actually you are right.  I suppose the only reason this has never
> shown up is because CHEKSUM_PARTIAL doesn't usually occur with
> forwarded packets.
> 
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Also applied, thanks.