[PATCH 1/2] e1000e: Don't return uninitialized stats

[PATCH 1/2] e1000e: Don't return uninitialized stats

[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.

Do like ixgbe and use dev_get_stats() which first zeroes out
rtnl_link_stats64.

Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 7aff68a4a4df..f117b90cdc2f 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -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

[PATCH 2/2] igb: Remove useless argument

[Benjamin Poirier]

Given that all callers of igb_update_stats() pass the same two arguments:
(adapter, &adapter->stats64), the second argument can be removed.

Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---
 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(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index acbc3abe2ddd..3f0c06847fc2 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -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 *);
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 737b664d004c..8c913958c2eb 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -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;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index be456bae8169..20da5e9d9d3c 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -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;
-- 
2.12.2

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Neftin, Sasha]

On 4/23/2017 15:53, Neftin, Sasha wrote:
> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Benjamin Poirier
> Sent: Saturday, April 22, 2017 00:20
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Stefan Priebe <s.priebe@profihost.ag>
> Subject: [Intel-wired-lan] [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.
>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
>   drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index 7aff68a4a4df..f117b90cdc2f 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -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
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@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

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Benjamin Poirier]

Sasha, please use reply-all to keep everyone in cc (including me...).

On 2017/04/24 11:17, Neftin, Sasha wrote:
> On 4/23/2017 15:53, Neftin, Sasha wrote:
> > -----Original Message-----
> > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Benjamin Poirier
> > Sent: Saturday, April 22, 2017 00:20
> > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Stefan Priebe <s.priebe@profihost.ag>
> > Subject: [Intel-wired-lan] [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.
> > 
> > Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > ---
> >   drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > index 7aff68a4a4df..f117b90cdc2f 100644
> > --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > @@ -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
> > 
> > _______________________________________________
> > Intel-wired-lan mailing list
> > Intel-wired-lan@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

No, it's not the same functionality because dev_get_stats() does a
memset on the rtnl_link_stats64 struct.

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

> in our lab shows 'tx_heartbeat_errors' counter reported as 0.
> 

Please see the mail I just sent to Paul Menzel <pmenzel@molgen.mpg.de>
for more information about the issue and how to reproduce it.

RE: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Benjamin Poirier
> Sent: Monday, April 24, 2017 12:10 PM
> To: Neftin, Sasha <sasha.neftin@intel.com>
> Cc: Kirsher@f1.synalogic.ca; Stefan Priebe <s.priebe@profihost.ag>;
> netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized
> stats
> 
> Sasha, please use reply-all to keep everyone in cc (including me...).
> 
> On 2017/04/24 11:17, Neftin, Sasha wrote:
> > On 4/23/2017 15:53, Neftin, Sasha wrote:
> > > -----Original Message-----
> > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org]
> On Behalf Of Benjamin Poirier
> > > Sent: Saturday, April 22, 2017 00:20
> > > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > > Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Stefan
> Priebe <s.priebe@profihost.ag>
> > > Subject: [Intel-wired-lan] [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.
> > >
> > > Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > ---
> > >   drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c
> b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > index 7aff68a4a4df..f117b90cdc2f 100644
> > > --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > @@ -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
> > >
> > > _______________________________________________
> > > Intel-wired-lan mailing list
> > > Intel-wired-lan@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
> 
> No, it's not the same functionality because dev_get_stats() does a
> memset on the rtnl_link_stats64 struct.
> 
> > 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 <stephen@networkplumber.org>
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 >
...
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/int
index 723025b..79651eb 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -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?

> 
> > in our lab shows 'tx_heartbeat_errors' counter reported as 0.
> >
> 
> Please see the mail I just sent to Paul Menzel <pmenzel@molgen.mpg.de>
> for more information about the issue and how to reproduce it.
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 5660 bytes --]

On Tue, 2017-04-25 at 07:10 +0000, Brown, Aaron F wrote:
> > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.
> > org] On
> > Behalf Of Benjamin Poirier
> > Sent: Monday, April 24, 2017 12:10 PM
> > To: Neftin, Sasha <sasha.neftin@intel.com>
> > Cc: Kirsher@f1.synalogic.ca; Stefan Priebe <s.priebe@profihost.ag>;
> > netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org
> > Subject: Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return
> > uninitialized
> > stats
> > 
> > Sasha, please use reply-all to keep everyone in cc (including
> > me...).
> > 
> > On 2017/04/24 11:17, Neftin, Sasha wrote:
> > > On 4/23/2017 15:53, Neftin, Sasha wrote:
> > > > -----Original Message-----
> > > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osu
> > > > osl.org]
> > 
> > On Behalf Of Benjamin Poirier
> > > > Sent: Saturday, April 22, 2017 00:20
> > > > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> > > > Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org;
> > > > Stefan
> > 
> > Priebe <s.priebe@profihost.ag>
> > > > Subject: [Intel-wired-lan] [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.
> > > > 
> > > > Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> > > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > > ---
> > > >    drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > 
> > b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > > index 7aff68a4a4df..f117b90cdc2f 100644
> > > > --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > > +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> > > > @@ -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
> > > > 
> > > > _______________________________________________
> > > > Intel-wired-lan mailing list
> > > > Intel-wired-lan@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
> > 
> > No, it's not the same functionality because dev_get_stats() does a
> > memset on the rtnl_link_stats64 struct.
> > 
> > > 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 <stephen@networkplumber.org>
> 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 >
> ...
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> b/drivers/net/ethernet/int
> index 723025b..79651eb 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -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?

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Benjamin Poirier]

[-- Attachment #1: Type: text/plain, Size: 2874 bytes --]

On 2017/04/25 02:07, Jeff Kirsher wrote:
[...]
> > > 
> > > 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 <stephen@networkplumber.org>
> > 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 >
> > ...
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> > b/drivers/net/ethernet/int
> > index 723025b..79651eb 100644
> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > @@ -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?
> 
> 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.

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 call
dev_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().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] e1000e: Don't return uninitialized stats

[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.

Do like ixgbe and use dev_get_stats() which first zeroes out
rtnl_link_stats64.

Fixes: 5944701df90d ("net: remove useless memset's in drivers get_stats64")
Reported-by: Stefan Priebe <s.priebe@profihost.ag>
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
---

Notes:
    Changes v1->v2:
    * add the Fixes tag

 drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index e23dbd9190d6..5b4d97570896 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -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);
 
-- 
2.12.2

Re: [PATCH v2] e1000e: Don't return uninitialized stats

[David Miller]

From: Benjamin Poirier <bpoirier@suse.com>
Date: Wed, 17 May 2017 16:24:13 -0400

> 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.
> 
> Fixes: 5944701df90d ("net: remove useless memset's in drivers get_stats64")
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>

Jeff, please be sure to pick this up, thanks.

Re: [PATCH v2] e1000e: Don't return uninitialized stats

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

On Thu, 2017-05-18 at 10:46 -0400, David Miller wrote:
> From: Benjamin Poirier <bpoirier@suse.com>
> Date: Wed, 17 May 2017 16:24:13 -0400
> 
> > 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.
> > 
> > Fixes: 5944701df90d ("net: remove useless memset's in drivers
> > get_stats64")
> > Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> 
> Jeff, please be sure to pick this up, thanks.

Yep, I have it in my tree, thanks.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2] e1000e: Don't return uninitialized stats

[Brown, Aaron F]

> From: Kirsher, Jeffrey T
> Sent: Friday, May 19, 2017 1:17 AM
> To: David Miller <davem@davemloft.net>; bpoirier@suse.com
> Cc: s.priebe@profihost.ag; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; pmenzel@molgen.mpg.de; Neftin, Sasha
> <sasha.neftin@intel.com>; Brown, Aaron F <aaron.f.brown@intel.com>;
> stephen@networkplumber.org
> Subject: Re: [PATCH v2] e1000e: Don't return uninitialized stats
> 
> On Thu, 2017-05-18 at 10:46 -0400, David Miller wrote:
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Date: Wed, 17 May 2017 16:24:13 -0400
> >
> > > 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.
> > >
> > > Fixes: 5944701df90d ("net: remove useless memset's in drivers
> > > get_stats64")
> > > Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Benjamin Poirier]

[-- Attachment #1: Type: text/plain, Size: 2940 bytes --]

On 2017/04/25 10:54, Stephen Hemminger wrote:
[...]
> > > The call to memset was removed from the upstream kernel with:
> > > -------------------------------------------------------------------
> > > -----------------
> > > commit 5944701df90d9577658e2354cc27c4ceaeca30fe
> > > Author: stephen hemminger <stephen@networkplumber.org>
> > > 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 >
> > > ...
> > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> > > b/drivers/net/ethernet/int
> > > index 723025b..79651eb 100644
> > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > > @@ -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?  
> > 
> > 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.
> 
> Lets's fix this in the base code.
> 
> From: Stephen Hemminger <sthemmin@microsoft.com>
> 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.
> 
> 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 in
5944701df90d, especially not the problem with e1000e.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Paul Menzel]

Dear Benjamin,


Thank you for your fix.

On 04/21/17 23:20, Benjamin Poirier wrote:
> 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

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[Benjamin Poirier]

On 2017/04/24 10:23, Paul Menzel wrote:
> Dear Benjamin,
> 
> 
> Thank you for your fix.
> 
> On 04/21/17 23:20, Benjamin Poirier wrote:
> > 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@linux-zxe0:/usr/local/src/linux# git log -n1 --oneline
fc1f8f4f310a net: ipv6: send unsolicited NA if enabled for all interfaces
root@linux-zxe0:/usr/local/src/linux# ethtool -i eth0
driver: e1000e
[...]
root@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:

--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -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@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

Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats

[David Miller]

From: Benjamin Poirier <bpoirier@suse.com>
Date: Mon, 24 Apr 2017 12:01:51 -0700

> On 2017/04/24 10:23, Paul Menzel wrote:
>> Dear Benjamin,
>> 
>> 
>> Thank you for your fix.
>> 
>> On 04/21/17 23:20, Benjamin Poirier wrote:
>> > 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 agree.

I read over these code paths earlier today and came to the same exact
conclusion.