[PATCH net-next] net: More vlan tests before registering netdevice

[PATCH net-next] net: More vlan tests before registering netdevice

[Yuval Mintz]

When register_netdevice() is called, netdevice's vlan filtering feature
and supplied callbacks are checked to see the vlan implementation is
not buggy.
This adds an additional test - see that the vlan_features were filled
correctly, as the vlan devices inherits those as its own features;
Incorrect values set there would later prevent the vlan interface from being
registered itself [as it doesn't implement the filtering ndos].

Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
---
Hi Dave,

Not sure why take such a defensive approach regarding this feature.
Perhaps it would have been better to simply remove these checks altogether.

Cheers,
Yuval
---

 net/core/dev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 3f191da..8a663b2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6253,10 +6253,11 @@ int register_netdevice(struct net_device *dev)
 		}
 	}
 
-	if (((dev->hw_features | dev->features) &
-	     NETIF_F_HW_VLAN_CTAG_FILTER) &&
-	    (!dev->netdev_ops->ndo_vlan_rx_add_vid ||
-	     !dev->netdev_ops->ndo_vlan_rx_kill_vid)) {
+	if ((((dev->hw_features | dev->features) &
+	      NETIF_F_HW_VLAN_CTAG_FILTER) &&
+	     (!dev->netdev_ops->ndo_vlan_rx_add_vid ||
+	      !dev->netdev_ops->ndo_vlan_rx_kill_vid)) ||
+	    (dev->vlan_features & NETIF_F_HW_VLAN_CTAG_FILTER) {
 		netdev_WARN(dev, "Buggy VLAN acceleration in driver!\n");
 		ret = -EINVAL;
 		goto err_uninit;
-- 
1.9.3

Re: [PATCH net-next] net: More vlan tests before registering netdevice

[David Miller]

From: Yuval Mintz <Yuval.Mintz@qlogic.com>
Date: Wed, 31 Dec 2014 08:35:36 +0200

> When register_netdevice() is called, netdevice's vlan filtering feature
> and supplied callbacks are checked to see the vlan implementation is
> not buggy.
> This adds an additional test - see that the vlan_features were filled
> correctly, as the vlan devices inherits those as its own features;
> Incorrect values set there would later prevent the vlan interface from being
> registered itself [as it doesn't implement the filtering ndos].
> 
> Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
 ...
> Not sure why take such a defensive approach regarding this feature.
> Perhaps it would have been better to simply remove these checks altogether.

I guess this is fine as a defensive test, so I'll apply this.

But do you actually know of any devices which have violated this rule
either now or in the past?  I quickly tried to audit the entire tree
for this right now and found no problems.

Re: [PATCH net-next] net: More vlan tests before registering netdevice

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 02 Jan 2015 15:56:21 -0500 (EST)

> From: Yuval Mintz <Yuval.Mintz@qlogic.com>
> Date: Wed, 31 Dec 2014 08:35:36 +0200
> 
>> When register_netdevice() is called, netdevice's vlan filtering feature
>> and supplied callbacks are checked to see the vlan implementation is
>> not buggy.
>> This adds an additional test - see that the vlan_features were filled
>> correctly, as the vlan devices inherits those as its own features;
>> Incorrect values set there would later prevent the vlan interface from being
>> registered itself [as it doesn't implement the filtering ndos].
>> 
>> Signed-off-by: Yuval Mintz <Yuval.Mintz@qlogic.com>
>  ...
>> Not sure why take such a defensive approach regarding this feature.
>> Perhaps it would have been better to simply remove these checks altogether.
> 
> I guess this is fine as a defensive test, so I'll apply this.

Actually, reverted.

Nothing makes me more angry than a patch that wasn't even build tested:

net/core/dev.c: In function ‘register_netdevice’:
net/core/dev.c:6285:57: error: expected ‘)’ before ‘{’ token
net/core/dev.c:6378:1: error: expected expression before ‘}’ token
net/core/dev.c:6277:4: error: label ‘out’ used but not defined
net/core/dev.c:6378:1: warning: control reaches end of non-void function [-Wreturn-type]

RE: [PATCH net-next] net: More vlan tests before registering netdevice

[Yuval Mintz]

> > I guess this is fine as a defensive test, so I'll apply this.
> > But do you actually know of any devices which have violated this rule either 
> > now or in the past?
> > I quickly tried to audit the entire tree for this right now and found no problems.
> 
> Actually, reverted.
> 
> Nothing makes me more angry than a patch that wasn't even build tested:
> 
> net/core/dev.c: In function ‘register_netdevice’:
> net/core/dev.c:6285:57: error: expected ‘)’ before ‘{’ token
> net/core/dev.c:6378:1: error: expected expression before ‘}’ token
> net/core/dev.c:6277:4: error: label ‘out’ used but not defined
> net/core/dev.c:6378:1: warning: control reaches end of non-void function [-
> Wreturn-type]

Sorry about that.
Don't know how that happened, but obviously I can't make any excuses.

Regarding your previous question - I hit it while implementing vlan filtering
offload on a new [yet-to-be-published] qlogic driver; Don't know if anyone
actually had that issue in-tree.