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

Message-ID: <98c62087-0a95-4d4d-9e9f-9d62a530af67@amd.com>
Date: Thu, 24 Apr 2025 18:26:57 +0530
From: "Badole, Vishal" <vishal.badole@....com>
To: Paolo Abeni <pabeni@...hat.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/24/2025 2:24 PM, Paolo Abeni wrote:
> 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
> 
> 
Thank you for your observations. I agree with your points. For the first 
case, I will remove the automatic update of NETIF_F_RXHASH based on the 
NETIF_F_RXCSUM value, as checksum offloading functions correctly without 
it, and I will include this change in the next patch version. For the 
second case, I will retain the automatic updates for header split and 
virtual network, as they depend on NETIF_F_RXCSUM for consistent 
configuration and there is no alternative method to modify their state.

Thanks,
Vishal