Re: [PATCH v2 net-next 1/4] net: synchronize proc_do_rss_key() with netdev_rss_key_fill()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new `extern bool netdev_rss_key_initialized;` is added to the
  extern block in net/core/dev.h that is introduced by the comment `/*…

--

net: synchronize proc_do_rss_key() with netdev_rss_key_fill()

This adds a netdev_rss_key_initialized boolean that netdev_rss_key_fill()
publishes with an smp_wmb() once netdev_rss_key[] has been populated, and
pairs it with an smp_rmb() in proc_do_rss_key(), which now prints zero
bytes until the flag is set.

> diff --git a/net/core/dev.h b/net/core/dev.h
> index b757faead4d1a..0127b4d03e525 100644
> --- a/net/core/dev.h
> +++ b/net/core/dev.h
> @@ -95,6 +95,7 @@ extern int		netdev_unregister_timeout_secs;
>  extern int		weight_p;
>  extern int		dev_weight_rx_bias;
>  extern int		dev_weight_tx_bias;
> +extern bool		netdev_rss_key_initialized;

[Severity: Low]
This isn't a bug, but is this the right block for the new extern?  The
block is introduced by this comment in net/core/dev.h:

	/* sysctls not referred to from outside net/core/ */
	extern int		netdev_unregister_timeout_secs;
	extern int		weight_p;
	extern int		dev_weight_rx_bias;
	extern int		dev_weight_tx_bias;
	extern bool		netdev_rss_key_initialized;

netdev_rss_key_initialized is not backed by a sysctl entry in
net/core/sysctl_net_core.c, and it is defined and written outside
net/core/, in netdev_rss_key_fill() in net/ethtool/ioctl.c, which is why
this patch also needs to add #include "../core/dev.h" there.

Also, netdev_rss_key[] and netdev_rss_key_fill() are declared in
include/linux/netdevice.h, so the flag that says whether the key is valid
is not visible at the declaration site of the key it describes.  Would
declaring it next to netdev_rss_key in include/linux/netdevice.h, or
adjusting the comment, be clearer?

[ ... ]

> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 4b0bc503f9307..b2820f02ca279 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -34,6 +34,7 @@
>  #include <net/netdev_lock.h>
>  #include <net/netdev_queues.h>
>  
> +#include "../core/dev.h"
>  #include "common.h"
>  
>  /* State held across locks and calls for commands which have devlink fallback */
> @@ -1302,11 +1303,17 @@ static int ethtool_copy_validate_indir(u32 *indir, void __user *useraddr,
>  }
>  
>  u8 netdev_rss_key[NETDEV_RSS_KEY_LEN] __read_mostly;
> +bool netdev_rss_key_initialized __read_mostly;
>  
>  void netdev_rss_key_fill(void *buffer, size_t len)
>  {
>  	BUG_ON(len > sizeof(netdev_rss_key));
>  	net_get_random_once(netdev_rss_key, sizeof(netdev_rss_key));
> +	if (unlikely(!READ_ONCE(netdev_rss_key_initialized))) {
> +		/* Pair with smp_rmb() in proc_do_rss_key(). */
> +		smp_wmb();
> +		WRITE_ONCE(netdev_rss_key_initialized, true);
> +	}
>  	memcpy(buffer, netdev_rss_key, len);
>  }
>  EXPORT_SYMBOL(netdev_rss_key_fill);

The same placement mismatch appears to persist later in the series: in
"net: ethtool: generate RSS keys that spread flows over all queues" both
netdev_rss_key and netdev_rss_key_initialized are declared in that same
net/core/dev.h block while being defined in net/ethtool/common.c.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922163458.3900996-1-edumazet%40google.com