netdev - Re: [PATCH net-next v2 6/8] bnxt_en: Implement ethtool -X to set indirection table.

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: <20200703163726.63321b67@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 3 Jul 2020 16:37:26 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Michael Chan <michael.chan@...adcom.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 6/8] bnxt_en: Implement ethtool -X to set
 indirection table.

On Fri,  3 Jul 2020 03:19:45 -0400 Michael Chan wrote:
> With the new infrastructure in place, we can now support the setting of
> the indirection table from ethtool.
> 
> When changing channels, in a rare case that firmware cannot reserve the
> rings that were promised, the RSS map will revert to default.
> 
> v2: When changing channels, if the RSS table size changes and RSS map
>     is non-default, return error.

Sorry, still not what I had in mind :(

> Signed-off-by: Michael Chan <michael.chan@...adcom.com>

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 6c90a94..0edb692 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -6061,6 +6061,10 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
>  		rx = rx_rings << 1;
>  	cp = sh ? max_t(int, tx, rx_rings) : tx + rx_rings;
>  	bp->tx_nr_rings = tx;
> +
> +	/* Reset the RSS indirection if we cannot reserve all the RX rings */
> +	if (rx_rings != bp->rx_nr_rings)
> +		bp->dev->priv_flags &= ~IFF_RXFH_CONFIGURED;

Ethtool core will check that if user set RSS table explicitly and now
changes ring count - the RSS table will not point to rings which will
no longer exist. IOW if RSS table is set we're likely increasing the
number of rings, and I'd venture to guess that the FW is not gonna give
us less rings than we previously had. So it seems like we may be
clearing the flag, and the RSS table unnecessarily here.

What I was suggesting, was that it perhaps be better to modulo the
number or rings in __bnxt_fill_hw_rss_tbl* by the actual table size,
but leave the user-set RSS table in place. That'd be the lowest LoC.

Also I still think that there should be a warning printed when FW gave
us less rings than expected.

>  	bp->rx_nr_rings = rx_rings;
>  	bp->cp_nr_rings = cp;
>  
> @@ -8265,7 +8269,8 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  			rc = bnxt_init_int_mode(bp);
>  		bnxt_ulp_irq_restart(bp, rc);
>  	}
> -	bnxt_set_dflt_rss_indir_tbl(bp);
> +	if (!netif_is_rxfh_configured(bp->dev))
> +		bnxt_set_dflt_rss_indir_tbl(bp);
>  
>  	if (rc) {
>  		netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 46f3978..9098818 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -926,6 +926,13 @@ static int bnxt_set_channels(struct net_device *dev,
>  		return rc;
>  	}
>  
> +	if (bnxt_get_nr_rss_ctxs(bp, req_rx_rings) !=
> +	    bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings) &&
> +	    (dev->priv_flags & IFF_RXFH_CONFIGURED)) {

In this case copy the old values over and zero-fill the new rings.

> +		netdev_warn(dev, "RSS table size change required, RSS table entries must be default to proceed\n");
> +		return -EINVAL;
> +	}
> +
>  	if (netif_running(dev)) {
>  		if (BNXT_PF(bp)) {
>  			/* TODO CHIMP_FW: Send message to all VF's
> @@ -1315,6 +1322,35 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
>  	return 0;
>  }
>  
> +static int bnxt_set_rxfh(struct net_device *dev, const u32 *indir,
> +			 const u8 *key, const u8 hfunc)
> +{
> +	struct bnxt *bp = netdev_priv(dev);
> +	int rc = 0;
> +
> +	if (hfunc && hfunc != ETH_RSS_HASH_TOP)
> +		return -EOPNOTSUPP;
> +
> +	if (key)
> +		return -EOPNOTSUPP;
> +
> +	if (indir) {
> +		u32 i, pad, tbl_size = bnxt_get_rxfh_indir_size(dev);
> +
> +		for (i = 0; i < tbl_size; i++)
> +			bp->rss_indir_tbl[i] = indir[i];
> +		pad = bp->rss_indir_tbl_entries - tbl_size;
> +		if (pad)
> +			memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u16));
> +	}

I see in patch 4 there is a:

	bool no_rss = !(vnic->flags & BNXT_VNIC_RSS_FLAG);

Should there be some check here to only allow users to change the
indirection table if RSS is supported / allowed?