[Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
Stephen Hemminger
stephen at networkplumber.org
Tue Apr 25 17:54:05 UTC 2017
On Tue, 25 Apr 2017 02:07:52 -0700
Jeff Kirsher <jeffrey.t.kirsher at intel.com> wrote:
> On Tue, 2017-04-25 at 07:10 +0000, Brown, Aaron F wrote:
> > > 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 <sasha.neftin at intel.com>
> > > Cc: Kirsher at f1.synalogic.ca; Stefan Priebe <s.priebe at profihost.ag>;
> > > 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
> > > 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 at lists.osu
> > > > > osl.org]
> > >
> > > On Behalf Of Benjamin Poirier
> > > > > Sent: Saturday, April 22, 2017 00:20
> > > > > To: Kirsher, Jeffrey T <jeffrey.t.kirsher at intel.com>
> > > > > Cc: netdev at vger.kernel.org; intel-wired-lan at lists.osuosl.org;
> > > > > Stefan
> > >
> > > Priebe <s.priebe at 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 at profihost.ag>
> > > > > Signed-off-by: Benjamin Poirier <bpoirier at 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 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
> > >
> > > 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 at 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 at 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")
Signed-off-by: Stephen Hemminger <sthemmin at microsoft.com>
---
net/core/dev.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 533a6d6f6092..07d27d3feb37 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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);
--
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>
More information about the Intel-wired-lan
mailing list