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

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] [day] [month] [year] [list]

Message-ID: <CAE4R7bB+tsGjvfGMtZDVf7ipvCXNdPSaFQ9mwB4nSpSTvsRE8w@mail.gmail.com>
Date:	Fri, 16 Jan 2015 11:53:07 -0800
From:	Scott Feldman <sfeldma@...il.com>
To:	David Ahern <dsahern@...il.com>
Cc:	Florian Fainelli <f.fainelli@...il.com>,
	Netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH] net: rocker: Add basic netdev counters

On Wed, Jan 14, 2015 at 3:54 PM, David Ahern <dsahern@...il.com> wrote:
> 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@...il.com>
>>> Cc: Scott Feldman <sfeldma@...il.com>
>>> Cc: Jiri Pirko <jiri@...nulli.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.

This is better.  But also need to consider err code returned from
device in same routine.  If the desc was marked with err, then I think
we can assume pkt wasn't sent so tx_packets/bytes stats should not be
incremented, but some tx err stat should, based on desc err code.

-scott
--
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