act_nat: the checksum of ICMP doesn't have pseudo header
Subsystems (3)
PATCH 1/1 act_nat: the checksum of ICMP doesn't have pseudo header
after updating the value of the ICMP payload, inet_proto_csum_replace4() should be called with zero pseudohdr.
----
net/sched/act_nat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/sched/act_nat.c +1 -1
@@ -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 :
Quoted 4 lines · 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 <[email protected]>
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,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 9 lines · Herbert Xu
Quoted 4 lines · 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 <[email protected]>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.
Signature · 2 lines
Regards, Changli Gao([email protected])
Quoted 3 lines · Changli Gao
Is it really necessary, and I have noticed that netfilter doesn't call
inet_proto_csum_replace4in 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,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 5 lines · Herbert Xu
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 donelater(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.
Signature · 2 lines
Regards, Changli Gao([email protected])
Herbert Xu
Acked-byQuoted 8 lines · Changli Gao
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 donelater(by ether skb_checksum_help() or NIC hardware). In fact, as there isn't any pseudo header, theicmph->checksumshould be always ZERO, otherwiseskb_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.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
David Miller
maintainerFrom: Herbert Xu <[email protected]>
Date: Fri, 30 Jul 2010 22:30:16 +0800
Quoted 10 lines · Changli Gao
Quoted 8 lines
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 donelater(by ether skb_checksum_help() or NIC hardware). In fact, as there isn't any pseudo header, theicmph->checksumshould be always ZERO, otherwiseskb_checksum_help()or NIC will give the wrong checksums, when the checksum is partial.
Quoted 5 lines
Actually you are right. I suppose the only reason this has never shown up is because
CHEKSUM_PARTIALdoesn't usually occur with forwarded packets.Acked-by Herbert Xu <[email protected]>
Also applied, thanks.