| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 Apr 2009 19:03:58 -0400
From: Neil Horman <nhorman@...driver.com>
To: Eric Dumazet <dada1@...mosbay.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] snmp: add missing counters for RFC 4293
On Tue, Apr 21, 2009 at 10:45:49PM +0200, Eric Dumazet wrote:
> Neil Horman a écrit :
> > On Tue, Apr 21, 2009 at 09:58:51PM +0200, Eric Dumazet wrote:
> >> Neil Horman a écrit :
> >>> The IP MIB (RFC 4293) defines stats for InOctets, OutOctets, InMcastOctets and
> >>> OutMcastOctets:
> >>> http://tools.ietf.org/html/rfc4293
> >>> But it seems we don't track those in any way that easy to separate from other
> >>> protocols. This patch adds those missing counters to the stats file. Tested
> >>> successfully by me
> >>>
> >>> Signed-off-by: Neil Horman <nhorman@...driver.com>
> >>>
> >>>
> >>> include/linux/snmp.h | 4 ++++
> >>> net/ipv4/ip_input.c | 4 ++++
> >>> net/ipv4/ip_output.c | 3 +++
> >>> net/ipv4/proc.c | 4 ++++
> >>> net/ipv6/ip6_input.c | 4 ++++
> >>> net/ipv6/ip6_output.c | 8 ++++++++
> >>> net/ipv6/mcast.c | 9 +++++++++
> >>> net/ipv6/ndisc.c | 3 +++
> >>> net/ipv6/proc.c | 4 ++++
> >>> net/ipv6/raw.c | 6 ++++++
> >>> 10 files changed, 49 insertions(+)
> >>>
> >>> diff --git a/include/linux/snmp.h b/include/linux/snmp.h
> >>> index aee3f1e..95c17f6 100644
> >>> --- a/include/linux/snmp.h
> >>> +++ b/include/linux/snmp.h
> >>> @@ -19,6 +19,8 @@ enum
> >>> {
> >>> IPSTATS_MIB_NUM = 0,
> >>> IPSTATS_MIB_INRECEIVES, /* InReceives */
> >>> + IPSTATS_MIB_INOCTETS, /* InOctets */
> >>> + IPSTATS_MIB_INMCASTOCTETS, /* InMcastOctets */
> >>> IPSTATS_MIB_INHDRERRORS, /* InHdrErrors */
> >>> IPSTATS_MIB_INTOOBIGERRORS, /* InTooBigErrors */
> >>> IPSTATS_MIB_INNOROUTES, /* InNoRoutes */
> >>> @@ -29,6 +31,8 @@ enum
> >>> IPSTATS_MIB_INDELIVERS, /* InDelivers */
> >>> IPSTATS_MIB_OUTFORWDATAGRAMS, /* OutForwDatagrams */
> >>> IPSTATS_MIB_OUTREQUESTS, /* OutRequests */
> >>> + IPSTATS_MIB_OUTOCTETS, /* OutOctets */
> >>> + IPSTATS_MIB_OUTMCASTOCTETS, /* OutMcastOctets */
> >>> IPSTATS_MIB_OUTDISCARDS, /* OutDiscards */
> >>> IPSTATS_MIB_OUTNOROUTES, /* OutNoRoutes */
> >>> IPSTATS_MIB_REASMTIMEOUT, /* ReasmTimeout */
> >>> diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> >>> index 1a58a6f..bc9169b 100644
> >>> --- a/net/ipv4/ip_input.c
> >>> +++ b/net/ipv4/ip_input.c
> >>> @@ -385,6 +385,7 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
> >>> goto drop;
> >>>
> >>> IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INRECEIVES);
> >>> + IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_INOCTETS, skb->len);
> >>>
> >>> if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) {
> >>> IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INDISCARDS);
> >>> @@ -396,6 +397,9 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
> >>>
> >>> iph = ip_hdr(skb);
> >>>
> >>> + if (ipv4_is_multicast(iph->daddr))
> >>> + IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_INMCASTOCTETS, skb->len);
> >>> +
> >>> /*
> >>> * RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum.
> >>> *
> >>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> >>> index 3e7e910..8a68dc2 100644
> >>> --- a/net/ipv4/ip_output.c
> >>> +++ b/net/ipv4/ip_output.c
> >>> @@ -245,6 +245,8 @@ int ip_mc_output(struct sk_buff *skb)
> >>> * If the indicated interface is up and running, send the packet.
> >>> */
> >>> IP_INC_STATS(dev_net(dev), IPSTATS_MIB_OUTREQUESTS);
> >>> + IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_OUTOCTETS, skb->len);
> >>> + IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_OUTMCASTOCTETS, skb->len);
> >> So you use the _BH variant right after IP_INC_STATS() ?
> >> Which one is right (or which one is false ?)
> >>
> >
> > Both are correct (right now), at least as far as I can tell. I'm not 100% sure why ADD
> > was named ADD_BH, except for the fact that it doesn't toggle the use of the mib
> > array passed in. I think the right solution would be to simply rename
> > IP_ADD_STATS_BH to IP_ADD_STATS, and modify its implementation to access
> > mib[!in_softirq()] rather than just mib[0]. I had planned to do this in a
> > followup cleanup patch. Alternatively, I could add a new IP_ADD_STATS to be
> > complete, but I don't really see the advantage (asside from not checking
> > in_softirq quite as often).
> >
>
> Both usages in the same function cannot be correct.
>
> Either you use _BH variant because you know you are in softirq
> (and non preemptable) context.
>
> Either you use non_BH variant because you are in possibly preemptable context.
>
> Mixing both just proves there is a problem.
>
> And we use _BH variant because it is currently faster (this might change in 2.6.31
> thanks to per_cpu infra changes)
>
>
>
Yeah, Self-NAK. I'm sorry, you're right. I had myself convinced it didn't
really matter in the use-cases, but looking back over it, I'm not sure what I
was thinking. I need to implement IP_ADD_STATS (currently it doesn't exist),
and replace all the use cases in this patch. Simple enough. I'll repost in the
AM.
Thanks for the whack with the clue stick :)
Neil
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists