[PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[Or Gerlitz]

The current implementations all use dev_uc_add_excl() and such whose API
doesn't support vlans, so we can't make it with NICs HW for now.

Fixes: f6f6424ba773 ('net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del')
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c |    5 +++++
 net/core/rtnetlink.c                        |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 0a7ea4c..a5f2660 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -7549,6 +7549,11 @@ static int i40e_ndo_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	if (!(pf->flags & I40E_FLAG_SRIOV_ENABLED))
 		return -EOPNOTSUPP;
 
+	if (vid) {
+		pr_info("%s: vlans aren't supported yet for dev_uc|mc_add()\n", dev->name);
+		return -EINVAL;
+	}
+
 	/* Hardware does not support aging addresses so if a
 	 * ndm_state is given only allow permanent addresses
 	 */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d06107d..9cf6fe9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2368,6 +2368,11 @@ int ndo_dflt_fdb_add(struct ndmsg *ndm,
 		return err;
 	}
 
+	if (vid) {
+		pr_info("%s: vlans aren't supported yet for dev_uc|mc_add()\n", dev->name);
+		return err;
+	}
+
 	if (is_unicast_ether_addr(addr) || is_link_local_ether_addr(addr))
 		err = dev_uc_add_excl(dev, addr);
 	else if (is_multicast_ether_addr(addr))
-- 
1.7.1

Re: [PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[Jiri Pirko]

Sun, Dec 14, 2014 at 05:19:05PM CET, ogerlitz@mellanox.com wrote:
>The current implementations all use dev_uc_add_excl() and such whose API
>doesn't support vlans, so we can't make it with NICs HW for now.
>
>Fixes: f6f6424ba773 ('net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del')

Maybe I'm missing something, but this commit did not introduce the
problem. If was there already before when NDA_VLAN was set and ignored.

But other than this. I like the patch

Reviewed-by: Jiri Pirko <jiri@resnulli.us>



>Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
>---
> drivers/net/ethernet/intel/i40e/i40e_main.c |    5 +++++
> net/core/rtnetlink.c                        |    5 +++++
> 2 files changed, 10 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>index 0a7ea4c..a5f2660 100644
>--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>@@ -7549,6 +7549,11 @@ static int i40e_ndo_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
> 	if (!(pf->flags & I40E_FLAG_SRIOV_ENABLED))
> 		return -EOPNOTSUPP;
> 
>+	if (vid) {
>+		pr_info("%s: vlans aren't supported yet for dev_uc|mc_add()\n", dev->name);
>+		return -EINVAL;
>+	}
>+
> 	/* Hardware does not support aging addresses so if a
> 	 * ndm_state is given only allow permanent addresses
> 	 */
>diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>index d06107d..9cf6fe9 100644
>--- a/net/core/rtnetlink.c
>+++ b/net/core/rtnetlink.c
>@@ -2368,6 +2368,11 @@ int ndo_dflt_fdb_add(struct ndmsg *ndm,
> 		return err;
> 	}
> 
>+	if (vid) {
>+		pr_info("%s: vlans aren't supported yet for dev_uc|mc_add()\n", dev->name);
>+		return err;
>+	}
>+
> 	if (is_unicast_ether_addr(addr) || is_link_local_ether_addr(addr))
> 		err = dev_uc_add_excl(dev, addr);
> 	else if (is_multicast_ether_addr(addr))
>-- 
>1.7.1
>

Re: [PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[Or Gerlitz]

On Sun, Dec 14, 2014 at 9:23 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Sun, Dec 14, 2014 at 05:19:05PM CET, ogerlitz@mellanox.com wrote:
>>The current implementations all use dev_uc_add_excl() and such whose API
>>doesn't support vlans, so we can't make it with NICs HW for now.
>>
>>Fixes: f6f6424ba773 ('net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del')
>
> Maybe I'm missing something, but this commit did not introduce the
> problem.

This commit introduced the vid param to ndo_fdb_add and ndo_dflt_fdb_add
which further call the dev_uc_add APIs... so it did introduced the
ability to provide VID into these APIs, right? and we want to protect
against anyone using this ability @ this point.

> If was there already before when NDA_VLAN was set and ignored.
> But other than this. I like the patch
>
> Reviewed-by: Jiri Pirko <jiri@resnulli.us>

Re: [PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[Jiri Pirko]

Sun, Dec 14, 2014 at 09:14:27PM CET, gerlitz.or@gmail.com wrote:
>On Sun, Dec 14, 2014 at 9:23 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Sun, Dec 14, 2014 at 05:19:05PM CET, ogerlitz@mellanox.com wrote:
>>>The current implementations all use dev_uc_add_excl() and such whose API
>>>doesn't support vlans, so we can't make it with NICs HW for now.
>>>
>>>Fixes: f6f6424ba773 ('net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del')
>>
>> Maybe I'm missing something, but this commit did not introduce the
>> problem.
>
>This commit introduced the vid param to ndo_fdb_add and ndo_dflt_fdb_add
>which further call the dev_uc_add APIs... so it did introduced the
>ability to provide VID into these APIs, right? and we want to protect
>against anyone using this ability @ this point.

That is in-kernel change. Vs. usespace the patch is a no-op. If userspace
fills up NDA_VLAN, it is ignored before the patch as well as after. No
behaviour change, just +- cosmetics.

>
>> If was there already before when NDA_VLAN was set and ignored.
>> But other than this. I like the patch
>>
>> Reviewed-by: Jiri Pirko <jiri@resnulli.us>

Re: [PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[Jamal Hadi Salim]

On 12/14/14 17:33, Jiri Pirko wrote:
> Sun, Dec 14, 2014 at 09:14:27PM CET, gerlitz.or@gmail.com wrote:
>> On Sun, Dec 14, 2014 at 9:23 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Sun, Dec 14, 2014 at 05:19:05PM CET, ogerlitz@mellanox.com wrote:
>>>> The current implementations all use dev_uc_add_excl() and such whose API
>>>> doesn't support vlans, so we can't make it with NICs HW for now.
>>>>
>>>> Fixes: f6f6424ba773 ('net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del')
>>>
>>> Maybe I'm missing something, but this commit did not introduce the
>>> problem.
>>
>> This commit introduced the vid param to ndo_fdb_add and ndo_dflt_fdb_add
>> which further call the dev_uc_add APIs... so it did introduced the
>> ability to provide VID into these APIs, right? and we want to protect
>> against anyone using this ability @ this point.
>
> That is in-kernel change. Vs. usespace the patch is a no-op. If userspace
> fills up NDA_VLAN, it is ignored before the patch as well as after. No
> behaviour change, just +- cosmetics.
>

Indeed doesnt break anything, but:
I think this brings to the forefront what these dev_uc/mc addresses
are supposed to be here. I am sure there is a good reason to tie them
to the fdb API - I am just not grokking it at the moment.
The concept of a vlanid makes sense for an fdb entry. It
starts to break when you start tying in vlans - i.e i am not aware
of multicast/unicast device entries which are tied to vlans.
Maybe this is some SRIOV thing?

cheers,
jamal

Re: [PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[Jeff Kirsher]

On Sun, Dec 14, 2014 at 8:19 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
> The current implementations all use dev_uc_add_excl() and such whose API
> doesn't support vlans, so we can't make it with NICs HW for now.
>
> Fixes: f6f6424ba773 ('net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del')
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c |    5 +++++
>  net/core/rtnetlink.c                        |    5 +++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
>

For the Intel driver changes...
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Re: [PATCH net] net: Disallow providing non zero VLAN ID for NIC drivers FDB add flow

[David Miller]

From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Sun, 14 Dec 2014 18:19:05 +0200

> The current implementations all use dev_uc_add_excl() and such whose API
> doesn't support vlans, so we can't make it with NICs HW for now.
> 
> Fixes: f6f6424ba773 ('net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del')
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>

Applied, thanks.