netdev - Re: [net-next v3 13/15] iecm: Add 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]

Message-ID: <2b2a00cc0198328f1a0f3c9ccb6004a611a60011.camel@perches.com>
Date:   Thu, 25 Jun 2020 20:29:25 -0700
From:   Joe Perches <joe@...ches.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net
Cc:     Alice Michael <alice.michael@...el.com>, netdev@...r.kernel.org,
        nhorman@...hat.com, sassmann@...hat.com,
        Alan Brady <alan.brady@...el.com>,
        Phani Burra <phani.r.burra@...el.com>,
        Joshua Hay <joshua.a.hay@...el.com>,
        Madhu Chittim <madhu.chittim@...el.com>,
        Pavan Kumar Linga <pavan.kumar.linga@...el.com>,
        Donald Skidmore <donald.c.skidmore@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Sridhar Samudrala <sridhar.samudrala@...el.com>
Subject: Re: [net-next v3 13/15] iecm: Add ethtool

On Thu, 2020-06-25 at 19:07 -0700, Jeff Kirsher wrote:
> From: Alice Michael <alice.michael@...el.com>
> 
> Implement ethtool interface for the common module.
[]
> diff --git a/drivers/net/ethernet/intel/iecm/iecm_ethtool.c b/drivers/net/ethernet/intel/iecm/iecm_ethtool.c
[]
> +/* Stats associated with a Tx queue */
> +static const struct iecm_stats iecm_gstrings_tx_queue_stats[] = {
> +	IECM_QUEUE_STAT("%s-%u.packets", q_stats.tx.packets),
> +	IECM_QUEUE_STAT("%s-%u.bytes", q_stats.tx.bytes),
> +};
> +
> +static const struct iecm_stats iecm_gstrings_rx_queue_stats[] = {
> +	IECM_QUEUE_STAT("%s-%u.packets", q_stats.rx.packets),
> +	IECM_QUEUE_STAT("%s-%u.bytes", q_stats.rx.bytes),
> +	IECM_QUEUE_STAT("%s-%u.generic_csum", q_stats.rx.generic_csum),
> +	IECM_QUEUE_STAT("%s-%u.basic_csum", q_stats.rx.basic_csum),
> +	IECM_QUEUE_STAT("%s-%u.csum_err", q_stats.rx.csum_err),
> +	IECM_QUEUE_STAT("%s-%u.hsplit_buf_overflow", q_stats.rx.hsplit_hbo),
> +};
> +
> +#define IECM_TX_QUEUE_STATS_LEN		ARRAY_SIZE(iecm_gstrings_tx_queue_stats)
> +#define IECM_RX_QUEUE_STATS_LEN		ARRAY_SIZE(iecm_gstrings_rx_queue_stats)
> +
> +/**
> + * __iecm_add_stat_strings - copy stat strings into ethtool buffer
> + * @p: ethtool supplied buffer
> + * @stats: stat definitions array
> + * @size: size of the stats array
> + *
> + * Format and copy the strings described by stats into the buffer pointed at
> + * by p.
> + */
> +static void __iecm_add_stat_strings(u8 **p, const struct iecm_stats stats[],
> +				    const unsigned int size, ...)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < size; i++) {
> +		va_list args;
> +
> +		va_start(args, size);
> +		vsnprintf((char *)*p, ETH_GSTRING_LEN,
> +			  stats[i].stat_string, args);
> +		*p += ETH_GSTRING_LEN;
> +		va_end(args);
> +	}
> +}

Slightly dangerous to have a possible mismatch between the varargs
and the actual constant format spec.

Perhaps safer to use something like:

static const struct iecm_stats iecm_gstrings_tx_queue_stats[] = {
	IECM_QUEUE_STAT("packets", q_stats.tx.packets),
	IECM_QUEUE_STAT("bytes", q_stats.tx.bytes),
};

Perhaps use const char * and unsigned int instead of varargs
so this formats the output without va_start/end

	snprintf(*p, ETH_GSTRING_LEN, "%s-%u.%s", type, index, stats[i].stat_string);

> static void __iecm_add_stat_strings(u8 **p, const struct iecm_stats stats[],
> +
> +/**
> + * iecm_add_stat_strings - copy stat strings into ethtool buffer
> + * @p: ethtool supplied buffer
> + * @stats: stat definitions array
> + *
> + * Format and copy the strings described by the const static stats value into
> + * the buffer pointed at by p.
> + *
> + * The parameter @stats is evaluated twice, so parameters with side effects
> + * should be avoided. Additionally, stats must be an array such that
> + * ARRAY_SIZE can be called on it.
> + */
> +#define iecm_add_stat_strings(p, stats, ...) \
> +	__iecm_add_stat_strings(p, stats, ARRAY_SIZE(stats), ## __VA_ARGS__)
> +

>