netdev - Re: [PATCH net-next 2/2] gve: Add RSS adminq commands and ethtool support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240802162131.0eb94666@kernel.org>
Date: Fri, 2 Aug 2024 16:21:31 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Praveen Kaligineedi <pkaligineedi@...gle.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, willemb@...gle.com, jeroendb@...gle.com,
 shailend@...gle.com, hramamurthy@...gle.com, jfraker@...gle.com, Ziwei Xiao
 <ziweixiao@...gle.com>
Subject: Re: [PATCH net-next 2/2] gve: Add RSS adminq commands and ethtool
 support

On Thu,  1 Aug 2024 18:28:34 -0700 Praveen Kaligineedi wrote:
> +static int gve_set_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh,
> +			struct netlink_ext_ack *extack)
> +{
> +	struct gve_priv *priv = netdev_priv(netdev);
> +	struct gve_rss_config rss_config = {0};

I never remember the exact rules, are you sure this is the one that 
is guaranteed per standard to zero all the fields?

> +	u32 *indir = rxfh->indir;
> +	u8 hfunc = rxfh->hfunc;
> +	u8 *key = rxfh->key;
> +	int err = 0;
> +
> +	if (!priv->rss_key_size || !priv->rss_lut_size)
> +		return -EOPNOTSUPP;
> +
> +	switch (hfunc) {
> +	case ETH_RSS_HASH_NO_CHANGE:
> +		break;
> +	case ETH_RSS_HASH_TOP:
> +		rss_config.hash_alg = ETH_RSS_HASH_TOP;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (key) {
> +		rss_config.hash_key = kvcalloc(priv->rss_key_size,
> +					       sizeof(*rss_config.hash_key), GFP_KERNEL);

key is a bitstream...
IOW this code is like allocating a string with 
	calloc(length, sizeof(char)) 
:S

But what is the point of the allocations in this function in the first
place? You kvcalloc here, copy, call gve_adminq_configure_rss()
which dma_alloc_coherent() and copies into that, again.

> +		if (!rss_config.hash_key)
> +			return -ENOMEM;
> +
> +		memcpy(rss_config.hash_key, key, priv->rss_key_size * sizeof(*key));
> +	}
> +
> +	if (indir) {
> +		rss_config.hash_lut = kvcalloc(priv->rss_lut_size,
> +					       sizeof(*rss_config.hash_lut), GFP_KERNEL);
> +		if (!rss_config.hash_lut) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +
> +		memcpy(rss_config.hash_lut, indir, priv->rss_lut_size * sizeof(*indir));
> +	}
> +
> +	err = gve_adminq_configure_rss(priv, &rss_config);
> +
> +out:
> +	kvfree(rss_config.hash_lut);
> +	kvfree(rss_config.hash_key);
> +	return err;
> +}