netdev - RE: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI aggregator node ID information

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: <BL0PR11MB3521FE2860D9AABE3A4312D48F8CA@BL0PR11MB3521.namprd11.prod.outlook.com>
Date: Thu, 14 Dec 2023 09:55:22 +0000
From: "Romanowski, Rafal" <rafal.romanowski@...el.com>
To: Simon Horman <horms@...nel.org>, "Keller, Jacob E"
	<jacob.e.keller@...el.com>
CC: ivecera <ivecera@...hat.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
	Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI
 aggregator node ID information

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of
> Simon Horman
> Sent: Sunday, December 10, 2023 12:45 PM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: ivecera <ivecera@...hat.com>; netdev@...r.kernel.org; Nguyen, Anthony
> L <anthony.l.nguyen@...el.com>; Intel Wired LAN <intel-wired-
> lan@...ts.osuosl.org>; Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net] ice: stop trashing VF VSI
> aggregator node ID information
> 
> + Ivan Vecera <ivecera@...hat.com>
> 
> On Wed, Dec 06, 2023 at 12:19:05PM -0800, Jacob Keller wrote:
> > When creating new VSIs, they are assigned into an aggregator node in
> > the scheduler tree. Information about which aggregator node a VSI is
> > assigned into is maintained by the vsi->agg_node structure. In
> > ice_vsi_decfg(), this information is being destroyed, by overwriting
> > the valid flag and the agg_id field to zero.
> >
> > For VF VSIs, this breaks the aggregator node configuration replay,
> > which depends on this information. This results in VFs being inserted
> > into the default aggregator node. The resulting configuration will
> > have unexpected Tx bandwidth sharing behavior.
> >
> > This was broken by commit 6624e780a577 ("ice: split ice_vsi_setup into
> > smaller functions"), which added the block to reset the agg_node data.
> >
> > The vsi->agg_node structure is not managed by the scheduler code, but
> > is instead a wrapper around an aggregator node ID that is tracked at
> > the VSI layer. Its been around for a long time, and its primary
> > purpose was for handling VFs. The SR-IOV VF reset flow does not make
> > use of the standard VSI rebuild/replay logic, and uses vsi->agg_node
> > as part of its handling to rebuild the aggregator node configuration.
> >
> > The logic for aggregator nodes stretches  back to early ice driver
> > code from commit b126bd6bcd67 ("ice: create scheduler aggregator node
> > config and move
> > VSIs")
> >
> > The logic in ice_vsi_decfg() which trashes the ice_agg_node data is
> > clearly wrong. It destroys information that is necessary for handling
> > VF reset,. It is also not the correct way to actually remove a VSI
> > from an aggregator node. For that, we need to implement logic in the
> > scheduler code. Further, non-VF VSIs properly replay their aggregator
> > configuration using existing scheduler replay logic.
> >
> > To fix the VF replay logic, remove this broken aggregator node cleanup
> > logic. This is the simplest way to immediately fix this.
> >
> > This ensures that VFs will have proper aggregate configuration after a
> > reset. This is especially important since VFs often perform resets as
> > part of their reconfiguration flows. Without fixing this, VFs will be
> > placed in the default aggregator node and Tx bandwidth will not be
> > shared in the expected and configured manner.
> >
> > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller
> > functions")
> > Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> > ---
> > This is the simplest fix to resolve the aggregator node problem.
> > However, I think we should clean this up properly. I don't know why
> > the VF VSIs have their own custom code for replaying aggregator
> > configuration. I also think its odd that there is both structures to
> > track aggregator information in ice_sched.c, but we use a separate
> > structure in ice.h for the ice_vsi structure. I plan to investigate
> > this and clean it up in next. However, I wanted to get a smaller fix out to net
> sooner rather than later.
> 
> Less is more, for now :)
> 
> Reviewed-by: Simon Horman <horms@...nel.org>
> 
> I've added Ivan to the CC list in case he wants to review this too.
> 
> >
> >  drivers/net/ethernet/intel/ice/ice_lib.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> > b/drivers/net/ethernet/intel/ice/ice_lib.c
> > index 4b1e56396293..de7ba87af45d 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c

Tested-by: Rafal Romanowski <rafal.romanowski@...el.com>