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: <PH0PR11MB5095BE6E3FBAC90901DD6EB9D6852@PH0PR11MB5095.namprd11.prod.outlook.com>
Date: Thu, 24 Apr 2025 15:29:03 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: "Badole, Vishal" <vishal.badole@....com>, Paolo Abeni <pabeni@...hat.com>,
	"Shyam-sundar.S-k@....com" <Shyam-sundar.S-k@....com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "davem@...emloft.net"
	<davem@...emloft.net>, "Dumazet, Eric" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "Thomas.Lendacky@....com"
	<Thomas.Lendacky@....com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "stable@...r.kernel.org" <stable@...r.kernel.org>, "Raju.Rangoju@....com"
	<Raju.Rangoju@....com>
Subject: RE: [PATCH net V2] amd-xgbe: Fix to ensure dependent features are
 toggled with RX checksum offload



> -----Original Message-----
> From: Badole, Vishal <vishal.badole@....com>
> Sent: Thursday, April 24, 2025 5:57 AM
> To: Paolo Abeni <pabeni@...hat.com>; Keller, Jacob E
> <jacob.e.keller@...el.com>; Shyam-sundar.S-k@....com;
> andrew+netdev@...n.ch; davem@...emloft.net; Dumazet, Eric
> <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

Sounds reasonable to me, thanks!