netdev - Re: [PATCH net-next v2] ethtool: add netlink based get rxfh 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 7 Nov 2022 18:25:49 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Sudheer Mogilappagari <sudheer.mogilappagari@...el.com>
Cc:     netdev@...r.kernel.org, mkubecek@...e.cz, andrew@...n.ch,
        corbet@....net, sridhar.samudrala@...el.com,
        anthony.l.nguyen@...el.com
Subject: Re: [PATCH net-next v2] ethtool: add netlink based get rxfh support

On Fri,  4 Nov 2022 16:42:44 -0700 Sudheer Mogilappagari wrote:
> Implement RXFH_GET request to get RSS table, hash key
> and hash function of an interface. This is netlink
> equivalent implementation of ETHTOOL_GRSSH ioctl request.

Motivation would be good to have, if any. Are you going to add new
fields or is it simply to fill in the implementation gap we have in
the netlink version?

> +RXFH_GET
> +========
> +
> +Get RSS table, hash key and hash function info like ``ETHTOOL_GRSSH``
> +ioctl request.


Can we describe in more detail which commands are reimplemented?
Otherwise calling the command RXFH makes little sense.
We may be better of using RSS in the name in the first place.

> +Request contents:
> +
> +=====================================  ======  ==========================
> +  ``ETHTOOL_A_RXFH_HEADER``            nested  request header
> +  ``ETHTOOL_A_RXFH_RSS_CONTEXT``       u32     context number
> + ====================================  ======  ==========================
> +
> +Kernel response contents:
> +
> +=====================================  ======  ==========================
> +  ``ETHTOOL_A_RXFH_HEADER``            nested  reply header
> +  ``ETHTOOL_A_RXFH_RSS_CONTEXT``       u32     RSS context number
> +  ``ETHTOOL_A_RXFH_INDIR_SIZE``        u32     RSS Indirection table size
> +  ``ETHTOOL_A_RXFH_KEY_SIZE``          u32     RSS hash key size
> +  ``ETHTOOL_A_RXFH_HFUNC``             u32     RSS hash func

This is u8 in the implementation, please make the implementation u32 as
documented.

> +  ``ETHTOOL_A_RXFH_RSS_CONFIG``        u32     Indir table and hkey bytes

These should really be separate attributes.

Do we even need the indir_size and key_size given every attribute has 
a length so user can just look at the length of the attrs to see the
length?

> +static int rxfh_prepare_data(const struct ethnl_req_info *req_base,
> +			     struct ethnl_reply_data *reply_base,
> +			     struct genl_info *info)
> +{
> +	struct rxfh_reply_data *data = RXFH_REPDATA(reply_base);
> +	struct net_device *dev = reply_base->dev;
> +	struct ethtool_rxfh *rxfh = &data->rxfh;
> +	struct ethnl_req_info req_info = {};
> +	struct nlattr **tb = info->attrs;
> +	u32 indir_size = 0, hkey_size = 0;
> +	const struct ethtool_ops *ops;
> +	u32 total_size, indir_bytes;
> +	bool mod = false;
> +	u8 dev_hfunc = 0;
> +	u8 *hkey = NULL;
> +	u8 *rss_config;
> +	int ret;
> +
> +	ops = dev->ethtool_ops;
> +	if (!ops->get_rxfh)
> +		return -EOPNOTSUPP;
> +
> +	ret = ethnl_parse_header_dev_get(&req_info,
> +					 tb[ETHTOOL_A_RXFH_HEADER],
> +					 genl_info_net(info), info->extack,
> +					 true);

Why are you parsing again?

You hook in ethnl_default_doit() and ethnl_default_dumpit(),
which should call the parsing for you already.

> +	if (ret < 0)
> +		return ret;
> +
> +	ethnl_update_u32(&rxfh->rss_context, tb[ETHTOOL_A_RXFH_RSS_CONTEXT],
> +			 &mod);

ethnl_update_u32() is for when you're updating the config.
You can use plain netlink helpers to get request arguments.

> +	/* Some drivers don't handle rss_context */
> +	if (rxfh->rss_context && !ops->get_rxfh_context) {
> +		ret = -EOPNOTSUPP;
> +		goto out_dev;
> +	}
> +
> +	ret = ethnl_ops_begin(dev);
> +	if (ret < 0)
> +		goto out_dev;
> +
> +	if (ops->get_rxfh_indir_size)
> +		indir_size = ops->get_rxfh_indir_size(dev);
> +	if (ops->get_rxfh_key_size)
> +		hkey_size = ops->get_rxfh_key_size(dev);
> +
> +	indir_bytes = indir_size * sizeof(rxfh->rss_config[0]);
> +	total_size = indir_bytes + hkey_size;
> +	rss_config = kzalloc(total_size, GFP_USER);

GFP_KERNEL is enough here

> +	if (!rss_config) {
> +		ret = -ENOMEM;
> +		goto out_ops;
> +	}
> +
> +	if (indir_size) {
> +		data->rss_config = (u32 *)rss_config;
> +		rxfh->indir_size = indir_size;
> +	}
> +
> +	if (hkey_size) {
> +		hkey = rss_config + indir_bytes;
> +		rxfh->key_size = hkey_size;
> +	}
> +
> +	if (rxfh->rss_context)
> +		ret = ops->get_rxfh_context(dev, data->rss_config, hkey,
> +					    &dev_hfunc, rxfh->rss_context);
> +	else
> +		ret = ops->get_rxfh(dev, data->rss_config, hkey, &dev_hfunc);

This will not be sufficient for dump, I'm afraid.

For dump we need to find a way to dump all contexts in all devices.
Which may require extending the driver API. Maybe drop the dump
implementation for now?

> +	rxfh->hfunc = dev_hfunc;
> +
> +out_ops:
> +	ethnl_ops_complete(dev);
> +out_dev:
> +	ethnl_parse_header_dev_put(&req_info);
> +	return ret;
> +}