ICMP: ICMP_MIB_OUTMSGS increment duplicated

PATCH In review Wang Chen 5 participants 19 messages Jan 16, 2008 – Jan 21, 2008

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Wang Chen

author

2008-01-16 10:00 UTC #0 lore raw

PATCH 1/1 ICMP: ICMP_MIB_OUTMSGS increment duplicated

In tree net-2.6.25, commit "96793b482540f3a26e2188eaf75cb56b7829d3e3" made a mistake.

In that patch, David L added a icmp_out_count() in ip_push_pending_frames(), remove icmp_out_count() from icmp_reply(). But he forgot to remove icmp_out_count() from icmp_send() too.
Since icmp_send and icmp_reply will call icmp_push_reply, which will call ip_push_pending_frames, a duplicated increment happened in icmp_send.

This patch remove the icmp_out_count from icmp_send too.

Signed-off-by Wang Chen <[email protected]>

---

net/ipv4/icmp.c +0 -1

diff -Nurp linux-2.6.24.rc8.org/net/ipv4/icmp.c linux-2.6.24.rc8/net/ipv4/icmp.c
@@ -540,7 +540,6 @@ void icmp_send(struct sk_buff *skb_in, i
 	icmp_param.data.icmph.checksum	 = 0;
 	icmp_param.skb	  = skb_in;
 	icmp_param.offset = skb_network_offset(skb_in);
-	icmp_out_count(icmp_param.data.icmph.type);
 	inet_sk(icmp_socket->sk)->tos = tos;
 	ipc.addr = iph->saddr;
 	ipc.opt = &icmp_param.replyopts;

--
WCN

Herbert Xu

2008-01-16 11:49 UTC #1 lore raw

Quoted 8 lines · Wang Chen

In tree net-2.6.25, commit "96793b482540f3a26e2188eaf75cb56b7829d3e3" made a mistake.
In that patch, David L added a icmp_out_count() in ip_push_pending_frames(), remove icmp_out_count() from icmp_reply(). But he forgot to remove icmp_out_count() from icmp_send() too.
Since icmp_send and icmp_reply will call icmp_push_reply, which will call ip_push_pending_frames, a duplicated increment happened in icmp_send.

Actually having the icmp_out_count call in ip_push_pending_frames seems inconsistent. Having it there means that we count raw socket ICMP packets too. But we don't do that for any other protocol, e.g., raw UDP packets don't get counted.

So was the inclusion of raw ICMP packets intentional?

Cheers,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Stevens

2008-01-16 16:23 UTC #2 lore raw

Herbert Xu <[email protected]> wrote on 01/16/2008 03:49:01 AM:

Quoted 2 lines

Actually having the icmp_out_count call in ip_push_pending_frames seems inconsistent. Having it there means that we count raw socket ICMP

packets

Quoted 2 lines

too. But we don't do that for any other protocol, e.g., raw UDP packets don't get counted.

Herbert,
The patch was to support the ICMPMsgStats table. Since none of
certain
types of output ICMP messages are generated by the kernel, but are required
by the RFC, counting raw sockets is intentional (and the only way those ICMP
types can be counted at all).
Raw UDP packets would not be counted either before or after the
patch,
but aren't part of the ICMPMsgStats table. Adding those might be worthwhile,
but it isn't quite the hole that the ICMP out stats were, since there is a cooked interface for UDP output that counts the common use, at least.

Wang,
I think your patch is correct; did you test the same case for
IPv6?

+-DLS

Herbert Xu

2008-01-16 23:17 UTC #3 lore raw

Quoted 14 lines · David Stevens

The patch was to support the ICMPMsgStats table. Since none of
certain
types of output ICMP messages are generated by the kernel, but are required
by the RFC, counting raw sockets is intentional (and the only way those ICMP
types can be counted at all).
Raw UDP packets would not be counted either before or after the
patch,
but aren't part of the ICMPMsgStats table. Adding those might be worthwhile,
but it isn't quite the hole that the ICMP out stats were, since there is a cooked interface for UDP output that counts the common use, at least.

Fair enough. How about moving this code back into icmp.c and just add a new count call in raw.c? The push pending function is used on the UDP fast path so the leaner it is the better.

Thanks,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Stevens

2008-01-17 00:10 UTC #4 lore raw

[email protected] wrote on 01/16/2008 03:17:29 PM:

Quoted 3 lines

Fair enough. How about moving this code back into icmp.c and just add a new count call in raw.c? The push pending function is used on the UDP fast path so the leaner it is the better.

I started out with it there, but it certainly wasn't
cleaner. You have to peek on the queue, which I didn't want to do for all the SMP issues that brings in, and then replicate all the code push_pending_frames does to get you a protocol header pointer in one buffer, and there are multiple ways in the raw path to get you there (so the counter code itself would appear in multiple places).
I don't think the 2 instructions measurably impact
anything in the fast path and that is the earliest common point for the multiple paths to generate ICMP output where the header and type are available without replicating code. So, simplicity is exactly why I put it there, rather than the "obvious" places at the higher layer. :-) It's probably the better, single place to put the other protocol "out messages" counters, too, since those probably also have multiple paths that end up here, but ICMPMsgStats was all that patch was after.

                                                +-DLS

Wang Chen

author

2008-01-17 00:57 UTC #5 lore raw

In reply to David Stevens

David Stevens said the following on 2008-1-17 0:23:

Quoted 4 lines

Wang,
I think your patch is correct; did you test the same case for
IPv6?

I've tested IPv4, but IPv6 not yet.
If Davem accept this one, I will see the IPv6, or you take care of it.

--
WCN

Wang Chen

author

2008-01-21 09:48 UTC #6 lore raw

PATCH 1/2 IPV6: ICMP6_MIB_OUTMSGS increment duplicated

In reply to Wang Chen

[IPV6]: ICMP6_MIB_OUTMSGS increment duplicated

icmpv6_send() calls ip6_push_pending_frames() indirectly. Both ip6_push_pending_frames() and icmpv6_send() increment counter ICMP6_MIB_OUTMSGS.

This patch remove the increment from icmpv6_send.

Signed-off-by Wang Chen <[email protected]>

net/ipv6/icmp.c +0 -2

@@ -458,8 +458,6 @@ void icmpv6_send(struct sk_buff *skb, in
 	}
 	err = icmpv6_push_pending_frames(sk, &fl, &tmp_hdr, len + sizeof(struct icmp6hdr));
 
-	ICMP6_INC_STATS_BH(idev, ICMP6_MIB_OUTMSGS);
-
 out_put:
 	if (likely(idev != NULL))
 		in6_dev_put(idev);

YOSHIFUJI Hideaki / 吉藤英明

Acked-by on v1 1/2

2008-01-21 09:58 UTC #7 lore raw

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

In article <[email protected]> (at Mon, 21 Jan 2008 17:46:32 +0800), Wang Chen <[email protected]> says:

Quoted 9 lines

[IPV6]: ICMP6_MIB_OUTMSGS increment duplicated
icmpv6_send() calls ip6_push_pending_frames() indirectly. Both ip6_push_pending_frames() and icmpv6_send() increment counter ICMP6_MIB_OUTMSGS.
This patch remove the increment from icmpv6_send.
Signed-off-by Wang Chen <[email protected]>

Acked-by YOSHIFUJI Hideaki <[email protected]>

--yoshfuji

David Miller

maintainer

2008-01-21 10:57 UTC #8 lore raw

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

In reply to Wang Chen

From: Wang Chen <[email protected]>
Date: Mon, 21 Jan 2008 17:46:32 +0800

Quoted 9 lines

[IPV6]: ICMP6_MIB_OUTMSGS increment duplicated
icmpv6_send() calls ip6_push_pending_frames() indirectly. Both ip6_push_pending_frames() and icmpv6_send() increment counter ICMP6_MIB_OUTMSGS.
This patch remove the increment from icmpv6_send.
Signed-off-by Wang Chen <[email protected]>

Which tree are these two changes targetted at?

Wang Chen

author

2008-01-21 11:01 UTC #9 lore raw

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

David Miller said the following on 2008-1-21 18:57:

Quoted 6 lines

Quoted 6 lines
[IPV6]: ICMP6_MIB_OUTMSGS increment duplicated
icmpv6_send() calls ip6_push_pending_frames() indirectly. Both ip6_push_pending_frames() and icmpv6_send() increment counter ICMP6_MIB_OUTMSGS.

Quoted 7 lines

Quoted 3 lines
This patch remove the increment from icmpv6_send.
Signed-off-by Wang Chen <[email protected]>
Which tree are these two changes targetted at?

Dave, because the network of my working place can't clone git tree, I used 2.6.24-rc8 to make the patches.
Can you apply them to net-2.6 tree?

David Miller

maintainer

2008-01-21 11:06 UTC #10 lore raw

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

From: Wang Chen <[email protected]>
Date: Mon, 21 Jan 2008 18:59:43 +0800

Quoted 2 lines

Dave, because the network of my working place can't clone git tree, I used 2.6.24-rc8 to make the patches.

Does someone know how to setup GIT to update the HTTP fetch information as a post-push hook?

Quoted 1 line

Can you apply them to net-2.6 tree?

Done.

Wang Chen

author

2008-01-21 11:13 UTC #11 lore raw

Re: [PATCH 1/2] IPV6: ICMP6_MIB_OUTMSGS increment duplicated

David Miller said the following on 2008-1-21 19:06:

Quoted 3 lines

Does someone know how to setup GIT to update the HTTP fetch information as a post-push hook?

Seems Herbert knows how to do it.
Maybe run git-update-server-info every time when you rebase your tree.

Quoted 4 lines

Quoted 1 line
Can you apply them to net-2.6 tree?
Done.

Thanks.

--
WCN

Wang Chen

author

2008-01-21 09:48 UTC #12 lore raw

PATCH 2/2 IPV6: RFC 2011 compatibility broken

In reply to Wang Chen

[IPV6]: RFC 2011 compatibility broken

The snmp6 entry name was changed, and it broke compatibility to RFC 2011.

Signed-off-by Wang Chen <[email protected]>

net/ipv6/proc.c +2 -2

@@ -88,7 +88,7 @@ static char *icmp6type2name[256] = {
 	[ICMPV6_PKT_TOOBIG] = "PktTooBigs",
 	[ICMPV6_TIME_EXCEED] = "TimeExcds",
 	[ICMPV6_PARAMPROB] = "ParmProblems",
-	[ICMPV6_ECHO_REQUEST] = "EchoRequest",
+	[ICMPV6_ECHO_REQUEST] = "Echos",
 	[ICMPV6_ECHO_REPLY] = "EchoReplies",
 	[ICMPV6_MGM_QUERY] = "GroupMembQueries",
 	[ICMPV6_MGM_REPORT] = "GroupMembResponses",

@@ -98,7 +98,7 @@ static char *icmp6type2name[256] = {
 	[NDISC_ROUTER_SOLICITATION] = "RouterSolicits",
 	[NDISC_NEIGHBOUR_ADVERTISEMENT] = "NeighborAdvertisements",
 	[NDISC_NEIGHBOUR_SOLICITATION] = "NeighborSolicits",
-	[NDISC_REDIRECT] = "NeighborRedirects",
+	[NDISC_REDIRECT] = "Redirects",
 };

YOSHIFUJI Hideaki / 吉藤英明

Acked-by on v1 2/2

2008-01-21 09:58 UTC #13 lore raw

Re: [PATCH 2/2] IPV6: RFC 2011 compatibility broken

In article <[email protected]> (at Mon, 21 Jan 2008 17:46:44 +0800), Wang Chen <[email protected]> says:

Quoted 4 lines

The snmp6 entry name was changed, and it broke compatibility to RFC 2011.
Signed-off-by Wang Chen <[email protected]>

Acked-by YOSHIFUJI Hideaki <[email protected]>

--yoshfuji

David Stevens

Acked-by on v1 2/2

2008-01-21 19:17 UTC #14 lore raw

Re: [PATCH 2/2] IPV6: RFC 2011 compatibility broken

In reply to Wang Chen

RFC 2011 doesn't apply to IPv6, and the internal names of /proc entries are not used by the SNMP protocol, but it is an unintentional incompatibility with the
previous Linux entry names, so I agree. :-)

                                        +-DLS

Acked-by David L Stevens <[email protected]>

[email protected] wrote on 01/21/2008 01:46:44 AM:

Quoted 7 lines

[IPV6]: RFC 2011 compatibility broken
The snmp6 entry name was changed, and it broke compatibility to RFC 2011.
Signed-off-by Wang Chen <[email protected]>

Wang Chen

author

2008-01-21 11:16 UTC #15 lore raw

In reply to Wang Chen

Dave, how about this one.
It's like that one of IPV6.

Wang Chen said the following on 2008-1-16 17:59:

Quoted 25 lines

In tree net-2.6.25, commit "96793b482540f3a26e2188eaf75cb56b7829d3e3" made a mistake.
In that patch, David L added a icmp_out_count() in ip_push_pending_frames(), remove icmp_out_count() from icmp_reply(). But he forgot to remove icmp_out_count() from icmp_send() too.
Since icmp_send and icmp_reply will call icmp_push_reply, which will call ip_push_pending_frames, a duplicated increment happened in icmp_send.
This patch remove the icmp_out_count from icmp_send too.
Signed-off-by Wang Chen <[email protected]>
---
diff -Nurp linux-2.6.24.rc8.org/net/ipv4/icmp.c linux-2.6.24.rc8/net/ipv4/icmp.c
net/ipv4/icmp.c +0 -1
@@ -540,7 +540,6 @@ void icmp_send(struct sk_buff *skb_in, i
 	icmp_param.data.icmph.checksum	 = 0;
 	icmp_param.skb	  = skb_in;
 	icmp_param.offset = skb_network_offset(skb_in);
-	icmp_out_count(icmp_param.data.icmph.type);
 	inet_sk(icmp_socket->sk)->tos = tos;
 	ipc.addr = iph->saddr;
 	ipc.opt = &icmp_param.replyopts;

David Miller

maintainer

2008-01-21 11:25 UTC #16 lore raw

From: Wang Chen <[email protected]>
Date: Mon, 21 Jan 2008 19:15:07 +0800

Quoted 2 lines

Dave, how about this one.
It's like that one of IPV6.

I am confused, this changelog for this patch mentions changes made only in the net-2.6.25 tree.

Yet you just told me the two other patches should be applied to net-2.6

What is the exact story here?

Thank you.

Wang Chen

author

2008-01-21 11:36 UTC #17 lore raw

David Miller said the following on 2008-1-21 19:25:

Quoted 3 lines

I am confused, this changelog for this patch mentions changes made only in the net-2.6.25 tree.

Although I wrote that I find David.S's patch in net-2.6.25 was wrong, the guilty patch also in net-2.6 tree. Here is the web link.
http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=96793b482540f3a26e2188eaf75cb56b7829d3e3

So, I think apply my patches to net-2.6 is ok.

David Miller

maintainer

2008-01-21 11:40 UTC #18 lore raw

From: Wang Chen <[email protected]>
Date: Mon, 21 Jan 2008 19:34:56 +0800

Quoted 10 lines

David Miller said the following on 2008-1-21 19:25:
Quoted 3 lines
I am confused, this changelog for this patch mentions changes made only in the net-2.6.25 tree.
Although I wrote that I find David.S's patch in net-2.6.25 was wrong, the guilty patch also in net-2.6 tree. Here is the web link.
http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=96793b482540f3a26e2188eaf75cb56b7829d3e3
So, I think apply my patches to net-2.6 is ok.

Thank you for explaining this.

Patch applied to net-2.6