| Message ID | 7F861DC0615E0C47A872E6F3C5FCDDBD05EB28F4@BPXM14GP.gisp.nec.co.jp |
|---|---|
| State | Superseded |
| Delegated to: | Jeff Kirsher |
| Headers | show |
> -----Original Message----- > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > Sent: Tuesday, May 19, 2015 5:06 PM > To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org > Cc: Skidmore, Donald C; Or Gerlitz; David Miller; Linux Netdev List; > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com; > Choi, Sy Jong; Edward Cree; Rony Efraim > Subject: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > > Implement the new netdev op to trust VF in ixgbe and make VF multicast > promiscuous mode enabled only in trusted VF. > > The administrator can make VF trusted by ip command which supports trust > message. > # ip link set dev eth0 vf 1 trust on > > After making VF untrusted, ixgbe disables VF multicast promiscuous feature > requested from VF. > # ip link set dev eth0 vf 1 trust off > > Only trusted VF can enable VF multicast promiscuous mode and handle over > 30 IPv6 addresses on VM, because VF multicast promiscuous mode may hurt > performance. > > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com> > CC: Choi, Sy Jong <sy.jong.choi@intel.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 + > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++++ > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 38 > +++++++++++++++++++++++--- > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 2 ++ > 4 files changed, 42 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > index 08e65b6..5181a4d 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > @@ -153,6 +153,7 @@ struct vf_data_storage { > u16 vlan_count; > u8 spoofchk_enabled; > bool rss_query_enabled; > + u8 trusted; > unsigned int vf_api; > }; > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index b1ea707..263cb40 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -3679,6 +3679,10 @@ static void ixgbe_configure_virtualization(struct > ixgbe_adapter *adapter) > /* Enable/Disable RSS query feature */ > ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i, > adapter- > >vfinfo[i].rss_query_enabled); > + > + /* Reconfigure features in trusted */ > + ixgbe_ndo_set_vf_trust(adapter->netdev, i, > + adapter->vfinfo[i].trusted); > } > } > > @@ -8182,6 +8186,7 @@ static const struct net_device_ops > ixgbe_netdev_ops = { > .ndo_set_vf_rate = ixgbe_ndo_set_vf_bw, > .ndo_set_vf_spoofchk = ixgbe_ndo_set_vf_spoofchk, > .ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en, > + .ndo_set_vf_trust = ixgbe_ndo_set_vf_trust, > .ndo_get_vf_config = ixgbe_ndo_get_vf_config, > .ndo_get_stats64 = ixgbe_get_stats64, > #ifdef CONFIG_IXGBE_DCB > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > index 615f651..6c602bc 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > @@ -117,8 +117,11 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter > *adapter) > */ > adapter->vfinfo[i].rss_query_enabled = 0; > > - /* Turn multicast promiscuous mode off for all VFs */ > + /* Disallow VF multicast promiscuous capability > + * and turn it off for all VFs > + */ > adapter->vfinfo[i].mc_promisc = false; > + adapter->vfinfo[i].trusted = false; > } > > return 0; > @@ -329,9 +332,14 @@ static int ixgbe_enable_vf_mc_promisc(struct > ixgbe_adapter *adapter, u32 vf) > hw = &adapter->hw; > vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf)); > > - e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > - > - vmolr |= IXGBE_VMOLR_MPE; > + if (adapter->vfinfo[vf].trusted) { > + e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > + vmolr |= IXGBE_VMOLR_MPE; > + } else { > + e_info(drv, "VF %u: disabling multicast promiscuous " > + "on untrusted VF.\n", vf); > + vmolr &= ~IXGBE_VMOLR_MPE; > + } > > IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr); > > @@ -1492,6 +1500,27 @@ int ixgbe_ndo_set_vf_rss_query_en(struct > net_device *netdev, int vf, > return 0; > } > > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool > +setting) { > + struct ixgbe_adapter *adapter = netdev_priv(netdev); > + > + if (vf >= adapter->num_vfs) > + return -EINVAL; > + > + /* nothing to do */ > + if (adapter->vfinfo[vf].trusted == setting) > + return 0; > + > + adapter->vfinfo[vf].trusted = setting; > + > + /* Reconfigure features which are only allowed for trusted VF */ > + /* VF multicast promiscuous mode */ > + if (adapter->vfinfo[vf].mc_promisc) > + ixgbe_enable_vf_mc_promisc(adapter, vf); > + > + return 0; > +} > + > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > int vf, struct ifla_vf_info *ivi) { @@ -1506,5 +1535,6 > @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev, > ivi->qos = adapter->vfinfo[vf].pf_qos; > ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled; > ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled; > + ivi->trusted = adapter->vfinfo[vf].trusted; > return 0; > } > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > index 2c197e6..d85e6fc 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > @@ -49,6 +49,8 @@ int ixgbe_ndo_set_vf_bw(struct net_device *netdev, > int vf, int min_tx_rate, int ixgbe_ndo_set_vf_spoofchk(struct net_device > *netdev, int vf, bool setting); int ixgbe_ndo_set_vf_rss_query_en(struct > net_device *netdev, int vf, > bool setting); > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, > + int vf, bool setting); > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > int vf, struct ifla_vf_info *ivi); void > ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter); > -- > 1.8.3.1 Hey Hiroshi, In general I like your patch set. There is a little complexity I’m not sure I understand. I'm assuming that: adapter->vfinfo[vf].trusted - Clearly stores if the PF trusts a given VF (i.e. allows it to go into "risky" configurations) What I'm a bit unclear about is: adapter->vfinfo[vf].mc_promisc - This seems to record that the VF at one time as requested over 30 MC. I don't understand the reason for this bit. Wouldn't it be simpler and more straightforward to simply use the trusted bit? I guess specifically I don't understand why we would call ixgbe_enable_vf_mc_promisc() in ixgbe_ndo_set_vf_trust() if mc_promisc is set. Wouldn't just setting the trusted bit allow the next IXGBE_VF_SET_MC_PROMISC mailbox message to (possibly) turn on MC Promisc mode? Thanks, -Don Skidmore <donald.c.skidmore@intel.com>
> > -----Original Message----- > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > Sent: Tuesday, May 19, 2015 5:06 PM > > To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org > > Cc: Skidmore, Donald C; Or Gerlitz; David Miller; Linux Netdev List; > > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com; > > Choi, Sy Jong; Edward Cree; Rony Efraim > > Subject: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > > > > Implement the new netdev op to trust VF in ixgbe and make VF multicast > > promiscuous mode enabled only in trusted VF. > > > > The administrator can make VF trusted by ip command which supports trust > > message. > > # ip link set dev eth0 vf 1 trust on > > > > After making VF untrusted, ixgbe disables VF multicast promiscuous feature > > requested from VF. > > # ip link set dev eth0 vf 1 trust off > > > > Only trusted VF can enable VF multicast promiscuous mode and handle over > > 30 IPv6 addresses on VM, because VF multicast promiscuous mode may hurt > > performance. > > > > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > > Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com> > > CC: Choi, Sy Jong <sy.jong.choi@intel.com> > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 + > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++++ > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 38 > > +++++++++++++++++++++++--- > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 2 ++ > > 4 files changed, 42 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > index 08e65b6..5181a4d 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > @@ -153,6 +153,7 @@ struct vf_data_storage { > > u16 vlan_count; > > u8 spoofchk_enabled; > > bool rss_query_enabled; > > + u8 trusted; > > unsigned int vf_api; > > }; > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index b1ea707..263cb40 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -3679,6 +3679,10 @@ static void ixgbe_configure_virtualization(struct > > ixgbe_adapter *adapter) > > /* Enable/Disable RSS query feature */ > > ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i, > > adapter- > > >vfinfo[i].rss_query_enabled); > > + > > + /* Reconfigure features in trusted */ > > + ixgbe_ndo_set_vf_trust(adapter->netdev, i, > > + adapter->vfinfo[i].trusted); > > } > > } > > > > @@ -8182,6 +8186,7 @@ static const struct net_device_ops > > ixgbe_netdev_ops = { > > .ndo_set_vf_rate = ixgbe_ndo_set_vf_bw, > > .ndo_set_vf_spoofchk = ixgbe_ndo_set_vf_spoofchk, > > .ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en, > > + .ndo_set_vf_trust = ixgbe_ndo_set_vf_trust, > > .ndo_get_vf_config = ixgbe_ndo_get_vf_config, > > .ndo_get_stats64 = ixgbe_get_stats64, > > #ifdef CONFIG_IXGBE_DCB > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > index 615f651..6c602bc 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > @@ -117,8 +117,11 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter > > *adapter) > > */ > > adapter->vfinfo[i].rss_query_enabled = 0; > > > > - /* Turn multicast promiscuous mode off for all VFs */ > > + /* Disallow VF multicast promiscuous capability > > + * and turn it off for all VFs > > + */ > > adapter->vfinfo[i].mc_promisc = false; > > + adapter->vfinfo[i].trusted = false; > > } > > > > return 0; > > @@ -329,9 +332,14 @@ static int ixgbe_enable_vf_mc_promisc(struct > > ixgbe_adapter *adapter, u32 vf) > > hw = &adapter->hw; > > vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf)); > > > > - e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > > - > > - vmolr |= IXGBE_VMOLR_MPE; > > + if (adapter->vfinfo[vf].trusted) { > > + e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > > + vmolr |= IXGBE_VMOLR_MPE; > > + } else { > > + e_info(drv, "VF %u: disabling multicast promiscuous " > > + "on untrusted VF.\n", vf); > > + vmolr &= ~IXGBE_VMOLR_MPE; > > + } > > > > IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr); > > > > @@ -1492,6 +1500,27 @@ int ixgbe_ndo_set_vf_rss_query_en(struct > > net_device *netdev, int vf, > > return 0; > > } > > > > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool > > +setting) { > > + struct ixgbe_adapter *adapter = netdev_priv(netdev); > > + > > + if (vf >= adapter->num_vfs) > > + return -EINVAL; > > + > > + /* nothing to do */ > > + if (adapter->vfinfo[vf].trusted == setting) > > + return 0; > > + > > + adapter->vfinfo[vf].trusted = setting; > > + > > + /* Reconfigure features which are only allowed for trusted VF */ > > + /* VF multicast promiscuous mode */ > > + if (adapter->vfinfo[vf].mc_promisc) > > + ixgbe_enable_vf_mc_promisc(adapter, vf); > > + > > + return 0; > > +} > > + > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > int vf, struct ifla_vf_info *ivi) { @@ -1506,5 +1535,6 > > @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > ivi->qos = adapter->vfinfo[vf].pf_qos; > > ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled; > > ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled; > > + ivi->trusted = adapter->vfinfo[vf].trusted; > > return 0; > > } > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > index 2c197e6..d85e6fc 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > @@ -49,6 +49,8 @@ int ixgbe_ndo_set_vf_bw(struct net_device *netdev, > > int vf, int min_tx_rate, int ixgbe_ndo_set_vf_spoofchk(struct net_device > > *netdev, int vf, bool setting); int ixgbe_ndo_set_vf_rss_query_en(struct > > net_device *netdev, int vf, > > bool setting); > > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, > > + int vf, bool setting); > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > int vf, struct ifla_vf_info *ivi); void > > ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter); > > -- > > 1.8.3.1 > > Hey Hiroshi, > > In general I like your patch set. There is a little complexity I’m not sure I understand. I'm assuming that: > > adapter->vfinfo[vf].trusted - Clearly stores if the PF trusts a given VF (i.e. allows it to go into "risky" configurations) > > What I'm a bit unclear about is: > > adapter->vfinfo[vf].mc_promisc - This seems to record that the VF at one time as requested over 30 MC. Yes, this is for keeping MC promisc request from VF. > > I don't understand the reason for this bit. Wouldn't it be simpler and more straightforward to simply use the trusted > bit? I guess specifically I don't understand why we would call ixgbe_enable_vf_mc_promisc() in ixgbe_ndo_set_vf_trust() > if mc_promisc is set. Wouldn't just setting the trusted bit allow the next IXGBE_VF_SET_MC_PROMISC mailbox message to > (possibly) turn on MC Promisc mode? I think that it's better to separate the features, to trust VF and to enable MC promisc. We might add other VF features which should be allowed only trusted VF. And it's good to have a knob to trust for allowing a such feature dynamically. The administrator can turn off or on MC promisc whenever it's needed. thanks, Hiroshi > > Thanks, > -Don Skidmore <donald.c.skidmore@intel.com> >
> -----Original Message----- > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > Sent: Wednesday, May 20, 2015 9:13 PM > To: Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org > Cc: Or Gerlitz; David Miller; Linux Netdev List; nhorman@redhat.com; > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward Cree; > Rony Efraim > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > -----Original Message----- > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > > Sent: Tuesday, May 19, 2015 5:06 PM > > > To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org > > > Cc: Skidmore, Donald C; Or Gerlitz; David Miller; Linux Netdev List; > > > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com; > Choi, > > > Sy Jong; Edward Cree; Rony Efraim > > > Subject: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > > > > > > Implement the new netdev op to trust VF in ixgbe and make VF > > > multicast promiscuous mode enabled only in trusted VF. > > > > > > The administrator can make VF trusted by ip command which supports > > > trust message. > > > # ip link set dev eth0 vf 1 trust on > > > > > > After making VF untrusted, ixgbe disables VF multicast promiscuous > > > feature requested from VF. > > > # ip link set dev eth0 vf 1 trust off > > > > > > Only trusted VF can enable VF multicast promiscuous mode and handle > > > over > > > 30 IPv6 addresses on VM, because VF multicast promiscuous mode may > > > hurt performance. > > > > > > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > > > Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com> > > > CC: Choi, Sy Jong <sy.jong.choi@intel.com> > > > --- > > > drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 + > > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++++ > > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 38 > > > +++++++++++++++++++++++--- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 2 ++ > > > 4 files changed, 42 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > index 08e65b6..5181a4d 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > @@ -153,6 +153,7 @@ struct vf_data_storage { > > > u16 vlan_count; > > > u8 spoofchk_enabled; > > > bool rss_query_enabled; > > > + u8 trusted; > > > unsigned int vf_api; > > > }; > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > index b1ea707..263cb40 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > @@ -3679,6 +3679,10 @@ static void > > > ixgbe_configure_virtualization(struct > > > ixgbe_adapter *adapter) > > > /* Enable/Disable RSS query feature */ > > > ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i, > > > adapter- > > > >vfinfo[i].rss_query_enabled); > > > + > > > + /* Reconfigure features in trusted */ > > > + ixgbe_ndo_set_vf_trust(adapter->netdev, i, > > > + adapter->vfinfo[i].trusted); > > > } > > > } > > > > > > @@ -8182,6 +8186,7 @@ static const struct net_device_ops > > > ixgbe_netdev_ops = { > > > .ndo_set_vf_rate = ixgbe_ndo_set_vf_bw, > > > .ndo_set_vf_spoofchk = ixgbe_ndo_set_vf_spoofchk, > > > .ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en, > > > + .ndo_set_vf_trust = ixgbe_ndo_set_vf_trust, > > > .ndo_get_vf_config = ixgbe_ndo_get_vf_config, > > > .ndo_get_stats64 = ixgbe_get_stats64, > > > #ifdef CONFIG_IXGBE_DCB > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > index 615f651..6c602bc 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > @@ -117,8 +117,11 @@ static int __ixgbe_enable_sriov(struct > > > ixgbe_adapter > > > *adapter) > > > */ > > > adapter->vfinfo[i].rss_query_enabled = 0; > > > > > > - /* Turn multicast promiscuous mode off for all VFs */ > > > + /* Disallow VF multicast promiscuous capability > > > + * and turn it off for all VFs > > > + */ > > > adapter->vfinfo[i].mc_promisc = false; > > > + adapter->vfinfo[i].trusted = false; > > > } > > > > > > return 0; > > > @@ -329,9 +332,14 @@ static int ixgbe_enable_vf_mc_promisc(struct > > > ixgbe_adapter *adapter, u32 vf) > > > hw = &adapter->hw; > > > vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf)); > > > > > > - e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > > > - > > > - vmolr |= IXGBE_VMOLR_MPE; > > > + if (adapter->vfinfo[vf].trusted) { > > > + e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > > > + vmolr |= IXGBE_VMOLR_MPE; > > > + } else { > > > + e_info(drv, "VF %u: disabling multicast promiscuous " > > > + "on untrusted VF.\n", vf); > > > + vmolr &= ~IXGBE_VMOLR_MPE; > > > + } > > > > > > IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr); > > > > > > @@ -1492,6 +1500,27 @@ int ixgbe_ndo_set_vf_rss_query_en(struct > > > net_device *netdev, int vf, > > > return 0; > > > } > > > > > > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool > > > +setting) { > > > + struct ixgbe_adapter *adapter = netdev_priv(netdev); > > > + > > > + if (vf >= adapter->num_vfs) > > > + return -EINVAL; > > > + > > > + /* nothing to do */ > > > + if (adapter->vfinfo[vf].trusted == setting) > > > + return 0; > > > + > > > + adapter->vfinfo[vf].trusted = setting; > > > + > > > + /* Reconfigure features which are only allowed for trusted VF */ > > > + /* VF multicast promiscuous mode */ > > > + if (adapter->vfinfo[vf].mc_promisc) > > > + ixgbe_enable_vf_mc_promisc(adapter, vf); > > > + > > > + return 0; > > > +} > > > + > > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > > int vf, struct ifla_vf_info *ivi) { @@ -1506,5 +1535,6 > @@ > > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > > ivi->qos = adapter->vfinfo[vf].pf_qos; > > > ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled; > > > ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled; > > > + ivi->trusted = adapter->vfinfo[vf].trusted; > > > return 0; > > > } > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > index 2c197e6..d85e6fc 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > @@ -49,6 +49,8 @@ int ixgbe_ndo_set_vf_bw(struct net_device > *netdev, > > > int vf, int min_tx_rate, int ixgbe_ndo_set_vf_spoofchk(struct > > > net_device *netdev, int vf, bool setting); int > > > ixgbe_ndo_set_vf_rss_query_en(struct > > > net_device *netdev, int vf, > > > bool setting); > > > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, > > > + int vf, bool setting); > > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > > int vf, struct ifla_vf_info *ivi); void > > > ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter); > > > -- > > > 1.8.3.1 > > > > Hey Hiroshi, > > > > In general I like your patch set. There is a little complexity I’m not sure I > understand. I'm assuming that: > > > > adapter->vfinfo[vf].trusted - Clearly stores if the PF trusts a given > > VF (i.e. allows it to go into "risky" configurations) > > > > What I'm a bit unclear about is: > > > > adapter->vfinfo[vf].mc_promisc - This seems to record that the VF at one > time as requested over 30 MC. > > Yes, this is for keeping MC promisc request from VF. > > > > > I don't understand the reason for this bit. Wouldn't it be simpler and more > straightforward to simply use the trusted > > bit? I guess specifically I don't understand why we would call > ixgbe_enable_vf_mc_promisc() in ixgbe_ndo_set_vf_trust() > > if mc_promisc is set. Wouldn't just setting the trusted bit allow the > > next IXGBE_VF_SET_MC_PROMISC mailbox message to > > (possibly) turn on MC Promisc mode? > > I think that it's better to separate the features, to trust VF and to enable MC > promisc. > We might add other VF features which should be allowed only trusted VF. > And it's good to have a knob to trust for allowing a such feature dynamically. > The administrator can turn off or on MC promisc whenever it's needed. My thoughts were that by enabling a VF to be trusted you would be allowing that VF to enter into MC promisc mode. To actually enter promisc MC mode all the VF would need to do is request more than 30 MC groups through the mailbox to the PF. This all makes sense to me and seems to be what you are doing. My confusion comes, which I didn't explain all that well in my last email, comes from the following flow. In the case where we are NOT trusted (adapter->vfinfo[vf].trusted == false). - we get a IXGBE_VF_SET_MC_PROMISC from the VF. - enter ixgbe_set_vf_mc_promisc() + set adapter->vfinfo[vf].mc_promisc = enable + Call return ixgbe_enable_vf_mc_promisc() ++ we do NOT set IXGBE_VMOLR_MPE since (adapter->vfinfo[vf].trusted == false) - later some one turns on the trusted nod for this VF. - ixgbe_ndo_set_vf_trust() sets the trusted flag - <this is the part I don't understand> if the mc_promisc flag set, which it is from above, we call ixgbe_enable_vf_mc_promisc(). It seems to me we are saving the fact that at one time in the past the VF requested to enter MC Promisc Mode (IXGBE_VF_SET_MC_PROMISC) and failed since we were NOT trusted at the time. Later if we do set this VF as "trusted" we remember that fact and put the VF immediately into MC Promisc Mode. Is this your take as well? My concerns comes from entering MC Promisc Mode based on something that happened in the past. This may not be as big an issues since I notice the flag that remembers this state is cleared on reset or bringing the adapter down. Still I don't see the reason for saving that this request was made. When it was made the VF wasn't "trusted" and we failed, like we should have. Later when the VF becomes "trusted" I would expect it to NOT enter the MC promisc state until the VF requested for it again. Am I missing something in my understanding here? Thanks, -Don Skidmore <donald.c.skidmore@intel.com>
> > -----Original Message----- > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > Sent: Wednesday, May 20, 2015 9:13 PM > > To: Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org > > Cc: Or Gerlitz; David Miller; Linux Netdev List; nhorman@redhat.com; > > sassmann@redhat.com; jogreene@redhat.com; Choi, Sy Jong; Edward Cree; > > Rony Efraim > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > -----Original Message----- > > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > > > Sent: Tuesday, May 19, 2015 5:06 PM > > > > To: Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org > > > > Cc: Skidmore, Donald C; Or Gerlitz; David Miller; Linux Netdev List; > > > > nhorman@redhat.com; sassmann@redhat.com; jogreene@redhat.com; > > Choi, > > > > Sy Jong; Edward Cree; Rony Efraim > > > > Subject: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > > > > > > > > Implement the new netdev op to trust VF in ixgbe and make VF > > > > multicast promiscuous mode enabled only in trusted VF. > > > > > > > > The administrator can make VF trusted by ip command which supports > > > > trust message. > > > > # ip link set dev eth0 vf 1 trust on > > > > > > > > After making VF untrusted, ixgbe disables VF multicast promiscuous > > > > feature requested from VF. > > > > # ip link set dev eth0 vf 1 trust off > > > > > > > > Only trusted VF can enable VF multicast promiscuous mode and handle > > > > over > > > > 30 IPv6 addresses on VM, because VF multicast promiscuous mode may > > > > hurt performance. > > > > > > > > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> > > > > Reviewed-by: Hayato Momma <h-momma@ce.jp.nec.com> > > > > CC: Choi, Sy Jong <sy.jong.choi@intel.com> > > > > --- > > > > drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 + > > > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 ++++ > > > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 38 > > > > +++++++++++++++++++++++--- > > > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h | 2 ++ > > > > 4 files changed, 42 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > > index 08e65b6..5181a4d 100644 > > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h > > > > @@ -153,6 +153,7 @@ struct vf_data_storage { > > > > u16 vlan_count; > > > > u8 spoofchk_enabled; > > > > bool rss_query_enabled; > > > > + u8 trusted; > > > > unsigned int vf_api; > > > > }; > > > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > > index b1ea707..263cb40 100644 > > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > > > @@ -3679,6 +3679,10 @@ static void > > > > ixgbe_configure_virtualization(struct > > > > ixgbe_adapter *adapter) > > > > /* Enable/Disable RSS query feature */ > > > > ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i, > > > > adapter- > > > > >vfinfo[i].rss_query_enabled); > > > > + > > > > + /* Reconfigure features in trusted */ > > > > + ixgbe_ndo_set_vf_trust(adapter->netdev, i, > > > > + adapter->vfinfo[i].trusted); > > > > } > > > > } > > > > > > > > @@ -8182,6 +8186,7 @@ static const struct net_device_ops > > > > ixgbe_netdev_ops = { > > > > .ndo_set_vf_rate = ixgbe_ndo_set_vf_bw, > > > > .ndo_set_vf_spoofchk = ixgbe_ndo_set_vf_spoofchk, > > > > .ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en, > > > > + .ndo_set_vf_trust = ixgbe_ndo_set_vf_trust, > > > > .ndo_get_vf_config = ixgbe_ndo_get_vf_config, > > > > .ndo_get_stats64 = ixgbe_get_stats64, > > > > #ifdef CONFIG_IXGBE_DCB > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > > index 615f651..6c602bc 100644 > > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > > @@ -117,8 +117,11 @@ static int __ixgbe_enable_sriov(struct > > > > ixgbe_adapter > > > > *adapter) > > > > */ > > > > adapter->vfinfo[i].rss_query_enabled = 0; > > > > > > > > - /* Turn multicast promiscuous mode off for all VFs */ > > > > + /* Disallow VF multicast promiscuous capability > > > > + * and turn it off for all VFs > > > > + */ > > > > adapter->vfinfo[i].mc_promisc = false; > > > > + adapter->vfinfo[i].trusted = false; > > > > } > > > > > > > > return 0; > > > > @@ -329,9 +332,14 @@ static int ixgbe_enable_vf_mc_promisc(struct > > > > ixgbe_adapter *adapter, u32 vf) > > > > hw = &adapter->hw; > > > > vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf)); > > > > > > > > - e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > > > > - > > > > - vmolr |= IXGBE_VMOLR_MPE; > > > > + if (adapter->vfinfo[vf].trusted) { > > > > + e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); > > > > + vmolr |= IXGBE_VMOLR_MPE; > > > > + } else { > > > > + e_info(drv, "VF %u: disabling multicast promiscuous " > > > > + "on untrusted VF.\n", vf); > > > > + vmolr &= ~IXGBE_VMOLR_MPE; > > > > + } > > > > > > > > IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr); > > > > > > > > @@ -1492,6 +1500,27 @@ int ixgbe_ndo_set_vf_rss_query_en(struct > > > > net_device *netdev, int vf, > > > > return 0; > > > > } > > > > > > > > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool > > > > +setting) { > > > > + struct ixgbe_adapter *adapter = netdev_priv(netdev); > > > > + > > > > + if (vf >= adapter->num_vfs) > > > > + return -EINVAL; > > > > + > > > > + /* nothing to do */ > > > > + if (adapter->vfinfo[vf].trusted == setting) > > > > + return 0; > > > > + > > > > + adapter->vfinfo[vf].trusted = setting; > > > > + > > > > + /* Reconfigure features which are only allowed for trusted VF */ > > > > + /* VF multicast promiscuous mode */ > > > > + if (adapter->vfinfo[vf].mc_promisc) > > > > + ixgbe_enable_vf_mc_promisc(adapter, vf); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > > > int vf, struct ifla_vf_info *ivi) { @@ -1506,5 +1535,6 > > @@ > > > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > > > ivi->qos = adapter->vfinfo[vf].pf_qos; > > > > ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled; > > > > ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled; > > > > + ivi->trusted = adapter->vfinfo[vf].trusted; > > > > return 0; > > > > } > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > > index 2c197e6..d85e6fc 100644 > > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h > > > > @@ -49,6 +49,8 @@ int ixgbe_ndo_set_vf_bw(struct net_device > > *netdev, > > > > int vf, int min_tx_rate, int ixgbe_ndo_set_vf_spoofchk(struct > > > > net_device *netdev, int vf, bool setting); int > > > > ixgbe_ndo_set_vf_rss_query_en(struct > > > > net_device *netdev, int vf, > > > > bool setting); > > > > +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, > > > > + int vf, bool setting); > > > > int ixgbe_ndo_get_vf_config(struct net_device *netdev, > > > > int vf, struct ifla_vf_info *ivi); void > > > > ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter); > > > > -- > > > > 1.8.3.1 > > > > > > Hey Hiroshi, > > > > > > In general I like your patch set. There is a little complexity I’m not sure I > > understand. I'm assuming that: > > > > > > adapter->vfinfo[vf].trusted - Clearly stores if the PF trusts a given > > > VF (i.e. allows it to go into "risky" configurations) > > > > > > What I'm a bit unclear about is: > > > > > > adapter->vfinfo[vf].mc_promisc - This seems to record that the VF at one > > time as requested over 30 MC. > > > > Yes, this is for keeping MC promisc request from VF. > > > > > > > > I don't understand the reason for this bit. Wouldn't it be simpler and more > > straightforward to simply use the trusted > > > bit? I guess specifically I don't understand why we would call > > ixgbe_enable_vf_mc_promisc() in ixgbe_ndo_set_vf_trust() > > > if mc_promisc is set. Wouldn't just setting the trusted bit allow the > > > next IXGBE_VF_SET_MC_PROMISC mailbox message to > > > (possibly) turn on MC Promisc mode? > > > > I think that it's better to separate the features, to trust VF and to enable MC > > promisc. > > We might add other VF features which should be allowed only trusted VF. > > And it's good to have a knob to trust for allowing a such feature dynamically. > > The administrator can turn off or on MC promisc whenever it's needed. > > > My thoughts were that by enabling a VF to be trusted you would be allowing that VF to enter into MC promisc mode. To > actually enter promisc MC mode all the VF would need to do is request more than 30 MC groups through the mailbox to the > PF. This all makes sense to me and seems to be what you are doing. > > My confusion comes, which I didn't explain all that well in my last email, comes from the following flow. > > In the case where we are NOT trusted (adapter->vfinfo[vf].trusted == false). > > - we get a IXGBE_VF_SET_MC_PROMISC from the VF. > - enter ixgbe_set_vf_mc_promisc() > + set adapter->vfinfo[vf].mc_promisc = enable > + Call return ixgbe_enable_vf_mc_promisc() > ++ we do NOT set IXGBE_VMOLR_MPE since (adapter->vfinfo[vf].trusted == false) > > - later some one turns on the trusted nod for this VF. > - ixgbe_ndo_set_vf_trust() sets the trusted flag > - <this is the part I don't understand> if the mc_promisc flag set, which it is from above, we call > ixgbe_enable_vf_mc_promisc(). > > > It seems to me we are saving the fact that at one time in the past the VF requested to enter MC Promisc Mode > (IXGBE_VF_SET_MC_PROMISC) and failed since we were NOT trusted at the time. Later if we do set this VF as "trusted" we > remember that fact and put the VF immediately into MC Promisc Mode. Is this your take as well? > > My concerns comes from entering MC Promisc Mode based on something that happened in the past. This may not be as big > an issues since I notice the flag that remembers this state is cleared on reset or bringing the adapter down. Still I > don't see the reason for saving that this request was made. When it was made the VF wasn't "trusted" and we failed, like > we should have. Later when the VF becomes "trusted" I would expect it to NOT enter the MC promisc state until the VF > requested for it again. I think your concerns are related to some operational assumptions. My basic concept is, not to change the behavior of VM, existing user operation. I mean that I didn't think it's better that the user should check the both of the ixgbevf driver can deal with new API and the VF is trusted. Now, I think the point is who takes care whether the VF is trusted. Right? It seems that you think the VF user should handle that user is trusted and do something with a notice that "you're trusted or untrusted" from the host. Is that correct? I made it in PF side, because it looks easy to handle it. If something to do in VF side, I think ixgbevf driver should handle it. thanks, Hiroshi
> -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > Behalf Of Hiroshi Shimamoto > Sent: Thursday, May 21, 2015 7:31 PM > To: Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, Sy > Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > sassmann@redhat.com > Subject: Re: [Intel-wired-lan] [PATCH v5 3/3] ixgbe: Add new ndo to trust > VF > [big snip] > I think your concerns are related to some operational assumptions. > My basic concept is, not to change the behavior of VM, existing user > operation. > I mean that I didn't think it's better that the user should check the both > of the ixgbevf driver can deal with new API and the VF is trusted. > > Now, I think the point is who takes care whether the VF is trusted. Right? > It seems that you think the VF user should handle that user is trusted and > do something with a notice that "you're trusted or untrusted" from the > host. > Is that correct? > I made it in PF side, because it looks easy to handle it. If something to > do in VF side, I think ixgbevf driver should handle it. Setting the VF trusted mode feature should only be allowed through the PF as it is the only trusted entity from the start. We do not want the VF being able to decide for itself to be trusted. - Greg > > thanks, > Hiroshi > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@lists.osuosl.org > http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> -----Original Message----- > From: Rose, Gregory V > Sent: Friday, May 22, 2015 8:08 AM > To: Hiroshi Shimamoto; Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > sassmann@redhat.com > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > -----Original Message----- > > From: Intel-wired-lan > > [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Hiroshi > > Shimamoto > > Sent: Thursday, May 21, 2015 7:31 PM > > To: Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > > lan@lists.osuosl.org > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > sassmann@redhat.com > > Subject: Re: [Intel-wired-lan] [PATCH v5 3/3] ixgbe: Add new ndo to > > trust VF > > > > [big snip] > > > I think your concerns are related to some operational assumptions. > > My basic concept is, not to change the behavior of VM, existing user > > operation. > > I mean that I didn't think it's better that the user should check the > > both of the ixgbevf driver can deal with new API and the VF is trusted. > > > > Now, I think the point is who takes care whether the VF is trusted. Right? > > It seems that you think the VF user should handle that user is trusted > > and do something with a notice that "you're trusted or untrusted" from > > the host. > > Is that correct? > > I made it in PF side, because it looks easy to handle it. If something > > to do in VF side, I think ixgbevf driver should handle it. > > Setting the VF trusted mode feature should only be allowed through the PF > as it is the only trusted entity from the start. We do not want the VF being > able to decide for itself to be trusted. > > - Greg > I completely agree with Greg and never meant to imply anything else. The PF should be where a given VF is made "trusted". Likewise a VF can get promoted to MC Promiscuous buy requesting over 30 MC groups. I like this and your patch currently does this. So for example below: PF VF ---------- ----------- Set given VF as trusted Request 30+ MC groups via Mail Box Put PF in MC Promiscuous mode What I am concerned about is the following flow where we seem to store the fact the VF requests more than 30+ MC groups so that we can automatically enter MC Promisc Mode if that VF is ever made trusted. PF VF ----------- ---------- Currently VF is NOT trusted Request 30+ MC groups via Mail Box Do NOT put PF in MC Promisc (hw->mac.mc_promisc = true) < Some time later > Set given VF as trusted (because mc_promisc set) Put PF in MC Promisc I don't like the fact that the PF remembers that the VF was denied MC Promiscuous mode in the past. And because of that automatically put the VF in MC Promiscuous mode when it becomes trusted. Maybe showing in code what I would like removed/added would be more helpful, probably should have started doing that. :) I would remove this bit of code from ixgbe_ndo_set_vf_trust(): int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool setting) { struct ixgbe_adapter *adapter = netdev_priv(netdev); if (vf >= adapter->num_vfs) return -EINVAL; /* nothing to do */ if (adapter->vfinfo[vf].trusted == setting) return 0; adapter->vfinfo[vf].trusted = setting; - /* Reconfigure features which are only allowed for trusted VF */ - /* VF multicast promiscuous mode */ - if (adapter->vfinfo[vf].mc_promisc) - ixgbe_enable_vf_mc_promisc(adapter, vf); return 0; } This of course would be we should not set mc_promisc ever if we are NOT trusted (adapter->vfinfo[vf].trusted) so in ixgbe_set_vf_mc_promisc() I would add or something like it: static int ixgbe_set_vf_mc_promisc(struct ixgbe_adapter *adapter, u32 *msgbuf, u32 vf) { bool enable = !!msgbuf[1]; /* msgbuf contains the flag to enable */ switch (adapter->vfinfo[vf].vf_api) { case ixgbe_mbox_api_12: break; default: return -1; } + /* have to be trusted */ + If (!adapter->vfinfo[vf].trusted) + Return 0; + /* nothing to do */ if (adapter->vfinfo[vf].mc_promisc == enable) return 0; adapter->vfinfo[vf].mc_promisc = enable; if (enable) return ixgbe_enable_vf_mc_promisc(adapter, vf); else return ixgbe_disable_vf_mc_promisc(adapter, vf); } Does this better explain what my concern is? Thanks, -Don Skidmore <donald.c.skidmore@intel.com>
> > -----Original Message----- > > From: Rose, Gregory V > > Sent: Friday, May 22, 2015 8:08 AM > > To: Hiroshi Shimamoto; Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > > lan@lists.osuosl.org > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > sassmann@redhat.com > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > -----Original Message----- > > > From: Intel-wired-lan > > > [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Hiroshi > > > Shimamoto > > > Sent: Thursday, May 21, 2015 7:31 PM > > > To: Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > > > lan@lists.osuosl.org > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > sassmann@redhat.com > > > Subject: Re: [Intel-wired-lan] [PATCH v5 3/3] ixgbe: Add new ndo to > > > trust VF > > > > > > > [big snip] > > > > > I think your concerns are related to some operational assumptions. > > > My basic concept is, not to change the behavior of VM, existing user > > > operation. > > > I mean that I didn't think it's better that the user should check the > > > both of the ixgbevf driver can deal with new API and the VF is trusted. > > > > > > Now, I think the point is who takes care whether the VF is trusted. Right? > > > It seems that you think the VF user should handle that user is trusted > > > and do something with a notice that "you're trusted or untrusted" from > > > the host. > > > Is that correct? > > > I made it in PF side, because it looks easy to handle it. If something > > > to do in VF side, I think ixgbevf driver should handle it. > > > > Setting the VF trusted mode feature should only be allowed through the PF > > as it is the only trusted entity from the start. We do not want the VF being > > able to decide for itself to be trusted. > > > > - Greg > > > > I completely agree with Greg and never meant to imply anything else. > > The PF should be where a given VF is made "trusted". Likewise a VF can get promoted to MC Promiscuous buy requesting > over 30 MC groups. I like this and your patch currently does this. So for example below: > > PF VF > ---------- ----------- > Set given VF as trusted > Request 30+ MC groups via Mail Box > Put PF in MC Promiscuous mode > > > What I am concerned about is the following flow where we seem to store the fact the VF requests more than 30+ MC groups > so that we can automatically enter MC Promisc Mode if that VF is ever made trusted. > > PF VF > ----------- ---------- > Currently VF is NOT trusted > Request 30+ MC groups via Mail Box > Do NOT put PF in MC Promisc > (hw->mac.mc_promisc = true) > > < Some time later > > > Set given VF as trusted > (because mc_promisc set) Put PF in MC Promisc > > > I don't like the fact that the PF remembers that the VF was denied MC Promiscuous mode in the past. And because of that > automatically put the VF in MC Promiscuous mode when it becomes trusted. Maybe showing in code what I would like removed/added > would be more helpful, probably should have started doing that. :) Do you mean that VF should care about it is trusted or not? Should VF request MC Promisc again when it's trusted? Or, do you mean VF never be trusted during its (or VM's) lifetime? And what do you think about being untrusted from trusted state? > > I would remove this bit of code from ixgbe_ndo_set_vf_trust(): > > int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool > setting) { > struct ixgbe_adapter *adapter = netdev_priv(netdev); > > if (vf >= adapter->num_vfs) > return -EINVAL; > > /* nothing to do */ > if (adapter->vfinfo[vf].trusted == setting) > return 0; > > adapter->vfinfo[vf].trusted = setting; > > - /* Reconfigure features which are only allowed for trusted VF */ > - /* VF multicast promiscuous mode */ > - if (adapter->vfinfo[vf].mc_promisc) > - ixgbe_enable_vf_mc_promisc(adapter, vf); I understand, you don't think we need to have a capability to enable/disable MC Promisc on the fly. > > return 0; > } > > This of course would be we should not set mc_promisc ever if we are NOT trusted (adapter->vfinfo[vf].trusted) so in > ixgbe_set_vf_mc_promisc() I would add or something like it: > > static int ixgbe_set_vf_mc_promisc(struct ixgbe_adapter *adapter, > u32 *msgbuf, u32 vf) > { > bool enable = !!msgbuf[1]; /* msgbuf contains the flag to enable */ > > switch (adapter->vfinfo[vf].vf_api) { > case ixgbe_mbox_api_12: > break; > default: > return -1; > } > > + /* have to be trusted */ > + If (!adapter->vfinfo[vf].trusted) > + Return 0; Should we return an error to VF to inform it isn't trusted? > + > /* nothing to do */ > if (adapter->vfinfo[vf].mc_promisc == enable) > return 0; > > adapter->vfinfo[vf].mc_promisc = enable; > > if (enable) > return ixgbe_enable_vf_mc_promisc(adapter, vf); > else > return ixgbe_disable_vf_mc_promisc(adapter, vf); } > > > Does this better explain what my concern is? yes, I see, but I would like to clarify the above things. thanks, Hiroshi
> -----Original Message----- > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > Sent: Monday, May 25, 2015 6:00 PM > To: Skidmore, Donald C; Rose, Gregory V; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > sassmann@redhat.com > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > -----Original Message----- > > > From: Rose, Gregory V > > > Sent: Friday, May 22, 2015 8:08 AM > > > To: Hiroshi Shimamoto; Skidmore, Donald C; Kirsher, Jeffrey T; > > > intel-wired- lan@lists.osuosl.org > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > sassmann@redhat.com > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > > -----Original Message----- > > > > From: Intel-wired-lan > > > > [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of > > > > Hiroshi Shimamoto > > > > Sent: Thursday, May 21, 2015 7:31 PM > > > > To: Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > > > > lan@lists.osuosl.org > > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > > sassmann@redhat.com > > > > Subject: Re: [Intel-wired-lan] [PATCH v5 3/3] ixgbe: Add new ndo > > > > to trust VF > > > > > > > > > > [big snip] > > > > > > > I think your concerns are related to some operational assumptions. > > > > My basic concept is, not to change the behavior of VM, existing > > > > user operation. > > > > I mean that I didn't think it's better that the user should check > > > > the both of the ixgbevf driver can deal with new API and the VF is > trusted. > > > > > > > > Now, I think the point is who takes care whether the VF is trusted. > Right? > > > > It seems that you think the VF user should handle that user is > > > > trusted and do something with a notice that "you're trusted or > > > > untrusted" from the host. > > > > Is that correct? > > > > I made it in PF side, because it looks easy to handle it. If > > > > something to do in VF side, I think ixgbevf driver should handle it. > > > > > > Setting the VF trusted mode feature should only be allowed through > > > the PF as it is the only trusted entity from the start. We do not > > > want the VF being able to decide for itself to be trusted. > > > > > > - Greg > > > > > > > I completely agree with Greg and never meant to imply anything else. > > > > The PF should be where a given VF is made "trusted". Likewise a VF > > can get promoted to MC Promiscuous buy requesting over 30 MC groups. I > like this and your patch currently does this. So for example below: > > > > PF VF > > ---------- ----------- > > Set given VF as trusted > > Request 30+ MC groups via Mail Box > Put PF in MC Promiscuous mode > > > > > > What I am concerned about is the following flow where we seem to store > > the fact the VF requests more than 30+ MC groups so that we can > automatically enter MC Promisc Mode if that VF is ever made trusted. > > > > PF VF > > ----------- ---------- > > Currently VF is NOT trusted > > Request 30+ MC groups via Mail Box > Do NOT put PF in MC Promisc > > (hw->mac.mc_promisc = true) > > > > < Some time later > > > > > Set given VF as trusted > > (because mc_promisc set) Put PF in MC Promisc > > > > > > I don't like the fact that the PF remembers that the VF was denied MC > > Promiscuous mode in the past. And because of that automatically put > > the VF in MC Promiscuous mode when it becomes trusted. Maybe > showing > > in code what I would like removed/added would be more helpful, > > probably should have started doing that. :) > > Do you mean that VF should care about it is trusted or not? > Should VF request MC Promisc again when it's trusted? > Or, do you mean VF never be trusted during its (or VM's) lifetime? I think the VF shouldn't directly know whether it is trusted or not. It should request MC Promisc and get it if it is trusted and not if it is not trusted. So if you (as the system admin know you have a VF that will need to request MC Promisc make sure you promote that VF to trusted before assigning it to a VM. That way when it requests MC Promisc the PF will be able to grant it. > > And what do you think about being untrusted from trusted state? This is an interesting question. If we allowed a VM to go from trusted -> untrusted we would have to turn off any "special" configuration that trusted allowed. Maybe in such cases we could reset the PF? And of course require all the "special" configuration (MC Promisc) to default to off after being reset. > > > > > I would remove this bit of code from ixgbe_ndo_set_vf_trust(): > > > > int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool > > setting) { > > struct ixgbe_adapter *adapter = netdev_priv(netdev); > > > > if (vf >= adapter->num_vfs) > > return -EINVAL; > > > > /* nothing to do */ > > if (adapter->vfinfo[vf].trusted == setting) > > return 0; > > > > adapter->vfinfo[vf].trusted = setting; > > > > - /* Reconfigure features which are only allowed for trusted VF */ > > - /* VF multicast promiscuous mode */ > > - if (adapter->vfinfo[vf].mc_promisc) > > - ixgbe_enable_vf_mc_promisc(adapter, vf); > > I understand, you don't think we need to have a capability to enable/disable > MC Promisc on the fly. > > > > > return 0; > > } > > > > This of course would be we should not set mc_promisc ever if we are > > NOT trusted (adapter->vfinfo[vf].trusted) so in > > ixgbe_set_vf_mc_promisc() I would add or something like it: > > > > static int ixgbe_set_vf_mc_promisc(struct ixgbe_adapter *adapter, > > u32 *msgbuf, u32 vf) > > { > > bool enable = !!msgbuf[1]; /* msgbuf contains the flag to enable > */ > > > > switch (adapter->vfinfo[vf].vf_api) { > > case ixgbe_mbox_api_12: > > break; > > default: > > return -1; > > } > > > > + /* have to be trusted */ > > + If (!adapter->vfinfo[vf].trusted) > > + Return 0; > > Should we return an error to VF to inform it isn't trusted? This too is a valid point. Currently we would just not do it (MC Promisc) and the VF would have to figure that out for itself. Passing a NAK back to the VF might be nicer. :) Of course I assumed the sysadm would know that he/she wanted to give a VF trusted status and would do that before the VF was even assigned to a VM, so the issue would never come up. Maybe that is not valid for your use case? Thanks, -Don
> -----Original Message----- > From: Skidmore, Donald C > Sent: Tuesday, May 26, 2015 10:46 AM > To: Hiroshi Shimamoto; Rose, Gregory V; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, Sy > Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > sassmann@redhat.com > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > [snip] > > > -----Original Message----- > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > Sent: Monday, May 25, 2015 6:00 PM > > To: Skidmore, Donald C; Rose, Gregory V; Kirsher, Jeffrey T; > > intel-wired- lan@lists.osuosl.org > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > sassmann@redhat.com > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > Do you mean that VF should care about it is trusted or not? > > Should VF request MC Promisc again when it's trusted? > > Or, do you mean VF never be trusted during its (or VM's) lifetime? > > I think the VF shouldn't directly know whether it is trusted or not That's completely irrevelant. The person administering the PF will be the person who provided trusted privileges to the VF. He'll then *tell* or somehow other communicate to the person administering the VF (probably himself/herself) and then proceed to execute commands on that VF that require trusted privileges. If the VF does not have trusted privileges then the commands to add VLAN filters, set promiscuous modes, and any other privileged commands will fail. Let's not get too fancy with this. It's simple - the host VMM admin provides trusted privileges to the VF. The person administering the VF (if in fact it is not the same person, it usually will be) will proceed to do things that require VF trusted privileges. . It > should request MC Promisc and get it if it is trusted and not if it is not > trusted. So if you (as the system admin know you have a VF that will need > to request MC Promisc make sure you promote that VF to trusted before > assigning it to a VM. That way when it requests MC Promisc the PF will be > able to grant it. > Multicast promiscuous should be allowed for the VFs. We already allow VFs to set whatever multicast filters they want so if they want to go into MPE then so what? We don't care. It's not a security risk. Right now, without any modification, the VF can set 30 multicast filters and listen. It can then remove those and set another 30 filters and listen. And so on and so on. So if a VF can already listen on any MC filter it wants then why this artificial restriction on MC promiscuous mode. We don't care about this case. Unicast promiscuous is the security risk and I think we've handled that. > > > > > And what do you think about being untrusted from trusted state? > > This is an interesting question. If we allowed a VM to go from trusted -> > untrusted we would have to turn off any "special" configuration that > trusted allowed. Maybe in such cases we could reset the PF? And of > course require all the "special" configuration (MC Promisc) to default to > off after being reset. > To remove privileges from a VF that you're already set to privileged will require destruction of the VF VSI and VFLR to the VF - after it comes up it can't do any further privileged operations. [snip > This too is a valid point. Currently we would just not do it (MC Promisc) > and the VF would have to figure that out for itself. Passing a NAK back > to the VF might be nicer. :) Of course I assumed the sysadm would know > that he/she wanted to give a VF trusted status and would do that before > the VF was even assigned to a VM, so the issue would never come up. Maybe > that is not valid for your use case? Let's not worry about MC promiscuous mode. As I pointed out above we already let VFs set any MC address filters they want so that horse has already left the barn. Focus on getting the VF privileged mode configuration going and then we're well on our way to accomplishing what we need to do. - Greg
> > -----Original Message----- > > From: Skidmore, Donald C > > Sent: Tuesday, May 26, 2015 10:46 AM > > To: Hiroshi Shimamoto; Rose, Gregory V; Kirsher, Jeffrey T; intel-wired- > > lan@lists.osuosl.org > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, Sy > > Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > sassmann@redhat.com > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > [snip] > > > > > > -----Original Message----- > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > > Sent: Monday, May 25, 2015 6:00 PM > > > To: Skidmore, Donald C; Rose, Gregory V; Kirsher, Jeffrey T; > > > intel-wired- lan@lists.osuosl.org > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > sassmann@redhat.com > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > Do you mean that VF should care about it is trusted or not? > > > Should VF request MC Promisc again when it's trusted? > > > Or, do you mean VF never be trusted during its (or VM's) lifetime? > > > > I think the VF shouldn't directly know whether it is trusted or not > > That's completely irrevelant. The person administering the PF will be the person who provided trusted privileges to the > VF. He'll then *tell* or somehow other communicate to the person administering the VF (probably himself/herself) and > then proceed to execute commands on that VF that require trusted privileges. > > If the VF does not have trusted privileges then the commands to add VLAN filters, set promiscuous modes, and any other > privileged commands will fail. > > Let's not get too fancy with this. It's simple - the host VMM admin provides trusted privileges to the VF. The person > administering the VF (if in fact it is not the same person, it usually will be) will proceed to do things that require > VF trusted privileges. Now I think that it's better to have an interface between PF and VF to know the VF is trusted. Otherwise VM cannot know whether its VF is trusted, that prevents automatic operations. Or add another communicating interface outside of ixgbe PF-VF mbox API? > > > . It > > should request MC Promisc and get it if it is trusted and not if it is not > > trusted. So if you (as the system admin know you have a VF that will need > > to request MC Promisc make sure you promote that VF to trusted before > > assigning it to a VM. That way when it requests MC Promisc the PF will be > > able to grant it. > > > > Multicast promiscuous should be allowed for the VFs. We already allow VFs to set whatever multicast filters they want > so if they want to go into MPE then so what? We don't care. It's not a security risk. Right now, without any modification, > the VF can set 30 multicast filters and listen. It can then remove those and set another 30 filters and listen. And > so on and so on. So if a VF can already listen on any MC filter it wants then why this artificial restriction on MC promiscuous > mode. I'm fine with that, previously I mentioned about that. Without resetting PF, we can listen every MC packet which hash was set. PF reset will restore the last 30 MC addresses per VF. Also there is a single hash entries table, all VFs will got a MC packet which hash was set in the table. If a VF user set a filter, other users will receive that MC packet. > > We don't care about this case. Unicast promiscuous is the security risk and I think we've handled that. So, should we separate the discussion, about trusting VF operation and about MC promiscuous? > > > > > > > > > And what do you think about being untrusted from trusted state? > > > > This is an interesting question. If we allowed a VM to go from trusted -> > > untrusted we would have to turn off any "special" configuration that > > trusted allowed. Maybe in such cases we could reset the PF? And of > > course require all the "special" configuration (MC Promisc) to default to > > off after being reset. > > > > To remove privileges from a VF that you're already set to privileged will require destruction of the VF VSI and VFLR to > the VF - after it comes up it can't do any further privileged operations. yeah, sounds good to reset VF on changing privilege. > > [snip > > > This too is a valid point. Currently we would just not do it (MC Promisc) > > and the VF would have to figure that out for itself. Passing a NAK back > > to the VF might be nicer. :) Of course I assumed the sysadm would know > > that he/she wanted to give a VF trusted status and would do that before > > the VF was even assigned to a VM, so the issue would never come up. Maybe > > that is not valid for your use case? > > Let's not worry about MC promiscuous mode. As I pointed out above we already let VFs set any MC address filters they > want so that horse has already left the barn. Do you think that VF MC promiscuous mode isn't needed to handle under trusted mode, right? thanks, Hiroshi > > Focus on getting the VF privileged mode configuration going and then we're well on our way to accomplishing what we need > to do. > > - Greg
> -----Original Message----- > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > Sent: Tuesday, May 26, 2015 5:28 PM > To: Rose, Gregory V; Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, Sy > Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > sassmann@redhat.com > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > -----Original Message----- > > > From: Skidmore, Donald C > > > Sent: Tuesday, May 26, 2015 10:46 AM > > > To: Hiroshi Shimamoto; Rose, Gregory V; Kirsher, Jeffrey T; > > > intel-wired- lan@lists.osuosl.org > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > sassmann@redhat.com > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > > [snip] > > > > > > > > > -----Original Message----- > > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > > > Sent: Monday, May 25, 2015 6:00 PM > > > > To: Skidmore, Donald C; Rose, Gregory V; Kirsher, Jeffrey T; > > > > intel-wired- lan@lists.osuosl.org > > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > > sassmann@redhat.com > > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > > > > Do you mean that VF should care about it is trusted or not? > > > > Should VF request MC Promisc again when it's trusted? > > > > Or, do you mean VF never be trusted during its (or VM's) lifetime? > > > > > > I think the VF shouldn't directly know whether it is trusted or not > > > > That's completely irrevelant. The person administering the PF will be > > the person who provided trusted privileges to the VF. He'll then > > *tell* or somehow other communicate to the person administering the VF > (probably himself/herself) and then proceed to execute commands on that VF > that require trusted privileges. > > > > If the VF does not have trusted privileges then the commands to add > > VLAN filters, set promiscuous modes, and any other privileged commands > will fail. > > > > Let's not get too fancy with this. It's simple - the host VMM admin > > provides trusted privileges to the VF. The person administering the > > VF (if in fact it is not the same person, it usually will be) will > proceed to do things that require VF trusted privileges. > > Now I think that it's better to have an interface between PF and VF to > know the VF is trusted. > Otherwise VM cannot know whether its VF is trusted, that prevents > automatic operations. Agreed, it would be silly for the VF to have privileges but not know that it can use them! > Or add another communicating interface outside of ixgbe PF-VF mbox API? We can't depend on any given vendor specific interface. I'd add a very clear comment in the Physical Function ndo op that gives a VF trusted privileges that it is up to the driver to notify the VF driver. But yes, in the case of Intel drivers the mailbox or admin queue (for i40e) would be the mechanism to do that. I know you have some ixgbe patches that coincide with this patch so that's a good place to look. > > > > > > > . It > > > should request MC Promisc and get it if it is trusted and not if it > > > is not trusted. So if you (as the system admin know you have a VF > > > that will need to request MC Promisc make sure you promote that VF > > > to trusted before assigning it to a VM. That way when it requests > > > MC Promisc the PF will be able to grant it. > > > > > > > Multicast promiscuous should be allowed for the VFs. We already allow > > VFs to set whatever multicast filters they want so if they want to go > > into MPE then so what? We don't care. It's not a security risk. > > Right now, without any modification, the VF can set 30 multicast > > filters and listen. It can then remove those and set another 30 filters > and listen. And so on and so on. So if a VF can already listen on any MC > filter it wants then why this artificial restriction on MC promiscuous > mode. > > I'm fine with that, previously I mentioned about that. > Without resetting PF, we can listen every MC packet which hash was set. > PF reset will restore the last 30 MC addresses per VF. > > Also there is a single hash entries table, all VFs will got a MC packet > which hash was set in the table. If a VF user set a filter, other users > will receive that MC packet. > > > > > We don't care about this case. Unicast promiscuous is the security risk > and I think we've handled that. > > So, should we separate the discussion, about trusting VF operation and > about MC promiscuous? Yes. And to my mind it shouldn't really be in the context of virtual function privilege or trust. > > > > > > > > > > > > > > And what do you think about being untrusted from trusted state? > > > > > > This is an interesting question. If we allowed a VM to go from > > > trusted -> untrusted we would have to turn off any "special" > > > configuration that trusted allowed. Maybe in such cases we could > > > reset the PF? And of course require all the "special" configuration > > > (MC Promisc) to default to off after being reset. > > > > > > > To remove privileges from a VF that you're already set to privileged > > will require destruction of the VF VSI and VFLR to the VF - after it > comes up it can't do any further privileged operations. > > yeah, sounds good to reset VF on changing privilege. > > > > > [snip > > > > > This too is a valid point. Currently we would just not do it (MC > > > Promisc) and the VF would have to figure that out for itself. > > > Passing a NAK back to the VF might be nicer. :) Of course I assumed > > > the sysadm would know that he/she wanted to give a VF trusted status > > > and would do that before the VF was even assigned to a VM, so the > > > issue would never come up. Maybe that is not valid for your use case? > > > > Let's not worry about MC promiscuous mode. As I pointed out above we > > already let VFs set any MC address filters they want so that horse has > already left the barn. > > Do you think that VF MC promiscuous mode isn't needed to handle under > trusted mode, right? Correct, that's my opinion on it given the fact that historically there has never been any restriction on setting MC addresses by the VF. See my comments above in the respect. Thanks and regards Hiroshi, - Greg > > thanks, > Hiroshi > > > > > Focus on getting the VF privileged mode configuration going and then > > we're well on our way to accomplishing what we need to do. > > > > - Greg
On 27/05/15 01:27, Hiroshi Shimamoto wrote: >>> I think the VF shouldn't directly know whether it is trusted or not >> That's completely irrevelant. The person administering the PF will be the person who provided trusted privileges to the >> VF. He'll then *tell* or somehow other communicate to the person administering the VF (probably himself/herself) and >> then proceed to execute commands on that VF that require trusted privileges. >> >> If the VF does not have trusted privileges then the commands to add VLAN filters, set promiscuous modes, and any other >> privileged commands will fail. >> >> Let's not get too fancy with this. It's simple - the host VMM admin provides trusted privileges to the VF. The person >> administering the VF (if in fact it is not the same person, it usually will be) will proceed to do things that require >> VF trusted privileges. > Now I think that it's better to have an interface between PF and VF to know the VF is trusted. > Otherwise VM cannot know whether its VF is trusted, that prevents automatic operations. I don't think this is right. The approach to VF trust/permissions issues we took in sfc (and that I'd recommend following) was that all drivers will always _attempt_ actions on the assumption they have privilege, then if they in fact don't, they get an EPERM (either from the firmware or from some proxy in the PF driver) and they deal with that appropriately. So in this case the VF would say "I have more than 30 mcast addrs, I need promisc", it would try to set promisc, the PF would decide "it's not trusted, Denied" and the VF would get an EPERM back. At which point the VF would presumably insert as many mcast addresses as it could and warn that it hadn't got them all (or, if the whole thing was triggered by adding a new mcast addr, it might be able to pass EPERM back to whoever requested it). Then if the VF's privilege is changed, VFLR it so that it re-does all of its setup this time subject to the new permissions. (Alternatively, if the privilege is strictly increased, you can choose not to do this and instead the VF driver may be told from userspace to re-do the relevant setup, in which case traffic interruption may be able to be avoided.) The advantage of this model is that the VF driver doesn't have to know what permissions different operations require; that knowledge can live in one place only (in our case the firmware, in your case it sounds like the PF driver) allowing the policy to be changed without problems with version skew. The disadvantages are that the amount of control-plane traffic is increased (try, fail, try something else), and reinitialisation on privilege change may cause traffic interruption. But the former isn't a worry since the control plane traffic volume is so small. The latter would still have to happen on privilege decrease in any case, as Greg stated. The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.
> -----Original Message----- > From: Edward Cree [mailto:ecree@solarflare.com] > Sent: Wednesday, May 27, 2015 2:04 AM > To: Hiroshi Shimamoto > Cc: Rose, Gregory V; Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org; nhorman@redhat.com; jogreene@redhat.com; Linux > Netdev List; Choi, Sy Jong; Rony Efraim; David Miller; Or Gerlitz; > sassmann@redhat.com > Subject: Re: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > On 27/05/15 01:27, Hiroshi Shimamoto wrote: > >>> I think the VF shouldn't directly know whether it is trusted or not > >> That's completely irrevelant. The person administering the PF will > >> be the person who provided trusted privileges to the VF. He'll then > >> *tell* or somehow other communicate to the person administering the > VF (probably himself/herself) and then proceed to execute commands on > that VF that require trusted privileges. > >> > >> If the VF does not have trusted privileges then the commands to add > >> VLAN filters, set promiscuous modes, and any other privileged commands > will fail. > >> > >> Let's not get too fancy with this. It's simple - the host VMM admin > >> provides trusted privileges to the VF. The person administering the > >> VF (if in fact it is not the same person, it usually will be) will proceed to do > things that require VF trusted privileges. > > Now I think that it's better to have an interface between PF and VF to > know the VF is trusted. > > Otherwise VM cannot know whether its VF is trusted, that prevents > automatic operations. > I don't think this is right. The approach to VF trust/permissions issues we > took in sfc (and that I'd recommend following) was that all drivers will always > _attempt_ actions on the assumption they have privilege, then if they in fact > don't, they get an EPERM (either from the firmware or from some proxy in > the PF driver) and they deal with that appropriately. > > So in this case the VF would say "I have more than 30 mcast addrs, I need > promisc", it would try to set promisc, the PF would decide "it's not trusted, > Denied" and the VF would get an EPERM back. At which point the VF would > presumably insert as many mcast addresses as it could and warn that it > hadn't got them all (or, if the whole thing was triggered by adding a new > mcast addr, it might be able to pass EPERM back to whoever requested it). > > Then if the VF's privilege is changed, VFLR it so that it re-does all of its setup > this time subject to the new permissions. (Alternatively, if the privilege is > strictly increased, you can choose not to do this and instead the VF driver > may be told from userspace to re-do the relevant setup, in which case traffic > interruption may be able to be avoided.) > > The advantage of this model is that the VF driver doesn't have to know what > permissions different operations require; that knowledge can live in one > place only (in our case the firmware, in your case it sounds like the PF driver) > allowing the policy to be changed without problems with version skew. > > The disadvantages are that the amount of control-plane traffic is increased > (try, fail, try something else), and reinitialisation on privilege change may > cause traffic interruption. But the former isn't a worry since the control plane > traffic volume is so small. The latter would still have to happen on privilege > decrease in any case, as Greg stated. This is how I had envisioned it working as well. The VF can ask for what it wants, whether it receives it is dependent on if the PF considers it trusted or not. As Hiroshi already mentioned (among others) we may need to have the mailbox protocol extended to allow the PF to tell the VF it didn't get what was asked for. > The information contained in this message is confidential and is intended for > the addressee(s) only. If you have received this message in error, please > notify the sender immediately and delete the message. Unless you are an > addressee (or authorized to receive for an addressee), you may not use, > copy or disclose to anyone this message or any information contained in this > message. The unauthorized use, disclosure, copying or alteration of this > message is strictly prohibited.
> -----Original Message----- > From: Skidmore, Donald C > Sent: Wednesday, May 27, 2015 8:34 AM > To: Edward Cree; Hiroshi Shimamoto > Cc: Rose, Gregory V; Kirsher, Jeffrey T; intel-wired-lan@lists.osuosl.org; > nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, Sy Jong; > Rony Efraim; David Miller; Or Gerlitz; sassmann@redhat.com > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > -----Original Message----- > > From: Edward Cree [mailto:ecree@solarflare.com] > > Sent: Wednesday, May 27, 2015 2:04 AM > > To: Hiroshi Shimamoto > > Cc: Rose, Gregory V; Skidmore, Donald C; Kirsher, Jeffrey T; > > intel-wired- lan@lists.osuosl.org; nhorman@redhat.com; > > jogreene@redhat.com; Linux Netdev List; Choi, Sy Jong; Rony Efraim; > > David Miller; Or Gerlitz; sassmann@redhat.com > > Subject: Re: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > On 27/05/15 01:27, Hiroshi Shimamoto wrote: > > >>> I think the VF shouldn't directly know whether it is trusted or > > >>> not > > >> That's completely irrevelant. The person administering the PF will > > >> be the person who provided trusted privileges to the VF. He'll > > >> then > > >> *tell* or somehow other communicate to the person administering the > > VF (probably himself/herself) and then proceed to execute commands on > > that VF that require trusted privileges. > > >> > > >> If the VF does not have trusted privileges then the commands to add > > >> VLAN filters, set promiscuous modes, and any other privileged > > >> commands > > will fail. > > >> > > >> Let's not get too fancy with this. It's simple - the host VMM > > >> admin provides trusted privileges to the VF. The person > > >> administering the VF (if in fact it is not the same person, it > > >> usually will be) will proceed to do > > things that require VF trusted privileges. > > > Now I think that it's better to have an interface between PF and VF > > > to > > know the VF is trusted. > > > Otherwise VM cannot know whether its VF is trusted, that prevents > > automatic operations. > > I don't think this is right. The approach to VF trust/permissions > > issues we took in sfc (and that I'd recommend following) was that all > > drivers will always _attempt_ actions on the assumption they have > > privilege, then if they in fact don't, they get an EPERM (either from > > the firmware or from some proxy in the PF driver) and they deal with > that appropriately. > > > > So in this case the VF would say "I have more than 30 mcast addrs, I > > need promisc", it would try to set promisc, the PF would decide "it's > > not trusted, Denied" and the VF would get an EPERM back. At which > > point the VF would presumably insert as many mcast addresses as it > > could and warn that it hadn't got them all (or, if the whole thing was > > triggered by adding a new mcast addr, it might be able to pass EPERM > back to whoever requested it). > > > > Then if the VF's privilege is changed, VFLR it so that it re-does all > > of its setup this time subject to the new permissions. > > (Alternatively, if the privilege is strictly increased, you can choose > > not to do this and instead the VF driver may be told from userspace to > > re-do the relevant setup, in which case traffic interruption may be > > able to be avoided.) > > > > The advantage of this model is that the VF driver doesn't have to know > > what permissions different operations require; that knowledge can live > > in one place only (in our case the firmware, in your case it sounds > > like the PF driver) allowing the policy to be changed without problems > with version skew. > > > > The disadvantages are that the amount of control-plane traffic is > > increased (try, fail, try something else), and reinitialisation on > > privilege change may cause traffic interruption. But the former isn't > > a worry since the control plane traffic volume is so small. The > > latter would still have to happen on privilege decrease in any case, as > Greg stated. > > This is how I had envisioned it working as well. The VF can ask for what > it wants, whether it receives it is dependent on if the PF considers it > trusted or not. As Hiroshi already mentioned (among others) we may need > to have the mailbox protocol extended to allow the PF to tell the VF it > didn't get what was asked for. Hmm... well I think it's easier in the Intel case to just tell the VF it has privileges and it reduces complexity of all the additional NAKs and retries. That said, if other vendors wish to go that route then they do not *have* to tell the VF that it has additional privileges and then they can add all the additional complexity of the NAKs and retries. There's nothing to prevent any vendor from notifying a VF that it has privileges and there's nothing that require that they do. This should be a vendor specific detail. In the case of Intel controllers - I prefer that we create an Intel specific mailbox message that tells the VF it's got privileges and cuts down on the amount of hand shaking that goes on in mailbox (igb, ixgbe) or admin queue (i40e) messaging. So now that I've stated my preference let me also state that I do not want to hold up acceptance of the Hiroshi's if_link patch that sets the trusted/privileged state for the VF while we further discuss this driver specific detail. - Greg
> -----Original Message----- > From: Rose, Gregory V > Sent: Tuesday, May 26, 2015 7:01 PM > To: Hiroshi Shimamoto; Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > sassmann@redhat.com > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > -----Original Message----- > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > Sent: Tuesday, May 26, 2015 5:28 PM > > To: Rose, Gregory V; Skidmore, Donald C; Kirsher, Jeffrey T; > > intel-wired- lan@lists.osuosl.org > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > sassmann@redhat.com > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > -----Original Message----- > > > > From: Skidmore, Donald C > > > > Sent: Tuesday, May 26, 2015 10:46 AM > > > > To: Hiroshi Shimamoto; Rose, Gregory V; Kirsher, Jeffrey T; > > > > intel-wired- lan@lists.osuosl.org > > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > > sassmann@redhat.com > > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > -----Original Message----- > > > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > > > > Sent: Monday, May 25, 2015 6:00 PM > > > > > To: Skidmore, Donald C; Rose, Gregory V; Kirsher, Jeffrey T; > > > > > intel-wired- lan@lists.osuosl.org > > > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or > > > > > Gerlitz; sassmann@redhat.com > > > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > > > > > > > Do you mean that VF should care about it is trusted or not? > > > > > Should VF request MC Promisc again when it's trusted? > > > > > Or, do you mean VF never be trusted during its (or VM's) lifetime? > > > > > > > > I think the VF shouldn't directly know whether it is trusted or > > > > not > > > > > > That's completely irrevelant. The person administering the PF will > > > be the person who provided trusted privileges to the VF. He'll then > > > *tell* or somehow other communicate to the person administering the > > > VF > > (probably himself/herself) and then proceed to execute commands on > > that VF that require trusted privileges. > > > > > > If the VF does not have trusted privileges then the commands to add > > > VLAN filters, set promiscuous modes, and any other privileged > > > commands > > will fail. > > > > > > Let's not get too fancy with this. It's simple - the host VMM admin > > > provides trusted privileges to the VF. The person administering the > > > VF (if in fact it is not the same person, it usually will be) will > > proceed to do things that require VF trusted privileges. > > > > Now I think that it's better to have an interface between PF and VF to > > know the VF is trusted. > > Otherwise VM cannot know whether its VF is trusted, that prevents > > automatic operations. > > Agreed, it would be silly for the VF to have privileges but not know that it can > use them! > > > Or add another communicating interface outside of ixgbe PF-VF mbox API? > > We can't depend on any given vendor specific interface. I'd add a very clear > comment in the Physical Function ndo op that gives a VF trusted privileges > that it is up to the driver to notify the VF driver. But yes, in the case of Intel > drivers the mailbox or admin queue (for i40e) would be the mechanism to do > that. I know you have some ixgbe patches that coincide with this patch so > that's a good place to look. > Now why I am not against this (VF knowing it is "trusted") happening I don't see the need for it either. I believe the same could be accomplished by allowing the PF to ask for whatever configuration it wants and some requests will not be granted by the PF unless the VF is trusted. Given, this may require an extension of the mailbox messages to allow for NAK's to make it clear to the VF the request wasn't granted. However like Greg mentions above this need not be requirement, different drivers could implement this way or not. > > > > > > > > > > > . It > > > > should request MC Promisc and get it if it is trusted and not if > > > > it is not trusted. So if you (as the system admin know you have a > > > > VF that will need to request MC Promisc make sure you promote that > > > > VF to trusted before assigning it to a VM. That way when it > > > > requests MC Promisc the PF will be able to grant it. > > > > > > > > > > Multicast promiscuous should be allowed for the VFs. We already > > > allow VFs to set whatever multicast filters they want so if they > > > want to go into MPE then so what? We don't care. It's not a security risk. > > > Right now, without any modification, the VF can set 30 multicast > > > filters and listen. It can then remove those and set another 30 > > > filters > > and listen. And so on and so on. So if a VF can already listen on > > any MC filter it wants then why this artificial restriction on MC > > promiscuous mode. > > > > I'm fine with that, previously I mentioned about that. > > Without resetting PF, we can listen every MC packet which hash was set. > > PF reset will restore the last 30 MC addresses per VF. > > > > Also there is a single hash entries table, all VFs will got a MC > > packet which hash was set in the table. If a VF user set a filter, > > other users will receive that MC packet. > > > > > > > > We don't care about this case. Unicast promiscuous is the security > > > risk > > and I think we've handled that. > > > > So, should we separate the discussion, about trusting VF operation and > > about MC promiscuous? > > Yes. And to my mind it shouldn't really be in the context of virtual function > privilege or trust. If I remember correct the reasoning behind requiring a VF to be "trusted" to enter MC Promiscuous mode was not for security as much as the very real potential that it would have a large negative impact on all the other VF due to frame duplication. So rather than allow any VF to enter this mode and adversely affect all the other VF's we require that the VF be "trusted", implying that PF understands this risk and is ok with it. > > > > > > > > > > > > > > > > > > > > And what do you think about being untrusted from trusted state? > > > > > > > > This is an interesting question. If we allowed a VM to go from > > > > trusted -> untrusted we would have to turn off any "special" > > > > configuration that trusted allowed. Maybe in such cases we could > > > > reset the PF? And of course require all the "special" > > > > configuration (MC Promisc) to default to off after being reset. > > > > > > > > > > To remove privileges from a VF that you're already set to privileged > > > will require destruction of the VF VSI and VFLR to the VF - after it > > comes up it can't do any further privileged operations. > > > > yeah, sounds good to reset VF on changing privilege. > > > > > > > > [snip > > > > > > > This too is a valid point. Currently we would just not do it (MC > > > > Promisc) and the VF would have to figure that out for itself. > > > > Passing a NAK back to the VF might be nicer. :) Of course I > > > > assumed the sysadm would know that he/she wanted to give a VF > > > > trusted status and would do that before the VF was even assigned > > > > to a VM, so the issue would never come up. Maybe that is not valid for > your use case? > > > > > > Let's not worry about MC promiscuous mode. As I pointed out above > > > we already let VFs set any MC address filters they want so that > > > horse has > > already left the barn. > > > > Do you think that VF MC promiscuous mode isn't needed to handle under > > trusted mode, right? > > Correct, that's my opinion on it given the fact that historically there has never > been any restriction on setting MC addresses by the VF. See my comments > above in the respect. > > Thanks and regards Hiroshi, > > - Greg > > > > > thanks, > > Hiroshi > > > > > > > > Focus on getting the VF privileged mode configuration going and then > > > we're well on our way to accomplishing what we need to do. > > > > > > - Greg
> -----Original Message----- > From: Skidmore, Donald C > Sent: Wednesday, May 27, 2015 9:01 AM > To: Rose, Gregory V; Hiroshi Shimamoto; Kirsher, Jeffrey T; intel-wired- > lan@lists.osuosl.org > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, Sy > Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > sassmann@redhat.com > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > -----Original Message----- > > From: Rose, Gregory V > > Sent: Tuesday, May 26, 2015 7:01 PM > > To: Hiroshi Shimamoto; Skidmore, Donald C; Kirsher, Jeffrey T; > > intel-wired- lan@lists.osuosl.org > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > sassmann@redhat.com > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF [snip] > > > > > > > > We can't depend on any given vendor specific interface. I'd add a > > very clear comment in the Physical Function ndo op that gives a VF > > trusted privileges that it is up to the driver to notify the VF > > driver. But yes, in the case of Intel drivers the mailbox or admin > > queue (for i40e) would be the mechanism to do that. I know you have > > some ixgbe patches that coincide with this patch so that's a good place > to look. > > > > Now why I am not against this (VF knowing it is "trusted") happening I > don't see the need for it either. I believe the same could be > accomplished by allowing the PF to ask for whatever configuration it wants > and some requests will not be granted by the PF unless the VF is trusted. > Given, this may require an extension of the mailbox messages to allow for > NAK's to make it clear to the VF the request wasn't granted. > > However like Greg mentions above this need not be requirement, different > drivers could implement this way or not. > > > > > > > > > > > > > > > > . It > > > > > should request MC Promisc and get it if it is trusted and not if > > > > > it is not trusted. So if you (as the system admin know you have > > > > > a VF that will need to request MC Promisc make sure you promote > > > > > that VF to trusted before assigning it to a VM. That way when > > > > > it requests MC Promisc the PF will be able to grant it. > > > > > > > > > > > > > Multicast promiscuous should be allowed for the VFs. We already > > > > allow VFs to set whatever multicast filters they want so if they > > > > want to go into MPE then so what? We don't care. It's not a > security risk. > > > > Right now, without any modification, the VF can set 30 multicast > > > > filters and listen. It can then remove those and set another 30 > > > > filters > > > and listen. And so on and so on. So if a VF can already listen on > > > any MC filter it wants then why this artificial restriction on MC > > > promiscuous mode. > > > > > > I'm fine with that, previously I mentioned about that. > > > Without resetting PF, we can listen every MC packet which hash was > set. > > > PF reset will restore the last 30 MC addresses per VF. > > > > > > Also there is a single hash entries table, all VFs will got a MC > > > packet which hash was set in the table. If a VF user set a filter, > > > other users will receive that MC packet. > > > > > > > > > > > We don't care about this case. Unicast promiscuous is the security > > > > risk > > > and I think we've handled that. > > > > > > So, should we separate the discussion, about trusting VF operation > > > and about MC promiscuous? > > > > Yes. And to my mind it shouldn't really be in the context of virtual > > function privilege or trust. > > If I remember correct the reasoning behind requiring a VF to be "trusted" > to enter MC Promiscuous mode was not for security as much as the very real > potential that it would have a large negative impact on all the other VF > due to frame duplication. So rather than allow any VF to enter this mode > and adversely affect all the other VF's we require that the VF be > "trusted", implying that PF understands this risk and is ok with it. > Allowing promiscuous mode operation by the VF was really a security concern during the initial design phases for SR-IOV way back in 2007/2008 but since Intel Virtual Functions for 1Gig and 10Gig didn't support promiscuous anyway it was a moot point. Limitations on the number of multicast address filters available to a VF were simply an implementation detail due to the size of the mailbox - one that had nothing to do with security or worries about flooding the PCIe bus. That said, if the community feels that allowing multicast promiscuous for VFs should be part of being "trusted" then I'm fine with that and have no objection to requiring "trusted" mode for enablement of multicast promiscuous mode. - Greg
On 27/05/15 16:55, Rose, Gregory V wrote: > There's nothing to prevent any vendor from notifying a VF that it has privileges and there's nothing that require that they do. This should be a vendor specific detail. Agreed - purely a driver implementation detail. > So now that I've stated my preference let me also state that I do not want to hold up acceptance of the Hiroshi's if_link patch that sets the trusted/privileged state for the VF while we further discuss this driver specific detail. Agreed. The information contained in this message is confidential and is intended for the addressee(s) only. If you have received this message in error, please notify the sender immediately and delete the message. Unless you are an addressee (or authorized to receive for an addressee), you may not use, copy or disclose to anyone this message or any information contained in this message. The unauthorized use, disclosure, copying or alteration of this message is strictly prohibited.
> > -----Original Message----- > > From: Rose, Gregory V > > Sent: Tuesday, May 26, 2015 7:01 PM > > To: Hiroshi Shimamoto; Skidmore, Donald C; Kirsher, Jeffrey T; intel-wired- > > lan@lists.osuosl.org > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > sassmann@redhat.com > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > -----Original Message----- > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > > Sent: Tuesday, May 26, 2015 5:28 PM > > > To: Rose, Gregory V; Skidmore, Donald C; Kirsher, Jeffrey T; > > > intel-wired- lan@lists.osuosl.org > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; Choi, > > > Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > sassmann@redhat.com > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > -----Original Message----- > > > > > From: Skidmore, Donald C > > > > > Sent: Tuesday, May 26, 2015 10:46 AM > > > > > To: Hiroshi Shimamoto; Rose, Gregory V; Kirsher, Jeffrey T; > > > > > intel-wired- lan@lists.osuosl.org > > > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or Gerlitz; > > > > > sassmann@redhat.com > > > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com] > > > > > > Sent: Monday, May 25, 2015 6:00 PM > > > > > > To: Skidmore, Donald C; Rose, Gregory V; Kirsher, Jeffrey T; > > > > > > intel-wired- lan@lists.osuosl.org > > > > > > Cc: nhorman@redhat.com; jogreene@redhat.com; Linux Netdev List; > > > > > > Choi, Sy Jong; Rony Efraim; David Miller; Edward Cree; Or > > > > > > Gerlitz; sassmann@redhat.com > > > > > > Subject: RE: [PATCH v5 3/3] ixgbe: Add new ndo to trust VF > > > > > > > > > > > > > > > > > > Do you mean that VF should care about it is trusted or not? > > > > > > Should VF request MC Promisc again when it's trusted? > > > > > > Or, do you mean VF never be trusted during its (or VM's) lifetime? > > > > > > > > > > I think the VF shouldn't directly know whether it is trusted or > > > > > not > > > > > > > > That's completely irrevelant. The person administering the PF will > > > > be the person who provided trusted privileges to the VF. He'll then > > > > *tell* or somehow other communicate to the person administering the > > > > VF > > > (probably himself/herself) and then proceed to execute commands on > > > that VF that require trusted privileges. > > > > > > > > If the VF does not have trusted privileges then the commands to add > > > > VLAN filters, set promiscuous modes, and any other privileged > > > > commands > > > will fail. > > > > > > > > Let's not get too fancy with this. It's simple - the host VMM admin > > > > provides trusted privileges to the VF. The person administering the > > > > VF (if in fact it is not the same person, it usually will be) will > > > proceed to do things that require VF trusted privileges. > > > > > > Now I think that it's better to have an interface between PF and VF to > > > know the VF is trusted. > > > Otherwise VM cannot know whether its VF is trusted, that prevents > > > automatic operations. > > > > Agreed, it would be silly for the VF to have privileges but not know that it can > > use them! > > > > > Or add another communicating interface outside of ixgbe PF-VF mbox API? > > > > We can't depend on any given vendor specific interface. I'd add a very clear > > comment in the Physical Function ndo op that gives a VF trusted privileges > > that it is up to the driver to notify the VF driver. But yes, in the case of Intel > > drivers the mailbox or admin queue (for i40e) would be the mechanism to do > > that. I know you have some ixgbe patches that coincide with this patch so > > that's a good place to look. > > > > Now why I am not against this (VF knowing it is "trusted") happening I don't see the need for it either. I believe the > same could be accomplished by allowing the PF to ask for whatever configuration it wants and some requests will not be > granted by the PF unless the VF is trusted. Given, this may require an extension of the mailbox messages to allow for > NAK's to make it clear to the VF the request wasn't granted. > > However like Greg mentions above this need not be requirement, different drivers could implement this way or not. > Now I'm preparing a patchset to handle an error against VF MC promisc request. I'm not sure that which is better to have new mailbox API which indicates VF is trusted. I made a patchset which doesn't add new API but handles error against VF MC promisc request. Will submit it. thanks, Hiroshi
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h index 08e65b6..5181a4d 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h @@ -153,6 +153,7 @@ struct vf_data_storage { u16 vlan_count; u8 spoofchk_enabled; bool rss_query_enabled; + u8 trusted; unsigned int vf_api; }; diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index b1ea707..263cb40 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -3679,6 +3679,10 @@ static void ixgbe_configure_virtualization(struct ixgbe_adapter *adapter) /* Enable/Disable RSS query feature */ ixgbe_ndo_set_vf_rss_query_en(adapter->netdev, i, adapter->vfinfo[i].rss_query_enabled); + + /* Reconfigure features in trusted */ + ixgbe_ndo_set_vf_trust(adapter->netdev, i, + adapter->vfinfo[i].trusted); } } @@ -8182,6 +8186,7 @@ static const struct net_device_ops ixgbe_netdev_ops = { .ndo_set_vf_rate = ixgbe_ndo_set_vf_bw, .ndo_set_vf_spoofchk = ixgbe_ndo_set_vf_spoofchk, .ndo_set_vf_rss_query_en = ixgbe_ndo_set_vf_rss_query_en, + .ndo_set_vf_trust = ixgbe_ndo_set_vf_trust, .ndo_get_vf_config = ixgbe_ndo_get_vf_config, .ndo_get_stats64 = ixgbe_get_stats64, #ifdef CONFIG_IXGBE_DCB diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c index 615f651..6c602bc 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c @@ -117,8 +117,11 @@ static int __ixgbe_enable_sriov(struct ixgbe_adapter *adapter) */ adapter->vfinfo[i].rss_query_enabled = 0; - /* Turn multicast promiscuous mode off for all VFs */ + /* Disallow VF multicast promiscuous capability + * and turn it off for all VFs + */ adapter->vfinfo[i].mc_promisc = false; + adapter->vfinfo[i].trusted = false; } return 0; @@ -329,9 +332,14 @@ static int ixgbe_enable_vf_mc_promisc(struct ixgbe_adapter *adapter, u32 vf) hw = &adapter->hw; vmolr = IXGBE_READ_REG(hw, IXGBE_VMOLR(vf)); - e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); - - vmolr |= IXGBE_VMOLR_MPE; + if (adapter->vfinfo[vf].trusted) { + e_info(drv, "VF %u: enabling multicast promiscuous\n", vf); + vmolr |= IXGBE_VMOLR_MPE; + } else { + e_info(drv, "VF %u: disabling multicast promiscuous " + "on untrusted VF.\n", vf); + vmolr &= ~IXGBE_VMOLR_MPE; + } IXGBE_WRITE_REG(hw, IXGBE_VMOLR(vf), vmolr); @@ -1492,6 +1500,27 @@ int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf, return 0; } +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, int vf, bool setting) +{ + struct ixgbe_adapter *adapter = netdev_priv(netdev); + + if (vf >= adapter->num_vfs) + return -EINVAL; + + /* nothing to do */ + if (adapter->vfinfo[vf].trusted == setting) + return 0; + + adapter->vfinfo[vf].trusted = setting; + + /* Reconfigure features which are only allowed for trusted VF */ + /* VF multicast promiscuous mode */ + if (adapter->vfinfo[vf].mc_promisc) + ixgbe_enable_vf_mc_promisc(adapter, vf); + + return 0; +} + int ixgbe_ndo_get_vf_config(struct net_device *netdev, int vf, struct ifla_vf_info *ivi) { @@ -1506,5 +1535,6 @@ int ixgbe_ndo_get_vf_config(struct net_device *netdev, ivi->qos = adapter->vfinfo[vf].pf_qos; ivi->spoofchk = adapter->vfinfo[vf].spoofchk_enabled; ivi->rss_query_en = adapter->vfinfo[vf].rss_query_enabled; + ivi->trusted = adapter->vfinfo[vf].trusted; return 0; } diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h index 2c197e6..d85e6fc 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h @@ -49,6 +49,8 @@ int ixgbe_ndo_set_vf_bw(struct net_device *netdev, int vf, int min_tx_rate, int ixgbe_ndo_set_vf_spoofchk(struct net_device *netdev, int vf, bool setting); int ixgbe_ndo_set_vf_rss_query_en(struct net_device *netdev, int vf, bool setting); +int ixgbe_ndo_set_vf_trust(struct net_device *netdev, + int vf, bool setting); int ixgbe_ndo_get_vf_config(struct net_device *netdev, int vf, struct ifla_vf_info *ivi); void ixgbe_check_vf_rate_limit(struct ixgbe_adapter *adapter);