| Message ID | 1280448258-1893-1-git-send-email-xiaosuo@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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,
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.
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,
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.
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html