linux-kernel - Re: [PATCH net-next v2 13/14] ixgbe: preserve RSS indirection table across admin down/up

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] [day] [month] [year] [list]

Message-ID: <20251023162711.97625-2-enjuk@amazon.com>
Date: Fri, 24 Oct 2025 01:26:38 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <kuba@...nel.org>
CC: <aleksander.lobakin@...el.com>, <andrew+netdev@...n.ch>,
	<anthony.l.nguyen@...el.com>, <corbet@....net>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <enjuk@...zon.com>, <horms@...nel.org>,
	<jacob.e.keller@...el.com>, <jiri@...nulli.us>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <przemyslaw.kitszel@...el.com>, <sx.rinitha@...el.com>
Subject: Re: [PATCH net-next v2 13/14] ixgbe: preserve RSS indirection table across admin down/up

On Wed, 22 Oct 2025 17:26:49 -0700, Jakub Kicinski wrote:

>On Wed, 22 Oct 2025 12:40:45 +0900 Kohei Enju wrote:
>> On Tue, 21 Oct 2025 16:10:06 -0700, Jakub Kicinski wrote:
>> >On Tue, 21 Oct 2025 12:59:34 +0900 Kohei Enju wrote:  
>> >> For example, consider a scenario where the queue count is 8 with user
>> >> configuration containing values from 0 to 7. When queue count changes
>> >> from 8 to 4 and we skip the reinitialization in this scenario, entries
>> >> pointing to queues 4-7 become invalid. The same issue applies when the
>> >> RETA table size changes.  
>> >
>> >Core should reject this. See ethtool_check_max_channel()  
>> 
>> Indeed, you're right that the situation above will be rejected. I missed
>> it.
>> 
>> BTW, I think reinitializing the RETA table when queue count changes or
>> RETA table size changes is reasonable for predictability and safety.
>> Does this approach make sense to you?
>
>Yes, if !netif_is_rxfh_configured() re-initializing is expected.

I got it.

>
>> >> Furthermore, IIUC, adding netif_is_rxfh_configured() to the current
>> >> condition wouldn't provide additional benefit. When parameters remain
>> >> unchanged, regardless of netif_is_rxfh_configured(), we already preserve
>> >> the RETA entries which might be user-configured or default values,   
>> >
>> >User may decide to "isolate" (take out of RSS) a lower queue,
>> >to configure it for AF_XDP or other form of zero-copy. Install
>> >explicit rules to direct traffic to that queue. If you reset
>> >the RSS table random traffic will get stranded in the ZC queue
>> >(== dropped).
>> 
>> You're correct about the ZC queue scenario. The original implementation
>> (before this patch) would indeed cause this problem by unconditionally
>> reinitializing.
>> 
>> I believe this patch addresses that issue - it preserves the user
>> configuration since neither queue count nor RETA table size changes in
>> that case. If I'm misunderstanding your scenario, please let me know.
>> 
>> I could update the logic to explicitly check netif_is_rxfh_configured()
>> as in [1], though the actual behavior would be the same as [2] since
>> the default RETA table is a deterministic function of (rss_indices,
>> reta_entries):
>> 
>> [1] Check user configuration explicitly:
>>     if (!netif_is_rxfh_configured(adapter->netdev) ||
>>         adapter->last_rss_indices != rss_i ||
>>         adapter->last_reta_entries != reta_entries) {
>>         // reinitialize
>>     }
>> 
>> [2] Current patch:
>>     if (adapter->last_rss_indices != rss_i ||
>>         adapter->last_reta_entries != reta_entries) {
>>         // reinitialize
>>     }
>> 
>> Do you have any preference between these approaches, or would you
>> recommend a different solution?
>
>I was expecting something like:
>
>if (netif_is_rxfh_configured(adapter->netdev)) {
>	if (!check_that_rss_is_okay()) {
>		/* This should never happen, barring FW errors etc */
>		warn("user configuration lost due to XYZ");
>		reinit();
>	}
>} else if (...rss_ind != rss_id ||
>           ...reta_entries != reta_entries) {
>	reinit();
>}

Thank you for clarification. 

At first glance, noting that check_that_rss_is_okay() would return false
when the RETA table size is larger than the previous one, since
user-configuration doesn't exist for the expanded portion of the RETA
table. This should happen in realistic scenarios even though there are
no hardware-related or HW errors.

Anyway I'll refine the patch using netif_is_rxfh_configured() and then
submit to iwl-next first.