Skip to contentSkip to conversation

unexport icmpmsg_statistics

PATCH Applied Adrian Bunk 4 participants 11 messages Oct 24, 2007 – Oct 26, 2007

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Adrian Bunk

author
#0 lore raw

PATCH 1/1 unexport icmpmsg_statistics

This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).

Signed-off-by Adrian Bunk <[email protected]>

---
4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b

net/ipv4/icmp.c +0 -1
index 272c69e..233de06 100644
@@ -1104,5 +1104,4 @@ void __init icmp_init(struct net_proto_family *ops)
 EXPORT_SYMBOL(icmp_err_convert);
 EXPORT_SYMBOL(icmp_send);
 EXPORT_SYMBOL(icmp_statistics);
-EXPORT_SYMBOL(icmpmsg_statistics);
 EXPORT_SYMBOL(xrlim_allow);
#1 lore raw

[email protected] wrote on 10/24/2007 09:24:10 AM:

net/ipv4/icmp.c void __init icmp_init(struct net_proto_family

Quoted 11 lines

This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).

Signed-off-by Adrian Bunk <[email protected]>

---
4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b

net/ipv4/icmp.c +0 -0
index 272c69e..233de06 100644
@@ -1104,5 +1104,4 @@ void __init icmp_init(struct net_proto_family

*ops)

net/ipv4/icmp.c void __init icmp_init(struct net_proto_family *ops) · line 1107

Quoted 5 lines
net/ipv4/icmp.c +0 -1
 EXPORT_SYMBOL(icmp_err_convert);
 EXPORT_SYMBOL(icmp_send);
 EXPORT_SYMBOL(icmp_statistics);
-EXPORT_SYMBOL(icmpmsg_statistics);
 EXPORT_SYMBOL(xrlim_allow);

"icmpmsg_statistics" belongs with (and replaces some of the
old...)
"icmp_statistics". I'm not sure that any modules use it, but I think you should remove both or neither.

                                                                +-DLS
author
#2 lore raw
Quoted 20 lines · David Stevens

[email protected] wrote on 10/24/2007 09:24:10 AM:

net/ipv4/icmp.c void __init icmp_init(struct net_proto_family

Quoted 11 lines

This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).

Signed-off-by Adrian Bunk <[email protected]>

---
4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b

net/ipv4/icmp.c +0 -0
index 272c69e..233de06 100644
@@ -1104,5 +1104,4 @@ void __init icmp_init(struct net_proto_family

*ops)

net/ipv4/icmp.c void __init icmp_init(struct net_proto_family *ops) · line 1107

Quoted 5 lines
net/ipv4/icmp.c +0 -1
 EXPORT_SYMBOL(icmp_err_convert);
 EXPORT_SYMBOL(icmp_send);
 EXPORT_SYMBOL(icmp_statistics);
-EXPORT_SYMBOL(icmpmsg_statistics);
 EXPORT_SYMBOL(xrlim_allow);
Quoted 4 lines

"icmpmsg_statistics" belongs with (and replaces some of the
old...)
"icmp_statistics". I'm not sure that any modules use it, but I think you should remove both or neither.

icmp_statistics is used by the dccp_ipv4 and sctp modules.

icmpmsg_statistics is not used by any modules.

Quoted 1 line
                                                                +-DLS

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#3 lore raw

[email protected] wrote on 10/24/2007 12:14:37 PM:

Quoted 21 lines · David Stevens
Quoted 20 lines

[email protected] wrote on 10/24/2007 09:24:10 AM:

net/ipv4/icmp.c void __init icmp_init(struct net_proto_family

Quoted 11 lines

This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).

Signed-off-by Adrian Bunk <[email protected]>

---
4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b

net/ipv4/icmp.c +0 -0
index 272c69e..233de06 100644
@@ -1104,5 +1104,4 @@ void __init icmp_init(struct net_proto_family

*ops)

Quoted 5 lines
net/ipv4/icmp.c +0 -1
 EXPORT_SYMBOL(icmp_err_convert);
 EXPORT_SYMBOL(icmp_send);
 EXPORT_SYMBOL(icmp_statistics);
-EXPORT_SYMBOL(icmpmsg_statistics);
 EXPORT_SYMBOL(xrlim_allow);
Quoted 3 lines
Quoted 3 lines

"icmpmsg_statistics" belongs with (and replaces some of the
old...)
"icmp_statistics". I'm not sure that any modules use it, but I think

you

Quoted 3 lines
Quoted 1 line

should remove both or neither.

icmp_statistics is used by the dccp_ipv4 and sctp modules.

The only items left in icmp_statistics are "InMsgs, InErrs,
OutMsgs, OutErrs",
so if dccp and sctp are sending or receiving any in or out ICMP messages, they
should be using the new macros (which reference icmpmsg_statistics, not icmp_statistics) to count them.
I took a quick look at SCTP. I don't know if that's going through
icmp_rcv()
or not; if so, I think it's double-counting; if not, then it isn't counting the
individual types (as it should), and it should have ICMPMSG macros doing that.

So, again, icmpmsg_statistics either should stay exported, or
neither
icmpmsg_statistics nor icmp_statistics should be exported (depending on how
SCTP and DCCP code is resolved). It's incorrect in the current code to incrememnt ICMP_MIB_INMSGS without incrementing one of the types too.

+-DLS

#4 lore raw

I took a look at the DCCP references, and I think they're just incrementing the wrong MIB variable -- e.g., it's incrementing ICMP_MIB_INERRORS when the skb length is less than the header indicates. That's not an ICMP_MIB_INERRORS error, that's an IPSTATS_MIB_INHDRERRORS error. ICMP_MIB_INERRORS is when you receive an ICMP error packet; an IP header error is something else entirely.

That's followed by a failed lookup incrementing ICMP_MIB_INERRORS which should be an unknown port error in the transport MIB (assuming it has one-- it's not an ICMP error; could be an IP error, if the address isn't local, rather than unknown port).

In SCTP, it appears to have similar problems. SCTP errors are not ICMP errors, though it perhaps should be calling icmp_send() to send one to the offending host for some of the cases.

I haven't seen any ICMP-relevant stats correctly referenced in these yet.

I don't want to patch them directly, since I can't easily test them; if someone who works with DCCP and SCTP would like to, I'd be happy to review. Any volunteers?

                                                +-DLS
#5 lore raw
Quoted 17 lines · David Stevens

I took a look at the DCCP references, and I think they're just incrementing the wrong MIB variable -- e.g., it's incrementing ICMP_MIB_INERRORS when the skb length is less than the header indicates. That's not an ICMP_MIB_INERRORS error, that's an IPSTATS_MIB_INHDRERRORS error. ICMP_MIB_INERRORS is when you receive an ICMP error packet; an IP header error is something else entirely.

That's followed by a failed lookup incrementing ICMP_MIB_INERRORS which should be an unknown port error in the transport MIB (assuming it has one-- it's not an ICMP error; could be an IP error, if the address isn't local, rather than unknown port).

In SCTP, it appears to have similar problems. SCTP errors are not ICMP errors, though it perhaps should be calling icmp_send() to send one to the offending host for some of the cases.

Quoted 6 lines

I haven't seen any ICMP-relevant stats correctly referenced in these yet.

I don't want to patch them directly, since I can't easily test them; if someone who works with DCCP and SCTP would like to, I'd be happy to review. Any volunteers?

I'll take a look at the SCTP ones. Thanks for review.

-vlad

Quoted 8 lines
                                                +-DLS

-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw

My bad -- I see what it's doing, and it looks ok after all.

I thought I saw an INMSGS (but didn't). These are ICMP errors that went through icmp_rcv() and were counted correctly before getting to the protocol error handlers. These are failures due mostly to not having enough, or the right protocol info in the error packet being handled. I'm not sure I'd count those as ICMP errors, since the ICMP header itself is correct, but ok...

SCTP doesn't look so bad, though I think the references are still questionable (but debatable) as ICMP errors.

sctp_v4_err is incrementing ICMP_MIB_INERRORS if there isn't enough IP header to find the ports, I see. I'm not sure that counts as an ICMP error, but it's not so terrible.

It's doing the same thing if a lookup fails to match "vtag" from the encapsulated error packet. Again, I don't know that those are ICMP errors (which normally are something wrong with the ICMP header).

So, I stand corrected, and sorry about the histrionics. Since these are arguably ICMP errors, and since errors is the only thing being MIB-counted in DCCP and SCTP, then it now looks ok to me as-is, and also ok to remove icmpmsg_statistics from exporting.

                                        +-DLS
#7 lore raw
Quoted 2 lines · David Stevens

My bad -- I see what it's doing, and it looks ok after all.

jinx, crossed in flight. ;)

#8 lore raw
In reply to David Stevens
Quoted 12 lines · David Stevens

My bad -- I see what it's doing, and it looks ok after all.

I thought I saw an INMSGS (but didn't). These are ICMP errors that went through icmp_rcv() and were counted correctly before getting to the protocol error handlers. These are failures due mostly to not having enough, or the right protocol info in the error packet being handled. I'm not sure I'd count those as ICMP errors, since the ICMP header itself is correct, but ok...

SCTP doesn't look so bad, though I think the references are still questionable (but debatable) as ICMP errors.

Quoted 8 lines

sctp_v4_err is incrementing ICMP_MIB_INERRORS if there isn't enough IP header to find the ports, I see. I'm not sure that counts as an ICMP error, but it's not so terrible.

It's doing the same thing if a lookup fails to match "vtag" from the encapsulated error packet. Again, I don't know that those are ICMP errors (which normally are something wrong with the ICMP header).

This particular case is the one that bugs the most, but that error matches best.

Seems like all ULPs treat socket lookup error as ICMP_MIB_INERRORS (tcp, udp, sctp, dccp).

SCTP is a little special in that in needs to check one one piece of data (the 'vtag') to correctly identify the connection. If that piece doesn't match, we treat that as the same error.

-vlad

maintainer Applied
#9 lore raw
In reply to David Stevens

From: David Stevens <[email protected]>
Date: Wed, 24 Oct 2007 13:43:14 -0700

Quoted 5 lines

So, I stand corrected, and sorry about the histrionics. Since these are arguably ICMP errors, and since errors is the only thing being MIB-counted in DCCP and SCTP, then it now looks ok to me as-is, and also ok to remove icmpmsg_statistics from exporting.

I've applied Adrian's export removal patch, thanks!

#10 lore raw
In reply to David Stevens
Quoted 7 lines · David Stevens

I took a look at the DCCP references, and I think they're just incrementing the wrong MIB variable -- e.g., it's incrementing ICMP_MIB_INERRORS when the skb length is less than the header indicates. That's not an ICMP_MIB_INERRORS error, that's an IPSTATS_MIB_INHDRERRORS error. ICMP_MIB_INERRORS is when you receive an ICMP error packet; an IP header error is something else entirely.

Looking at icmp_rcv(), ICMP_MIB_INERRORS is incremented if:
a) checksum fails
b) no enough room in skb for icmp header
c) type out of bound
and other error conditions while processing ICMP packet.

Are all of these wrong as well?

There are other places that increment this statistic for errors during processing.

Quoted 9 lines

That's followed by a failed lookup incrementing ICMP_MIB_INERRORS which should be an unknown port error in the transport MIB (assuming it has one-- it's not an ICMP error; could be an IP error, if the address isn't local, rather than unknown port).

In SCTP, it appears to have similar problems. SCTP errors are not ICMP errors, though it perhaps should be calling icmp_send() to send one to the offending host for some of the cases.

I'll skip the insufficient buffer space statistic yet, since, per above, it's not clear which one should be used.

Others, I agree are move ULP errors, but the mibs don't account for those yet.

-vlad