Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
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
> > > 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
> 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.
signature.asc
Description: Digital signature
Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
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
> > 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().
signature.asc
Description: Digital signature
Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
On Tue, 2017-04-25 at 07:10 +, 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
> > Cc: kirs...@f1.synalogic.ca; Stefan Priebe ;
> > netdev@vger.kernel.org; intel-wired-...@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-boun...@lists.osu
> > > > osl.org]
> >
> > On Behalf Of Benjamin Poirier
> > > > Sent: Saturday, April 22, 2017 00:20
> > > > To: Kirsher, Jeffrey T
> > > > Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org;
> > > > Stefan
> >
> > Priebe
> > > > 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
> > > > Signed-off-by: Benjamin Poirier
> > > > ---
> > > > 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-...@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
> 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_adapte
RE: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@lists.osuosl.org] On
> Behalf Of Benjamin Poirier
> Sent: Monday, April 24, 2017 12:10 PM
> To: Neftin, Sasha
> Cc: kirs...@f1.synalogic.ca; Stefan Priebe ;
> netdev@vger.kernel.org; intel-wired-...@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-boun...@lists.osuosl.org]
> On Behalf Of Benjamin Poirier
> > > Sent: Saturday, April 22, 2017 00:20
> > > To: Kirsher, Jeffrey T
> > > Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org; Stefan
> Priebe
> > > 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
> > > Signed-off-by: Benjamin Poirier
> > > ---
> > > 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-...@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
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
> for more information about the issue and how to reproduce it.
> ___
> Intel-wired-lan mailing list
> intel-wired-...@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
From: Benjamin Poirier 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.
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-boun...@lists.osuosl.org] On > > Behalf Of Benjamin Poirier > > Sent: Saturday, April 22, 2017 00:20 > > To: Kirsher, Jeffrey T > > Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org; Stefan Priebe > > > > 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 > > Signed-off-by: Benjamin Poirier > > --- > > 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-...@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 for more information about the issue and how to reproduce it.
Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
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 = 0xc9000282bc10 (gdb) p /x 18446744072101618112 $2 = 0xa028e1c0 (gdb) p /x 18446612150964469760 $3 = 0x880457a44000 ... 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 = 0x
Re: [Intel-wired-lan] [PATCH 1/2] e1000e: Don't return uninitialized stats
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
On 4/23/2017 15:53, Neftin, Sasha wrote: -Original Message- From: Intel-wired-lan [mailto:intel-wired-lan-boun...@lists.osuosl.org] On Behalf Of Benjamin Poirier Sent: Saturday, April 22, 2017 00:20 To: Kirsher, Jeffrey T Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org; Stefan Priebe 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 Signed-off-by: Benjamin Poirier --- 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-...@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
[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 Signed-off-by: Benjamin Poirier --- 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
