Re: [PATCH v2 net-next 2/4] net: ethtool: generate RSS keys that spread flows over all queues

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] netdev_rss_key_init() (net/ethtool/common.c:1546) is invoked
  through DO_ONCE() from netdev_rss_key_fill()…
- [Medium] The fixup introduces key-independent collision families in the
  bit that selects the queue.
- [Low] Stale cross-file memory-ordering comment.
- [Low] net/core/dev.h:93 introduces the block with '/* sysctls not
  referred to from outside net/core/ */', and this patch adds 'extern u8…

--

net: ethtool: generate RSS keys that spread flows over all queues

This moves netdev_rss_key and netdev_rss_key_fill() from net/ethtool/ioctl.c
to net/ethtool/common.c and replaces the plain random draw with a generator
that forces some key bits so the Toeplitz matrices of the q lowest bits of a
hashed field are non singular, then rejects keys where two 32-bit key windows
alias.

> diff --git a/net/core/dev.h b/net/core/dev.h
> index 0127b4d03e525..a5e22b2eae523 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 u8		netdev_rss_key[NETDEV_RSS_KEY_LEN] __read_mostly;
>  extern bool		netdev_rss_key_initialized;

[Severity: Low]
This isn't a bug, but the block these two lines join is introduced by:

net/core/dev.h:
	/* sysctls not referred to from outside net/core/ */
	extern int		netdev_unregister_timeout_secs;

netdev_rss_key is now defined in net/ethtool/common.c and read from
net/ethtool/common.c:netdev_rss_key_fill(), and netdev_rss_key_initialized
(added by the preceding patch of the series) is defined there too.

Should the grouping comment be adjusted, or should these two declarations
live somewhere that is not described as net/core/ private?

> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 23db40618fed1..b4e766e60d38a 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c

[ ... ]

> @@ -1400,3 +1403,174 @@ enum ethtool_link_medium ethtool_str_to_medium(const char *str)
>  	return ETHTOOL_LINK_MEDIUM_NONE;
>  }
>  EXPORT_SYMBOL_GPL(ethtool_str_to_medium);
> +
> +u8 netdev_rss_key[NETDEV_RSS_KEY_LEN] __read_mostly;
> +bool netdev_rss_key_initialized __read_mostly;

[ ... ]

> +static void netdev_rss_key_fixup_field(u8 *key, unsigned int lsb)
> +{
> +	bool d[NETDEV_RSS_KEY_SPAN];
> +	unsigned int i, t;
> +
> +	for (t = 0; t < NETDEV_RSS_KEY_SPAN; t++)
> +		d[t] = netdev_rss_key_bit(key, lsb + 31 - t);
> +
> +	for (i = 0; 2 * i < NETDEV_RSS_KEY_SPAN; i++) {
> +		bool value = true;
> +
> +		for (t = i; t < 2 * i; t++)
> +			value ^= d[t];
> +
> +		d[2 * i] = value;
> +	}

[Severity: Medium]
Does this fixup create collision families that hold for every generated key?

d[t] is the key bit that field bit t contributes to the lowest hash bit, and
the loop above enforces, for i = 0..7, that the XOR of d over the set
S_i = {i .. 2i} equals 1:

	d[0] = 1
	d[1] ^ d[2] = 1
	d[2] ^ d[3] ^ d[4] = 1
	...

XORing any two of those relations gives a set of field bit positions over
which the contributions to the lowest hash bit cancel for any key the
generator produces. On a 16-bit field, S_0 xor S_i for i = 1..7 gives seven
independent masks:

	0x0007, 0x001d, 0x0079, 0x01f1, 0x07e1, 0x1fc1, 0x7f81

So starting from source port 0x8000 and XORing arbitrary subsets of those
masks yields 128 distinct source ports, all of them above 0x8000 and so all
unprivileged, that always land on the same value of the lowest hash bit.

With ethtool_rxfh_indir_default(), the queue is index % nq, so for a
power-of-two queue count those 128 flows are confined to half the queues,
and to a single queue when nq is 2:

include/linux/ethtool.h:ethtool_rxfh_indir_default() {
	return index % n_rx_rings;
}

drivers/net/ethernet/google/gve/gve_main.c:gve_init_rss_config() combines
both, filling hash_lut with ethtool_rxfh_indir_default() and the key with
netdev_rss_key_fill().

netdev_rss_key_aliases() does not catch this since it compares whole 32-bit
windows, whereas the relations above are XOR relations among their lowest
bits only.

The changelog says the key "is a secret that stops a remote attacker from
steering flows onto a single queue". With the relations being forced by the
q = 1 and q = 2 guarantee (non-singularity alone gives d[0] = 1 and
d[2] = 1 ^ d[1]), is that still accurate, and could the commit message and
the new scaling.rst paragraph say what a chosen-port attacker can still do?

[ ... ]

> +static bool netdev_rss_key_aliases(const u8 *key, unsigned int bits)
> +{
> +	u32 windows[NETDEV_RSS_KEY_LEN * BITS_PER_BYTE / 16];
> +	unsigned int i, j, n, r;
> +
> +	for (r = 0; r < 16; r++) {
> +		n = 0;
> +		for (i = r; i + 32 <= bits; i += 16)
> +			windows[n++] = netdev_rss_key_window(key, i);
> +
> +		for (i = 0; i < n; i++)
> +			for (j = i + 1; j < n; j++)
> +				if (windows[i] == windows[j])
> +					return true;

[ ... ]

> +static void netdev_rss_key_init(u8 *key, size_t len)
> +{
> +	unsigned int lsb, bits = len * BITS_PER_BYTE;
> +
> +	/* Four keys out of five come out of the fixup free of aliases, so
> +	 * drawing another one is both simpler and cheaper than repairing.
> +	 */
> +	do {
> +		get_random_bytes(key, len);
> +
> +		/* A field ending at bit @lsb uses key bits [.. , @lsb + 31],
> +		 * so stop as soon as a 32-bit window no longer fits in the
> +		 * key.
> +		 */
> +		for (lsb = 15; lsb + 32 <= bits; lsb += 16)
> +			netdev_rss_key_fixup_field(key, lsb);
> +	} while (netdev_rss_key_aliases(key, bits));

[Severity: Medium]
How long can this loop keep hard interrupts disabled?

DO_ONCE() below runs the callback under the global once_lock with IRQs off:

lib/once.c:__do_once_start() {
	spin_lock_irqsave(&once_lock, *flags);

and the lock is only released in __do_once_done() after the callback
returns. Previously the callback was a single 256-byte get_random_bytes().
It now does get_random_bytes(256), 126 netdev_rss_key_fixup_field() calls
and netdev_rss_key_aliases(), which builds up to 127 windows for each of 16
residues and compares all pairs.

The changelog measures one pass:

	"DO_ONCE() runs the generator under a spinlock with hard IRQs
	disabled, so keep the windows of a class in an array rather than
	recomputing both sides of every pair: 116 us instead of 254 us for
	the worst case"

and the do/while has no cap, so with roughly one draw in five rejected the
duration is only bounded probabilistically. Other CPUs entering
netdev_rss_key_fill() spin on once_lock for that whole time.

Would it be reasonable to generate the key outside the once lock and only
publish it there? Note DO_ONCE_SLEEPABLE() does not look usable as a
replacement: tg3 reaches netdev_rss_key_fill() from tg3_reset_hw(), called
by tg3_init_hw() under tg3_full_lock(), i.e. with a spinlock held.

> +
> +	/* Pair with smp_rmb() in proc_do_rss_key(). */
> +	smp_wmb();
> +	WRITE_ONCE(netdev_rss_key_initialized, true);
> +}

[Severity: Low]
The barrier moved here from netdev_rss_key_fill(), but the reader side still
points at the old place:

net/core/sysctl_net_core.c:proc_do_rss_key() {
	initialized = READ_ONCE(netdev_rss_key_initialized);
	/* Pair with smp_wmb() in netdev_rss_key_fill(). */
	smp_rmb();

After this patch netdev_rss_key_fill() contains no barrier at all, so only
one half of the documented pairing names the other. That reader comment came
in with the preceding patch of the series ("net: synchronize
proc_do_rss_key() with netdev_rss_key_fill()") and no later patch in the
series touches net/core/sysctl_net_core.c. Should it be updated to name
netdev_rss_key_init()?

> +
> +void netdev_rss_key_fill(void *buffer, size_t len)
> +{
> +	if (WARN_ON_ONCE(len > sizeof(netdev_rss_key)))
> +		len = sizeof(netdev_rss_key);
> +	DO_ONCE(netdev_rss_key_init, netdev_rss_key, sizeof(netdev_rss_key));
> +	memcpy(buffer, netdev_rss_key, len);
> +}
> +EXPORT_SYMBOL(netdev_rss_key_fill);

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