netdev - Re: [PATCH 5/5] alx: add stats to ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 2 Jan 2014 19:19:15 +0100
From:	Sabrina Dubroca <sd@...asysnail.net>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH 5/5] alx: add stats to ethtool

[2014-01-02, 14:09:14] Johannes Berg wrote:
> On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:
> 
> > +static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = {
> 
> You should probably have a comment here and on the stats struct
> declaration that they must absolutely match in order/size/etc.


With these comments, and something similar before
__alx_update_hw_stats, should I use the code for __alx_update_hw_stats
mentionned in mail 0?


/* Statistics counters collected by the MAC
 *
 * The order of the fields must match the strings in alx_gstrings_stats
 * See ethtool.c
 */
struct alx_hw_stats {
	/* ... */
}

/* The order of these strings must match the order of the fields in
 * struct alx_hw_stats
 * See hw.h
 */
static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = {
	/* ... */
}



> Maybe try to put in some BUILD_BUG_ON() as well, at least checking the
> sizeof() vs. ARRAY_SIZE*sizeof(u64) - that might already have caught the
> bug that Ben pointed out.


static void alx_get_ethtool_stats(struct net_device *netdev,
				  struct ethtool_stats *estats, u64 *data)
{
	struct alx_priv *alx = netdev_priv(netdev);
	struct alx_hw *hw = &alx->hw;

	spin_lock(&alx->stats_lock);

	__alx_update_hw_stats(hw);
	BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) <
		     ALX_NUM_STATS * sizeof(u64));
	memcpy(data, &hw->stats.rx_ok, ALX_NUM_STATS * sizeof(u64));

	spin_unlock(&alx->stats_lock);
}

This way, rx_ok doesn't need to come first in struct alx_hw_stats, and
the size of the structure is checked as you said.



> > +	"rx_packets",
> 
> Is it useful to provide stats that are already elsewhere? Then again, it
> doesn't really hurt and simplifies the code ...

You're right, rx_packets is the sum of all the rx_SIZE_RANGE_packets
and is redundant information. The values split by size range are only
for ethtool. ndo_get_stats(64) uses only the sum. I think this
information is more relevant to the user, and anyway, it's available,
so we might as well provide it.


Thanks,

-- 
Sabrina Dubroca
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html