[patch net-next 00/10] mlxsw: Enable VRF offload

[patch net-next 00/10] mlxsw: Enable VRF offload

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Ido says:

Packets received from netdevs enslaved to different VRF devices are
forwarded using different FIB tables. In the Spectrum ASIC this is
achieved by binding different router interfaces (RIFs) to different
virtual routers (VRs). Each RIF represents an enslaved netdev and each
VR has its own FIB table according to which packets are forwarded.

The first three patches mark the default FIB rules and extend the FIB
notification chain to include the rule's info as part of the
RULE_{ADD,DEL} events. This allows offloading drivers to sanitize the
rules they don't support and flush their tables.

Patches 4-5 introduce small changes in the VRF driver to allow capable
drivers to more easily offload VRFs.

Finally, the last patches gradually add support for VRFs in the mlxsw
driver. First, on top of port netdevs, stacked LAG and VLAN devices and
then on top of bridges.

Some limitations I would like to point out:

1) The old model where 'oif' / 'iif' rules were programmed for each L3
master device isn't supported. Upon insertion of these rules the driver
will flush its tables and forwarding will be done by the kernel instead.
It's inferior in every way to the single 'l3mdev' rule, so this shouldn't
be an issue.

2) Inter-VRF routes pointing to a VRF device aren't offloaded. Packets
hitting these routes will be forwarded by the kernel. Inter-VRF routes
pointing to netdevs enslaved to a different VRF are offloaded.

3) There's a small discrepancy between the kernel's datapath and the
device's. By default, packets forwarded by the kernel first do a lookup
in the LOCAL table and then in the VRF's table (assuming no match). In
the device, lookup is done only in the VRF's table, which is probably
the intended behavior.

Ido Schimmel (10):
  net: fib_rules: Add default rule indication
  ipv4: fib_rules: Add notifier info to FIB rules notifications
  ipv4: fib_rules: Dump FIB rules when registering FIB notifier
  net: Add netif_is_vrf_master helper
  net: vrf: Set slave's private flag before linking
  mlxsw: spectrum_router: Associate RIFs with correct VR
  mlxsw: spectrum_router: Don't destroy RIF if L3 slave
  mlxsw: spectrum_router: Add support for VRFs
  mlxsw: spectrum_router: Add support for VRFs on top of bridges
  mlxsw: spectrum_router: Don't abort on l3mdev rules

 drivers/net/ethernet/mellanox/mlxsw/Kconfig        |  1 +
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     | 71 ++++++++++++++++-
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h     |  8 ++
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 89 +++++++++++++++++++++-
 drivers/net/ethernet/rocker/Kconfig                |  2 +-
 drivers/net/ethernet/rocker/rocker_main.c          | 15 +++-
 drivers/net/vrf.c                                  | 10 ++-
 include/linux/netdevice.h                          |  8 ++
 include/net/fib_rules.h                            |  3 +-
 include/net/ip_fib.h                               |  7 ++
 net/core/fib_rules.c                               |  1 +
 net/ipv4/fib_rules.c                               | 32 ++++++--
 12 files changed, 224 insertions(+), 23 deletions(-)

-- 
2.7.4

[patch net-next 01/10] net: fib_rules: Add default rule indication

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

Currently, when non-default (custom) FIB rules are used, devices capable
of layer 3 offloading flush their tables and let the kernel do the
forwarding instead.

When these devices' drivers are loaded they register to the FIB
notification chain, which lets them know about the existence of any
custom FIB rules. This is done by sending a RULE_ADD notification based
on the value of 'net->ipv4.fib_has_custom_rules'.

This approach is problematic when VRF offload is taken into account, as
upon the creation of the first VRF netdev, a l3mdev rule is programmed
to direct skbs to the VRF's table.

Instead of merely reading the above value and sending a single RULE_ADD
notification, we should iterate over all the FIB rules and send a
detailed notification for each, thereby allowing offloading drivers to
sanitize the rules they don't support and potentially flush their
tables.

While l3mdev rules are uniquely marked, the default rules are not.
Therefore, when they are being notified they might invoke offloading
drivers to unnecessarily flush their tables. Solve this by uniquely
marking the default rules.

Add a 'def' member to the FIB rule struct and set it whenever a default
rule is created.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/fib_rules.h | 3 ++-
 net/core/fib_rules.c    | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/net/fib_rules.h b/include/net/fib_rules.h
index 8dbfdf7..8103f99 100644
--- a/include/net/fib_rules.h
+++ b/include/net/fib_rules.h
@@ -23,7 +23,8 @@ struct fib_rule {
 	u32			table;
 	u8			action;
 	u8			l3mdev;
-	/* 2 bytes hole, try to use */
+	u8			def;
+	/* 1 byte hole, try to use */
 	u32			target;
 	__be64			tun_id;
 	struct fib_rule __rcu	*ctarget;
diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
index b6791d9..fa92c71 100644
--- a/net/core/fib_rules.c
+++ b/net/core/fib_rules.c
@@ -39,6 +39,7 @@ int fib_default_rule_add(struct fib_rules_ops *ops,
 	r->flags = flags;
 	r->fr_net = ops->fro_net;
 	r->uid_range = fib_kuid_range_unset;
+	r->def = 1;
 
 	r->suppress_prefixlen = -1;
 	r->suppress_ifgroup = -1;
-- 
2.7.4

[patch net-next 02/10] ipv4: fib_rules: Add notifier info to FIB rules notifications

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

Whenever a FIB rule is added or removed, a notification is sent in the
FIB notification chain. However, listeners don't have a way to tell
which rule was added or removed.

This is problematic as we would like to give listeners the ability to
decide which action to execute based on the notified rule. Specifically,
offloading drivers should be able to determine if they support the
reflection of the notified FIB rule and flush their LPM tables in case
they don't.

Do that by adding a notifier info to these notifications and embed the
common FIB rule struct in it.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/ip_fib.h |  7 +++++++
 net/ipv4/fib_rules.c | 13 ++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index d9cee96..79fc314 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -213,6 +213,13 @@ struct fib_entry_notifier_info {
 	u32 tb_id;
 };
 
+#ifdef CONFIG_IP_MULTIPLE_TABLES
+struct fib_rule_notifier_info {
+	struct fib_notifier_info info; /* must be first */
+	struct fib_rule *rule;
+};
+#endif
+
 struct fib_nh_notifier_info {
 	struct fib_notifier_info info; /* must be first */
 	struct fib_nh *fib_nh;
diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
index 2892109..d14fc0a 100644
--- a/net/ipv4/fib_rules.c
+++ b/net/ipv4/fib_rules.c
@@ -165,11 +165,14 @@ static struct fib_table *fib_empty_table(struct net *net)
 }
 
 static int call_fib_rule_notifiers(struct net *net,
-				   enum fib_event_type event_type)
+				   enum fib_event_type event_type,
+				   struct fib_rule *rule)
 {
-	struct fib_notifier_info info;
+	struct fib_rule_notifier_info info = {
+		.rule = rule,
+	};
 
-	return call_fib_notifiers(net, event_type, &info);
+	return call_fib_notifiers(net, event_type, &info.info);
 }
 
 void fib_rules_notify(struct net *net, struct notifier_block *nb)
@@ -236,7 +239,7 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
 	rule4->tos = frh->tos;
 
 	net->ipv4.fib_has_custom_rules = true;
-	call_fib_rule_notifiers(net, FIB_EVENT_RULE_ADD);
+	call_fib_rule_notifiers(net, FIB_EVENT_RULE_ADD, rule);
 
 	err = 0;
 errout:
@@ -258,7 +261,7 @@ static int fib4_rule_delete(struct fib_rule *rule)
 		net->ipv4.fib_num_tclassid_users--;
 #endif
 	net->ipv4.fib_has_custom_rules = true;
-	call_fib_rule_notifiers(net, FIB_EVENT_RULE_DEL);
+	call_fib_rule_notifiers(net, FIB_EVENT_RULE_DEL, rule);
 errout:
 	return err;
 }
-- 
2.7.4

[patch net-next 03/10] ipv4: fib_rules: Dump FIB rules when registering FIB notifier

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

In commit c3852ef7f2f8 ("ipv4: fib: Replay events when registering FIB
notifier") we dumped the FIB tables and replayed the events to the
passed notification block.

However, we merely sent a RULE_ADD notification in case custom rules
were in use. As explained in previous patches, this approach won't work
anymore. Instead, we should notify the caller about all the FIB rules
and let it act accordingly.

Upon registration to the FIB notification chain, replay a RULE_ADD
notification for each programmed FIB rule, custom or not. The integrity
of the dump is ensured by the mechanism introduced in the above
mentioned commit.

Prevent regressions by making sure current listeners correctly sanitize
the notified rules.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/Kconfig           |  1 +
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 11 ++++++++++-
 drivers/net/ethernet/rocker/Kconfig                   |  2 +-
 drivers/net/ethernet/rocker/rocker_main.c             | 15 +++++++++++++--
 net/ipv4/fib_rules.c                                  | 19 ++++++++++++++++---
 5 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/Kconfig b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
index ef23eae..a549a89 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
@@ -73,6 +73,7 @@ config MLXSW_SWITCHX2
 config MLXSW_SPECTRUM
 	tristate "Mellanox Technologies Spectrum support"
 	depends on MLXSW_CORE && MLXSW_PCI && NET_SWITCHDEV && VLAN_8021Q
+	depends on IP_MULTIPLE_TABLES
 	depends on PSAMPLE || PSAMPLE=n
 	select PARMAN
 	default m
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 80345a1..9d4766f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -45,6 +45,7 @@
 #include <net/neighbour.h>
 #include <net/arp.h>
 #include <net/ip_fib.h>
+#include <net/fib_rules.h>
 
 #include "spectrum.h"
 #include "core.h"
@@ -2514,6 +2515,7 @@ struct mlxsw_sp_fib_event_work {
 	struct work_struct work;
 	union {
 		struct fib_entry_notifier_info fen_info;
+		struct fib_rule_notifier_info fr_info;
 		struct fib_nh_notifier_info fnh_info;
 	};
 	struct mlxsw_sp *mlxsw_sp;
@@ -2548,7 +2550,9 @@ static void mlxsw_sp_router_fib_event_work(struct work_struct *work)
 		break;
 	case FIB_EVENT_RULE_ADD: /* fall through */
 	case FIB_EVENT_RULE_DEL:
-		mlxsw_sp_router_fib4_abort(mlxsw_sp);
+		if (!fib_work->fr_info.rule->def)
+			mlxsw_sp_router_fib4_abort(mlxsw_sp);
+		fib_rule_put(fib_work->fr_info.rule);
 		break;
 	case FIB_EVENT_NH_ADD: /* fall through */
 	case FIB_EVENT_NH_DEL:
@@ -2591,6 +2595,11 @@ static int mlxsw_sp_router_fib_event(struct notifier_block *nb,
 		 */
 		fib_info_hold(fib_work->fen_info.fi);
 		break;
+	case FIB_EVENT_RULE_ADD: /* fall through */
+	case FIB_EVENT_RULE_DEL:
+		memcpy(&fib_work->fr_info, ptr, sizeof(fib_work->fr_info));
+		fib_rule_get(fib_work->fr_info.rule);
+		break;
 	case FIB_EVENT_NH_ADD: /* fall through */
 	case FIB_EVENT_NH_DEL:
 		memcpy(&fib_work->fnh_info, ptr, sizeof(fib_work->fnh_info));
diff --git a/drivers/net/ethernet/rocker/Kconfig b/drivers/net/ethernet/rocker/Kconfig
index b9952ef..36937dc 100644
--- a/drivers/net/ethernet/rocker/Kconfig
+++ b/drivers/net/ethernet/rocker/Kconfig
@@ -17,7 +17,7 @@ if NET_VENDOR_ROCKER
 
 config ROCKER
 	tristate "Rocker switch driver (EXPERIMENTAL)"
-	depends on PCI && NET_SWITCHDEV && BRIDGE
+	depends on PCI && NET_SWITCHDEV && BRIDGE && IP_MULTIPLE_TABLES
 	---help---
 	  This driver supports Rocker switch device.
 
diff --git a/drivers/net/ethernet/rocker/rocker_main.c b/drivers/net/ethernet/rocker/rocker_main.c
index b712ec2..f9b00e5 100644
--- a/drivers/net/ethernet/rocker/rocker_main.c
+++ b/drivers/net/ethernet/rocker/rocker_main.c
@@ -33,6 +33,7 @@
 #include <net/rtnetlink.h>
 #include <net/netevent.h>
 #include <net/arp.h>
+#include <net/fib_rules.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <generated/utsrelease.h>
 
@@ -2175,7 +2176,10 @@ static const struct switchdev_ops rocker_port_switchdev_ops = {
 
 struct rocker_fib_event_work {
 	struct work_struct work;
-	struct fib_entry_notifier_info fen_info;
+	union {
+		struct fib_entry_notifier_info fen_info;
+		struct fib_rule_notifier_info fr_info;
+	};
 	struct rocker *rocker;
 	unsigned long event;
 };
@@ -2202,7 +2206,9 @@ static void rocker_router_fib_event_work(struct work_struct *work)
 		break;
 	case FIB_EVENT_RULE_ADD: /* fall through */
 	case FIB_EVENT_RULE_DEL:
-		rocker_world_fib4_abort(rocker);
+		if (!fib_work->fr_info.rule->def)
+			rocker_world_fib4_abort(rocker);
+		fib_rule_put(fib_work->fr_info.rule);
 		break;
 	}
 	rtnl_unlock();
@@ -2233,6 +2239,11 @@ static int rocker_router_fib_event(struct notifier_block *nb,
 		 */
 		fib_info_hold(fib_work->fen_info.fi);
 		break;
+	case FIB_EVENT_RULE_ADD: /* fall through */
+	case FIB_EVENT_RULE_DEL:
+		memcpy(&fib_work->fr_info, ptr, sizeof(fib_work->fr_info));
+		fib_rule_get(fib_work->fr_info.rule);
+		break;
 	}
 
 	queue_work(rocker->rocker_owq, &fib_work->work);
diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c
index d14fc0a..724dc3a 100644
--- a/net/ipv4/fib_rules.c
+++ b/net/ipv4/fib_rules.c
@@ -164,6 +164,17 @@ static struct fib_table *fib_empty_table(struct net *net)
 	return NULL;
 }
 
+static int call_fib_rule_notifier(struct notifier_block *nb, struct net *net,
+				  enum fib_event_type event_type,
+				  struct fib_rule *rule)
+{
+	struct fib_rule_notifier_info info = {
+		.rule = rule,
+	};
+
+	return call_fib_notifier(nb, net, event_type, &info.info);
+}
+
 static int call_fib_rule_notifiers(struct net *net,
 				   enum fib_event_type event_type,
 				   struct fib_rule *rule)
@@ -175,12 +186,14 @@ static int call_fib_rule_notifiers(struct net *net,
 	return call_fib_notifiers(net, event_type, &info.info);
 }
 
+/* Called with rcu_read_lock() */
 void fib_rules_notify(struct net *net, struct notifier_block *nb)
 {
-	struct fib_notifier_info info;
+	struct fib_rules_ops *ops = net->ipv4.rules_ops;
+	struct fib_rule *rule;
 
-	if (net->ipv4.fib_has_custom_rules)
-		call_fib_notifier(nb, net, FIB_EVENT_RULE_ADD, &info);
+	list_for_each_entry_rcu(rule, &ops->rules_list, list)
+		call_fib_rule_notifier(nb, net, FIB_EVENT_RULE_ADD, rule);
 }
 
 static const struct nla_policy fib4_rule_policy[FRA_MAX+1] = {
-- 
2.7.4

Re: [patch net-next 03/10] ipv4: fib_rules: Dump FIB rules when registering FIB notifier

[David Ahern]

On 3/13/17 1:38 AM, Jiri Pirko wrote:
> From: Ido Schimmel <idosch@mellanox.com>
> 
> In commit c3852ef7f2f8 ("ipv4: fib: Replay events when registering FIB
> notifier") we dumped the FIB tables and replayed the events to the
> passed notification block.
> 
> However, we merely sent a RULE_ADD notification in case custom rules
> were in use. As explained in previous patches, this approach won't work
> anymore. Instead, we should notify the caller about all the FIB rules
> and let it act accordingly.
> 
> Upon registration to the FIB notification chain, replay a RULE_ADD
> notification for each programmed FIB rule, custom or not. The integrity
> of the dump is ensured by the mechanism introduced in the above
> mentioned commit.
> 
> Prevent regressions by making sure current listeners correctly sanitize
> the notified rules.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlxsw/Kconfig           |  1 +
>  drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 11 ++++++++++-
>  drivers/net/ethernet/rocker/Kconfig                   |  2 +-
>  drivers/net/ethernet/rocker/rocker_main.c             | 15 +++++++++++++--
>  net/ipv4/fib_rules.c                                  | 19 ++++++++++++++++---
>  5 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/Kconfig b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
> index ef23eae..a549a89 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/Kconfig
> +++ b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
> @@ -73,6 +73,7 @@ config MLXSW_SWITCHX2
>  config MLXSW_SPECTRUM
>  	tristate "Mellanox Technologies Spectrum support"
>  	depends on MLXSW_CORE && MLXSW_PCI && NET_SWITCHDEV && VLAN_8021Q
> +	depends on IP_MULTIPLE_TABLES
>  	depends on PSAMPLE || PSAMPLE=n
>  	select PARMAN
>  	default m

Why require multiple tables just b/c the h/w has the ability to support
multiple tables?

Re: [patch net-next 03/10] ipv4: fib_rules: Dump FIB rules when registering FIB notifier

[Ido Schimmel]

On Mon, Mar 13, 2017 at 08:52:45AM -0600, David Ahern wrote:
> On 3/13/17 1:38 AM, Jiri Pirko wrote:
> > From: Ido Schimmel <idosch@mellanox.com>
> > 
> > In commit c3852ef7f2f8 ("ipv4: fib: Replay events when registering FIB
> > notifier") we dumped the FIB tables and replayed the events to the
> > passed notification block.
> > 
> > However, we merely sent a RULE_ADD notification in case custom rules
> > were in use. As explained in previous patches, this approach won't work
> > anymore. Instead, we should notify the caller about all the FIB rules
> > and let it act accordingly.
> > 
> > Upon registration to the FIB notification chain, replay a RULE_ADD
> > notification for each programmed FIB rule, custom or not. The integrity
> > of the dump is ensured by the mechanism introduced in the above
> > mentioned commit.
> > 
> > Prevent regressions by making sure current listeners correctly sanitize
> > the notified rules.
> > 
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> > ---
> >  drivers/net/ethernet/mellanox/mlxsw/Kconfig           |  1 +
> >  drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 11 ++++++++++-
> >  drivers/net/ethernet/rocker/Kconfig                   |  2 +-
> >  drivers/net/ethernet/rocker/rocker_main.c             | 15 +++++++++++++--
> >  net/ipv4/fib_rules.c                                  | 19 ++++++++++++++++---
> >  5 files changed, 41 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/Kconfig b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
> > index ef23eae..a549a89 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/Kconfig
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/Kconfig
> > @@ -73,6 +73,7 @@ config MLXSW_SWITCHX2
> >  config MLXSW_SPECTRUM
> >  	tristate "Mellanox Technologies Spectrum support"
> >  	depends on MLXSW_CORE && MLXSW_PCI && NET_SWITCHDEV && VLAN_8021Q
> > +	depends on IP_MULTIPLE_TABLES
> >  	depends on PSAMPLE || PSAMPLE=n
> >  	select PARMAN
> >  	default m
> 
> Why require multiple tables just b/c the h/w has the ability to support
> multiple tables?

No... I defined 'struct fib_rule_notifier_info' based on
CONFIG_IP_MULTIPLE_TABLES cause I thought it's needed for the embedded
'struct fib_rule', but now that I'm looking again I see that it's not
the case.

Will drop it. Thanks.

[patch net-next 04/10] net: Add netif_is_vrf_master helper

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

Drivers capable of offloading VRF configurations need to know the ports
are enslaved to an actual VRF device and not some other L3 master.

Add a flag to indicate netdev is a VRF master and a corresponding
helper.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/vrf.c         | 2 +-
 include/linux/netdevice.h | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 22379da..5c9a98e 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -1257,7 +1257,7 @@ static int vrf_newlink(struct net *src_net, struct net_device *dev,
 	if (vrf->tb_id == RT_TABLE_UNSPEC)
 		return -EINVAL;
 
-	dev->priv_flags |= IFF_L3MDEV_MASTER;
+	dev->priv_flags |= IFF_L3MDEV_MASTER | IFF_VRF_MASTER;
 
 	err = register_netdevice(dev);
 	if (err)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 97456b25..7908d31 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1356,6 +1356,7 @@ struct net_device_ops {
  * @IFF_PHONY_HEADROOM: the headroom value is controlled by an external
  *	entity (i.e. the master device for bridged veth)
  * @IFF_MACSEC: device is a MACsec device
+ * @IFF_VRF_MASTER: device is a VRF master device
  */
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
@@ -1386,6 +1387,7 @@ enum netdev_priv_flags {
 	IFF_RXFH_CONFIGURED		= 1<<25,
 	IFF_PHONY_HEADROOM		= 1<<26,
 	IFF_MACSEC			= 1<<27,
+	IFF_VRF_MASTER			= 1<<28,
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
@@ -1415,6 +1417,7 @@ enum netdev_priv_flags {
 #define IFF_TEAM			IFF_TEAM
 #define IFF_RXFH_CONFIGURED		IFF_RXFH_CONFIGURED
 #define IFF_MACSEC			IFF_MACSEC
+#define IFF_VRF_MASTER			IFF_VRF_MASTER
 
 /**
  *	struct net_device - The DEVICE structure.
@@ -4155,6 +4158,11 @@ static inline bool netif_supports_nofcs(struct net_device *dev)
 	return dev->priv_flags & IFF_SUPP_NOFCS;
 }
 
+static inline bool netif_is_vrf_master(const struct net_device *dev)
+{
+	return dev->priv_flags & IFF_VRF_MASTER;
+}
+
 static inline bool netif_is_l3_master(const struct net_device *dev)
 {
 	return dev->priv_flags & IFF_L3MDEV_MASTER;
-- 
2.7.4

Re: [patch net-next 04/10] net: Add netif_is_vrf_master helper

[David Ahern]

On 3/13/17 1:38 AM, Jiri Pirko wrote:
> From: Ido Schimmel <idosch@mellanox.com>
> 
> Drivers capable of offloading VRF configurations need to know the ports
> are enslaved to an actual VRF device and not some other L3 master.
> 
> Add a flag to indicate netdev is a VRF master and a corresponding
> helper.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  drivers/net/vrf.c         | 2 +-
>  include/linux/netdevice.h | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 

IFF_VRF_MASTER was renamed IFF_L3MDEV_MASTER 18 months ago.

Re: [patch net-next 04/10] net: Add netif_is_vrf_master helper

[Ido Schimmel]

On Mon, Mar 13, 2017 at 08:39:19AM -0600, David Ahern wrote:
> On 3/13/17 1:38 AM, Jiri Pirko wrote:
> > From: Ido Schimmel <idosch@mellanox.com>
> > 
> > Drivers capable of offloading VRF configurations need to know the ports
> > are enslaved to an actual VRF device and not some other L3 master.
> > 
> > Add a flag to indicate netdev is a VRF master and a corresponding
> > helper.
> > 
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> > ---
> >  drivers/net/vrf.c         | 2 +-
> >  include/linux/netdevice.h | 8 ++++++++
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> 
> IFF_VRF_MASTER was renamed IFF_L3MDEV_MASTER 18 months ago.

But IFF_L3MDEV_MASTER isn't specific to the VRF driver. It can be set by
other drivers including future ones that might be introduced. I need to
allow enslavement to a VRF master, but reject others.

Re: [patch net-next 04/10] net: Add netif_is_vrf_master helper

[David Ahern]

On 3/13/17 9:01 AM, Ido Schimmel wrote:
> On Mon, Mar 13, 2017 at 08:39:19AM -0600, David Ahern wrote:
>> On 3/13/17 1:38 AM, Jiri Pirko wrote:
>>> From: Ido Schimmel <idosch@mellanox.com>
>>>
>>> Drivers capable of offloading VRF configurations need to know the ports
>>> are enslaved to an actual VRF device and not some other L3 master.
>>>
>>> Add a flag to indicate netdev is a VRF master and a corresponding
>>> helper.
>>>
>>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>> ---
>>>  drivers/net/vrf.c         | 2 +-
>>>  include/linux/netdevice.h | 8 ++++++++
>>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>>
>>
>> IFF_VRF_MASTER was renamed IFF_L3MDEV_MASTER 18 months ago.
> 
> But IFF_L3MDEV_MASTER isn't specific to the VRF driver. It can be set by
> other drivers including future ones that might be introduced. I need to
> allow enslavement to a VRF master, but reject others.
> 

Why isn't an L3MDEV associated with a FIB table sufficient? ie., the
L3MDEV_MASTER flag is set and the driver impements l3mdev_fib_table. At
that point, what is specific to a VRF device that the offload relies on?

Re: [patch net-next 04/10] net: Add netif_is_vrf_master helper

[Ido Schimmel]

On Mon, Mar 13, 2017 at 09:15:32AM -0600, David Ahern wrote:
> On 3/13/17 9:01 AM, Ido Schimmel wrote:
> > On Mon, Mar 13, 2017 at 08:39:19AM -0600, David Ahern wrote:
> >> On 3/13/17 1:38 AM, Jiri Pirko wrote:
> >>> From: Ido Schimmel <idosch@mellanox.com>
> >>>
> >>> Drivers capable of offloading VRF configurations need to know the ports
> >>> are enslaved to an actual VRF device and not some other L3 master.
> >>>
> >>> Add a flag to indicate netdev is a VRF master and a corresponding
> >>> helper.
> >>>
> >>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> >>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> >>> ---
> >>>  drivers/net/vrf.c         | 2 +-
> >>>  include/linux/netdevice.h | 8 ++++++++
> >>>  2 files changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>
> >> IFF_VRF_MASTER was renamed IFF_L3MDEV_MASTER 18 months ago.
> > 
> > But IFF_L3MDEV_MASTER isn't specific to the VRF driver. It can be set by
> > other drivers including future ones that might be introduced. I need to
> > allow enslavement to a VRF master, but reject others.
> > 
> 
> Why isn't an L3MDEV associated with a FIB table sufficient? ie., the
> L3MDEV_MASTER flag is set and the driver impements l3mdev_fib_table. At
> that point, what is specific to a VRF device that the offload relies on?

The one thing specific to the VRF driver is that it only does that. If
tomorrow a new driver is introduced and in addition to packet forwarding
according to l3mdev_fib_table it also does something else, then I want
to be sure mlxsw will be able to support it.

Current approach seems cleaner to me, but I don't mind dropping this
patch and introduce it when it's actually needed (if at all). OK?

[patch net-next 05/10] net: vrf: Set slave's private flag before linking

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

Allow listeners of the subsequent CHANGEUPPER notification to retrieve
the VRF's table ID by calling l3mdev_fib_table() with the slave netdev.
Without this change, the netdev won't be considered an L3 slave and the
function would return 0.

This is consistent with other master device such as bridge and bond that
set the slave's private flag before linking. It also makes
do_vrf_{add,del}_slave() symmetric.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/vrf.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 5c9a98e..8151ec2 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -746,14 +746,18 @@ static int do_vrf_add_slave(struct net_device *dev, struct net_device *port_dev)
 {
 	int ret;
 
+	port_dev->priv_flags |= IFF_L3MDEV_SLAVE;
 	ret = netdev_master_upper_dev_link(port_dev, dev, NULL, NULL);
 	if (ret < 0)
-		return ret;
+		goto err;
 
-	port_dev->priv_flags |= IFF_L3MDEV_SLAVE;
 	cycle_netdev(port_dev);
 
 	return 0;
+
+err:
+	port_dev->priv_flags &= ~IFF_L3MDEV_SLAVE;
+	return ret;
 }
 
 static int vrf_add_slave(struct net_device *dev, struct net_device *port_dev)
-- 
2.7.4

Re: [patch net-next 05/10] net: vrf: Set slave's private flag before linking

[David Ahern]

On 3/13/17 1:38 AM, Jiri Pirko wrote:
> From: Ido Schimmel <idosch@mellanox.com>
> 
> Allow listeners of the subsequent CHANGEUPPER notification to retrieve
> the VRF's table ID by calling l3mdev_fib_table() with the slave netdev.
> Without this change, the netdev won't be considered an L3 slave and the
> function would return 0.
> 
> This is consistent with other master device such as bridge and bond that
> set the slave's private flag before linking. It also makes
> do_vrf_{add,del}_slave() symmetric.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  drivers/net/vrf.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 


Acked-by: David Ahern <dsa@cumulusnetworks.com>

[patch net-next 06/10] mlxsw: spectrum_router: Associate RIFs with correct VR

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

When a router interface (RIF) is created due to a netdev being enslaved
to a VRF master, then it should be associated with the appropriate
virtual router (VR) and not the default one.

If netdev is a VRF slave, lookup the VR based on the VRF's table ID.
Otherwise default to the MAIN table.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 9d4766f..0eeb556 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -46,6 +46,7 @@
 #include <net/arp.h>
 #include <net/ip_fib.h>
 #include <net/fib_rules.h>
+#include <net/l3mdev.h>
 
 #include "spectrum.h"
 #include "core.h"
@@ -2760,6 +2761,7 @@ mlxsw_sp_vport_rif_sp_create(struct mlxsw_sp_port *mlxsw_sp_vport,
 			     struct net_device *l3_dev)
 {
 	struct mlxsw_sp *mlxsw_sp = mlxsw_sp_vport->mlxsw_sp;
+	u32 tb_id = l3mdev_fib_table(l3_dev);
 	struct mlxsw_sp_vr *vr;
 	struct mlxsw_sp_fid *f;
 	struct mlxsw_sp_rif *r;
@@ -2770,7 +2772,7 @@ mlxsw_sp_vport_rif_sp_create(struct mlxsw_sp_port *mlxsw_sp_vport,
 	if (rif == MLXSW_SP_INVALID_RIF)
 		return ERR_PTR(-ERANGE);
 
-	vr = mlxsw_sp_vr_get(mlxsw_sp, RT_TABLE_MAIN);
+	vr = mlxsw_sp_vr_get(mlxsw_sp, tb_id ? : RT_TABLE_MAIN);
 	if (IS_ERR(vr))
 		return ERR_CAST(vr);
 
@@ -3008,6 +3010,7 @@ static int mlxsw_sp_rif_bridge_create(struct mlxsw_sp *mlxsw_sp,
 				      struct net_device *l3_dev,
 				      struct mlxsw_sp_fid *f)
 {
+	u32 tb_id = l3mdev_fib_table(l3_dev);
 	struct mlxsw_sp_vr *vr;
 	struct mlxsw_sp_rif *r;
 	u16 rif;
@@ -3017,7 +3020,7 @@ static int mlxsw_sp_rif_bridge_create(struct mlxsw_sp *mlxsw_sp,
 	if (rif == MLXSW_SP_INVALID_RIF)
 		return -ERANGE;
 
-	vr = mlxsw_sp_vr_get(mlxsw_sp, RT_TABLE_MAIN);
+	vr = mlxsw_sp_vr_get(mlxsw_sp, tb_id ? : RT_TABLE_MAIN);
 	if (IS_ERR(vr))
 		return PTR_ERR(vr);
 
-- 
2.7.4

[patch net-next 07/10] mlxsw: spectrum_router: Don't destroy RIF if L3 slave

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

We usually destroy the netdev's router interface (RIF) when the last IP
address is removed from it.

However, we shouldn't do that if it's enslaved to an L3 master device.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 0eeb556..64f0dc7 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -41,6 +41,7 @@
 #include <linux/in6.h>
 #include <linux/notifier.h>
 #include <linux/inetdevice.h>
+#include <linux/netdevice.h>
 #include <net/netevent.h>
 #include <net/neighbour.h>
 #include <net/arp.h>
@@ -2658,7 +2659,7 @@ static bool mlxsw_sp_rif_should_config(struct mlxsw_sp_rif *r,
 			return true;
 		return false;
 	case NETDEV_DOWN:
-		if (r && !in_dev->ifa_list)
+		if (r && !in_dev->ifa_list && !netif_is_l3_slave(r->dev))
 			return true;
 		/* It is possible we already removed the RIF ourselves
 		 * if it was assigned to a netdev that is now a bridge
-- 
2.7.4

[patch net-next 08/10] mlxsw: spectrum_router: Add support for VRFs

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

Allow port netdevs, LAG and VLAN devices stacked on top of these to be
enslaved to a VRF master device.

Upon enslavement, create a router interface (RIF) for the enslaved
netdev and associate it with a virtual router (VR) based on the VRF's
table ID.

If a RIF already exists for the netdev (f.e., due to the existence of an
IP address), then it's deleted and a new one is created with the
appropriate VR binding.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     | 19 ++++++++--
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h     |  4 +++
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 41 ++++++++++++++++++++++
 3 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 475499b..29d8082 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3951,7 +3951,8 @@ static int mlxsw_sp_netdevice_port_upper_event(struct net_device *dev,
 		upper_dev = info->upper_dev;
 		if (!is_vlan_dev(upper_dev) &&
 		    !netif_is_lag_master(upper_dev) &&
-		    !netif_is_bridge_master(upper_dev))
+		    !netif_is_bridge_master(upper_dev) &&
+		    !netif_is_vrf_master(upper_dev))
 			return -EINVAL;
 		if (!info->linking)
 			break;
@@ -3991,6 +3992,11 @@ static int mlxsw_sp_netdevice_port_upper_event(struct net_device *dev,
 			else
 				mlxsw_sp_port_lag_leave(mlxsw_sp_port,
 							upper_dev);
+		} else if (netif_is_vrf_master(upper_dev)) {
+			if (info->linking)
+				err = mlxsw_sp_port_vrf_join(mlxsw_sp_port);
+			else
+				mlxsw_sp_port_vrf_leave(mlxsw_sp_port);
 		} else {
 			err = -EINVAL;
 			WARN_ON(1);
@@ -4353,14 +4359,16 @@ static int mlxsw_sp_netdevice_vport_event(struct net_device *dev,
 	switch (event) {
 	case NETDEV_PRECHANGEUPPER:
 		upper_dev = info->upper_dev;
-		if (!netif_is_bridge_master(upper_dev))
+		if (!netif_is_bridge_master(upper_dev) &&
+		    !netif_is_vrf_master(upper_dev))
 			return -EINVAL;
 		if (!info->linking)
 			break;
 		/* We can't have multiple VLAN interfaces configured on
 		 * the same port and being members in the same bridge.
 		 */
-		if (!mlxsw_sp_port_master_bridge_check(mlxsw_sp_port,
+		if (netif_is_bridge_master(upper_dev) &&
+		    !mlxsw_sp_port_master_bridge_check(mlxsw_sp_port,
 						       upper_dev))
 			return -EINVAL;
 		break;
@@ -4372,6 +4380,11 @@ static int mlxsw_sp_netdevice_vport_event(struct net_device *dev,
 								 upper_dev);
 			else
 				mlxsw_sp_vport_bridge_leave(mlxsw_sp_vport);
+		} else if (netif_is_vrf_master(upper_dev)) {
+			if (info->linking)
+				err = mlxsw_sp_vport_vrf_join(mlxsw_sp_vport);
+			else
+				mlxsw_sp_vport_vrf_leave(mlxsw_sp_vport);
 		} else {
 			err = -EINVAL;
 			WARN_ON(1);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 5502232..60004d9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -578,6 +578,10 @@ int mlxsw_sp_inetaddr_event(struct notifier_block *unused,
 			    unsigned long event, void *ptr);
 void mlxsw_sp_rif_bridge_destroy(struct mlxsw_sp *mlxsw_sp,
 				 struct mlxsw_sp_rif *r);
+int mlxsw_sp_vport_vrf_join(struct mlxsw_sp_port *mlxsw_sp_vport);
+void mlxsw_sp_vport_vrf_leave(struct mlxsw_sp_port *mlxsw_sp_vport);
+int mlxsw_sp_port_vrf_join(struct mlxsw_sp_port *mlxsw_sp_port);
+void mlxsw_sp_port_vrf_leave(struct mlxsw_sp_port *mlxsw_sp_port);
 
 int mlxsw_sp_kvdl_alloc(struct mlxsw_sp *mlxsw_sp, unsigned int entry_count);
 void mlxsw_sp_kvdl_free(struct mlxsw_sp *mlxsw_sp, int entry_index);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 64f0dc7..cbb0362 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -3224,6 +3224,47 @@ int mlxsw_sp_netdevice_router_port_event(struct net_device *dev)
 	return err;
 }
 
+int mlxsw_sp_vport_vrf_join(struct mlxsw_sp_port *mlxsw_sp_vport)
+{
+	struct mlxsw_sp_fid *f = mlxsw_sp_vport_fid_get(mlxsw_sp_vport);
+	struct net_device *dev = mlxsw_sp_vport->dev;
+
+	/* In case vPort already has a RIF, then we need to drop it.
+	 * A new one will be created using the VRF's VR.
+	 */
+	if (f && f->r)
+		mlxsw_sp_vport_rif_sp_leave(mlxsw_sp_vport);
+
+	return mlxsw_sp_vport_rif_sp_join(mlxsw_sp_vport, dev);
+}
+
+void mlxsw_sp_vport_vrf_leave(struct mlxsw_sp_port *mlxsw_sp_vport)
+{
+	mlxsw_sp_vport_rif_sp_leave(mlxsw_sp_vport);
+}
+
+int mlxsw_sp_port_vrf_join(struct mlxsw_sp_port *mlxsw_sp_port)
+{
+	struct mlxsw_sp_port *mlxsw_sp_vport;
+
+	mlxsw_sp_vport = mlxsw_sp_port_vport_find(mlxsw_sp_port, 1);
+	if (WARN_ON(!mlxsw_sp_vport))
+		return -EINVAL;
+
+	return mlxsw_sp_vport_vrf_join(mlxsw_sp_vport);
+}
+
+void mlxsw_sp_port_vrf_leave(struct mlxsw_sp_port *mlxsw_sp_port)
+{
+	struct mlxsw_sp_port *mlxsw_sp_vport;
+
+	mlxsw_sp_vport = mlxsw_sp_port_vport_find(mlxsw_sp_port, 1);
+	if (WARN_ON(!mlxsw_sp_vport))
+		return;
+
+	mlxsw_sp_vport_vrf_leave(mlxsw_sp_vport);
+}
+
 static void mlxsw_sp_router_fib_dump_flush(struct notifier_block *nb)
 {
 	struct mlxsw_sp *mlxsw_sp = container_of(nb, struct mlxsw_sp, fib_nb);
-- 
2.7.4

[patch net-next 09/10] mlxsw: spectrum_router: Add support for VRFs on top of bridges

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

In a similar fashion to the previous patch, allow bridges and VLAN
devices on top of bridges to be enslaved to a VRF master device.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     | 52 +++++++++++++++++++++-
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h     |  4 ++
 .../net/ethernet/mellanox/mlxsw/spectrum_router.c  | 26 +++++++++++
 3 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 29d8082..a3f579e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -4111,7 +4111,7 @@ static int mlxsw_sp_netdevice_bridge_event(struct net_device *br_dev,
 	switch (event) {
 	case NETDEV_PRECHANGEUPPER:
 		upper_dev = info->upper_dev;
-		if (!is_vlan_dev(upper_dev))
+		if (!is_vlan_dev(upper_dev) && !netif_is_vrf_master(upper_dev))
 			return -EINVAL;
 		if (is_vlan_dev(upper_dev) &&
 		    br_dev != mlxsw_sp->master_bridge.dev)
@@ -4126,6 +4126,12 @@ static int mlxsw_sp_netdevice_bridge_event(struct net_device *br_dev,
 			else
 				mlxsw_sp_master_bridge_vlan_unlink(mlxsw_sp,
 								   upper_dev);
+		} else if (netif_is_vrf_master(upper_dev)) {
+			if (info->linking)
+				err = mlxsw_sp_bridge_vrf_join(mlxsw_sp,
+							       br_dev);
+			else
+				mlxsw_sp_bridge_vrf_leave(mlxsw_sp, br_dev);
 		} else {
 			err = -EINVAL;
 			WARN_ON(1);
@@ -4415,6 +4421,47 @@ static int mlxsw_sp_netdevice_lag_vport_event(struct net_device *lag_dev,
 	return 0;
 }
 
+static int mlxsw_sp_netdevice_bridge_vlan_event(struct net_device *vlan_dev,
+						unsigned long event, void *ptr)
+{
+	struct netdev_notifier_changeupper_info *info;
+	struct mlxsw_sp *mlxsw_sp;
+	int err = 0;
+
+	mlxsw_sp = mlxsw_sp_lower_get(vlan_dev);
+	if (!mlxsw_sp)
+		return 0;
+
+	info = ptr;
+
+	switch (event) {
+	case NETDEV_PRECHANGEUPPER:
+		/* VLAN devices are only allowed on top of the
+		 * VLAN-aware bridge.
+		 */
+		if (WARN_ON(vlan_dev_real_dev(vlan_dev) !=
+			    mlxsw_sp->master_bridge.dev))
+			return -EINVAL;
+		if (!netif_is_vrf_master(info->upper_dev))
+			return -EINVAL;
+		break;
+	case NETDEV_CHANGEUPPER:
+		if (netif_is_vrf_master(info->upper_dev)) {
+			if (info->linking)
+				err = mlxsw_sp_bridge_vrf_join(mlxsw_sp,
+							       vlan_dev);
+			else
+				mlxsw_sp_bridge_vrf_leave(mlxsw_sp, vlan_dev);
+		} else {
+			err = -EINVAL;
+			WARN_ON(1);
+		}
+		break;
+	}
+
+	return err;
+}
+
 static int mlxsw_sp_netdevice_vlan_event(struct net_device *vlan_dev,
 					 unsigned long event, void *ptr)
 {
@@ -4427,6 +4474,9 @@ static int mlxsw_sp_netdevice_vlan_event(struct net_device *vlan_dev,
 	else if (netif_is_lag_master(real_dev))
 		return mlxsw_sp_netdevice_lag_vport_event(real_dev, event, ptr,
 							  vid);
+	else if (netif_is_bridge_master(real_dev))
+		return mlxsw_sp_netdevice_bridge_vlan_event(vlan_dev, event,
+							    ptr);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 60004d9..0e223f6 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -582,6 +582,10 @@ int mlxsw_sp_vport_vrf_join(struct mlxsw_sp_port *mlxsw_sp_vport);
 void mlxsw_sp_vport_vrf_leave(struct mlxsw_sp_port *mlxsw_sp_vport);
 int mlxsw_sp_port_vrf_join(struct mlxsw_sp_port *mlxsw_sp_port);
 void mlxsw_sp_port_vrf_leave(struct mlxsw_sp_port *mlxsw_sp_port);
+int mlxsw_sp_bridge_vrf_join(struct mlxsw_sp *mlxsw_sp,
+			     struct net_device *l3_dev);
+void mlxsw_sp_bridge_vrf_leave(struct mlxsw_sp *mlxsw_sp,
+			       struct net_device *l3_dev);
 
 int mlxsw_sp_kvdl_alloc(struct mlxsw_sp *mlxsw_sp, unsigned int entry_count);
 void mlxsw_sp_kvdl_free(struct mlxsw_sp *mlxsw_sp, int entry_index);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index cbb0362..de54382 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -3265,6 +3265,32 @@ void mlxsw_sp_port_vrf_leave(struct mlxsw_sp_port *mlxsw_sp_port)
 	mlxsw_sp_vport_vrf_leave(mlxsw_sp_vport);
 }
 
+int mlxsw_sp_bridge_vrf_join(struct mlxsw_sp *mlxsw_sp,
+			     struct net_device *l3_dev)
+{
+	struct mlxsw_sp_fid *f;
+
+	f = mlxsw_sp_bridge_fid_get(mlxsw_sp, l3_dev);
+	if (WARN_ON(!f))
+		return -EINVAL;
+
+	if (f->r)
+		mlxsw_sp_rif_bridge_destroy(mlxsw_sp, f->r);
+
+	return mlxsw_sp_rif_bridge_create(mlxsw_sp, l3_dev, f);
+}
+
+void mlxsw_sp_bridge_vrf_leave(struct mlxsw_sp *mlxsw_sp,
+			       struct net_device *l3_dev)
+{
+	struct mlxsw_sp_fid *f;
+
+	f = mlxsw_sp_bridge_fid_get(mlxsw_sp, l3_dev);
+	if (WARN_ON(!f))
+		return;
+	mlxsw_sp_rif_bridge_destroy(mlxsw_sp, f->r);
+}
+
 static void mlxsw_sp_router_fib_dump_flush(struct notifier_block *nb)
 {
 	struct mlxsw_sp *mlxsw_sp = container_of(nb, struct mlxsw_sp, fib_nb);
-- 
2.7.4

[patch net-next 10/10] mlxsw: spectrum_router: Don't abort on l3mdev rules

[Jiri Pirko]

From: Ido Schimmel <idosch@mellanox.com>

Now that port netdevs can be enslaved to a VRF master we need to make
sure the device's routing tables won't be flushed upon the insertion of
a l3mdev rule.

Note that we assume the notified l3mdev rule is a simple rule as used by
the VRF master. We don't check for the presence of other selectors such
as 'iif' and 'oif'.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index de54382..fa73ee2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -2552,7 +2552,8 @@ static void mlxsw_sp_router_fib_event_work(struct work_struct *work)
 		break;
 	case FIB_EVENT_RULE_ADD: /* fall through */
 	case FIB_EVENT_RULE_DEL:
-		if (!fib_work->fr_info.rule->def)
+		if (!fib_work->fr_info.rule->def &&
+		    !fib_work->fr_info.rule->l3mdev)
 			mlxsw_sp_router_fib4_abort(mlxsw_sp);
 		fib_rule_put(fib_work->fr_info.rule);
 		break;
-- 
2.7.4

Re: [patch net-next 10/10] mlxsw: spectrum_router: Don't abort on l3mdev rules

[David Ahern]

On 3/13/17 1:38 AM, Jiri Pirko wrote:
> From: Ido Schimmel <idosch@mellanox.com>
> 
> Now that port netdevs can be enslaved to a VRF master we need to make
> sure the device's routing tables won't be flushed upon the insertion of
> a l3mdev rule.
> 
> Note that we assume the notified l3mdev rule is a simple rule as used by
> the VRF master. We don't check for the presence of other selectors such
> as 'iif' and 'oif'.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> index de54382..fa73ee2 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> @@ -2552,7 +2552,8 @@ static void mlxsw_sp_router_fib_event_work(struct work_struct *work)
>  		break;
>  	case FIB_EVENT_RULE_ADD: /* fall through */
>  	case FIB_EVENT_RULE_DEL:
> -		if (!fib_work->fr_info.rule->def)
> +		if (!fib_work->fr_info.rule->def &&
> +		    !fib_work->fr_info.rule->l3mdev)
>  			mlxsw_sp_router_fib4_abort(mlxsw_sp);
>  		fib_rule_put(fib_work->fr_info.rule);
>  		break;
> 

You do not want to abort if the default rules are re-ordered. For
example, the rule for the local table is moved from priority 0 to just
before the main. ie., from this order:

$ ip ru ls
0:	from all lookup local
32766:	from all lookup main
32767:	from all lookup default

to this order
$ ip ru ls
1000:	from all lookup [l3mdev-table]
32765:	from all lookup local
32766:	from all lookup main
32767:	from all lookup default

should not abort offloads.

The default flag added to rules in patch 1 is not really needed; you can
key off basic references to hardcoded table ids: RT_TABLE_MAIN and
RT_TABLE_LOCAL are set in stone. Simple rules (from all lookup X) where
only the priority changes should be enough information to not abort.

In addition, if rules are added that reference oif or iif that is not a
port netdev for this h/w then why abort offloads? e.g, consider PBR
rules related to the management interface.

Re: [patch net-next 10/10] mlxsw: spectrum_router: Don't abort on l3mdev rules

[Ido Schimmel]

On Mon, Mar 13, 2017 at 08:59:11AM -0600, David Ahern wrote:
> On 3/13/17 1:38 AM, Jiri Pirko wrote:
> > From: Ido Schimmel <idosch@mellanox.com>
> > 
> > Now that port netdevs can be enslaved to a VRF master we need to make
> > sure the device's routing tables won't be flushed upon the insertion of
> > a l3mdev rule.
> > 
> > Note that we assume the notified l3mdev rule is a simple rule as used by
> > the VRF master. We don't check for the presence of other selectors such
> > as 'iif' and 'oif'.
> > 
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> > ---
> >  drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > index de54382..fa73ee2 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > @@ -2552,7 +2552,8 @@ static void mlxsw_sp_router_fib_event_work(struct work_struct *work)
> >  		break;
> >  	case FIB_EVENT_RULE_ADD: /* fall through */
> >  	case FIB_EVENT_RULE_DEL:
> > -		if (!fib_work->fr_info.rule->def)
> > +		if (!fib_work->fr_info.rule->def &&
> > +		    !fib_work->fr_info.rule->l3mdev)
> >  			mlxsw_sp_router_fib4_abort(mlxsw_sp);
> >  		fib_rule_put(fib_work->fr_info.rule);
> >  		break;
> > 
> 
> You do not want to abort if the default rules are re-ordered. For
> example, the rule for the local table is moved from priority 0 to just
> before the main. ie., from this order:

Are you aware of configurations employing the VRF device and leaving the
rule for the local table at priority 0?

> 
> $ ip ru ls
> 0:	from all lookup local
> 32766:	from all lookup main
> 32767:	from all lookup default
> 
> to this order
> $ ip ru ls
> 1000:	from all lookup [l3mdev-table]
> 32765:	from all lookup local
> 32766:	from all lookup main
> 32767:	from all lookup default
> 
> should not abort offloads.
> 
> The default flag added to rules in patch 1 is not really needed; you can
> key off basic references to hardcoded table ids: RT_TABLE_MAIN and
> RT_TABLE_LOCAL are set in stone. Simple rules (from all lookup X) where
> only the priority changes should be enough information to not abort.

OK. Will look into that. I didn't like having the local rule first, so
this might the solution.

> In addition, if rules are added that reference oif or iif that is not a
> port netdev for this h/w then why abort offloads? e.g, consider PBR
> rules related to the management interface.

I'm aware, but not sure this belongs in this patchset. I prefer to deal
with the l3mdev rule and re-ordering of the default rules first, then
move to more advanced use cases.

Thanks David.

Re: [patch net-next 10/10] mlxsw: spectrum_router: Don't abort on l3mdev rules

[David Ahern]

On 3/13/17 9:22 AM, Ido Schimmel wrote:
> On Mon, Mar 13, 2017 at 08:59:11AM -0600, David Ahern wrote:
>> On 3/13/17 1:38 AM, Jiri Pirko wrote:
>>> From: Ido Schimmel <idosch@mellanox.com>
>>>
>>> Now that port netdevs can be enslaved to a VRF master we need to make
>>> sure the device's routing tables won't be flushed upon the insertion of
>>> a l3mdev rule.
>>>
>>> Note that we assume the notified l3mdev rule is a simple rule as used by
>>> the VRF master. We don't check for the presence of other selectors such
>>> as 'iif' and 'oif'.
>>>
>>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>> ---
>>>  drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>>> index de54382..fa73ee2 100644
>>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>>> @@ -2552,7 +2552,8 @@ static void mlxsw_sp_router_fib_event_work(struct work_struct *work)
>>>  		break;
>>>  	case FIB_EVENT_RULE_ADD: /* fall through */
>>>  	case FIB_EVENT_RULE_DEL:
>>> -		if (!fib_work->fr_info.rule->def)
>>> +		if (!fib_work->fr_info.rule->def &&
>>> +		    !fib_work->fr_info.rule->l3mdev)
>>>  			mlxsw_sp_router_fib4_abort(mlxsw_sp);
>>>  		fib_rule_put(fib_work->fr_info.rule);
>>>  		break;
>>>
>>
>> You do not want to abort if the default rules are re-ordered. For
>> example, the rule for the local table is moved from priority 0 to just
>> before the main. ie., from this order:
> 
> Are you aware of configurations employing the VRF device and leaving the
> rule for the local table at priority 0?

There is no explicit requirement to re-order, but as I mentioned in one
of the switchdev calls you can get false hits on the local table when
the lookup really should have gone to the VRF table. IMO, best practice
for VRF is to move the the local table rule after the l3mdev / per-VRF
FIB rules.