| 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: <CAHS8izMJrB4MogOffo9T=Hc+6UX62eCvkESiSUeb9YUdLh6nQQ@mail.gmail.com>
Date: Mon, 28 Jul 2025 15:26:22 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org, io-uring@...r.kernel.org,
Eric Dumazet <edumazet@...gle.com>, Willem de Bruijn <willemb@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, andrew+netdev@...n.ch, horms@...nel.org,
davem@...emloft.net, sdf@...ichev.me, dw@...idwei.uk,
michael.chan@...adcom.com, dtatulea@...dia.com, ap420073@...il.com
Subject: Re: [RFC v1 13/22] net: add queue config validation callback
On Mon, Jul 28, 2025 at 4:03 AM Pavel Begunkov <asml.silence@...il.com> wrote:
>
> From: Jakub Kicinski <kuba@...nel.org>
>
> I imagine (tm) that as the number of per-queue configuration
> options grows some of them may conflict for certain drivers.
> While the drivers can obviously do all the validation locally
> doing so is fairly inconvenient as the config is fed to drivers
> piecemeal via different ops (for different params and NIC-wide
> vs per-queue).
>
> Add a centralized callback for validating the queue config
> in queue ops. The callback gets invoked before each queue restart
> and when ring params are modified.
>
> For NIC-wide changes the callback gets invoked for each active
> (or active to-be) queue, and additionally with a negative queue
> index for NIC-wide defaults. The NIC-wide check is needed in
> case all queues have an override active when NIC-wide setting
> is changed to an unsupported one. Alternatively we could check
> the settings when new queues are enabled (in the channel API),
> but accepting invalid config is a bad idea. Users may expect
> that resetting a queue override will always work.
>
> The "trick" of passing a negative index is a bit ugly, we may
> want to revisit if it causes confusion and bugs. Existing drivers
> don't care about the index so it "just works".
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
> ---
> include/net/netdev_queues.h | 12 ++++++++++++
> net/core/dev.h | 2 ++
> net/core/netdev_config.c | 20 ++++++++++++++++++++
> net/core/netdev_rx_queue.c | 6 ++++++
> net/ethtool/rings.c | 5 +++++
> 5 files changed, 45 insertions(+)
>
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index e3e7ecf91bac..f75313fc78ba 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -146,6 +146,14 @@ void netdev_stat_queue_sum(struct net_device *netdev,
> * defaults. Queue config structs are passed to this
> * helper before the user-requested settings are applied.
> *
> + * @ndo_queue_cfg_validate: (Optional) Check if queue config is supported.
> + * Called when configuration affecting a queue may be
> + * changing, either due to NIC-wide config, or config
> + * scoped to the queue at a specified index.
> + * When NIC-wide config is changed the callback will
> + * be invoked for all queues, and in addition to that
> + * with a negative queue index for the base settings.
> + *
> * @ndo_queue_mem_alloc: Allocate memory for an RX queue at the specified index.
> * The new memory is written at the specified address.
> *
> @@ -166,6 +174,10 @@ struct netdev_queue_mgmt_ops {
> void (*ndo_queue_cfg_defaults)(struct net_device *dev,
> int idx,
> struct netdev_queue_config *qcfg);
> + int (*ndo_queue_cfg_validate)(struct net_device *dev,
> + int idx,
> + struct netdev_queue_config *qcfg,
> + struct netlink_ext_ack *extack);
> int (*ndo_queue_mem_alloc)(struct net_device *dev,
> struct netdev_queue_config *qcfg,
> void *per_queue_mem,
> diff --git a/net/core/dev.h b/net/core/dev.h
> index 6d7f5e920018..e0d433fb6325 100644
> --- a/net/core/dev.h
> +++ b/net/core/dev.h
> @@ -99,6 +99,8 @@ void netdev_free_config(struct net_device *dev);
> int netdev_reconfig_start(struct net_device *dev);
> void __netdev_queue_config(struct net_device *dev, int rxq,
> struct netdev_queue_config *qcfg, bool pending);
> +int netdev_queue_config_revalidate(struct net_device *dev,
> + struct netlink_ext_ack *extack);
>
> /* netdev management, shared between various uAPI entry points */
> struct netdev_name_node {
> diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
> index bad2d53522f0..fc700b77e4eb 100644
> --- a/net/core/netdev_config.c
> +++ b/net/core/netdev_config.c
> @@ -99,3 +99,23 @@ void netdev_queue_config(struct net_device *dev, int rxq,
> __netdev_queue_config(dev, rxq, qcfg, true);
> }
> EXPORT_SYMBOL(netdev_queue_config);
> +
> +int netdev_queue_config_revalidate(struct net_device *dev,
> + struct netlink_ext_ack *extack)
> +{
> + const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
> + struct netdev_queue_config qcfg;
> + int i, err;
> +
> + if (!qops || !qops->ndo_queue_cfg_validate)
> + return 0;
> +
Shouldn't this be like return -EOPNOSUPP; or something? Otherwise how
do you protect drivers (GVE) that support queue API but don't support
a configuring a particular netdev_queue_config from core assuming that
the configuration took place?
> + for (i = -1; i < (int)dev->real_num_rx_queues; i++) {
> + netdev_queue_config(dev, i, &qcfg);
> + err = qops->ndo_queue_cfg_validate(dev, i, &qcfg, extack);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> index b0523eb44e10..7c691eb1a48b 100644
> --- a/net/core/netdev_rx_queue.c
> +++ b/net/core/netdev_rx_queue.c
> @@ -37,6 +37,12 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx,
>
> netdev_queue_config(dev, rxq_idx, &qcfg);
>
> + if (qops->ndo_queue_cfg_validate) {
> + err = qops->ndo_queue_cfg_validate(dev, rxq_idx, &qcfg, extack);
> + if (err)
> + goto err_free_old_mem;
> + }
> +
> err = qops->ndo_queue_mem_alloc(dev, &qcfg, new_mem, rxq_idx);
> if (err)
> goto err_free_old_mem;
> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
> index 6a74e7e4064e..7884d10c090f 100644
> --- a/net/ethtool/rings.c
> +++ b/net/ethtool/rings.c
> @@ -4,6 +4,7 @@
>
> #include "netlink.h"
> #include "common.h"
> +#include "../core/dev.h"
>
> struct rings_req_info {
> struct ethnl_req_info base;
> @@ -307,6 +308,10 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
> dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
> dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
>
> + ret = netdev_queue_config_revalidate(dev, info->extack);
> + if (ret)
> + return ret;
> +
> ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
> &kernel_ringparam, info->extack);
> return ret < 0 ? ret : 1;
> --
> 2.49.0
>
--
Thanks,
Mina
Powered by blists - more mailing lists