Patchwork [net-next] 8021q: validate SAN MAC address

login
register
mail settings
Submitter Michael Chan
Date Nov. 9, 2012, 10:54 p.m.
Message ID <1352501644-8444-1-git-send-email-mchan@broadcom.com>
Download mbox | patch
Permalink /patch/198157/
State Rejected
Delegated to: David Miller
Headers show

Comments

Michael Chan - Nov. 9, 2012, 10:54 p.m.
Some bnx2x multi-function devices are configured for FCoE only in
a PCI function and only has a SAN MAC address.  The current code
will fail ->ndo_validate_addr() when a VLAN device is brought up
over such a device.

Check the real device's dev_addrs[] for a valid SAN MAC address
when validating the MAC address if the regular MAC address is not
valid.

Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 net/8021q/vlan_dev.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)
Ben Hutchings - Nov. 9, 2012, 11:02 p.m.
On Fri, 2012-11-09 at 14:54 -0800, Michael Chan wrote:
> Some bnx2x multi-function devices are configured for FCoE only in
> a PCI function and only has a SAN MAC address.  The current code
> will fail ->ndo_validate_addr() when a VLAN device is brought up
> over such a device.
> 
> Check the real device's dev_addrs[] for a valid SAN MAC address
> when validating the MAC address if the regular MAC address is not
> valid.

So the VLAN device's own address is being completely ignored?

Ben.

> Signed-off-by: Michael Chan <mchan@broadcom.com>
> ---
>  net/8021q/vlan_dev.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
> 
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 6da96d4..9ef2d2e 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -346,6 +346,29 @@ static int vlan_dev_stop(struct net_device *dev)
>  	return 0;
>  }
>  
> +static int vlan_dev_validate_addr(struct net_device *dev)
> +{
> +	if (!is_valid_ether_addr(dev->dev_addr)) {
> +		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
> +
> +		if (real_dev) {
> +			struct netdev_hw_addr *ha;
> +
> +			rcu_read_lock();
> +			for_each_dev_addr(real_dev, ha) {
> +				if ((ha->type == NETDEV_HW_ADDR_T_SAN) &&
> +				    (is_valid_ether_addr(ha->addr))) {
> +					rcu_read_unlock();
> +					return 0;
> +				}
> +			}
> +			rcu_read_unlock();
> +		}
> +		return -EADDRNOTAVAIL;
> +	}
> +	return 0;
> +}
> +
>  static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
>  {
>  	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
> @@ -734,7 +757,7 @@ static const struct net_device_ops vlan_netdev_ops = {
>  	.ndo_open		= vlan_dev_open,
>  	.ndo_stop		= vlan_dev_stop,
>  	.ndo_start_xmit =  vlan_dev_hard_start_xmit,
> -	.ndo_validate_addr	= eth_validate_addr,
> +	.ndo_validate_addr	= vlan_dev_validate_addr,
>  	.ndo_set_mac_address	= vlan_dev_set_mac_address,
>  	.ndo_set_rx_mode	= vlan_dev_set_rx_mode,
>  	.ndo_change_rx_flags	= vlan_dev_change_rx_flags,
Michael Chan - Nov. 10, 2012, 12:25 a.m.
On Fri, 2012-11-09 at 23:02 +0000, Ben Hutchings wrote:
> > Some bnx2x multi-function devices are configured for FCoE only in
> > a PCI function and only has a SAN MAC address.  The current code
> > will fail ->ndo_validate_addr() when a VLAN device is brought up
> > over such a device.
> > 
> > Check the real device's dev_addrs[] for a valid SAN MAC address
> > when validating the MAC address if the regular MAC address is not
> > valid.
> 
> So the VLAN device's own address is being completely ignored?

No, it is not being ignored.  We validate the VLAN's regular MAC address
first.  If it is invalid, check further to see if the real device has a
SAN MAC address.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Nov. 10, 2012, 12:36 a.m.
On Fri, 2012-11-09 at 16:25 -0800, Michael Chan wrote:
> On Fri, 2012-11-09 at 23:02 +0000, Ben Hutchings wrote:
> > > Some bnx2x multi-function devices are configured for FCoE only in
> > > a PCI function and only has a SAN MAC address.  The current code
> > > will fail ->ndo_validate_addr() when a VLAN device is brought up
> > > over such a device.
> > > 
> > > Check the real device's dev_addrs[] for a valid SAN MAC address
> > > when validating the MAC address if the regular MAC address is not
> > > valid.
> > 
> > So the VLAN device's own address is being completely ignored?
> 
> No, it is not being ignored.  We validate the VLAN's regular MAC address
> first.  If it is invalid, check further to see if the real device has a
> SAN MAC address.

Sure, but in the case that the VLAN device is layered on one of these
FCoE SAN devices, the VLAN device's address isn't validated.  Which
presumably means it's not going to be used at all...

What does the VLAN device actually do in this case?  Is it a way of
setting a VID to be used for the FCoE encapsulation?

Ben.
Michael Chan - Nov. 10, 2012, 12:44 a.m.
On Sat, 2012-11-10 at 00:36 +0000, Ben Hutchings wrote: 
> On Fri, 2012-11-09 at 16:25 -0800, Michael Chan wrote:
> > On Fri, 2012-11-09 at 23:02 +0000, Ben Hutchings wrote:
> > > > Some bnx2x multi-function devices are configured for FCoE only in
> > > > a PCI function and only has a SAN MAC address.  The current code
> > > > will fail ->ndo_validate_addr() when a VLAN device is brought up
> > > > over such a device.
> > > > 
> > > > Check the real device's dev_addrs[] for a valid SAN MAC address
> > > > when validating the MAC address if the regular MAC address is not
> > > > valid.
> > > 
> > > So the VLAN device's own address is being completely ignored?
> > 
> > No, it is not being ignored.  We validate the VLAN's regular MAC address
> > first.  If it is invalid, check further to see if the real device has a
> > SAN MAC address.
> 
> Sure, but in the case that the VLAN device is layered on one of these
> FCoE SAN devices, the VLAN device's address isn't validated.  Which
> presumably means it's not going to be used at all...
> 
> What does the VLAN device actually do in this case?  Is it a way of
> setting a VID to be used for the FCoE encapsulation?

Yeah, fcoemon first gets the SAN MAC from the physical device (using
DCBNL) and uses it send out packets for VLAN discovery.  After the VLAN
has been discovered, it will create the VLAN device over the physical
device and will try to bring it up.  After that, fcoemon will continue
to use the SAN MAC over the VLAN device.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 13, 2012, 9:53 p.m.
From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 9 Nov 2012 14:54:04 -0800

> Some bnx2x multi-function devices are configured for FCoE only in
> a PCI function and only has a SAN MAC address.  The current code
> will fail ->ndo_validate_addr() when a VLAN device is brought up
> over such a device.
> 
> Check the real device's dev_addrs[] for a valid SAN MAC address
> when validating the MAC address if the regular MAC address is not
> valid.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>

I've read this thread and I still don't think this is a nice
change at all.

It's beyond hackish to allow configuring a VLAN with a completely
invalid MAC address just because the underlying real device just so
happens to have a storage MAC address.

I can only assume that, in this case, this code block is triggered
in vlan_dev_open():

	if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) {
		err = dev_uc_add(real_dev, dev->dev_addr);
		if (err < 0)
			goto out;
	}

And if so we're adding an invalid ethernet address to the underlying
device's UC list.  Yet another terrible aspect of this situation.

The more I look into this situation the more it looks like piling crap
on top of crap on top of crap.  It's a long depency chain of special
cases just to get this weird setup working.

I'm sorry, I'm not making the situation even worse by applying this
patch.  If you want to have real bypasses to allow VLAN VIDs to
be configured on SAN devices, create a real interface for it, rather
than hack to shreds the stuff we have already.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Chan - Nov. 13, 2012, 10:29 p.m.
On Tue, 2012-11-13 at 16:53 -0500, David Miller wrote:
> I can only assume that, in this case, this code block is triggered
> in vlan_dev_open():
> 
>         if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) {
>                 err = dev_uc_add(real_dev, dev->dev_addr);
>                 if (err < 0)
>                         goto out;
>         }

No, this is not triggered.  The addresses will be equal (and invalid) in
this case.  No addresses will be added.
> 
> And if so we're adding an invalid ethernet address to the underlying
> device's UC list.  Yet another terrible aspect of this situation.
> 
> The more I look into this situation the more it looks like piling crap
> on top of crap on top of crap.  It's a long depency chain of special
> cases just to get this weird setup working.
> 
> I'm sorry, I'm not making the situation even worse by applying this
> patch.  If you want to have real bypasses to allow VLAN VIDs to
> be configured on SAN devices, create a real interface for it, rather
> than hack to shreds the stuff we have already.

I don't understand.  We need to have VLAN tags inserted to packets using
that interface.  The VID is dynamically discovered by application.  How
do suggest we do that if we don't use 8021q?
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 13, 2012, 11:16 p.m.
From: "Michael Chan" <mchan@broadcom.com>
Date: Tue, 13 Nov 2012 14:29:48 -0800

> We need to have VLAN tags inserted to packets using that interface.
> The VID is dynamically discovered by application.  How do suggest we
> do that if we don't use 8021q?

Ok.  So what's the reason that the VLAN device isn't given a
valid MAC address, for example why not use the SAN one?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Chan - Nov. 13, 2012, 11:40 p.m.
On Tue, 2012-11-13 at 18:16 -0500, David Miller wrote: 
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Tue, 13 Nov 2012 14:29:48 -0800
> 
> > We need to have VLAN tags inserted to packets using that interface.
> > The VID is dynamically discovered by application.  How do suggest we
> > do that if we don't use 8021q?
> 
> Ok.  So what's the reason that the VLAN device isn't given a
> valid MAC address, for example why not use the SAN one?
> 

The VLAN netdev automatically gets the MAC address from the real
netdev->dev_addr.  In this case, the real netdev does not have a valid
dev_addr because the PCI function is a SAN device.  The SAN MAC is in a
different hw address list in netdev->dev_addrs.

I suppose we can just put the SAN MAC into the real netdev->dev_addr so
that the VLAN will automatically get it.  But this doesn't seem very
nice as we would be pretending to have a normal MAC address (for
networking) in this SAN device.  The networking MAC address is in a
different PCI function.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 14, 2012, 1:24 a.m.
From: "Michael Chan" <mchan@broadcom.com>
Date: Tue, 13 Nov 2012 15:40:29 -0800

> I suppose we can just put the SAN MAC into the real netdev->dev_addr so
> that the VLAN will automatically get it.  But this doesn't seem very
> nice as we would be pretending to have a normal MAC address (for
> networking) in this SAN device.  The networking MAC address is in a
> different PCI function.

I certainly would prefer if you took that approach.  At least in that
way the addressing of the netdev objects would appear more consistent.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Chan - Nov. 14, 2012, 1:50 a.m.
On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote:
> From: "Michael Chan" <mchan@broadcom.com>
> Date: Tue, 13 Nov 2012 15:40:29 -0800
> 
> > I suppose we can just put the SAN MAC into the real netdev->dev_addr so
> > that the VLAN will automatically get it.  But this doesn't seem very
> > nice as we would be pretending to have a normal MAC address (for
> > networking) in this SAN device.  The networking MAC address is in a
> > different PCI function.
> 
> I certainly would prefer if you took that approach.  At least in that
> way the addressing of the netdev objects would appear more consistent.
> 
Ok.  To be more complete, I think we need to add a flag or something to
such a netdev to indicate that it is a SAN device only.  What's your
opinion on that?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Fastabend - Nov. 14, 2012, 1:58 a.m.
On 11/13/2012 5:50 PM, Michael Chan wrote:
> On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote:
>> From: "Michael Chan" <mchan@broadcom.com>
>> Date: Tue, 13 Nov 2012 15:40:29 -0800
>>
>>> I suppose we can just put the SAN MAC into the real netdev->dev_addr so
>>> that the VLAN will automatically get it.  But this doesn't seem very
>>> nice as we would be pretending to have a normal MAC address (for
>>> networking) in this SAN device.  The networking MAC address is in a
>>> different PCI function.
>>
>> I certainly would prefer if you took that approach.  At least in that
>> way the addressing of the netdev objects would appear more consistent.
>>
> Ok.  To be more complete, I think we need to add a flag or something to
> such a netdev to indicate that it is a SAN device only.  What's your
> opinion on that?

Michael, how do you determine a L2 packet is a SAN type? Do you have
ACLs in the FW/hardware to prevent other types of L2 traffic from being
sent? I guess I'm asking what makes it a SAN only device.

.John

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 14, 2012, 1:58 a.m.
From: "Michael Chan" <mchan@broadcom.com>
Date: Tue, 13 Nov 2012 17:50:50 -0800

> On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote:
>> From: "Michael Chan" <mchan@broadcom.com>
>> Date: Tue, 13 Nov 2012 15:40:29 -0800
>> 
>> > I suppose we can just put the SAN MAC into the real netdev->dev_addr so
>> > that the VLAN will automatically get it.  But this doesn't seem very
>> > nice as we would be pretending to have a normal MAC address (for
>> > networking) in this SAN device.  The networking MAC address is in a
>> > different PCI function.
>> 
>> I certainly would prefer if you took that approach.  At least in that
>> way the addressing of the netdev objects would appear more consistent.
>> 
> Ok.  To be more complete, I think we need to add a flag or something to
> such a netdev to indicate that it is a SAN device only.  What's your
> opinion on that?

Yes, that would describe the limitation you mentioned.  Maybe
add a new netdev->priv_flags for this.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Chan - Nov. 14, 2012, 2:41 a.m.
On Tue, 2012-11-13 at 17:58 -0800, John Fastabend wrote: 
> On 11/13/2012 5:50 PM, Michael Chan wrote:
> > On Tue, 2012-11-13 at 20:24 -0500, David Miller wrote:
> >> From: "Michael Chan" <mchan@broadcom.com>
> >> Date: Tue, 13 Nov 2012 15:40:29 -0800
> >>
> >>> I suppose we can just put the SAN MAC into the real netdev->dev_addr so
> >>> that the VLAN will automatically get it.  But this doesn't seem very
> >>> nice as we would be pretending to have a normal MAC address (for
> >>> networking) in this SAN device.  The networking MAC address is in a
> >>> different PCI function.
> >>
> >> I certainly would prefer if you took that approach.  At least in that
> >> way the addressing of the netdev objects would appear more consistent.
> >>
> > Ok.  To be more complete, I think we need to add a flag or something to
> > such a netdev to indicate that it is a SAN device only.  What's your
> > opinion on that?
> 
> Michael, how do you determine a L2 packet is a SAN type? Do you have
> ACLs in the FW/hardware to prevent other types of L2 traffic from being
> sent? I guess I'm asking what makes it a SAN only device.
> 
Adding Eilon and Ariel from the bnx2x team to help answer the question.
I think we use ndo_select_queue() to put all FCoE related packets onto
the proper ring that is programmed with the proper DCB parameters.
Packets to the other rings will be dropped on such a device.

That's why a flag will be useful so that tools will know its limitation.


> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 6da96d4..9ef2d2e 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -346,6 +346,29 @@  static int vlan_dev_stop(struct net_device *dev)
 	return 0;
 }
 
+static int vlan_dev_validate_addr(struct net_device *dev)
+{
+	if (!is_valid_ether_addr(dev->dev_addr)) {
+		struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
+
+		if (real_dev) {
+			struct netdev_hw_addr *ha;
+
+			rcu_read_lock();
+			for_each_dev_addr(real_dev, ha) {
+				if ((ha->type == NETDEV_HW_ADDR_T_SAN) &&
+				    (is_valid_ether_addr(ha->addr))) {
+					rcu_read_unlock();
+					return 0;
+				}
+			}
+			rcu_read_unlock();
+		}
+		return -EADDRNOTAVAIL;
+	}
+	return 0;
+}
+
 static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
 {
 	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
@@ -734,7 +757,7 @@  static const struct net_device_ops vlan_netdev_ops = {
 	.ndo_open		= vlan_dev_open,
 	.ndo_stop		= vlan_dev_stop,
 	.ndo_start_xmit =  vlan_dev_hard_start_xmit,
-	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_validate_addr	= vlan_dev_validate_addr,
 	.ndo_set_mac_address	= vlan_dev_set_mac_address,
 	.ndo_set_rx_mode	= vlan_dev_set_rx_mode,
 	.ndo_change_rx_flags	= vlan_dev_change_rx_flags,