[2.6 patch] unexport icmpmsg_statistics

[2.6 patch] unexport icmpmsg_statistics

[Adrian Bunk]

This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---
4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b 
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 272c69e..233de06 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -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);

Re: [2.6 patch] unexport icmpmsg_statistics

[David Stevens]

netdev-owner@vger.kernel.org wrote on 10/24/2007 09:24:10 AM:

> This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).
> 
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
> 
> ---
> 4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b 
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 272c69e..233de06 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -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);

        "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

Re: [2.6 patch] unexport icmpmsg_statistics

[Adrian Bunk]

On Wed, Oct 24, 2007 at 12:07:45PM -0700, David Stevens wrote:
> netdev-owner@vger.kernel.org wrote on 10/24/2007 09:24:10 AM:
> 
> > This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).
> > 
> > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> > 
> > ---
> > 4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b 
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> > index 272c69e..233de06 100644
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -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);
> 
>         "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.

>                                                                 +-DLS

cu
Adrian

-- 

       "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

Re: [2.6 patch] unexport icmpmsg_statistics

[David Stevens]

netdev-owner@vger.kernel.org wrote on 10/24/2007 12:14:37 PM:

> On Wed, Oct 24, 2007 at 12:07:45PM -0700, David Stevens wrote:
> > netdev-owner@vger.kernel.org wrote on 10/24/2007 09:24:10 AM:
> > 
> > > This patch removes the unused EXPORT_SYMBOL(icmpmsg_statistics).
> > > 
> > > Signed-off-by: Adrian Bunk <bunk@kernel.org>
> > > 
> > > ---
> > > 4ce74657ac0b1bdcb4c7bc359d05643f8cc4a08b 
> > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> > > index 272c69e..233de06 100644
> > > --- a/net/ipv4/icmp.c
> > > +++ b/net/ipv4/icmp.c
> > > @@ -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);
> > 
> >         "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.

        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

Re: [2.6 patch] unexport icmpmsg_statistics

[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.

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

Re: [2.6 patch] unexport icmpmsg_statistics

[Vlad Yasevich]

David Stevens wrote:
> 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?

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

-vlad

> 
>                                                 +-DLS
> 
> -
> 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
> 

Re: [2.6 patch] unexport icmpmsg_statistics

[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.

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

Re: [2.6 patch] unexport icmpmsg_statistics

[Vlad Yasevich]

David Stevens wrote:
> My bad -- I see what it's doing, and it looks ok after all.
> 

jinx, crossed in flight. ;)

Re: [2.6 patch] unexport icmpmsg_statistics

[Vlad Yasevich]

David Stevens wrote:
> 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).

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

Re: [2.6 patch] unexport icmpmsg_statistics

[David Miller]

From: David Stevens <dlstevens@us.ibm.com>
Date: Wed, 24 Oct 2007 13:43:14 -0700

> 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!

Re: [2.6 patch] unexport icmpmsg_statistics

[Vlad Yasevich]

David Stevens wrote:
> 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.

> 
> 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