| 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
| ||
|
Message-ID: <20221123195422.bjvm7y7i3pmtbhag@lion.mk-sys.cz>
Date: Wed, 23 Nov 2022 20:54:22 +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 v5] ethtool: add netlink based get rss support
On Wed, Nov 23, 2022 at 10:48:46AM -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. In addition ETHTOOL_A_RSS_RINGS
> attribute is added to return queue/rings count to user space.
> This simplifies user space implementation while maintaining
> backward compatibility of output.
>
> 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>
> ---
> v5:
> -Updated documentation about ETHTOOL_A_RSS_RINGS attribute.
>
> v4:
> -Don't make context parameter mandatory.
> -Remove start/done ethtool_genl_ops for RSS_GET.
> -Add rings attribute to RSS_GET netlink message.
> -Fix documentation.
>
> 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.
> ---
> Documentation/networking/ethtool-netlink.rst | 29 +++-
> include/uapi/linux/ethtool_netlink.h | 15 ++
> net/ethtool/Makefile | 2 +-
> net/ethtool/netlink.c | 7 +
> net/ethtool/netlink.h | 2 +
> net/ethtool/rss.c | 169 +++++++++++++++++++
> 6 files changed, 222 insertions(+), 2 deletions(-)
> create mode 100644 net/ethtool/rss.c
>
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index bede24ef44fd..883555b8876b 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
[...]
> @@ -1687,6 +1689,31 @@ to control PoDL PSE Admin functions. This option is implementing
> ``IEEE 802.3-2018`` 30.15.1.2.1 acPoDLPSEAdminControl. See
> ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` for supported values.
>
> +RSS_GET
> +=======
> +
> +Get indirection table, hash key and hash function info associated with a
> +RSS context of an interface similar to ``ETHTOOL_GRSSH`` ioctl request.
> +In addition ring count information is also returned to user space.
> +
> +Request contents:
> +
> +===================================== ====== ==========================
> + ``ETHTOOL_A_RSS_HEADER`` nested request header
> + ``ETHTOOL_A_RSS_CONTEXT`` u32 context number
> + ==================================== ====== ==========================
> +
> +Kernel response contents:
> +
> +===================================== ====== ==========================
> + ``ETHTOOL_A_RSS_HEADER`` nested reply header
> + ``ETHTOOL_A_RSS_CONTEXT`` u32 RSS context number
> + ``ETHTOOL_A_RSS_HFUNC`` u32 RSS hash func
> + ``ETHTOOL_A_RSS_RINGS`` u32 Ring count
> + ``ETHTOOL_A_RSS_INDIR`` binary Indir table bytes
> + ``ETHTOOL_A_RSS_HKEY`` binary Hash key bytes
> + ==================================== ====== ==========================
It would be helpful to have some basic information about the attributes
here, e.g. a mention that ETHTOOL_A_RSS_CONTEXT is optional, the format
of ETHTOOL_A_RSS_INDIR and ETHTOOL_A_RSS_HKEY or which constants are
returned in ETHTOOL_A_RSS_HFUNC.
> +
> Request translation
> ===================
>
> @@ -1768,7 +1795,7 @@ are netlink only.
> ``ETHTOOL_GMODULEEEPROM`` ``ETHTOOL_MSG_MODULE_EEPROM_GET``
> ``ETHTOOL_GEEE`` ``ETHTOOL_MSG_EEE_GET``
> ``ETHTOOL_SEEE`` ``ETHTOOL_MSG_EEE_SET``
> - ``ETHTOOL_GRSSH`` n/a
> + ``ETHTOOL_GRSSH`` ``ETHTOOL_MSG_RSS_GET``
> ``ETHTOOL_SRSSH`` n/a
> ``ETHTOOL_GTUNABLE`` n/a
> ``ETHTOOL_STUNABLE`` n/a
Now that ETHTOOL_MSG_RSS_GET returns also the number of Rx rings,
ETHTOOL_GRXRINGS should also map to it.
> diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
> new file mode 100644
> index 000000000000..86b9e0de954c
> --- /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]) {
> + request->rss_context = 0;
> + return 0;
> + }
> +
> + request->rss_context = nla_get_u32(tb[ETHTOOL_A_RSS_CONTEXT]);
> + return 0;
> +}
Just a note: the request structure is guaranteed to be zeroed so that
if (tb[ETHTOOL_A_RSS_CONTEXT])
request->rss_context = nla_get_u32(tb[ETHTOOL_A_RSS_CONTEXT]);
return 0;
would suffice. But it can stay like this if you prefer.
> +
> +static int
> +rss_prepare_data(const struct ethnl_req_info *req_base,
> + struct ethnl_reply_data *reply_base, struct genl_info *info)
> +{
> + struct rss_reply_data *data = RSS_REPDATA(reply_base);
> + struct rss_req_info *request = RSS_REQINFO(req_base);
> + struct net_device *dev = reply_base->dev;
> + struct ethtool_rxnfc rings;
> + const struct ethtool_ops *ops;
> + u32 total_size, indir_bytes;
> + u8 dev_hfunc = 0;
> + u8 *rss_config;
> + int ret;
> +
> + ops = dev->ethtool_ops;
> + if (!ops->get_rxfh)
> + return -EOPNOTSUPP;
> +
> + /* Some drivers don't handle rss_context */
> + if (request->rss_context && !ops->get_rxfh_context)
> + return -EOPNOTSUPP;
> +
> + data->rss_context = request->rss_context;
> +
> + ret = ethnl_ops_begin(dev);
> + if (ret < 0)
> + return ret;
> +
> + data->indir_size = 0;
> + data->hkey_size = 0;
> + if (ops->get_rxfh_indir_size)
> + data->indir_size = ops->get_rxfh_indir_size(dev);
> + if (ops->get_rxfh_key_size)
> + data->hkey_size = ops->get_rxfh_key_size(dev);
> +
> + indir_bytes = data->indir_size * sizeof(u32);
> + total_size = indir_bytes + data->hkey_size;
> + rss_config = kzalloc(total_size, GFP_KERNEL);
> + if (!rss_config) {
> + ret = -ENOMEM;
> + goto out_ops;
> + }
> +
> + if (data->indir_size)
> + data->indir_table = (u32 *)rss_config;
> +
> + if (data->hkey_size)
> + data->hkey = rss_config + indir_bytes;
> +
> + if (data->rss_context)
> + ret = ops->get_rxfh_context(dev, data->indir_table, data->hkey,
> + &dev_hfunc, data->rss_context);
> + else
> + ret = ops->get_rxfh(dev, data->indir_table, data->hkey,
> + &dev_hfunc);
> +
> + if (ret)
> + goto out_ops;
> +
> + data->hfunc = dev_hfunc;
> + rings.cmd = ETHTOOL_GRXRINGS;
> + ops->get_rxnfc(dev, &rings, NULL);
> + data->rings = rings.data;
ops->get_rxnfc is not checked for null.
> +
> +out_ops:
> + ethnl_ops_complete(dev);
> + return ret;
> +}
> +
> +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)) + /* _RSS_RINGS */
> + nla_total_size(sizeof(u32)) * data->indir_size + /* _RSS_INDIR */
As discussed before, nla_total_size() counts also the netlink attribute
header so that nla_total_size(sizeof(u32)) is 8, not 4. Therefore we
need
nla_total_size(sizeof(u32) * data->indir_size)
instead. Fortunately it's incorrect "in the good direction"
(overestimating the length) but I would still prefer to have the correct
estimate.
> + data->hkey_size; /* _RSS_HKEY */
This should be nla_total_size(data->hkey_size) to count the attribute
header and potential padding.
> +
> + return len;
> +}
> +
> +static int
> +rss_fill_reply(struct sk_buff *skb, const struct ethnl_req_info *req_base,
> + const struct ethnl_reply_data *reply_base)
> +{
> + const struct rss_reply_data *data = RSS_REPDATA(reply_base);
> +
> + if (nla_put_u32(skb, ETHTOOL_A_RSS_CONTEXT, data->rss_context) ||
> + nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc) ||
> + nla_put_u32(skb, ETHTOOL_A_RSS_RINGS, data->rings) ||
> + nla_put(skb, ETHTOOL_A_RSS_INDIR,
> + sizeof(u32) * data->indir_size, data->indir_table) ||
> + nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data->hkey))
> + return -EMSGSIZE;
The optional attributes should be only added if they are to be returned.
Michal
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists