e1000e: Don't return uninitialized stats
Subsystems (3)
PATCH 1/2 e1000e: Don't return uninitialized stats
Some statistics passed to ethtool are garbage because e1000e_get_stats64() doesn't write them, for example: tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.
Do like ixgbe and use dev_get_stats() which first zeroes out rtnl_link_stats64.
--- drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/intel/e1000e/ethtool.c +1 -1
@@ -2063,7 +2063,7 @@ static void e1000_get_ethtool_stats(struct net_device *netdev,pm_runtime_get_sync (netdev->dev.parent); -e1000e_get_stats64 (netdev, &net_stats); +dev_get_stats (netdev, &net_stats);pm_runtime_put_sync (netdev->dev.parent);
Signature · 1 line
2.12.2
PATCH 2/2 igb: Remove useless argument
Given that all callers of igb_update_stats() pass the same two arguments:
(adapter, &adapter->stats64), the second argument can be removed.
--- drivers/net/ethernet/intel/igb/igb.h | 2 +- drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +- drivers/net/ethernet/intel/igb/igb_main.c | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/net/ethernet/intel/igb/igb.h +1 -1
@@ -593,7 +593,7 @@ void igb_setup_rctl(struct igb_adapter *);netdev_tx_t igb_xmit_frame_ring (struct sk_buff *,struct igb_ring *);void igb_unmap_and_free_tx_resource (struct igb_ring *,struct igb_tx_buffer *);void igb_alloc_rx_buffers (struct igb_ring *,u16 ); -void igb_update_stats (struct igb_adapter *,struct rtnl_link_stats64 *); +void igb_update_stats (struct igb_adapter *);bool igb_has_link (struct igb_adapter *adapter);void igb_set_ethtool_ops (struct net_device *);void igb_power_up_link (struct igb_adapter *);
drivers/net/ethernet/intel/igb/igb_ethtool.c +1 -1
@@ -2287,7 +2287,7 @@ static void igb_get_ethtool_stats(struct net_device *netdev,char *p;spin_lock (&adapter->stats64_lock); -igb_update_stats (adapter, net_stats); +igb_update_stats (adapter);for (i =0 ; i <IGB_GLOBAL_STATS_LEN ; i++) { p = (char *)adapter + igb_gstrings_stats[i].stat_offset;
drivers/net/ethernet/intel/igb/igb_main.c +5 -5
@@ -1815,7 +1815,7 @@ void igb_down(struct igb_adapter *adapter)/* record the stats before reset*/ spin_lock (&adapter->stats64_lock); -igb_update_stats (adapter, &adapter->stats64); +igb_update_stats (adapter);spin_unlock (&adapter->stats64_lock); adapter->link_speed =0 ;
@@ -4628,7 +4628,7 @@ static void igb_watchdog_task(struct work_struct *work) }spin_lock (&adapter->stats64_lock); -igb_update_stats (adapter, &adapter->stats64); +igb_update_stats (adapter);spin_unlock (&adapter->stats64_lock);for (i =0 ; i < adapter->num_tx_queues; i++) {
@@ -5410,7 +5410,7 @@ static void igb_get_stats64(struct net_device *netdev,struct igb_adapter *adapter =netdev_priv (netdev);spin_lock (&adapter->stats64_lock); -igb_update_stats (adapter, &adapter->stats64); +igb_update_stats (adapter);memcpy (stats, &adapter->stats64,sizeof (*stats));spin_unlock (&adapter->stats64_lock); }
@@ -5459,9 +5459,9 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)* igb_update_stats - Update the board statistics counters * @adapter: board private structure **/ -void igb_update_stats (struct igb_adapter *adapter, -struct rtnl_link_stats64 *net_stats) +void igb_update_stats (struct igb_adapter *adapter) { +struct rtnl_link_stats64 *net_stats = &adapter->stats64;struct e1000_hw *hw = &adapter->hw;struct pci_dev *pdev = adapter->pdev;u32 reg, mpc;
Signature · 1 line
2.12.2
A message that is not in the archive (sent off-list or not yet received): 630A6B92B7EDEB45A87E20D3D286660171182EED@hasmsx109.ger.corp.intel.com
Quoted 33 lines · Sasha
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Benjamin Poirier
Sent: Saturday, April 22, 2017 00:20
To: Kirsher, Jeffrey T <[email protected]>
Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Stefan Priebe <[email protected]>
Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized statsSome statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Do like ixgbe and use
dev_get_stats()which first zeroes outrtnl_link_stats64.Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>--- drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/ethernet/intel/e1000e/ethtool.c +1
-2@@ -2063,7 +2063,7 @@ static void e1000_get_ethtool_stats(struct net_device *netdev,pm_runtime_get_sync (netdev->dev.parent); -e1000e_get_stats64 (netdev, &net_stats); +dev_get_stats (netdev, &net_stats);pm_runtime_put_sync (netdev->dev.parent); --2.12.2
Quoted 4 lines
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Hello,
We would like to not accept this patch. Suggested generic method '*dev_get_stats' (net/core/dev.c) calls 'ops->ndo_get_stats64' method which eventually calls e1000e_get_stats64 (netdev.c) - so there is same functionality. Also, see that 'e1000e_get_stats64' method in netdev.c (line 5928) calls 'memset' with 0's before update statistics. Local sanity check in our lab shows 'tx_heartbeat_errors' counter reported as 0.
Thanks,
Sasha
Sasha, please use reply-all to keep everyone in cc (including me...).
Quoted 37 lines · Sasha
Quoted 29 lines · Sasha
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Benjamin Poirier
Sent: Saturday, April 22, 2017 00:20
To: Kirsher, Jeffrey T <[email protected]>
Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; Stefan Priebe <[email protected]>
Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized statsSome statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Do like ixgbe and use
dev_get_stats()which first zeroes outrtnl_link_stats64.Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>--- drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/ethernet/intel/e1000e/ethtool.c +1
-2@@ -2063,7 +2063,7 @@ static void e1000_get_ethtool_stats(struct net_device *netdev,pm_runtime_get_sync (netdev->dev.parent); -e1000e_get_stats64 (netdev, &net_stats); +dev_get_stats (netdev, &net_stats);pm_runtime_put_sync (netdev->dev.parent); --2.12.2
Quoted 4 lines
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lanHello,
Quoted 4 lines
We would like to not accept this patch. Suggested generic method '
*dev_get_stats' (net/core/dev.c) calls 'ops->ndo_get_stats64' method which eventually callse1000e_get_stats64(netdev.c) - so there is same functionality. Also, see that 'e1000e_get_stats64' method innetdev.c(line
No, it's not the same functionality because dev_get_stats() does a memset on the rtnl_link_stats64 struct.
Quoted 1 line
5928) calls 'memset' with 0's before update statistics. Local sanity check
I don't see any memset in e1000e_get_stats64(). What kernel version are you looking at?
Quoted 2 lines
in our lab shows '
tx_heartbeat_errors' counter reported as 0.
Please see the mail I just sent to Paul Menzel <[email protected]> for more information about the issue and how to reproduce it.
Quoted 63 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Benjamin Poirier
Sent: Monday, April 24, 2017 12:10 PM
To: Neftin, Sasha <[email protected]>
Cc: Kirsher at f1.synalogic.ca; Stefan Priebe <[email protected]>; netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized statsSasha, please use reply-all to keep everyone in cc (including me...).
Quoted 3 lines · Sasha
Quoted 2 lines · Sasha
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org]On Behalf Of Benjamin Poirier
Quoted 3 lines
Quoted 3 lines
Sent: Saturday, April 22, 2017 00:20
To: Kirsher, Jeffrey T <[email protected]>
Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; StefanPriebe <[email protected]>
Quoted 1 line
Quoted 1 line
Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized
stats
Quoted 2 lines
Quoted 2 lines
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Quoted 2 lines
Quoted 2 lines
Do like ixgbe and use
dev_get_stats()which first zeroes out
rtnl_link_stats64.Quoted 8 lines
Review
Quoted 8 lines
Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>--- drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/ethernet/intel/e1000e/ethtool.cQuoted 4 lines
drivers/net/ethernet/intel/e1000e/ethtool.c static void e1000_get_ethtool_stats(struct
Quoted 4 lines
index
7aff68a4a4df..f117b90cdc2f100644drivers/net/ethernet/intel/e1000e/ethtool.c +0
-0@@ -2063,7 +2063,7 @@ static void e1000_get_ethtool_stats(struct
net_device*netdev,Quoted 14 lines
Quoted 11 lines
drivers/net/ethernet/intel/e1000e/ethtool.c +1
-2pm_runtime_get_sync (netdev->dev.parent); -e1000e_get_stats64 (netdev, &net_stats); +dev_get_stats (netdev, &net_stats);pm_runtime_put_sync (netdev->dev.parent); --2.12.2
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lanHello,
Quoted 2 lines
We would like to not accept this patch. Suggested generic method '
*dev_get_stats' (net/core/dev.c) calls 'ops->ndo_get_stats64' methodwhich
Quoted 2 lines
eventually calls
e1000e_get_stats64(netdev.c) - so there is same functionality. Also, see that 'e1000e_get_stats64' method innetdev.c(line
Quoted 7 lines
No, it's not the same functionality because
dev_get_stats()does a memset on thertnl_link_stats64struct.Quoted 1 line
5928) calls 'memset' with 0's before update statistics. Local sanity check
I don't see any memset in
e1000e_get_stats64(). What kernel version are you looking at?
The call to memset was removed from the upstream kernel with:
------------------------------------------------------------------------------------
commit 5944701df90d9577658e2354cc27c4ceaeca30fe
Author: stephen hemminger <[email protected]> Date: Fri Jan 6 19:12:53 2017 -0800
net: remove useless memset's in drivers get_stats64
In dev_get_stats() the statistic structure storage has already been zeroed. Therefore network drivers do not need to call memset() again.
...
< changes to other drivers snipped out >
...
drivers/net/ethernet/intel/e1000e/netdev.c +0 -1
@@ -5925,7 +5925,6 @@ void e1000e_get_stats64(struct net_device *netdev, {struct e1000_adapter *adapter =netdev_priv (netdev); -memset (stats,0 ,sizeof (struct rtnl_link_stats64 ));spin_lock (&adapter->stats64_lock);e1000e_update_stats (adapter);/* Fill out the OS statistics structure */
------------------------------------------------------------------------------------
This also is where the bad counters start to show up for e1000e for my test systems. From this driver on I see (very) large values for tx_dropped, rx_over_errors and tx_fifo_errors on driver load (even before bringing the interface up. It seems the memset is not so useless for this driver after all. Would simply reverting the e1000e portion of this patch resolve the issue?
Quoted 10 lines
Quoted 3 lines
in our lab shows '
tx_heartbeat_errors' counter reported as 0.Please see the mail I just sent to Paul Menzel <[email protected]> for more information about the issue and how to reproduce it.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Quoted 124 lines · Aaron F
Quoted 84 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl. org] On
Behalf Of Benjamin Poirier
Sent: Monday, April 24, 2017 12:10 PM
To: Neftin, Sasha <[email protected]>
Cc: Kirsher at f1.synalogic.ca; Stefan Priebe <[email protected]>; netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized
statsSasha, please use reply-all to keep everyone in cc (including me...).
Quoted 4 lines · Sasha
Quoted 3 lines · Sasha
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osu osl.org]On Behalf Of Benjamin Poirier
Quoted 4 lines
Quoted 4 lines
Sent: Saturday, April 22, 2017 00:20
To: Kirsher, Jeffrey T <[email protected]>
Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; StefanPriebe <[email protected]>
Quoted 2 lines
Quoted 2 lines
Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized
stats
Quoted 2 lines
Quoted 2 lines
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors.
This leaks kernel memory to userspace and confuses users.Quoted 2 lines
Quoted 2 lines
Do like ixgbe and use
dev_get_stats()which first zeroes out
rtnl_link_stats64.Quoted 8 lines
Review
Quoted 8 lines
Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>---
??drivers/net/ethernet/intel/e1000e/ethtool.c| 2 +- ?? 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/ethernet/intel/e1000e/ethtool.cQuoted 5 lines
Quoted 5 lines
index
7aff68a4a4df..f117b90cdc2f100644drivers/net/ethernet/intel/e1000e/ethtool.c +0
-0@@ -2063,7 +2063,7 @@ static voide1000_get_ethtool_stats(struct
net_device*netdev,Quoted 12 lines
Quoted 11 lines
???????????pm_runtime_get_sync(netdev->dev.parent); -?e1000e_get_stats64(netdev,
&net_stats); +?dev_get_stats(netdev,&net_stats); ???????????pm_runtime_put_sync(netdev->dev.parent); --
2.12.2_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lanQuoted 5 lines
Hello,
We would like to not accept this patch. Suggested generic method '
*dev_get_stats' (net/core/dev.c) calls 'ops->ndo_get_stats64' methodwhich
Quoted 3 lines
eventually calls
e1000e_get_stats64(netdev.c) - so there is same functionality. Also, see that 'e1000e_get_stats64' method innetdev.c(lineNo, it's not the same functionality because
dev_get_stats()does a memset on thertnl_link_stats64struct.Quoted 6 lines
Quoted 2 lines
5928) calls 'memset' with 0's before update statistics.? Local sanity check
I don't see any memset in
e1000e_get_stats64(). What kernel version are
you looking at?The call to memset was removed from the upstream kernel with:
-------------------------------------------------------------------
-----------------
commit5944701df90d9577658e2354cc27c4ceaeca30fe
Author: stephen hemminger <[email protected]> Date:?? Fri Jan 6 19:12:53 2017 -0800??? net: remove useless memset's in drivers
get_stats64??? In
dev_get_stats()the statistic structure storage has already been
??? zeroed. Therefore network drivers do not need to callmemset()again.
...
< changes to other drivers snipped out >
...drivers/net/ethernet/intel/e1000e/netdev.c +0
-1@@ -5925,7 +5925,6 @@ void e1000e_get_stats64(struct net_device *netdev, ?{ ??????? struct e1000_adapter *adapter = netdev_priv(netdev); -?????? memset(stats, 0, sizeof(struct rtnl_link_stats64)); ??????? spin_lock(&adapter->stats64_lock); ??????? e1000e_update_stats(adapter);??????? /* Fill out the OS statistics structure */
-------------------------------------------------------------------
-----------------
Quoted 6 lines
This also is where the bad counters start to show up for e1000e for my test systems.? From this driver on I see (very) large values for
tx_dropped,rx_over_errorsandtx_fifo_errorson driver load (even before bringing the interface up.? It seems the memset is not so useless for this driver after all.? Would simply reverting the e1000e portion of this patch resolve the issue?
Looks like Aaron beat me to the punch on pointing out that we had this very code in there before. It appears that Stephen's assertion/assumption was incorrect about the stats structure being zero'd out, which is why we are seeing the issue.
I have no issue reverting Stephen's earlier patch, or do we want to
pursue why the stats structure is not zero'd out and resolve that
instead. Either way, just want to make sure we are all on the same
page as to the right solution so that we do not end up repeating this
in the future.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170425/81a30234/attachment.asc>
PATCH 1/2 e1000e: Don't return uninitialized stats
Quoted 137 lines · On Tue, 25 Apr 2017 02:07:52 -0700 Jeff Kirsher <[email protected]>…
Quoted 124 lines · Aaron F
Quoted 84 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl. org] On
Behalf Of Benjamin Poirier
Sent: Monday, April 24, 2017 12:10 PM
To: Neftin, Sasha <[email protected]>
Cc: Kirsher at f1.synalogic.ca; Stefan Priebe <[email protected]>; netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized
statsSasha, please use reply-all to keep everyone in cc (including me...).
Quoted 4 lines · Sasha
Quoted 3 lines · Sasha
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osu osl.org]On Behalf Of Benjamin Poirier
Quoted 4 lines
Quoted 4 lines
Sent: Saturday, April 22, 2017 00:20
To: Kirsher, Jeffrey T <[email protected]>
Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org; StefanPriebe <[email protected]>
Quoted 2 lines
Quoted 2 lines
Subject: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized
stats
Quoted 2 lines
Quoted 2 lines
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors.
This leaks kernel memory to userspace and confuses users.Quoted 2 lines
Quoted 2 lines
Do like ixgbe and use
dev_get_stats()which first zeroes out
rtnl_link_stats64.Quoted 8 lines
Review
Quoted 8 lines
Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>---
??drivers/net/ethernet/intel/e1000e/ethtool.c| 2 +- ?? 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/ethernet/intel/e1000e/ethtool.cQuoted 5 lines
Quoted 5 lines
index
7aff68a4a4df..f117b90cdc2f100644drivers/net/ethernet/intel/e1000e/ethtool.c +0
-0@@ -2063,7 +2063,7 @@ static voide1000_get_ethtool_stats(struct
net_device*netdev,Quoted 12 lines
Quoted 11 lines
???????????pm_runtime_get_sync(netdev->dev.parent); -?e1000e_get_stats64(netdev,
&net_stats); +?dev_get_stats(netdev,&net_stats); ???????????pm_runtime_put_sync(netdev->dev.parent); --
2.12.2_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lanQuoted 5 lines
Hello,
We would like to not accept this patch. Suggested generic method '
*dev_get_stats' (net/core/dev.c) calls 'ops->ndo_get_stats64' methodwhich
Quoted 3 lines
eventually calls
e1000e_get_stats64(netdev.c) - so there is same functionality. Also, see that 'e1000e_get_stats64' method innetdev.c(lineNo, it's not the same functionality because
dev_get_stats()does a memset on thertnl_link_stats64struct.Quoted 6 lines
Quoted 2 lines
5928) calls 'memset' with 0's before update statistics.? Local sanity check
I don't see any memset in
e1000e_get_stats64(). What kernel version are
you looking at?The call to memset was removed from the upstream kernel with:
-------------------------------------------------------------------
-----------------
commit5944701df90d9577658e2354cc27c4ceaeca30fe
Author: stephen hemminger <[email protected]> Date:?? Fri Jan 6 19:12:53 2017 -0800??? net: remove useless memset's in drivers
get_stats64??? In
dev_get_stats()the statistic structure storage has already been
??? zeroed. Therefore network drivers do not need to callmemset()again.
...
< changes to other drivers snipped out >
...drivers/net/ethernet/intel/e1000e/netdev.c +0
-1@@ -5925,7 +5925,6 @@ void e1000e_get_stats64(struct net_device *netdev, ?{ ??????? struct e1000_adapter *adapter = netdev_priv(netdev); -?????? memset(stats, 0, sizeof(struct rtnl_link_stats64)); ??????? spin_lock(&adapter->stats64_lock); ??????? e1000e_update_stats(adapter);??????? /* Fill out the OS statistics structure */
-------------------------------------------------------------------
-----------------Quoted 6 lines
This also is where the bad counters start to show up for e1000e for my test systems.? From this driver on I see (very) large values for
tx_dropped,rx_over_errorsandtx_fifo_errorson driver load (even before bringing the interface up.? It seems the memset is not so useless for this driver after all.? Would simply reverting the e1000e portion of this patch resolve the issue?Looks like Aaron beat me to the punch on pointing out that we had this very code in there before. It appears that Stephen's assertion/assumption was incorrect about the stats structure being zero'd out, which is why we are seeing the issue.
Quoted 5 lines
I have no issue reverting Stephen's earlier patch, or do we want to pursue why the stats structure is not zero'd out and resolve that instead. Either way, just want to make sure we are all on the same page as to the right solution so that we do not end up repeating this in the future.
Lets's fix this in the base code.
From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Apr 2017 10:50:19 -0700
Subject: [PATCH net] net: always zero statistics
Drivers with 32 bit statistics API also should get zeroed statistics.
--- net/core/dev.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
net/core/dev.c +6 -5
@@ -7607,14 +7607,15 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev, {const struct net_device_ops *ops = dev->netdev_ops; -if (ops->ndo_get_stats64) { -memset (storage,0 ,sizeof (*storage)); +memset (storage,0 ,sizeof (*storage)); + +if (ops->ndo_get_stats64) ops->ndo_get_stats64 (dev, storage); - }else if (ops->ndo_get_stats) { +else if (ops->ndo_get_stats)netdev_stats_to_stats64 (storage, ops->ndo_get_stats (dev)); - }else { +else netdev_stats_to_stats64 (storage, &dev->stats); - } + storage->rx_dropped +=atomic_long_read (&dev->rx_dropped); storage->tx_dropped +=atomic_long_read (&dev->tx_dropped); storage->rx_nohandler +=atomic_long_read (&dev->rx_nohandler);
Signature · 10 lines
2.11.0 -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 833 bytes Desc: OpenPGP digital signature URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170425/635f344e/attachment.asc>
On 2017/04/25 10:54, Stephen Hemminger wrote:
[...]
Quoted 53 lines
Quoted 45 lines
Quoted 33 lines
The call to memset was removed from the upstream kernel with:
-------------------------------------------------------------------
-----------------
commit5944701df90d9577658e2354cc27c4ceaeca30fe
Author: stephen hemminger <[email protected]> Date:?? Fri Jan 6 19:12:53 2017 -0800??? net: remove useless memset's in drivers
get_stats64??? In
dev_get_stats()the statistic structure storage has already been
??? zeroed. Therefore network drivers do not need to callmemset()again.
...
< changes to other drivers snipped out >
...drivers/net/ethernet/intel/e1000e/netdev.c +0
-1@@ -5925,7 +5925,6 @@ void e1000e_get_stats64(struct net_device *netdev, ?{ ??????? struct e1000_adapter *adapter = netdev_priv(netdev); -?????? memset(stats, 0, sizeof(struct rtnl_link_stats64)); ??????? spin_lock(&adapter->stats64_lock); ??????? e1000e_update_stats(adapter);??????? /* Fill out the OS statistics structure */
-------------------------------------------------------------------
-----------------Quoted 6 lines
This also is where the bad counters start to show up for e1000e for my test systems.? From this driver on I see (very) large values for
tx_dropped,rx_over_errorsandtx_fifo_errorson driver load (even before bringing the interface up.? It seems the memset is not so useless for this driver after all.? Would simply reverting the e1000e portion of this patch resolve the issue?Looks like Aaron beat me to the punch on pointing out that we had this very code in there before. It appears that Stephen's assertion/assumption was incorrect about the stats structure being zero'd out, which is why we are seeing the issue.
Quoted 5 lines
I have no issue reverting Stephen's earlier patch, or do we want to pursue why the stats structure is not zero'd out and resolve that instead. Either way, just want to make sure we are all on the same page as to the right solution so that we do not end up repeating this in the future.
Lets's fix this in the base code.
Quoted 7 lines
From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Apr 2017 10:50:19 -0700
Subject: [PATCH net] net: always zero statisticsDrivers with 32 bit statistics API also should get zeroed statistics.
Fixes 5944701df90d ("net: remove useless memset's in drivers get_stats64")
This is probably a good change to do but it doesn't fix anything in5944701df90d, especially not the problem with e1000e.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170425/c6f78092/attachment-0001.asc>
On 2017/04/25 02:07, Jeff Kirsher wrote:
[...]
Quoted 50 lines
Quoted 38 lines
Quoted 4 lines
I don't see any memset in
e1000e_get_stats64(). What kernel version are
you looking at?The call to memset was removed from the upstream kernel with:
-------------------------------------------------------------------
-----------------
commit5944701df90d9577658e2354cc27c4ceaeca30fe
Author: stephen hemminger <[email protected]> Date:?? Fri Jan 6 19:12:53 2017 -0800??? net: remove useless memset's in drivers
get_stats64??? In
dev_get_stats()the statistic structure storage has already been
??? zeroed. Therefore network drivers do not need to callmemset()again.
...
< changes to other drivers snipped out >
...drivers/net/ethernet/intel/e1000e/netdev.c +0
-1@@ -5925,7 +5925,6 @@ void e1000e_get_stats64(struct net_device *netdev, ?{ ??????? struct e1000_adapter *adapter = netdev_priv(netdev); -?????? memset(stats, 0, sizeof(struct rtnl_link_stats64)); ??????? spin_lock(&adapter->stats64_lock); ??????? e1000e_update_stats(adapter);??????? /* Fill out the OS statistics structure */
-------------------------------------------------------------------
-----------------Quoted 6 lines
This also is where the bad counters start to show up for e1000e for my test systems.? From this driver on I see (very) large values for
tx_dropped,rx_over_errorsandtx_fifo_errorson driver load (even before bringing the interface up.? It seems the memset is not so useless for this driver after all.? Would simply reverting the e1000e portion of this patch resolve the issue?Looks like Aaron beat me to the punch on pointing out that we had this very code in there before. It appears that Stephen's assertion/assumption was incorrect about the stats structure being zero'd out, which is why we are seeing the issue.
Quoted 5 lines
I have no issue reverting Stephen's earlier patch, or do we want to pursue why the stats structure is not zero'd out and resolve that instead. Either way, just want to make sure we are all on the same page as to the right solution so that we do not end up repeating this in the future.
If you revert the e1000e part of 5944701df90d ("net: remove useless
memset's in drivers get_stats64", v4.11-rc1) it will fix the issue with
ethtool but memset will be done twice for code paths that calldev_get_stats() (sysfs, rtnl, ...). Not a big deal but this is not a
problem in the approach I initially suggested. Alternatively, we could
put a memset in e1000_get_ethtool_stats().
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170425/afd4a93b/attachment.asc>
PATCH v2 1/1 e1000e: Don't return uninitialized stats
Some statistics passed to ethtool are garbage because e1000e_get_stats64() doesn't write them, for example: tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.
Do like ixgbe and use dev_get_stats() which first zeroes out rtnl_link_stats64.
---
Notes:
Changes v1->v2:
* add the Fixes tag
drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/intel/e1000e/ethtool.c +1 -1
@@ -2072,7 +2072,7 @@ static void e1000_get_ethtool_stats(struct net_device *netdev,pm_runtime_get_sync (netdev->dev.parent); -e1000e_get_stats64 (netdev, &net_stats); +dev_get_stats (netdev, &net_stats);pm_runtime_put_sync (netdev->dev.parent);
Signature · 1 line
2.12.2
David Miller
maintainerFrom: Benjamin Poirier <[email protected]>
Date: Wed, 17 May 2017 16:24:13 -0400
Quoted 10 lines
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Do like ixgbe and use
dev_get_stats()which first zeroes outrtnl_link_stats64.Fixes 5944701df90d ("net: remove useless memset's in drivers get_stats64")Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>
Jeff, please be sure to pick this up, thanks.
Quoted 11 lines · David Miller
From: Benjamin Poirier <[email protected]>
Date: Wed, 17 May 2017 16:24:13 -0400Quoted 8 lines
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()
doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Do like ixgbe and use
dev_get_stats()which first zeroes outrtnl_link_stats64.
Quoted 6 lines
Quoted 4 lines
Fixes 5944701df90d ("net: remove useless memset's in drivers
get_stats64")Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>Jeff, please be sure to pick this up, thanks.
Yep, I have it in my tree, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20170519/af01001c/attachment.asc>
Brown, Aaron F
Tested-byQuoted 9 lines
From: Kirsher, Jeffrey T
Sent: Friday, May 19, 2017 1:17 AM
To: David Miller <[email protected]>; bpoirier at suse.com
Cc: s.priebe at profihost.ag; intel-wired-lan at lists.osuosl.org; netdev at vger.kernel.org; pmenzel at molgen.mpg.de; Neftin, Sasha <[email protected]>; Brown, Aaron F <[email protected]>; stephen at networkplumber.org
Subject: Re: [PATCH v2] e1000e: Don't return uninitialized stats
Quoted 16 lines
Quoted 15 lines · David Miller
From: Benjamin Poirier <[email protected]>
Date: Wed, 17 May 2017 16:24:13 -0400Quoted 12 lines
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()
doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Do like ixgbe and use
dev_get_stats()which first zeroes outrtnl_link_stats64.Fixes 5944701df90d ("net: remove useless memset's in drivers
get_stats64")Reported-by Stefan Priebe <[email protected]>Signed-off-by Benjamin Poirier <[email protected]>
Dear Benjamin,
Thank you for your fix.
Quoted 3 lines · Benjamin Poirier
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.
Could you please give specific examples to reproduce the issue? That way your fix can also be tested.
[?]
Kind regards,
Paul
Quoted 5 lines · Paul Menzel
Dear Benjamin,
Thank you for your fix.
Quoted 8 lines
Quoted 3 lines · Benjamin Poirier
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Could you please give specific examples to reproduce the issue? That way your fix can also be tested.
Some fields in e1000_get_ethtool_stats()'s net_stats are not initialized by e1000e_get_stats64(). The structure is allocated on the stack, therefore, the value of those fields depends on previous stack content; that in turns depends on kernel version, compiler and previous execution path. I've tried on 8 machines with different kernel versions and it reproduced on 3.
root at linux-zxe0:/usr/local/src/linux# git log -n1 --oneline fc1f8f4f310a net: ipv6: send unsolicited NA if enabled for all interfaces root at linux-zxe0:/usr/local/src/linux# ethtool -i eth0
driver: e1000e
[...]
root at linux-zxe0:/usr/local/src/linux# ethtool -S eth0
NIC statistics:
rx_packets: 217
tx_packets: 153
rx_bytes: 23091
tx_bytes: 20533
rx_broadcast: 0
tx_broadcast: 6
rx_multicast: 0
tx_multicast: 10
rx_errors: 0
tx_errors: 0
tx_dropped: 18446683600612146192
multicast: 0
collisions: 0
rx_length_errors: 0
rx_over_errors: 70364470214850
rx_crc_errors: 0
rx_frame_errors: 0
rx_no_buffer_count: 0
rx_missed_errors: 0
tx_aborted_errors: 0
tx_carrier_errors: 0
tx_fifo_errors: 18446744072101618112
tx_heartbeat_errors: 18446612150964469760[...]
(gdb) p /x 18446683600612146192
$1 = 0xffffc9000282bc10
(gdb) p /x 18446744072101618112
$2 = 0xffffffffa028e1c0
(gdb) p /x 18446612150964469760
$3 = 0xffff880457a44000
... a bunch of kernel addresses
Inserting a dummy memset is a reliable way to show the issue:
drivers/net/ethernet/intel/e1000e/ethtool.c +2 -0
@@ -2061,6 +2061,8 @@ static void e1000_get_ethtool_stats(struct net_device *netdev,int i;char *p =NULL ; +memset (&net_stats,0xff ,sizeof (net_stats)); +pm_runtime_get_sync (netdev->dev.parent);e1000e_get_stats64 (netdev, &net_stats);
root at linux-zxe0:/usr/local/src/linux# ethtool -S eth0 NIC statistics:
rx_packets: 30
tx_packets: 29
rx_bytes: 2924
tx_bytes: 3012
rx_broadcast: 0
tx_broadcast: 6
rx_multicast: 0
tx_multicast: 7
rx_errors: 0
tx_errors: 0
tx_dropped: 18446744073709551615
multicast: 0
collisions: 0
rx_length_errors: 0
rx_over_errors: 18446744073709551615
rx_crc_errors: 0
rx_frame_errors: 0
rx_no_buffer_count: 0
rx_missed_errors: 0
tx_aborted_errors: 0
tx_carrier_errors: 0
tx_fifo_errors: 18446744073709551615
tx_heartbeat_errors: 18446744073709551615[...]
(gdb) p /x 18446744073709551615
$1 = 0xffffffffffffffff
David Miller
maintainerFrom: Benjamin Poirier <[email protected]>
Date: Mon, 24 Apr 2017 12:01:51 -0700
Quoted 15 lines · Paul Menzel
Quoted 5 lines
Dear Benjamin,
Thank you for your fix.
Quoted 8 lines
Quoted 3 lines · Benjamin Poirier
Some statistics passed to ethtool are garbage because
e1000e_get_stats64()doesn't write them, for example:tx_heartbeat_errors. This leaks kernel memory to userspace and confuses users.Could you please give specific examples to reproduce the issue? That way your fix can also be tested.
Quoted 5 lines
Some fields in e1000_get_ethtool_stats()'s
net_statsare not initialized bye1000e_get_stats64(). The structure is allocated on the stack, therefore, the value of those fields depends on previous stack content; that in turns depends on kernel version, compiler and previous execution path.
I agree.
I read over these code paths earlier today and came to the same exact conclusion.