| 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
| ||
|
Date: Tue, 19 Sep 2023 12:10:38 +0100
From: Martin Habets <martin.habets@....com>
To: <edward.cree@....com>
CC: <linux-net-drivers@....com>, <davem@...emloft.net>, <kuba@...nel.org>,
<edumazet@...gle.com>, <pabeni@...hat.com>, Edward Cree
<ecree.xilinx@...il.com>, <netdev@...r.kernel.org>,
<habetsm.xilinx@...il.com>, <sudheer.mogilappagari@...el.com>,
<jdamato@...tly.com>, <andrew@...n.ch>, <mw@...ihalf.com>,
<linux@...linux.org.uk>, <sgoutham@...vell.com>, <gakula@...vell.com>,
<sbhatta@...vell.com>, <hkelam@...vell.com>, <saeedm@...dia.com>,
<leon@...nel.org>
Subject: Re: [RFC PATCH v3 net-next 4/7] net: ethtool: let the core choose
RSS context IDs
On Tue, Sep 12, 2023 at 03:21:39PM +0100, edward.cree@....com wrote:
>
> From: Edward Cree <ecree.xilinx@...il.com>
>
> Add a new API to create/modify/remove RSS contexts, that passes in the
> newly-chosen context ID (not as a pointer) rather than leaving the
> driver to choose it on create. Also pass in the ctx, allowing drivers
> to easily use its private data area to store their hardware-specific
> state.
> Keep the existing .set_rxfh_context API for now as a fallback, but
> deprecate it.
>
> Signed-off-by: Edward Cree <ecree.xilinx@...il.com>
> ---
> include/linux/ethtool.h | 40 ++++++++++++++++++++++++--
> net/core/dev.c | 11 +++++--
> net/ethtool/ioctl.c | 64 +++++++++++++++++++++++++++++++----------
> 3 files changed, 94 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index f7317b53ab61..4fa2a7f6ed4c 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -747,10 +747,33 @@ struct ethtool_mm_stats {
> * @get_rxfh_context: Get the contents of the RX flow hash indirection table,
> * hash key, and/or hash function assiciated to the given rss context.
> * Returns a negative error code or zero.
> - * @set_rxfh_context: Create, remove and configure RSS contexts. Allows setting
> + * @create_rxfh_context: Create a new RSS context with the specified RX flow
> + * hash indirection table, hash key, and hash function.
> + * Arguments which are set to %NULL or zero will be populated to
> + * appropriate defaults by the driver.
> + * The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + * note that the indir table, hkey and hfunc are not yet populated as
> + * of this call. The driver does not need to update these; the core
> + * will do so if this op succeeds.
> + * If the driver provides this method, it must also provide
> + * @modify_rxfh_context and @remove_rxfh_context.
> + * Returns a negative error code or zero.
> + * @modify_rxfh_context: Reconfigure the specified RSS context. Allows setting
> * the contents of the RX flow hash indirection table, hash key, and/or
> - * hash function associated to the given context. Arguments which are set
> - * to %NULL or zero will remain unchanged.
> + * hash function associated with the given context.
> + * Arguments which are set to %NULL or zero will remain unchanged.
> + * The &struct ethtool_rxfh_context for this context is passed in @ctx;
> + * note that it will still contain the *old* settings. The driver does
> + * not need to update these; the core will do so if this op succeeds.
> + * Returns a negative error code or zero. An error code must be returned
> + * if at least one unsupported change was requested.
> + * @remove_rxfh_context: Remove the specified RSS context.
> + * The &struct ethtool_rxfh_context for this context is passed in @ctx.
> + * Returns a negative error code or zero.
> + * @set_rxfh_context: Deprecated API to create, remove and configure RSS
> + * contexts. Allows setting the contents of the RX flow hash indirection
> + * table, hash key, and/or hash function associated to the given context.
> + * Arguments which are set to %NULL or zero will remain unchanged.
> * Returns a negative error code or zero. An error code must be returned
> * if at least one unsupported change was requested.
> * @get_channels: Get number of channels.
> @@ -901,6 +924,17 @@ struct ethtool_ops {
> const u8 *key, const u8 hfunc);
> int (*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
> u8 *hfunc, u32 rss_context);
> + int (*create_rxfh_context)(struct net_device *,
> + struct ethtool_rxfh_context *ctx,
> + const u32 *indir, const u8 *key,
> + const u8 hfunc, u32 rss_context);
To return the rss_context this creates shouldn't it use a pointer to
rss_context here?
Matin
> + int (*modify_rxfh_context)(struct net_device *,
> + struct ethtool_rxfh_context *ctx,
> + const u32 *indir, const u8 *key,
> + const u8 hfunc, u32 rss_context);
> + int (*remove_rxfh_context)(struct net_device *,
> + struct ethtool_rxfh_context *ctx,
> + u32 rss_context);
> int (*set_rxfh_context)(struct net_device *, const u32 *indir,
> const u8 *key, const u8 hfunc,
> u32 *rss_context, bool delete);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 4bbb6bda7b7e..6b8e5fd8691b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10860,15 +10860,20 @@ static void netdev_rss_contexts_free(struct net_device *dev)
> struct ethtool_rxfh_context *ctx;
> u32 context;
>
> - if (!dev->ethtool_ops->set_rxfh_context)
> + if (!dev->ethtool_ops->create_rxfh_context &&
> + !dev->ethtool_ops->set_rxfh_context)
> return;
> idr_for_each_entry(&dev->ethtool->rss_ctx, ctx, context) {
> u32 *indir = ethtool_rxfh_context_indir(ctx);
> u8 *key = ethtool_rxfh_context_key(ctx);
>
> idr_remove(&dev->ethtool->rss_ctx, context);
> - dev->ethtool_ops->set_rxfh_context(dev, indir, key, ctx->hfunc,
> - &context, true);
> + if (dev->ethtool_ops->create_rxfh_context)
> + dev->ethtool_ops->remove_rxfh_context(dev, ctx, context);
> + else
> + dev->ethtool_ops->set_rxfh_context(dev, indir, key,
> + ctx->hfunc,
> + &context, true);
> kfree(ctx);
> }
> }
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index db596b61c6ab..4ce960a5ad4c 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1274,7 +1274,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
> return -EINVAL;
> /* Most drivers don't handle rss_context, check it's 0 as well */
> - if (rxfh.rss_context && !ops->set_rxfh_context)
> + if (rxfh.rss_context && !(ops->create_rxfh_context ||
> + ops->set_rxfh_context))
> return -EOPNOTSUPP;
> create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
>
> @@ -1349,8 +1350,28 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> }
> ctx->indir_size = dev_indir_size;
> ctx->key_size = dev_key_size;
> - ctx->hfunc = rxfh.hfunc;
> ctx->priv_size = ops->rxfh_priv_size;
> + /* Initialise to an empty context */
> + ctx->indir_no_change = ctx->key_no_change = 1;
> + ctx->hfunc = ETH_RSS_HASH_NO_CHANGE;
> + if (ops->create_rxfh_context) {
> + int ctx_id;
> +
> + /* driver uses new API, core allocates ID */
> + /* if rss_ctx_max_id is not specified (left as 0), it is
> + * treated as INT_MAX + 1 by idr_alloc
> + */
> + ctx_id = idr_alloc(&dev->ethtool->rss_ctx, ctx, 1,
> + dev->ethtool->rss_ctx_max_id,
> + GFP_KERNEL_ACCOUNT);
> + /* 0 is not allowed, so treat it like an error here */
> + if (ctx_id <= 0) {
> + kfree(ctx);
> + ret = -ENOMEM;
> + goto out;
> + }
> + rxfh.rss_context = ctx_id;
> + }
> } else if (rxfh.rss_context) {
> ctx = idr_find(&dev->ethtool->rss_ctx, rxfh.rss_context);
> if (!ctx) {
> @@ -1359,15 +1380,34 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> }
> }
>
> - if (rxfh.rss_context)
> - ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
> - &rxfh.rss_context, delete);
> - else
> + if (rxfh.rss_context) {
> + if (ops->create_rxfh_context) {
> + if (create)
> + ret = ops->create_rxfh_context(dev, ctx, indir,
> + hkey, rxfh.hfunc,
> + rxfh.rss_context);
> + else if (delete)
> + ret = ops->remove_rxfh_context(dev, ctx,
> + rxfh.rss_context);
> + else
> + ret = ops->modify_rxfh_context(dev, ctx, indir,
> + hkey, rxfh.hfunc,
> + rxfh.rss_context);
> + } else {
> + ret = ops->set_rxfh_context(dev, indir, hkey,
> + rxfh.hfunc,
> + &rxfh.rss_context, delete);
> + }
> + } else {
> ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
> + }
> if (ret) {
> - if (create)
> + if (create) {
> /* failed to create, free our new tracking entry */
> + if (ops->create_rxfh_context)
> + idr_remove(&dev->ethtool->rss_ctx, rxfh.rss_context);
> kfree(ctx);
> + }
> goto out;
> }
>
> @@ -1383,12 +1423,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> dev->priv_flags |= IFF_RXFH_CONFIGURED;
> }
> /* Update rss_ctx tracking */
> - if (create) {
> - /* Ideally this should happen before calling the driver,
> - * so that we can fail more cleanly; but we don't have the
> - * context ID until the driver picks it, so we have to
> - * wait until after.
> - */
> + if (create && !ops->create_rxfh_context) {
> + /* driver uses old API, it chose context ID */
> if (WARN_ON(idr_find(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
> /* context ID reused, our tracking is screwed */
> kfree(ctx);
> @@ -1398,8 +1434,6 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
> WARN_ON(idr_alloc(&dev->ethtool->rss_ctx, ctx, rxfh.rss_context,
> rxfh.rss_context + 1, GFP_KERNEL) !=
> rxfh.rss_context);
> - ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
> - ctx->key_no_change = !rxfh.key_size;
> }
> if (delete) {
> WARN_ON(idr_remove(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);
Powered by blists - more mailing lists