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

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

[Michael Chan]

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(-)

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,
-- 
1.7.1

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

[Ben Hutchings]

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,

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Michael Chan]

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.

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

[Ben Hutchings]

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.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

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

[Michael Chan]

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.

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

[David Miller]

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.

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

[Michael Chan]

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?
> 
> 

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

[David Miller]

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?

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

[Michael Chan]

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.

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

[David Miller]

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.

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

[Michael Chan]

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?

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

[John Fastabend]

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

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

[Michael Chan]

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.


> 

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

[David Miller]

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.