| 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: <cd1af256-1447-4b94-8cf5-8e41014f7bad@nvidia.com>
Date: Sun, 13 Jul 2025 14:08:48 +0300
From: Gal Pressman <gal@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
andrew+netdev@...n.ch, horms@...nel.org, donald.hunter@...il.com,
shuah@...nel.org, kory.maincent@...tlin.com, maxime.chevallier@...tlin.com,
sdf@...ichev.me, ecree.xilinx@...il.com
Subject: Re: [PATCH net-next 01/11] ethtool: rss: initial RSS_SET (indirection
table handling)
On 11/07/2025 4:52, Jakub Kicinski wrote:
> Add initial support for RSS_SET, for now only operations on
> the indirection table are supported.
>
> There are two special cases here:
> 1) resetting the table to defaults;
> 2) support for tables of different size.
>
> For (1) I use an empty Netlink attribute (array of size 0).
Makes sense.
> static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
> diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
> index 41ab9fc67652..7167fc3c27a0 100644
> --- a/net/ethtool/rss.c
> +++ b/net/ethtool/rss.c
> @@ -218,6 +218,8 @@ rss_prepare(const struct rss_req_info *request, struct net_device *dev,
> {
> rss_prepare_flow_hash(request, dev, data, info);
>
> + if (!dev->ethtool_ops->get_rxfh)
> + return 0;
What is this for?
> if (request->rss_context)
> return rss_prepare_ctx(request, dev, data, info);
> return rss_prepare_get(request, dev, data, info);
> @@ -466,6 +468,192 @@ void ethtool_rss_notify(struct net_device *dev, u32 rss_context)
> ethnl_notify(dev, ETHTOOL_MSG_RSS_NTF, &req_info.base);
> }
>
> +static int
> +ethnl_rss_set_validate(struct ethnl_req_info *req_info, struct genl_info *info)
> +{
> + const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
> + struct rss_req_info *request = RSS_REQINFO(req_info);
> + struct nlattr **tb = info->attrs;
> + struct nlattr *bad_attr = NULL;
> +
> + if (request->rss_context && !ops->create_rxfh_context)
> + bad_attr = bad_attr ?: tb[ETHTOOL_A_RSS_CONTEXT];
If we wish to be consistent with the ioctl flow, we should also check
that "at least one change was requested".
i.e., if (!tb[ETHTOOL_A_RSS_INDIR]) return err?
> +
> + if (bad_attr) {
> + NL_SET_BAD_ATTR(info->extack, bad_attr);
> + return -EOPNOTSUPP;
> + }
> +
> + return 1;
> +}
> +
> +static int
> +rss_set_prep_indir(struct net_device *dev, struct genl_info *info,
> + struct rss_reply_data *data, struct ethtool_rxfh_param *rxfh,
> + bool *reset, bool *mod)
> +{
> + const struct ethtool_ops *ops = dev->ethtool_ops;
> + struct netlink_ext_ack *extack = info->extack;
> + struct nlattr **tb = info->attrs;
> + struct ethtool_rxnfc rx_rings;
> + size_t alloc_size;
> + u32 user_size;
> + int i, err;
> +
> + if (!tb[ETHTOOL_A_RSS_INDIR])
> + return 0;
> + if (!data->indir_size)
> + return -EOPNOTSUPP;
> +
> + rx_rings.cmd = ETHTOOL_GRXRINGS;
> + err = ops->get_rxnfc(dev, &rx_rings, NULL);
> + if (err)
> + return err;
> +
> + if (nla_len(tb[ETHTOOL_A_RSS_INDIR]) % 4) {
> + NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_INDIR]);
> + return -EINVAL;
> + }
> + user_size = nla_len(tb[ETHTOOL_A_RSS_INDIR]) / 4;
> + if (!user_size) {
> + if (rxfh->rss_context) {
> + NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_RSS_INDIR],
> + "can't reset table for a context");
> + return -EINVAL;
> + }
> + *reset = true;
> + } else if (data->indir_size % user_size) {
> + NL_SET_ERR_MSG_ATTR_FMT(extack, tb[ETHTOOL_A_RSS_INDIR],
> + "size (%d) mismatch with device indir table (%d)",
> + user_size, data->indir_size);
> + return -EINVAL;
> + }
> +
> + rxfh->indir_size = data->indir_size;
> + alloc_size = array_size(data->indir_size, sizeof(rxfh->indir[0]));
> + rxfh->indir = kzalloc(alloc_size, GFP_KERNEL);
> + if (!rxfh->indir)
> + return -ENOMEM;
> +
> + nla_memcpy(rxfh->indir, tb[ETHTOOL_A_RSS_INDIR], alloc_size);
ethnl_update_binary() will take care of the explicit memcmp down the line?
> + for (i = 0; i < user_size; i++) {
> + if (rxfh->indir[i] < rx_rings.data)
> + continue;
> +
> + NL_SET_ERR_MSG_ATTR_FMT(extack, tb[ETHTOOL_A_RSS_INDIR],
> + "entry %d: queue out of range (%d)",
> + i, rxfh->indir[i]);
> + err = -EINVAL;
> + goto err_free;
> + }
> +
> + if (user_size) {
> + /* Replicate the user-provided table to fill the device table */
> + for (i = user_size; i < data->indir_size; i++)
> + rxfh->indir[i] = rxfh->indir[i % user_size];
> + } else {
> + for (i = 0; i < data->indir_size; i++)
> + rxfh->indir[i] =
> + ethtool_rxfh_indir_default(i, rx_rings.data);
Unless you wanted the mcmp to also take care of this case?
> + }
> +
> + *mod |= memcmp(rxfh->indir, data->indir_table, data->indir_size);
> +
> + return 0;
> +
> +err_free:
> + kfree(rxfh->indir);
> + rxfh->indir = NULL;
> + return err;
> +}
> +
> +static int
> +ethnl_rss_set(struct ethnl_req_info *req_info, struct genl_info *info)
> +{
> + struct rss_req_info *request = RSS_REQINFO(req_info);
> + bool indir_reset = false, indir_mod = false;
> + struct ethtool_rxfh_context *ctx = NULL;
> + struct net_device *dev = req_info->dev;
> + struct ethtool_rxfh_param rxfh = {};
> + struct nlattr **tb = info->attrs;
> + struct rss_reply_data data = {};
> + const struct ethtool_ops *ops;
> + bool mod = false;
> + int ret;
> +
> + ops = dev->ethtool_ops;
> + data.base.dev = dev;
> +
> + ret = rss_prepare(request, dev, &data, info);
> + if (ret)
> + return ret;
> +
> + rxfh.rss_context = request->rss_context;
> +
> + ret = rss_set_prep_indir(dev, info, &data, &rxfh,
> + &indir_reset, &indir_mod);
> + if (ret)
> + goto exit_clean_data;
> + mod |= indir_mod;
> +
> + rxfh.hfunc = ETH_RSS_HASH_NO_CHANGE;
> + rxfh.input_xfrm = RXH_XFRM_NO_CHANGE;
> +
> + mutex_lock(&dev->ethtool->rss_lock);
> + if (request->rss_context) {
> + ctx = xa_load(&dev->ethtool->rss_ctx, request->rss_context);
> + if (!ctx) {
> + ret = -ENOENT;
> + goto exit_unlock;
> + }
> + }
> +
> + if (!mod)
> + ret = 0; /* nothing to tell the driver */
> + else if (!rxfh.rss_context)
> + ret = ops->set_rxfh(dev, &rxfh, info->extack);
> + else
> + ret = ops->modify_rxfh_context(dev, ctx, &rxfh, info->extack);
> + if (ret)
> + goto exit_unlock;
> +
> + if (ctx)
> + rss_set_ctx_update(ctx, tb, &data, &rxfh);
> + else if (indir_reset)
> + dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
> + else if (indir_mod)
> + dev->priv_flags |= IFF_RXFH_CONFIGURED;
One can argue that IFF_RXFH_CONFIGURED should be set even if the
requested table is equal to the default one.
> +
> +exit_unlock:
> + mutex_unlock(&dev->ethtool->rss_lock);
> + kfree(rxfh.indir);
> +exit_clean_data:
> + rss_cleanup_data(&data.base);
> +
> + return ret ?: mod;
> +}
Powered by blists - more mailing lists