[PATCH net] net/8021q: Check the correct vlan filter capability

[PATCH net] net/8021q: Check the correct vlan filter capability

[Saeed Mahameed]

From: Gal Pressman <galp@mellanox.com>

Use dev->hw_features in vlan_hw_filter_capable instead of dev->features,
since dev->features can be turned on/off dynamically.

Netdev features can be changed dynamically to off after vlan_vid_add
was called, thus vlan_vid_del will skip ndo_vlan_rx_kill_vid and will
leave the device driver with un-freed resources.

Fixes: 8ad227ff89a7 ("net: vlan: add 802.1ad support")
Signed-off-by: Gal Pressman <galp@mellanox.com>
Cc: Patrick McHardy <kaber@trash.net>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 net/8021q/vlan.c      |    4 ++--
 net/8021q/vlan_core.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index d2cd9de..f744d8c 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -365,7 +365,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 	}
 
 	if ((event == NETDEV_UP) &&
-	    (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)) {
+	    (dev->hw_features & NETIF_F_HW_VLAN_CTAG_FILTER)) {
 		pr_info("adding VLAN 0 to HW filter on device %s\n",
 			dev->name);
 		vlan_vid_add(dev, htons(ETH_P_8021Q), 0);
@@ -417,7 +417,7 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		struct net_device *tmp;
 		LIST_HEAD(close_list);
 
-		if (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
+		if (dev->hw_features & NETIF_F_HW_VLAN_CTAG_FILTER)
 			vlan_vid_del(dev, htons(ETH_P_8021Q), 0);
 
 		/* Put all VLANs for this dev in the down state too.  */
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index e2ed698..bab95bd 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -162,10 +162,10 @@ static bool vlan_hw_filter_capable(const struct net_device *dev,
 				     const struct vlan_vid_info *vid_info)
 {
 	if (vid_info->proto == htons(ETH_P_8021Q) &&
-	    dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
+	    dev->hw_features & NETIF_F_HW_VLAN_CTAG_FILTER)
 		return true;
 	if (vid_info->proto == htons(ETH_P_8021AD) &&
-	    dev->features & NETIF_F_HW_VLAN_STAG_FILTER)
+	    dev->hw_features & NETIF_F_HW_VLAN_STAG_FILTER)
 		return true;
 	return false;
 }
-- 
1.7.1

Re: [PATCH net] net/8021q: Check the correct vlan filter capability

[David Miller]

From: Saeed Mahameed <saeedm@mellanox.com>
Date: Wed, 24 Feb 2016 18:39:19 +0200

> From: Gal Pressman <galp@mellanox.com>
> 
> Use dev->hw_features in vlan_hw_filter_capable instead of dev->features,
> since dev->features can be turned on/off dynamically.
> 
> Netdev features can be changed dynamically to off after vlan_vid_add
> was called, thus vlan_vid_del will skip ndo_vlan_rx_kill_vid and will
> leave the device driver with un-freed resources.

Are you sure the fix isn't to make vlan_vid_add() check ->features instead
of ->hw_features.

Should we really be trying to add VLAN filters when the user has
turned it off?

Re: [PATCH net] net/8021q: Check the correct vlan filter capability

[Saeed Mahameed]

>> Netdev features can be changed dynamically to off after vlan_vid_add
>> was called, thus vlan_vid_del will skip ndo_vlan_rx_kill_vid and will
>> leave the device driver with un-freed resources.
>
> Are you sure the fix isn't to make vlan_vid_add() check ->features instead
> of ->hw_features.

This is exactly what this fix suggests, "->features" is not consistent
and can be turned ON/OFF between vlan_add/del which can leave the NIC
driver in inconsistent state !

>
> Should we really be trying to add VLAN filters when the user has
> turned it off?

Well, I think it is debatable, but the current implementation is not
consistent, especially for adding vlan 0 by default and then the user
disables the vlan filter, this will cause the stack to never call the
nic ndo_vlan_rx_kill_vid for the pre added vlan 0 and vise versa call
kill_vid without add_vid, BUG ?

So i think we have two options, use this patch, and always trust to
delegate vlan_vid_add/del to the NIC when it's HW supports it, and the
nic will be smart enough to know what to do with it (in case vlan
filter is enabled/disabled). Or, for each vlan we can remember if it
was added to the NIC or not so the stack will know whether to clean it
up or not.

Re: [PATCH net] net/8021q: Check the correct vlan filter capability

[Saeed Mahameed]

On Wed, Feb 24, 2016 at 10:27 PM, Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
>>> Netdev features can be changed dynamically to off after vlan_vid_add
>>> was called, thus vlan_vid_del will skip ndo_vlan_rx_kill_vid and will
>>> leave the device driver with un-freed resources.
>>
>> Are you sure the fix isn't to make vlan_vid_add() check ->features instead
>> of ->hw_features.
>
> This is exactly what this fix suggests, "->features" is not consistent
> and can be turned ON/OFF between vlan_add/del which can leave the NIC
> driver in inconsistent state !
>
>>
>> Should we really be trying to add VLAN filters when the user has
>> turned it off?
>
> Well, I think it is debatable, but the current implementation is not
> consistent, especially for adding vlan 0 by default and then the user
> disables the vlan filter, this will cause the stack to never call the
> nic ndo_vlan_rx_kill_vid for the pre added vlan 0 and vise versa call
> kill_vid without add_vid, BUG ?
>
> So i think we have two options, use this patch, and always trust to
> delegate vlan_vid_add/del to the NIC when it's HW supports it, and the
> nic will be smart enough to know what to do with it (in case vlan
> filter is enabled/disabled). Or, for each vlan we can remember if it
> was added to the NIC or not so the stack will know whether to clean it
> up or not.

BTW we choose the first option since the "buggy" function is called
"vlan_hw_filter_capable" which lead us to decide that the function
should be looking for ->hw_features and not ->features.
and i think this way it makes more sense.

Re: [PATCH net] net/8021q: Check the correct vlan filter capability

[David Miller]

From: Saeed Mahameed <saeedm@dev.mellanox.co.il>
Date: Wed, 24 Feb 2016 22:27:16 +0200

>>> Netdev features can be changed dynamically to off after vlan_vid_add
>>> was called, thus vlan_vid_del will skip ndo_vlan_rx_kill_vid and will
>>> leave the device driver with un-freed resources.
>>
>> Are you sure the fix isn't to make vlan_vid_add() check ->features instead
>> of ->hw_features.
> 
> This is exactly what this fix suggests, "->features" is not consistent
> and can be turned ON/OFF between vlan_add/del which can leave the NIC
> driver in inconsistent state !

But the user changes the setting _exactly_ to control whether these
VLAN offloads occur or not.

>> Should we really be trying to add VLAN filters when the user has
>> turned it off?
> 
> Well, I think it is debatable, but the current implementation is not
> consistent, especially for adding vlan 0 by default and then the user
> disables the vlan filter, this will cause the stack to never call the
> nic ndo_vlan_rx_kill_vid for the pre added vlan 0 and vise versa call
> kill_vid without add_vid, BUG ?
> 
> So i think we have two options, use this patch, and always trust to
> delegate vlan_vid_add/del to the NIC when it's HW supports it, and the
> nic will be smart enough to know what to do with it (in case vlan
> filter is enabled/disabled). Or, for each vlan we can remember if it
> was added to the NIC or not so the stack will know whether to clean it
> up or not.

If automatically added VLANs are the issue, then we should specially
mark it such that it will get forcefully removed regardless of feature
settings.

The whole point of the separation of ->hw_features and ->features is
to separate what the card can do from what the user wants enabled or
not.

Therefore offload operations should be triggered by ->features not
->hw_features.

Any test on ->hw_features that is not a validation of a ->features
change request is a BIG RED FLAG and almost always a bug.

Re: [PATCH net] net/8021q: Check the correct vlan filter capability

[David Miller]

From: Saeed Mahameed <saeedm@dev.mellanox.co.il>
Date: Wed, 24 Feb 2016 22:35:16 +0200

> BTW we choose the first option since the "buggy" function is called
> "vlan_hw_filter_capable" which lead us to decide that the function
> should be looking for ->hw_features and not ->features.
> and i think this way it makes more sense.

Again, anything doing checks on ->hw_features other than for the
purposes of the validation of a ->feature change is a bug.

Re: [PATCH net] net/8021q: Check the correct vlan filter capability

[John Fastabend]

On 16-02-24 01:48 PM, David Miller wrote:
> From: Saeed Mahameed <saeedm@dev.mellanox.co.il>
> Date: Wed, 24 Feb 2016 22:27:16 +0200
> 
>>>> Netdev features can be changed dynamically to off after vlan_vid_add
>>>> was called, thus vlan_vid_del will skip ndo_vlan_rx_kill_vid and will
>>>> leave the device driver with un-freed resources.
>>>
>>> Are you sure the fix isn't to make vlan_vid_add() check ->features instead
>>> of ->hw_features.
>>
>> This is exactly what this fix suggests, "->features" is not consistent
>> and can be turned ON/OFF between vlan_add/del which can leave the NIC
>> driver in inconsistent state !
> 
> But the user changes the setting _exactly_ to control whether these
> VLAN offloads occur or not.
> 
>>> Should we really be trying to add VLAN filters when the user has
>>> turned it off?
>>
>> Well, I think it is debatable, but the current implementation is not
>> consistent, especially for adding vlan 0 by default and then the user
>> disables the vlan filter, this will cause the stack to never call the
>> nic ndo_vlan_rx_kill_vid for the pre added vlan 0 and vise versa call
>> kill_vid without add_vid, BUG ?
>>
>> So i think we have two options, use this patch, and always trust to
>> delegate vlan_vid_add/del to the NIC when it's HW supports it, and the
>> nic will be smart enough to know what to do with it (in case vlan
>> filter is enabled/disabled). Or, for each vlan we can remember if it
>> was added to the NIC or not so the stack will know whether to clean it
>> up or not.
> 
> If automatically added VLANs are the issue, then we should specially
> mark it such that it will get forcefully removed regardless of feature
> settings.
> 

I suspect the hardware driver should flush the vid list in hardware
when the feature flag is disabled and when it is enabled I guess we
would need to do something like vlan_vids_add_by_dev() and do an add
for all the upper dev vlans.

I guess with some helper functions this could be done generally in
the ethtool set_features code path by tracking down the vid list and
calling all the del and add routines for each vlan.

A bit more work but seems more correct then creating strange cases
where hw_fatures and features don't actually work as expected.

> The whole point of the separation of ->hw_features and ->features is
> to separate what the card can do from what the user wants enabled or
> not.
> 
> Therefore offload operations should be triggered by ->features not
> ->hw_features.
> 
> Any test on ->hw_features that is not a validation of a ->features
> change request is a BIG RED FLAG and almost always a bug.
>