netdev - Re: [PATCH] snmp: add missing counters for RFC 4293

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090424170655.GA15491@hmsreliant.think-freely.org>
Date:	Fri, 24 Apr 2009 13:06:55 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Eric Dumazet <dada1@...mosbay.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] snmp: add missing counters for RFC 4293

On Fri, Apr 24, 2009 at 04:10:15PM +0200, Eric Dumazet wrote:
> Neil Horman a écrit :
> 
> > 
> > Ok, last time (I hope :)). New patch, functionally identially.  changes are:
> > 
> > 1) Incorporate Erics suggestion to improve efficiency of SNMP_UPD_PO_STATS*
> > macros by reduing use of in_softirq and per_cpu_ptr.
> > 
> > 
> > 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>
> > 
> 
> Hi Neil
> 
> Some errors, please find my comments.
> 
Responses inline...

> > diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> > index a51fb33..7434f73 100644
> > --- a/net/ipv6/mcast.c
> > +++ b/net/ipv6/mcast.c
> > @@ -1449,7 +1449,8 @@ static void mld_sendpack(struct sk_buff *skb)
> >  	int err;
> >  	struct flowi fl;
> >  
> > -	IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTREQUESTS);
> > +	IP6_UPD_PO_STATS(net, idev, IPSTATS_MIB_OUT, skb->len);
> 
> Are you sure skb->len is setup at this point ?
> 
Yep, skbs from here are allocated in mld_newpack, and skb_put is always called
on them

> > +
> >  	payload_len = (skb->tail - skb->network_header) - sizeof(*pip6);
> >  	mldlen = skb->tail - skb->transport_header;
> >  	pip6->payload_len = htons(payload_len);
> > @@ -1479,7 +1480,7 @@ out:
> >  	if (!err) {
> >  		ICMP6MSGOUT_INC_STATS_BH(net, idev, ICMPV6_MLD2_REPORT);
> >  		ICMP6_INC_STATS_BH(net, idev, ICMP6_MIB_OUTMSGS);
> > -		IP6_INC_STATS_BH(net, idev, IPSTATS_MIB_OUTMCASTPKTS);
> > +		IP6_UPD_PO_STATS_BH(net, idev, IPSTATS_MIB_OUTMCAST, skb->len);
> 
> skb it not anymore available here, since it was 'given' to transmit.
> 
Yeah, I'll fix that.  I'll just record the length before I send it off, and use
that instead.

> >  	} else
> >  		IP6_INC_STATS_BH(net, idev, IPSTATS_MIB_OUTDISCARDS);
> >  
> > @@ -1774,8 +1775,8 @@ static void igmp6_send(struct in6_addr *addr, struct net_device *dev, int type)
> >  	struct flowi fl;
> >  
> >  	rcu_read_lock();
> > -	IP6_INC_STATS(net, __in6_dev_get(dev),
> > -		      IPSTATS_MIB_OUTREQUESTS);
> > +	IP6_UPD_PO_STATS(net, __in6_dev_get(dev),
> > +		      IPSTATS_MIB_OUT, skb->len);
> 
> 
> skb not yet initialized here, sorry, crash in progress...
> 
Yeah, that was stupid, I think I can fix that though by using the computation of
full_len though, since we know how much space we need right before the call to
sock_alloc_send_skb

> Please move this a few lines after a sucessfull  sock_alloc_send_skb() call
> 
> >  	rcu_read_unlock();
> >  	if (type == ICMPV6_MGM_REDUCTION)
> >  		snd_addr = &in6addr_linklocal_allrouters;
> > @@ -1844,7 +1845,7 @@ out:
> >  	if (!err) {
> >  		ICMP6MSGOUT_INC_STATS(net, idev, type);
> >  		ICMP6_INC_STATS(net, idev, ICMP6_MIB_OUTMSGS);
> > -		IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTMCASTPKTS);
> > +		IP6_UPD_PO_STATS(net, idev, IPSTATS_MIB_OUTMCAST, skb->len);
> 
> skb it not anymore available here, since it was 'given' to transmit.
> 
Copy that, I can reuse full_len here I believe.

Thanks for the eyes, I'll fix these up and repost after I test it a bit
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