| 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: <CAEAWyHcqNagO86fPe4TLVTU3XopRiNU1zcj7wTSf8bZH3Sg8YA@mail.gmail.com>
Date: Tue, 8 Apr 2025 15:15:11 -0700
From: Harshitha Ramamurthy <hramamurthy@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org, sdf@...ichev.me,
kuniyu@...zon.com, jdamato@...tly.com, bpf@...r.kernel.org
Subject: Re: [PATCH net-next v2 5/8] xdp: double protect netdev->xdp_flags
with netdev->lock
On Tue, Apr 8, 2025 at 1:00 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Protect xdp_features with netdev->lock. This way pure readers
> no longer have to take rtnl_lock to access the field.
>
> This includes calling NETDEV_XDP_FEAT_CHANGE under the lock.
> Looks like that's fine for bonding, the only "real" listener,
> it's the same as ethtool feature change.
>
> In terms of normal drivers - only GVE need special consideration
> (other drivers don't use instance lock or don't support XDP).
> It calls xdp_set_features_flag() helper from gve_init_priv() which
> in turn is called from gve_reset_recovery() (locked), or prior
> to netdev registration. So switch to _locked.
>
> Reviewed-by: Joe Damato <jdamato@...tly.com>
> Acked-by: Stanislav Fomichev <sdf@...ichev.me>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
Acked-by: Harshitha Ramamurthy <hramamurthy@...gle.com>
> ---
> CC: bpf@...r.kernel.org
> ---
> Documentation/networking/netdevices.rst | 1 +
> include/linux/netdevice.h | 2 +-
> include/net/xdp.h | 1 +
> drivers/net/ethernet/google/gve/gve_main.c | 2 +-
> net/core/lock_debug.c | 2 +-
> net/core/xdp.c | 12 +++++++++++-
> 6 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
> index 6c2d8945f597..d6357472d3f1 100644
> --- a/Documentation/networking/netdevices.rst
> +++ b/Documentation/networking/netdevices.rst
> @@ -354,6 +354,7 @@ For devices with locked ops, currently only the following notifiers are
> running under the lock:
> * ``NETDEV_REGISTER``
> * ``NETDEV_UP``
> +* ``NETDEV_XDP_FEAT_CHANGE``
>
> The following notifiers are running without the lock:
> * ``NETDEV_UNREGISTER``
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7242fb8a22fc..dece2ae396a1 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2526,7 +2526,7 @@ struct net_device {
> * @net_shaper_hierarchy, @reg_state, @threaded
> *
> * Double protects:
> - * @up, @moving_ns, @nd_net
> + * @up, @moving_ns, @nd_net, @xdp_flags
> *
> * Double ops protects:
> * @real_num_rx_queues, @real_num_tx_queues
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 48efacbaa35d..20e41b5ff319 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -616,6 +616,7 @@ struct xdp_metadata_ops {
> u32 bpf_xdp_metadata_kfunc_id(int id);
> bool bpf_dev_bound_kfunc_id(u32 btf_id);
> void xdp_set_features_flag(struct net_device *dev, xdp_features_t val);
> +void xdp_set_features_flag_locked(struct net_device *dev, xdp_features_t val);
> void xdp_features_set_redirect_target(struct net_device *dev, bool support_sg);
> void xdp_features_clear_redirect_target(struct net_device *dev);
> #else
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index f9a73c956861..7a249baee316 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -2185,7 +2185,7 @@ static void gve_set_netdev_xdp_features(struct gve_priv *priv)
> xdp_features = 0;
> }
>
> - xdp_set_features_flag(priv->dev, xdp_features);
> + xdp_set_features_flag_locked(priv->dev, xdp_features);
> }
>
> static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
> diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
> index b7f22dc92a6f..598c443ef2f3 100644
> --- a/net/core/lock_debug.c
> +++ b/net/core/lock_debug.c
> @@ -20,6 +20,7 @@ int netdev_debug_event(struct notifier_block *nb, unsigned long event,
> switch (cmd) {
> case NETDEV_REGISTER:
> case NETDEV_UP:
> + case NETDEV_XDP_FEAT_CHANGE:
> netdev_ops_assert_locked(dev);
> fallthrough;
> case NETDEV_DOWN:
> @@ -58,7 +59,6 @@ int netdev_debug_event(struct notifier_block *nb, unsigned long event,
> case NETDEV_OFFLOAD_XSTATS_DISABLE:
> case NETDEV_OFFLOAD_XSTATS_REPORT_USED:
> case NETDEV_OFFLOAD_XSTATS_REPORT_DELTA:
> - case NETDEV_XDP_FEAT_CHANGE:
> ASSERT_RTNL();
> break;
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index f86eedad586a..3cd0db9c9d2d 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -17,6 +17,7 @@
> #include <net/page_pool/helpers.h>
>
> #include <net/hotdata.h>
> +#include <net/netdev_lock.h>
> #include <net/xdp.h>
> #include <net/xdp_priv.h> /* struct xdp_mem_allocator */
> #include <trace/events/xdp.h>
> @@ -991,17 +992,26 @@ static int __init xdp_metadata_init(void)
> }
> late_initcall(xdp_metadata_init);
>
> -void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
> +void xdp_set_features_flag_locked(struct net_device *dev, xdp_features_t val)
> {
> val &= NETDEV_XDP_ACT_MASK;
> if (dev->xdp_features == val)
> return;
>
> + netdev_assert_locked_or_invisible(dev);
> dev->xdp_features = val;
>
> if (dev->reg_state == NETREG_REGISTERED)
> call_netdevice_notifiers(NETDEV_XDP_FEAT_CHANGE, dev);
> }
> +EXPORT_SYMBOL_GPL(xdp_set_features_flag_locked);
> +
> +void xdp_set_features_flag(struct net_device *dev, xdp_features_t val)
> +{
> + netdev_lock(dev);
> + xdp_set_features_flag_locked(dev, val);
> + netdev_unlock(dev);
> +}
> EXPORT_SYMBOL_GPL(xdp_set_features_flag);
>
> void xdp_features_set_redirect_target(struct net_device *dev, bool support_sg)
> --
> 2.49.0
>
Powered by blists - more mailing lists