Re: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

[Martin Habets <habetsm.xilinx@gmail.com>]

On Wed, Sep 27, 2023 at 07:13:37PM +0100, edward.cree@amd.com wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> While this is not needed to serialise the ethtool entry points (which
>  are all under RTNL), drivers may have cause to asynchronously access
>  dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to
>  do this safely without needing to take the RTNL.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

> ---
>  include/linux/ethtool.h | 3 +++
>  net/core/dev.c          | 5 +++++
>  net/ethtool/ioctl.c     | 7 +++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index c8963bde9289..d15a21bd6f12 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -1026,11 +1026,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>  /**
>   * struct ethtool_netdev_state - per-netdevice state for ethtool features
>   * @rss_ctx:		XArray of custom RSS contexts
> + * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
> + *			within RTNL.
>   * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
>   * @wol_enabled:	Wake-on-LAN is enabled
>   */
>  struct ethtool_netdev_state {
>  	struct xarray		rss_ctx;
> +	struct mutex		rss_lock;
>  	u32			rss_ctx_max_id;
>  	u32			wol_enabled:1;
>  };
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 69579d9cd7ba..c57456ed4be8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10074,6 +10074,7 @@ int register_netdevice(struct net_device *dev)
>  
>  	/* rss ctx ID 0 is reserved for the default context, start from 1 */
>  	xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1);
> +	mutex_init(&dev->ethtool->rss_lock);
>  
>  	spin_lock_init(&dev->addr_list_lock);
>  	netdev_set_addr_lockdep_class(dev);
> @@ -10882,6 +10883,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  	struct ethtool_rxfh_context *ctx;
>  	unsigned long context;
>  
> +	mutex_lock(&dev->ethtool->rss_lock);
>  	if (dev->ethtool_ops->create_rxfh_context ||
>  	    dev->ethtool_ops->set_rxfh_context)
>  		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
> @@ -10903,6 +10905,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  			kfree(ctx);
>  		}
>  	xa_destroy(&dev->ethtool->rss_ctx);
> +	mutex_unlock(&dev->ethtool->rss_lock);
>  }
>  
>  /**
> @@ -11016,6 +11019,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
>  		if (dev->netdev_ops->ndo_uninit)
>  			dev->netdev_ops->ndo_uninit(dev);
>  
> +		mutex_destroy(&dev->ethtool->rss_lock);
> +
>  		if (skb)
>  			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, portid, nlh);
>  
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 3920ddee3ee2..d21bbc92e6fc 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1258,6 +1258,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	u8 *rss_config;
>  	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
>  	bool create = false, delete = false;
> +	bool locked = false; /* dev->ethtool->rss_lock taken */
>  
>  	if (!ops->get_rxnfc || !ops->set_rxfh)
>  		return -EOPNOTSUPP;
> @@ -1335,6 +1336,10 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> +	if (rxfh.rss_context) {
> +		mutex_lock(&dev->ethtool->rss_lock);
> +		locked = true;
> +	}
>  	if (create) {
>  		if (delete) {
>  			ret = -EINVAL;
> @@ -1455,6 +1460,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	}
>  
>  out:
> +	if (locked)
> +		mutex_unlock(&dev->ethtool->rss_lock);
>  	kfree(rss_config);
>  	return ret;
>  }