linux-kernel - Re: [PATCH net V2] amd-xgbe: Fix to ensure dependent features are toggled with RX checksum offload

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: <d5114fb3-4ca8-4ab8-acb2-120a7b940d6f@redhat.com>
Date: Thu, 24 Apr 2025 10:54:07 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: "Badole, Vishal" <vishal.badole@....com>,
 Jacob Keller <jacob.e.keller@...el.com>, Shyam-sundar.S-k@....com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, Thomas.Lendacky@....com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: stable@...r.kernel.org, Raju.Rangoju@....com
Subject: Re: [PATCH net V2] amd-xgbe: Fix to ensure dependent features are
 toggled with RX checksum offload

On 4/23/25 9:57 AM, Badole, Vishal wrote:
> On 4/23/2025 3:50 AM, Jacob Keller wrote:
>> On 4/21/2025 7:04 AM, Vishal Badole wrote:
>>> According to the XGMAC specification, enabling features such as Layer 3
>>> and Layer 4 Packet Filtering, Split Header, Receive Side Scaling (RSS),
>>> and Virtualized Network support automatically selects the IPC Full
>>> Checksum Offload Engine on the receive side.
>>>
>>> When RX checksum offload is disabled, these dependent features must also
>>> be disabled to prevent abnormal behavior caused by mismatched feature
>>> dependencies.
>>>
>>> Ensure that toggling RX checksum offload (disabling or enabling) properly
>>> disables or enables all dependent features, maintaining consistent and
>>> expected behavior in the network device.
>>>
>>
>> My understanding based on previous changes I've made to Intel drivers,
>> the netdev community opinion here is that the driver shouldn't
>> automatically change user configuration like this. Instead, it should
>> reject requests to disable a feature if that isn't possible due to the
>> other requirements.
>>
>> In this case, that means checking and rejecting disable of Rx checksum
>> offload whenever the features which depend on it are enabled, and reject
>> requests to enable the features when Rx checksum is disabled.
> 
> Thank you for sharing your perspective and experience with Intel 
> drivers. From my understanding, the fix_features() callback in ethtool 
> handles enabling and disabling the dependent features required for the 
> requested feature to function correctly. It also ensures that the 
> correct status is reflected in ethtool and notifies the user.
> 
> However, if the user wishes to enable or disable those dependent 
> features again, they can do so using the appropriate ethtool settings.

AFAICS there are two different things here:

- automatic update of NETIF_F_RXHASH according to NETIF_F_RXCSUM value:
that should be avoid and instead incompatible changes should be rejected
with a suitable error message.

- automatic update of header split and vxlan depending on NETIF_F_RXCSUM
value: that could be allowed as AFAICS the driver does not currently
offer any other method to flip modify configuration (and make the state
consistent).

Thanks,

Paolo