[PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[Hiroshi Shimamoto]

From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

Disable hardware VLAN filtering if netdev->features VLAN flag is dropped.

In SR-IOV case, there is a use case which needs to disable VLAN filter.
For example, we need to make a network function with VF in virtualized
environment. That network function may be a software switch, a router
or etc. It means that that network function will be an end point which
terminates many VLANs.

In the current implementation, VLAN filtering always be turned on and
VF can receive only 63 VLANs. It means that only 63 VLANs can be used
and it's not enough at all for building a virtual router.

With this patch, if the user turns VLAN filtering off on the host, VF
can receive every VLAN packet.
The behavior is changed only if VLAN filtering is turned off by ethtool.

Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
CC: Choi, Sy Jong <sy.jong.choi@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 10 ++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d2df4e3..91ce3a8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3948,6 +3948,12 @@ void ixgbe_set_rx_mode(struct net_device *netdev)
 		hw->addr_ctrl.user_set_promisc = false;
 	}
 
+	/* Disable hardware VLAN filter if the feature flag is dropped */
+	if (!(netdev->features & NETIF_F_HW_VLAN_CTAG_FILTER)) {
+		dev_info(&adapter->pdev->dev, "Disable HW VLAN filter\n");
+		vlnctrl &= ~(IXGBE_VLNCTRL_VFE | IXGBE_VLNCTRL_CFIEN);
+	}
+
 	/*
 	 * Write addresses to available RAR registers, if there is not
 	 * sufficient space to store all the addresses then enable
@@ -7634,6 +7640,10 @@ static int ixgbe_set_features(struct net_device *netdev,
 	else
 		ixgbe_vlan_strip_disable(adapter);
 
+	/* reset if HW VLAN filter is changed */
+	if (changed & NETIF_F_HW_VLAN_CTAG_FILTER)
+		need_reset = true;
+
 	if (changed & NETIF_F_RXALL)
 		need_reset = true;
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
index 13916d8..5508d8a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c
@@ -776,6 +776,10 @@ static int ixgbe_set_vf_vlan_msg(struct ixgbe_adapter *adapter,
 	u32 bits;
 	u8 tcs = netdev_get_num_tc(adapter->netdev);
 
+	/* Ignore if VLAN filter is disabled */
+	if (!(adapter->netdev->features & NETIF_F_HW_VLAN_CTAG_FILTER))
+		return 0;
+
 	if (adapter->vfinfo[vf].pf_vlan || tcs) {
 		e_warn(drv,
 		       "VF %d attempted to override administratively set VLAN configuration\n"
-- 
1.9.0


------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [E1000-devel] [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

On Thu, 2014-11-13 at 08:28 +0000, Hiroshi Shimamoto wrote:
> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> 
> Disable hardware VLAN filtering if netdev->features VLAN flag is
> dropped.
> 
> In SR-IOV case, there is a use case which needs to disable VLAN
> filter.
> For example, we need to make a network function with VF in virtualized
> environment. That network function may be a software switch, a router
> or etc. It means that that network function will be an end point which
> terminates many VLANs.
> 
> In the current implementation, VLAN filtering always be turned on and
> VF can receive only 63 VLANs. It means that only 63 VLANs can be used
> and it's not enough at all for building a virtual router.
> 
> With this patch, if the user turns VLAN filtering off on the host, VF
> can receive every VLAN packet.
> The behavior is changed only if VLAN filtering is turned off by
> ethtool.
> 
> Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> CC: Choi, Sy Jong <sy.jong.choi@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 10 ++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |  4 ++++
>  2 files changed, 14 insertions(+)

Thanks Hiroshi, I will add your patch to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On Thu, 2014-11-13 at 08:28 +0000, Hiroshi Shimamoto wrote:
> From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> 
> Disable hardware VLAN filtering if netdev->features VLAN flag is dropped.
> 
> In SR-IOV case, there is a use case which needs to disable VLAN filter.
> For example, we need to make a network function with VF in virtualized
> environment. That network function may be a software switch, a router
> or etc. It means that that network function will be an end point which
> terminates many VLANs.
> 
> In the current implementation, VLAN filtering always be turned on and
> VF can receive only 63 VLANs. It means that only 63 VLANs can be used
> and it's not enough at all for building a virtual router.
> 
> With this patch, if the user turns VLAN filtering off on the host, VF
> can receive every VLAN packet.
> The behavior is changed only if VLAN filtering is turned off by ethtool.
[...]

What happens when VLAN filtering is turned back on and a VF uses too
many VLANs?  It seems like that should either be prevented (you can't
turn it back on) or the driver should log a message saying the VF is now
broken.

Ben.

-- 
Ben Hutchings
Beware of bugs in the above code;
I have only proved it correct, not tried it. - Donald Knuth

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

RE: [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[Hiroshi Shimamoto]

> Subject: Re: [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case
> 
> On Thu, 2014-11-13 at 08:28 +0000, Hiroshi Shimamoto wrote:
> > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> >
> > Disable hardware VLAN filtering if netdev->features VLAN flag is dropped.
> >
> > In SR-IOV case, there is a use case which needs to disable VLAN filter.
> > For example, we need to make a network function with VF in virtualized
> > environment. That network function may be a software switch, a router
> > or etc. It means that that network function will be an end point which
> > terminates many VLANs.
> >
> > In the current implementation, VLAN filtering always be turned on and
> > VF can receive only 63 VLANs. It means that only 63 VLANs can be used
> > and it's not enough at all for building a virtual router.
> >
> > With this patch, if the user turns VLAN filtering off on the host, VF
> > can receive every VLAN packet.
> > The behavior is changed only if VLAN filtering is turned off by ethtool.
> [...]
> 
> What happens when VLAN filtering is turned back on and a VF uses too
> many VLANs?  It seems like that should either be prevented (you can't
> turn it back on) or the driver should log a message saying the VF is now
> broken.

that's reasonable.
Will submit additional patch to take care about that.

thanks,
Hiroshi

> 
> Ben.
> 
> --
> Ben Hutchings
> Beware of bugs in the above code;
> I have only proved it correct, not tried it. - Donald Knuth

RE: [E1000-devel] [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[Hiroshi Shimamoto]

> Subject: Re: [E1000-devel] [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case
> 
> On Thu, 2014-11-13 at 08:28 +0000, Hiroshi Shimamoto wrote:
> > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> >
> > Disable hardware VLAN filtering if netdev->features VLAN flag is
> > dropped.
> >
> > In SR-IOV case, there is a use case which needs to disable VLAN
> > filter.
> > For example, we need to make a network function with VF in virtualized
> > environment. That network function may be a software switch, a router
> > or etc. It means that that network function will be an end point which
> > terminates many VLANs.
> >
> > In the current implementation, VLAN filtering always be turned on and
> > VF can receive only 63 VLANs. It means that only 63 VLANs can be used
> > and it's not enough at all for building a virtual router.
> >
> > With this patch, if the user turns VLAN filtering off on the host, VF
> > can receive every VLAN packet.
> > The behavior is changed only if VLAN filtering is turned off by
> > ethtool.
> >
> > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > CC: Choi, Sy Jong <sy.jong.choi@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 10 ++++++++++
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |  4 ++++
> >  2 files changed, 14 insertions(+)
> 
> Thanks Hiroshi, I will add your patch to my queue.

How about this patch?
It hasn't been in your tree,.
Is there any issue?

thanks,
Hiroshi

Re: [E1000-devel] [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

On Wed, 2015-02-25 at 00:51 +0000, Hiroshi Shimamoto wrote:
> > Subject: Re: [E1000-devel] [PATCH] ixgbe: make VLAN filter
> conditional in SR-IOV case
> > 
> > On Thu, 2014-11-13 at 08:28 +0000, Hiroshi Shimamoto wrote:
> > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > >
> > > Disable hardware VLAN filtering if netdev->features VLAN flag is
> > > dropped.
> > >
> > > In SR-IOV case, there is a use case which needs to disable VLAN
> > > filter.
> > > For example, we need to make a network function with VF in
> virtualized
> > > environment. That network function may be a software switch, a
> router
> > > or etc. It means that that network function will be an end point
> which
> > > terminates many VLANs.
> > >
> > > In the current implementation, VLAN filtering always be turned on
> and
> > > VF can receive only 63 VLANs. It means that only 63 VLANs can be
> used
> > > and it's not enough at all for building a virtual router.
> > >
> > > With this patch, if the user turns VLAN filtering off on the host,
> VF
> > > can receive every VLAN packet.
> > > The behavior is changed only if VLAN filtering is turned off by
> > > ethtool.
> > >
> > > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > CC: Choi, Sy Jong <sy.jong.choi@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 10 ++++++++++
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |  4 ++++
> > >  2 files changed, 14 insertions(+)
> > 
> > Thanks Hiroshi, I will add your patch to my queue.
> 
> How about this patch?
> It hasn't been in your tree,.
> Is there any issue?

This patch was dropped for two reasons.  First was Ben Hutchings issues
with the patch needed to be addressed.  Second, was due to a possible
security hole which is why VLAN filtering was not disabled in SRIOV
mode, where isolation is lost between VMs.

If you want to continue going forward with this change, a warning
message should be added, at least, warning the user of the possible
security issues.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [E1000-devel] [PATCH] ixgbe: make VLAN filter conditional in SR-IOV case

[Hiroshi Shimamoto]

> On Wed, 2015-02-25 at 00:51 +0000, Hiroshi Shimamoto wrote:
> > > Subject: Re: [E1000-devel] [PATCH] ixgbe: make VLAN filter
> > conditional in SR-IOV case
> > >
> > > On Thu, 2014-11-13 at 08:28 +0000, Hiroshi Shimamoto wrote:
> > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > >
> > > > Disable hardware VLAN filtering if netdev->features VLAN flag is
> > > > dropped.
> > > >
> > > > In SR-IOV case, there is a use case which needs to disable VLAN
> > > > filter.
> > > > For example, we need to make a network function with VF in
> > virtualized
> > > > environment. That network function may be a software switch, a
> > router
> > > > or etc. It means that that network function will be an end point
> > which
> > > > terminates many VLANs.
> > > >
> > > > In the current implementation, VLAN filtering always be turned on
> > and
> > > > VF can receive only 63 VLANs. It means that only 63 VLANs can be
> > used
> > > > and it's not enough at all for building a virtual router.
> > > >
> > > > With this patch, if the user turns VLAN filtering off on the host,
> > VF
> > > > can receive every VLAN packet.
> > > > The behavior is changed only if VLAN filtering is turned off by
> > > > ethtool.
> > > >
> > > > Signed-off-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > CC: Choi, Sy Jong <sy.jong.choi@intel.com>
> > > > ---
> > > >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 10 ++++++++++
> > > >  drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |  4 ++++
> > > >  2 files changed, 14 insertions(+)
> > >
> > > Thanks Hiroshi, I will add your patch to my queue.
> >
> > How about this patch?
> > It hasn't been in your tree,.
> > Is there any issue?
> 
> This patch was dropped for two reasons.  First was Ben Hutchings issues
> with the patch needed to be addressed.  Second, was due to a possible
> security hole which is why VLAN filtering was not disabled in SRIOV
> mode, where isolation is lost between VMs.
> 
> If you want to continue going forward with this change, a warning
> message should be added, at least, warning the user of the possible
> security issues.

okay, I understand.
I will submit a patch which has warning message.

thanks,
Hiroshi