Message-ID: <MN2PR11MB422120B08EC5537084B494F1E3540@MN2PR11MB4221.namprd11.prod.outlook.com>
Date:   Wed, 26 Aug 2020 22:23:45 +0000
From:   "Wyborny, Carolyn" <carolyn.wyborny@...el.com>
To:     Shannon Nelson <snelson@...sando.io>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Herbert, Tom" <tom.herbert@...el.com>
Subject: RE: [RFC PATCH net-next 0/2] Granular VF Trust Flags for SR-IOV

> -----Original Message-----
> From: Shannon Nelson <snelson@...sando.io>
> Sent: Tuesday, August 25, 2020 9:32 AM
> To: Wyborny, Carolyn <carolyn.wyborny@...el.com>;
> netdev@...r.kernel.org
> Cc: davem@...emloft.net; kuba@...nel.org; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>; Herbert, Tom <tom.herbert@...el.com>
> Subject: Re: [RFC PATCH net-next 0/2] Granular VF Trust Flags for SR-IOV
> 
> 
> 
> On 8/20/20 6:16 PM, Carolyn Wyborny wrote:
> > Proposal for Granular VF Trust Flags for SR-IOV
> >
> > I would like to propose extending the concept of VF trust in a more
> > granular way by creating VF trust flags. VF Trust Flags would allow more
> > flexibility in assigning privileges to VF's administratively in SR-IOV.
> > Users are asking for more configuration to be available in the VF.
> > Features for one use case like a firewall are not always wanted in a
> > different type of privilegd VF.  If a base set of generic privileges could be
> > configured in a more granular way, they can be combined in a more flexible
> > way by the user.
> >
> > The implementation would do this by by adding a new iflattribute for trust
> > flags which defines the flags in an nla_bitfield32.  The changes `would
> > also include changes to .ndo_set_vf_trust parameters, different or
> converted
> > settings in .ndo_get_vf_config, kernel validation of the trust flags and
> > driver changes for those that implement .ndo_set_vf_trust. There will also
> > be changes proposed for ip link in the iproute2 toolset.
> >
> > This patchset provides an example implementation that is not complete.
> > It does not include the full validation of the feature flags in the kernel,
> > all the helper macros likely needed for the trust flags nor all the driver
> > changes needed. It also needs a method for advertising supported
> privileges
> > and validation to ensure unsupported privileges are not being set.
> > It does have a simple example driver implementation in igb.  The full
> > patchset will include all these things.
> >
> > I'd like to start the discussion about the general idea and then begin the
> > dicussion about a base set of VF privleges that would be generic across the
> > device vendors.
> >
> > ---
> 
> Hi Carolyn, thanks for sending this out, and for your presentation at
> NetDev last week.  Here are some initial thoughts and questions I had to
> get the discussion going.
> 
> Would this ever need to be extended to the sub-function devices (sf)
> that some devlink threads are discussing?

Thanks Shannon,

Yes, in some form but IIUC, subfunctions are not SR-IOV and I'm not exactly sure of the interaction between them.  I realize that orchestration software also has this concept of trust/privilege but its handled at a higher layer than SR-IOV.  So, while this proposal is a narrow focus on SR-IOV specifically, I think this is a good question.  Perhaps an overall security model should be detailed or defined where there are gaps that shows the pieces and how they go together.  

> 
> What would the user-land side of this look like?  Would this be an
> extension of the existing ip link set dev <pf> vf <vfid> <attr>
> <value>?  How would these attributes be named?
Yes, I was thinking ip link set <pf> vf <vfid> trust val <val>, but am open to better ideas.

> 
> Will enabling the legacy trust include trusting all current and future
> trust items, or should it be limited to the current set?  If limited,
> then you might add a macro for VF_TRUST_F_ALL_LEGACY.  Not sure
> whether
> or not this would be a good thing.
My initial thinking was that current tools would use the flags, even if its legacy trust and the providing of legacy trust functionality would be done at the driver side.  Anything using the old nla_vf_trust attribute would need the correct handling in the drivers to provide what looks like legacy trust.  Also, any trust flag set, would cause the old trust setting to be configured in order to provide the needed backwards compatibility.

> 
> Instead of SPFCHK_DIS or "spoofchk_disable" - can we get replace the
> reverse logic and rename these?
So, this is intended for the privilege to disable the spoofchk feature.  It’s a security feature that's enabled by default.  I'm open to renaming though.  What do you think would be better?  

> 
> Permission bits might be needed for allowing RSS configuration and
> bandwidth limits.
Yes, as Srijeet mentioned.  Can add two for RSS config and bandwidth on the next submission.
> 
> Will there need to be more granularity around the Advanced Flow
> configuration abilities?
This is a good question and after looking around at the other driver's use of the original trust feature, I wasn't sure if we need just one flag here or more.  What would you suggest from your device perspective?

> 
> Should there be permission bits for changing settings found in
> ethtool-based settings like link-ksettings, coalesce, pause, speed, etc?
I think once we have this privilege scheme implemented, it might make sense to consider this.  This brings up another potential policy question around VF's knowing their configuration or not.  I would prefer to allow admin's to make policy decisions but in a virtualized world there are two layers of admin.  Maybe its best to support both models.  

> 
> How can we guide/manage this to be sure we don't end up with vendors
> pushing device specific permission bits or feature enabling bits rather
> than generic permissions?
I was hoping that the kernel submission process would genericize them flags by default, but perhaps that optimistic.  Is there another feature or system that has solved this potential problem or do we keep creating it for ourselves?    

> 
> Do we really need a typedef for vf_trust_flags_t, or can we keep with a
> simple type?
Possibly not, I'm open to design suggestions.  I think the bitfield ops and helpers are the more important things to implement well.  

Thanks,

Carolyn
> 
> Cheers,
> sln

Powered by blists - more mailing lists