Re: [PATCH] net: rocker: Add basic netdev counters

[David Ahern <dsahern@gmail.com>]

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<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
>>          inet6 fe80::5054:ff:fe12:3501  prefixlen 64  scopeid 0x20<link>
>>          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 <dsahern@gmail.com>
>> Cc: Scott Feldman <sfeldma@gmail.com>
>> Cc: Jiri Pirko <jiri@resnulli.us>
>> ---
>>   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