netdev - Re: [net-next 13/15] ice: Implement ethtool ops for channels

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: <20191121144246.04adde1a@cakuba.netronome.com>
Date:   Thu, 21 Nov 2019 14:42:46 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     davem@...emloft.net, Henry Tieman <henry.w.tieman@...el.com>,
        netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        Andrew Bowers <andrewx.bowers@...el.com>
Subject: Re: [net-next 13/15] ice: Implement ethtool ops for channels

On Wed, 20 Nov 2019 23:46:10 -0800, Jeff Kirsher wrote:
> +	curr_combined = ice_get_combined_cnt(vsi);
> +
> +	/* these checks are for cases where user didn't specify a particular
> +	 * value on cmd line but we get non-zero value anyway via
> +	 * get_channels(); look at ethtool.c in ethtool repository (the user
> +	 * space part), particularly, do_schannels() routine
> +	 */
> +	if (ch->rx_count == vsi->num_rxq - curr_combined)
> +		ch->rx_count = 0;
> +	if (ch->tx_count == vsi->num_txq - curr_combined)
> +		ch->tx_count = 0;
> +	if (ch->combined_count == curr_combined)
> +		ch->combined_count = 0;
> +
> +	if (!(ch->combined_count || (ch->rx_count && ch->tx_count))) {
> +		netdev_err(dev, "Please specify at least 1 Rx and 1 Tx channel\n");
> +		return -EINVAL;
> +	}
> +
> +	new_rx = ch->combined_count + ch->rx_count;
> +	new_tx = ch->combined_count + ch->tx_count;

The combined vs individual count logic looks correct to me which is not
common, so nice to see that!

> +	if (new_rx > ice_get_max_rxq(pf)) {
> +		netdev_err(dev, "Maximum allowed Rx channels is %d\n",
> +			   ice_get_max_rxq(pf));
> +		return -EINVAL;
> +	}
> +	if (new_tx > ice_get_max_txq(pf)) {
> +		netdev_err(dev, "Maximum allowed Tx channels is %d\n",
> +			   ice_get_max_txq(pf));
> +		return -EINVAL;
> +	}
> +
> +	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
> +
> +	if (new_rx)
> +		return ice_vsi_set_dflt_rss_lut(vsi, new_rx);

But I don't see you doing a netif_is_rxfh_configured() check, which is
supposed to prevent reconfiguring the RSS indirection table if it was
set up manually by the user.

This is doubly sad, because I believe that part of RSS infrastructure
was added by Jake Keller from Intel, so it should be caught by your
internal review, not to say tests.. :(

> +	return 0;
> +}