| 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
| ||
|
Message-ID: <49EEA881.4040001@cosmosbay.com>
Date: Wed, 22 Apr 2009 07:17:53 +0200
From: Eric Dumazet <dada1@...mosbay.com>
To: Neil Horman <nhorman@...driver.com>
CC: netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] snmp: add missing counters for RFC 4293
Neil Horman a écrit :
> 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 :)
You are welcome.
--
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