Message-ID: <89b961d92baebe8a2a541d2eb9ff3e1d9e9ddb52.camel@mellanox.com>
Date:   Wed, 30 Oct 2019 22:50:06 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     Ariel Levkovich <lariel@...lanox.com>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>
CC:     "dsahern@...il.com" <dsahern@...il.com>,
        "sbrivio@...hat.com" <sbrivio@...hat.com>,
        "nikolay@...ulusnetworks.com" <nikolay@...ulusnetworks.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "gerlitz.or@...il.com" <gerlitz.or@...il.com>,
        "sd@...asysnail.net" <sd@...asysnail.net>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [PATCH net-next 2/3] net: Add SRIOV VGT+ support

On Wed, 2019-10-30 at 14:34 -0700, Jakub Kicinski wrote:
> On Wed, 30 Oct 2019 20:44:44 +0000, Ariel Levkovich wrote:
> > VGT+ is a security feature that gives the administrator the ability
> > of
> > controlling the allowed vlan-ids list that can be
> > transmitted/received
> > from/to the VF.
> > The allowed vlan-ids list is called "trunk".
> > Admin can add/remove a range of allowed vlan-ids via iptool.
> > Example:
> > After this series of configuration :
> > 1) ip link set eth3 vf 0 trunk add 10 100 (allow vlan-id 10-100,
> > default tpid 0x8100)
> > 2) ip link set eth3 vf 0 trunk add 105 proto 802.1q (allow vlan-id
> > 105 tpid 0x8100)
> > 3) ip link set eth3 vf 0 trunk add 105 proto 802.1ad (allow vlan-id 
> > 105 tpid 0x88a8)
> > 4) ip link set eth3 vf 0 trunk rem 90 (block vlan-id 90)
> > 5) ip link set eth3 vf 0 trunk rem 50 60 (block vlan-ids 50-60)
> > 
> > The VF 0 can only communicate on vlan-ids: 10-49,61-89,91-100,105
> > with
> > tpid 0x8100 and vlan-id 105 with tpid 0x88a8.
> > 
> > For this purpose we added the following netlink sr-iov commands:
> > 
> > 1) IFLA_VF_VLAN_RANGE: used to add/remove allowed vlan-ids range.
> > We added the ifla_vf_vlan_range struct to specify the range we want
> > to
> > add/remove from the userspace.
> > We added ndo_add_vf_vlan_trunk_range and
> > ndo_del_vf_vlan_trunk_range
> > netdev ops to add/remove allowed vlan-ids range in the netdev.
> > 
> > 2) IFLA_VF_VLAN_TRUNK: used to query the allowed vlan-ids trunk.
> > We added trunk bitmap to the ifla_vf_info struct to get the current
> > allowed vlan-ids trunk from the netdev.
> > We added ifla_vf_vlan_trunk struct for sending the allowed vlan-ids
> > trunk to the userspace.
> > Since the trunk bitmap needs to contain a bit per possible enabled
> > vlan id, the size addition to ifla_vf_info is significant which may
> > create attribute length overrun when querying all the VFs.
> > 
> > Therefore, the return of the full bitmap is limited to the case
> > where the admin queries a specific VF only and for the VF list
> > query we introduce a new vf_info attribute called ifla_vf_vlan_mode
> > that will present the current VF tagging mode - VGT, VST or
> > VGT+(trunk).
> > 
> > Signed-off-by: Ariel Levkovich <lariel@...lanox.com>
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 3207e0b..da79976 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1067,6 +1067,10 @@ struct netdev_name_node {
> >   *      Hash Key. This is needed since on some devices VF share
> > this information
> >   *      with PF and querying it may introduce a theoretical
> > security risk.
> >   * int (*ndo_set_vf_rss_query_en)(struct net_device *dev, int vf,
> > bool setting);
> > + * int (*ndo_add_vf_vlan_trunk_range)(struct net_device *dev, int
> > vf,
> > + *				      u16 start_vid, u16 end_vid,
> > __be16 proto);
> > + * int (*ndo_del_vf_vlan_trunk_range)(struct net_device *dev, int
> > vf,
> > + *				      u16 start_vid, u16 end_vid,
> > __be16 proto);
> >   * int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct
> > sk_buff *skb);
> >   * int (*ndo_setup_tc)(struct net_device *dev, enum tc_setup_type
> > type,
> >   *		       void *type_data);
> > @@ -1332,6 +1336,14 @@ struct net_device_ops {
> >  	int			(*ndo_set_vf_rss_query_en)(
> >  						   struct net_device
> > *dev,
> >  						   int vf, bool
> > setting);
> > +	int			(*ndo_add_vf_vlan_trunk_range)(
> > +						   struct net_device
> > *dev,
> > +						   int vf, u16
> > start_vid,
> > +						   u16 end_vid, __be16
> > proto);
> > +	int			(*ndo_del_vf_vlan_trunk_range)(
> > +						   struct net_device
> > *dev,
> > +						   int vf, u16
> > start_vid,
> > +						   u16 end_vid, __be16
> > proto);
> >  	int			(*ndo_setup_tc)(struct net_device *dev,
> >  						enum tc_setup_type
> > type,
> >  						void *type_data);
> 
> Is this official Mellanox patch submission or do you guys need time
> to
> decide between each other if you like legacy VF ndos or not? ;-)

It is official :), as much as we want to move away from legacy mode, we
do still have two major customers that are not quite ready yet to move
to switchdev mode. the silver-lining here is that they are welling to
move to upstream kernel (advanced distros), but we need this feature in
legacy mode.

The ability to configure per VF ACL tables vlan filters is a must.

I tried to think of an API where we can expose the whole VF ACL tables
to users and let them configure it the way they want with TC flower
maybe (sort of hybrid legacy-switchdev mode that can act only on VF ACL
tables but not on the FDB). The problem with this is that it can easily
conflict with VST/trust mode and other settings that can be done via
legacy VF ndos... so i guess the complexity of such API is not worthy
and a simple vlan list filter API is more natural for legacy sriov ?!

Powered by blists - more mailing lists