netdev - Re: [PATCH v4 net-next 7/7] sfc: use new rxfh

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: Thu, 5 Oct 2023 21:54:24 +0100
From: Edward Cree <ecree.xilinx@...il.com>
To: edward.cree@....com, linux-net-drivers@....com, davem@...emloft.net,
 kuba@...nel.org, edumazet@...gle.com, pabeni@...hat.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 7/7] sfc: use new rxfh_context API

On 02/10/2023 14:01, Martin Habets wrote:
> On Wed, Sep 27, 2023 at 07:13:38PM +0100, edward.cree@....com wrote:
>> From: Edward Cree <ecree.xilinx@...il.com>
>>
>> The core is now responsible for allocating IDs and a memory region for
>>  us to store our state (struct efx_rss_context_priv), so we no longer
>>  need efx_alloc_rss_context_entry() and friends.
>> Since the contexts are now maintained by the core, use the core's lock
>>  (net_dev->ethtool->rss_lock), rather than our own mutex (efx->rss_lock),
>>  to serialise access against changes; and remove the now-unused
>>  efx->rss_lock from struct efx_nic.
>>
>> Signed-off-by: Edward Cree <ecree.xilinx@...il.com>
...
>> +		rc = efx_mcdi_rx_push_rss_context_config(efx, priv, indir, key,
>> +							 false);
>>  		if (rc)
>>  			netif_warn(efx, probe, efx->net_dev,
>> -				   "failed to restore RSS context %u, rc=%d"
>> +				   "failed to restore RSS context %lu, rc=%d"
>>  				   "; RSS filters may fail to be applied\n",
>> -				   ctx->user_id, rc);
>> +				   context, rc);
> 
> If this fails the state in the core is out-of-sync with that in the NIC.
> Should we remove the RSS context from efx->net_dev->ethtool->rss_ctx, or do
> we expect admins to do that manually?
> 
> Martin

I definitely think it's a bad idea to have drivers removing things
 from the array behind the kernel's back.  Besides, I think it ought
 to keep the configuration the user asked for, so that if they fix
 something and then trigger another MC reboot the original config
 will get reapplied.
Possibly if we go with Jakub's suggestion of a replay mechanism
 then the core can react to failure returns from the reoffload call,
 either by removing the context from the array or by setting some
 kind of 'broken' flag that can be reported to userspace in dumps.

-ed