From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [PATCH] net: rocker: Add basic netdev counters Date: Wed, 14 Jan 2015 16:54:49 -0700 Message-ID: <54B701C9.50500@gmail.com> References: <1421275161-99434-1-git-send-email-dsahern@gmail.com> <54B6F43C.4030207@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Scott Feldman , Jiri Pirko To: Florian Fainelli , netdev@vger.kernel.org Return-path: Received: from mail-pd0-f178.google.com ([209.85.192.178]:40003 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751695AbbANXyx (ORCPT ); Wed, 14 Jan 2015 18:54:53 -0500 Received: by mail-pd0-f178.google.com with SMTP id r10so12670806pdi.9 for ; Wed, 14 Jan 2015 15:54:52 -0800 (PST) In-Reply-To: <54B6F43C.4030207@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On 1/14/15 3:57 PM, Florian Fainelli wrote: > On 14/01/15 14:39, David Ahern wrote: >> Add packet and byte counters for RX and TX paths. >> >> $ ifconfig eth1 >> eth1: flags=4163 mtu 1500 >> inet6 fe80::5054:ff:fe12:3501 prefixlen 64 scopeid 0x20 >> ether 52:54:00:12:35:01 txqueuelen 1000 (Ethernet) >> RX packets 63 bytes 15813 (15.4 KiB) >> RX errors 0 dropped 0 overruns 0 frame 0 >> TX packets 79 bytes 17991 (17.5 KiB) >> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0 >> >> Signed-off-by: David Ahern >> Cc: Scott Feldman >> Cc: Jiri Pirko >> --- >> drivers/net/ethernet/rocker/rocker.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c >> index 2f398fa4b9e6..9743279d9121 100644 >> --- a/drivers/net/ethernet/rocker/rocker.c >> +++ b/drivers/net/ethernet/rocker/rocker.c >> @@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev) >> if (!desc_info) >> netif_stop_queue(dev); >> >> + dev->stats.tx_packets++; >> + dev->stats.tx_bytes += skb->len; > > Potential use after free, the skb pointer is certainly not valid anymore > here. > > BTW, increasing statistics here is valid because this is a driver for a > virtual piece of HW, which does not have TX reclaim/completion logic, > but if it did, statistics update should occur there, not in the > ndo_start_xmit() function. sure. I had considered putting in the rocker_port_poll_tx function like this: - dev_kfree_skb_any(rocker_desc_cookie_ptr_get(desc_info)); + + skb = rocker_desc_cookie_ptr_get(desc_info); + rocker_port->dev->stats.tx_packets++; + rocker_port->dev->stats.tx_bytes += skb->len; + + dev_kfree_skb_any(skb); I think this the reclaim point. > >> + >> return NETDEV_TX_OK; >> >> unmap_frags: >> @@ -3565,6 +3568,8 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev) >> rocker_tlv_nest_cancel(desc_info, frags); >> out: >> dev_kfree_skb(skb); >> + dev->stats.tx_dropped++; >> + >> return NETDEV_TX_OK; >> } >> >> @@ -3890,6 +3895,9 @@ static int rocker_port_rx_proc(struct rocker *rocker, >> skb->protocol = eth_type_trans(skb, rocker_port->dev); >> netif_receive_skb(skb); >> >> + rocker_port->dev->stats.rx_packets++; >> + rocker_port->dev->stats.rx_bytes += skb->len; > > Same here, past netif_receive_skb() you should not assume that this skb > reference is valid. right. I'll move the stats above the netif_receive_skb. Thanks, David