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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 2 Oct 2023 13:16:50 +0100
From: Martin Habets <habetsm.xilinx@...il.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,
	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: [PATCH v4 net-next 6/7] net: ethtool: add a mutex protecting RSS
 contexts

On Wed, Sep 27, 2023 at 07:13:37PM +0100, edward.cree@....com wrote:
> From: Edward Cree <ecree.xilinx@...il.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@...il.com>

Reviewed-by: Martin Habets <habetsm.xilinx@...il.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;
>  }