netdev - Re: [PATCH net-next v3] ethtool: add netlink based get rss 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]

Message-ID: <20221117031230.daicjtkix6kjuhsz@lion.mk-sys.cz>
Date:   Thu, 17 Nov 2022 04:12:30 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Sudheer Mogilappagari <sudheer.mogilappagari@...el.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, andrew@...n.ch,
        corbet@....net, sridhar.samudrala@...el.com,
        anthony.l.nguyen@...el.com
Subject: Re: [PATCH net-next v3] ethtool: add netlink based get rss support

On Wed, Nov 16, 2022 at 03:25:54PM -0800, Sudheer Mogilappagari wrote:
> Add netlink based support for "ethtool -x <dev> [context x]"
> command by implementing ETHTOOL_MSG_RSS_GET netlink message.
> This is equivalent to functionality provided via ETHTOOL_GRSSH
> in ioctl path. It fetches RSS table, hash key and hash function
> of an interface to user space.
> 
> This patch implements existing functionality available
> in ioctl path and enables addition of new RSS context
> based parameters in future.
> 
> Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@...el.com>
> ---
> v3:
> -Define parse_request and make use of ethnl_default_parse.
> -Have indir table and hask hey as seprate attributes.
> -Remove dumpit op for RSS_GET.
> -Use RSS instead of RXFH.
> 
> v2: Fix cleanup in error path instead of returning.
> ---
[...]
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index aaf7c6963d61..ad837f034ac3 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -51,6 +51,7 @@ enum {
>  	ETHTOOL_MSG_MODULE_SET,
>  	ETHTOOL_MSG_PSE_GET,
>  	ETHTOOL_MSG_PSE_SET,
> +	ETHTOOL_MSG_RSS_GET,
>  
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_USER_CNT,
> @@ -97,6 +98,7 @@ enum {
>  	ETHTOOL_MSG_MODULE_GET_REPLY,
>  	ETHTOOL_MSG_MODULE_NTF,
>  	ETHTOOL_MSG_PSE_GET_REPLY,
> +	ETHTOOL_MSG_RSS_GET_REPLY,
>  
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_KERNEL_CNT,
> @@ -880,6 +882,18 @@ enum {
>  	ETHTOOL_A_PSE_MAX = (__ETHTOOL_A_PSE_CNT - 1)
>  };
>  
> +enum {
> +	ETHTOOL_A_RSS_UNSPEC,
> +	ETHTOOL_A_RSS_HEADER,
> +	ETHTOOL_A_RSS_CONTEXT,		/* u32 */
> +	ETHTOOL_A_RSS_HFUNC,		/* u32 */
> +	ETHTOOL_A_RSS_INDIR,		/* array */
> +	ETHTOOL_A_RSS_HKEY,		/* array */

These two are binaries, not arrays.

> +
> +	__ETHTOOL_A_RSS_CNT,
> +	ETHTOOL_A_RSS_MAX = (__ETHTOOL_A_RSS_CNT - 1),
> +};
> +
>  /* generic netlink info */
>  #define ETHTOOL_GENL_NAME "ethtool"
>  #define ETHTOOL_GENL_VERSION 1
[...]
> diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
> new file mode 100644
> index 000000000000..f4a700db3e9b
> --- /dev/null
> +++ b/net/ethtool/rss.c
[...]
> +static int
> +rss_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
> +		  struct netlink_ext_ack *extack)
> +{
> +	struct rss_req_info *request = RSS_REQINFO(req_info);
> +
> +	if (!tb[ETHTOOL_A_RSS_CONTEXT])
> +		return -EINVAL;

Unlike with ioctl, we do not need to represent "no context" with
a special value of zero. It would be IMHO cleaner to represent request
and reply without context (which should be the most frequent case,
AFAICS) by absence of the ETHTOOL_A_RSS_CONTEXT attribute.

> +
> +	request->rss_context = nla_get_u32(tb[ETHTOOL_A_RSS_CONTEXT]);
> +	return 0;
> +}
[...]
> +static int
> +rss_reply_size(const struct ethnl_req_info *req_base,
> +	       const struct ethnl_reply_data *reply_base)
> +{
> +	const struct rss_reply_data *data = RSS_REPDATA(reply_base);
> +	int len;
> +
> +	len =  nla_total_size(sizeof(u32)) +	/* _RSS_CONTEXT */
> +	       nla_total_size(sizeof(u32)) +	/* _RSS_HFUNC */
> +	       nla_total_size(sizeof(u32)) * data->indir_size + /* _RSS_INDIR */

A nitpick: you pad the whole attribute, not each u32 separately, so that
it would make more sense to write this as

	 nla_total_size(sizeof(u32) * data->indir_size)

(The result will be the same, of course.)

Michal

> +	       data->hkey_size;			/* _RSS_HKEY */
> +
> +	return len;
> +}

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)