Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH resend net-next 0/2] Pass the BR_FDB_LOCAL information to switchdev drivers
@ 2021-04-14 16:52 Vladimir Oltean
  2021-04-14 16:52 ` [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify Vladimir Oltean
  2021-04-14 16:52 ` [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications Vladimir Oltean
  0 siblings, 2 replies; 9+ messages in thread
From: Vladimir Oltean @ 2021-04-14 16:52 UTC (permalink / raw)
  To: Jakub Kicinski, David S. Miller, netdev
  Cc: Ioana Ciornei, Vadym Kochan, Taras Chornyi, Jiri Pirko,
	Ido Schimmel, Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Nikolay Aleksandrov, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Vignesh Raghavendra, Linus Walleij, linux-omap,
	Vladimir Oltean

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Bridge FDB entries with the is_local flag are entries which are
terminated locally and not forwarded. Switchdev drivers might want to be
notified of these addresses so they can trap them. If they don't program
these entries to hardware, there is no guarantee that they will do the
right thing with these entries, and they won't be, let's say, flooded.

Ideally none of the switchdev drivers should ignore these entries, but
having access to the is_local bit is the bare minimum change that should
be done in the bridge layer, before this is even possible.

These 2 changes are extracted from the larger "RX filtering in DSA"
series:
https://patchwork.kernel.org/project/netdevbpf/patch/20210224114350.2791260-8-olteanv@gmail.com/
https://patchwork.kernel.org/project/netdevbpf/patch/20210224114350.2791260-9-olteanv@gmail.com/
and submitted separately, because they touch all switchdev drivers,
while the rest is mostly specific to DSA.

This change is not a functional one, in the sense that everybody still
ignores the local FDB entries, but this will be changed by further
patches at least for DSA.

Tobias Waldekranz (1):
  net: bridge: switchdev: refactor br_switchdev_fdb_notify

Vladimir Oltean (1):
  net: bridge: switchdev: include local flag in FDB notifications

 .../ethernet/freescale/dpaa2/dpaa2-switch.c   |  4 +-
 .../marvell/prestera/prestera_switchdev.c     |  2 +-
 .../mellanox/mlxsw/spectrum_switchdev.c       |  5 ++-
 drivers/net/ethernet/rocker/rocker_main.c     |  4 +-
 drivers/net/ethernet/ti/am65-cpsw-switchdev.c |  4 +-
 drivers/net/ethernet/ti/cpsw_switchdev.c      |  4 +-
 include/net/switchdev.h                       |  1 +
 net/bridge/br_switchdev.c                     | 44 +++++--------------
 net/dsa/slave.c                               |  2 +-
 9 files changed, 26 insertions(+), 44 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify
  2021-04-14 16:52 [PATCH resend net-next 0/2] Pass the BR_FDB_LOCAL information to switchdev drivers Vladimir Oltean
@ 2021-04-14 16:52 ` Vladimir Oltean
  2021-04-16 15:19   ` Ido Schimmel
  2021-04-19  8:50   ` Nikolay Aleksandrov
  2021-04-14 16:52 ` [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications Vladimir Oltean
  1 sibling, 2 replies; 9+ messages in thread
From: Vladimir Oltean @ 2021-04-14 16:52 UTC (permalink / raw)
  To: Jakub Kicinski, David S. Miller, netdev
  Cc: Ioana Ciornei, Vadym Kochan, Taras Chornyi, Jiri Pirko,
	Ido Schimmel, Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Nikolay Aleksandrov, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Vignesh Raghavendra, Linus Walleij, linux-omap,
	Tobias Waldekranz, Vladimir Oltean

From: Tobias Waldekranz <tobias@waldekranz.com>

Instead of having to add more and more arguments to
br_switchdev_fdb_call_notifiers, get rid of it and build the info
struct directly in br_switchdev_fdb_notify.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/bridge/br_switchdev.c | 41 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 1e24d9a2c9a7..c390f84adea2 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -107,25 +107,16 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	return 0;
 }
 
-static void
-br_switchdev_fdb_call_notifiers(bool adding, const unsigned char *mac,
-				u16 vid, struct net_device *dev,
-				bool added_by_user, bool offloaded)
-{
-	struct switchdev_notifier_fdb_info info;
-	unsigned long notifier_type;
-
-	info.addr = mac;
-	info.vid = vid;
-	info.added_by_user = added_by_user;
-	info.offloaded = offloaded;
-	notifier_type = adding ? SWITCHDEV_FDB_ADD_TO_DEVICE : SWITCHDEV_FDB_DEL_TO_DEVICE;
-	call_switchdev_notifiers(notifier_type, dev, &info.info, NULL);
-}
-
 void
 br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
 {
+	struct switchdev_notifier_fdb_info info = {
+		.addr = fdb->key.addr.addr,
+		.vid = fdb->key.vlan_id,
+		.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags),
+		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
+	};
+
 	if (!fdb->dst)
 		return;
 	if (test_bit(BR_FDB_LOCAL, &fdb->flags))
@@ -133,22 +124,12 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
 
 	switch (type) {
 	case RTM_DELNEIGH:
-		br_switchdev_fdb_call_notifiers(false, fdb->key.addr.addr,
-						fdb->key.vlan_id,
-						fdb->dst->dev,
-						test_bit(BR_FDB_ADDED_BY_USER,
-							 &fdb->flags),
-						test_bit(BR_FDB_OFFLOADED,
-							 &fdb->flags));
+		call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_DEVICE,
+					 fdb->dst->dev, &info.info, NULL);
 		break;
 	case RTM_NEWNEIGH:
-		br_switchdev_fdb_call_notifiers(true, fdb->key.addr.addr,
-						fdb->key.vlan_id,
-						fdb->dst->dev,
-						test_bit(BR_FDB_ADDED_BY_USER,
-							 &fdb->flags),
-						test_bit(BR_FDB_OFFLOADED,
-							 &fdb->flags));
+		call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_DEVICE,
+					 fdb->dst->dev, &info.info, NULL);
 		break;
 	}
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications
  2021-04-14 16:52 [PATCH resend net-next 0/2] Pass the BR_FDB_LOCAL information to switchdev drivers Vladimir Oltean
  2021-04-14 16:52 ` [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify Vladimir Oltean
@ 2021-04-14 16:52 ` Vladimir Oltean
  2021-04-16 10:11   ` Grygorii Strashko
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Vladimir Oltean @ 2021-04-14 16:52 UTC (permalink / raw)
  To: Jakub Kicinski, David S. Miller, netdev
  Cc: Ioana Ciornei, Vadym Kochan, Taras Chornyi, Jiri Pirko,
	Ido Schimmel, Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Nikolay Aleksandrov, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Vignesh Raghavendra, Linus Walleij, linux-omap,
	Vladimir Oltean, Tobias Waldekranz

From: Vladimir Oltean <vladimir.oltean@nxp.com>

As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify
switchdev for local FDB addresses") as well as in this discussion:
https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/

the switchdev notifiers for FDB entries managed to have a zero-day bug,
which was that drivers would not know what to do with local FDB entries,
because they were not told that they are local. The bug fix was to
simply not notify them of those addresses.

Let us now add the 'is_local' bit to bridge FDB entries, and make all
drivers ignore these entries by their own choice.

Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c        | 4 ++--
 drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 +-
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c   | 5 +++--
 drivers/net/ethernet/rocker/rocker_main.c                  | 4 ++--
 drivers/net/ethernet/ti/am65-cpsw-switchdev.c              | 4 ++--
 drivers/net/ethernet/ti/cpsw_switchdev.c                   | 4 ++--
 include/net/switchdev.h                                    | 1 +
 net/bridge/br_switchdev.c                                  | 3 +--
 net/dsa/slave.c                                            | 2 +-
 9 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
index 80efc8116963..d7c0e11b16f7 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
@@ -1832,7 +1832,7 @@ static void dpaa2_switch_event_work(struct work_struct *work)
 
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
-		if (!fdb_info->added_by_user)
+		if (!fdb_info->added_by_user || fdb_info->is_local)
 			break;
 		if (is_unicast_ether_addr(fdb_info->addr))
 			err = dpaa2_switch_port_fdb_add_uc(netdev_priv(dev),
@@ -1847,7 +1847,7 @@ static void dpaa2_switch_event_work(struct work_struct *work)
 					 &fdb_info->info, NULL);
 		break;
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
-		if (!fdb_info->added_by_user)
+		if (!fdb_info->added_by_user || fdb_info->is_local)
 			break;
 		if (is_unicast_ether_addr(fdb_info->addr))
 			dpaa2_switch_port_fdb_del_uc(netdev_priv(dev), fdb_info->addr);
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
index 49e052273f30..cb564890a3dc 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
@@ -798,7 +798,7 @@ static void prestera_fdb_event_work(struct work_struct *work)
 	switch (swdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 		fdb_info = &swdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
+		if (!fdb_info->added_by_user || fdb_info->is_local)
 			break;
 
 		err = prestera_port_fdb_set(port, fdb_info, true);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index c1f05c17557d..eeccd586e781 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -2916,7 +2916,8 @@ mlxsw_sp_switchdev_bridge_nve_fdb_event(struct mlxsw_sp_switchdev_event_work *
 		return;
 
 	if (switchdev_work->event == SWITCHDEV_FDB_ADD_TO_DEVICE &&
-	    !switchdev_work->fdb_info.added_by_user)
+	    (!switchdev_work->fdb_info.added_by_user ||
+	     switchdev_work->fdb_info.is_local))
 		return;
 
 	if (!netif_running(dev))
@@ -2971,7 +2972,7 @@ static void mlxsw_sp_switchdev_bridge_fdb_event_work(struct work_struct *work)
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 		fdb_info = &switchdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
+		if (!fdb_info->added_by_user || fdb_info->is_local)
 			break;
 		err = mlxsw_sp_port_fdb_set(mlxsw_sp_port, fdb_info, true);
 		if (err)
diff --git a/drivers/net/ethernet/rocker/rocker_main.c b/drivers/net/ethernet/rocker/rocker_main.c
index 3473d296b2e2..a46633606cae 100644
--- a/drivers/net/ethernet/rocker/rocker_main.c
+++ b/drivers/net/ethernet/rocker/rocker_main.c
@@ -2736,7 +2736,7 @@ static void rocker_switchdev_event_work(struct work_struct *work)
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 		fdb_info = &switchdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
+		if (!fdb_info->added_by_user || fdb_info->is_local)
 			break;
 		err = rocker_world_port_fdb_add(rocker_port, fdb_info);
 		if (err) {
@@ -2747,7 +2747,7 @@ static void rocker_switchdev_event_work(struct work_struct *work)
 		break;
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
 		fdb_info = &switchdev_work->fdb_info;
-		if (!fdb_info->added_by_user)
+		if (!fdb_info->added_by_user || fdb_info->is_local)
 			break;
 		err = rocker_world_port_fdb_del(rocker_port, fdb_info);
 		if (err)
diff --git a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
index d93ffd8a08b0..23cfb91e9c4d 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-switchdev.c
@@ -385,7 +385,7 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work)
 			   fdb->addr, fdb->vid, fdb->added_by_user,
 			   fdb->offloaded, port_id);
 
-		if (!fdb->added_by_user)
+		if (!fdb->added_by_user || fdb->is_local)
 			break;
 		if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
 			port_id = HOST_PORT_NUM;
@@ -401,7 +401,7 @@ static void am65_cpsw_switchdev_event_work(struct work_struct *work)
 			   fdb->addr, fdb->vid, fdb->added_by_user,
 			   fdb->offloaded, port_id);
 
-		if (!fdb->added_by_user)
+		if (!fdb->added_by_user || fdb->is_local)
 			break;
 		if (memcmp(port->slave.mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
 			port_id = HOST_PORT_NUM;
diff --git a/drivers/net/ethernet/ti/cpsw_switchdev.c b/drivers/net/ethernet/ti/cpsw_switchdev.c
index a72bb570756f..05a64fb7a04f 100644
--- a/drivers/net/ethernet/ti/cpsw_switchdev.c
+++ b/drivers/net/ethernet/ti/cpsw_switchdev.c
@@ -395,7 +395,7 @@ static void cpsw_switchdev_event_work(struct work_struct *work)
 			fdb->addr, fdb->vid, fdb->added_by_user,
 			fdb->offloaded, port);
 
-		if (!fdb->added_by_user)
+		if (!fdb->added_by_user || fdb->is_local)
 			break;
 		if (memcmp(priv->mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
 			port = HOST_PORT_NUM;
@@ -411,7 +411,7 @@ static void cpsw_switchdev_event_work(struct work_struct *work)
 			fdb->addr, fdb->vid, fdb->added_by_user,
 			fdb->offloaded, port);
 
-		if (!fdb->added_by_user)
+		if (!fdb->added_by_user || fdb->is_local)
 			break;
 		if (memcmp(priv->mac_addr, (u8 *)fdb->addr, ETH_ALEN) == 0)
 			port = HOST_PORT_NUM;
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 8c3218177136..f1a5a9a3634d 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -209,6 +209,7 @@ struct switchdev_notifier_fdb_info {
 	const unsigned char *addr;
 	u16 vid;
 	u8 added_by_user:1,
+	   is_local:1,
 	   offloaded:1;
 };
 
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index c390f84adea2..a5e601e41cb9 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -114,13 +114,12 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
 		.addr = fdb->key.addr.addr,
 		.vid = fdb->key.vlan_id,
 		.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags),
+		.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags),
 		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
 	};
 
 	if (!fdb->dst)
 		return;
-	if (test_bit(BR_FDB_LOCAL, &fdb->flags))
-		return;
 
 	switch (type) {
 	case RTM_DELNEIGH:
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 995e0e16f295..6e348d2222a9 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2329,7 +2329,7 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,
 		fdb_info = ptr;
 
 		if (dsa_slave_dev_check(dev)) {
-			if (!fdb_info->added_by_user)
+			if (!fdb_info->added_by_user || fdb_info->is_local)
 				return NOTIFY_OK;
 
 			dp = dsa_slave_to_port(dev);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications
  2021-04-14 16:52 ` [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications Vladimir Oltean
@ 2021-04-16 10:11   ` Grygorii Strashko
  2021-04-16 15:22   ` Ido Schimmel
  2021-04-19  8:51   ` Nikolay Aleksandrov
  2 siblings, 0 replies; 9+ messages in thread
From: Grygorii Strashko @ 2021-04-16 10:11 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, David S. Miller, netdev
  Cc: Ioana Ciornei, Vadym Kochan, Taras Chornyi, Jiri Pirko,
	Ido Schimmel, Ivan Vecera, Roopa Prabhu, Nikolay Aleksandrov,
	Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vignesh Raghavendra, Linus Walleij, linux-omap, Vladimir Oltean,
	Tobias Waldekranz



On 14/04/2021 19:52, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify
> switchdev for local FDB addresses") as well as in this discussion:
> https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/
> 
> the switchdev notifiers for FDB entries managed to have a zero-day bug,
> which was that drivers would not know what to do with local FDB entries,
> because they were not told that they are local. The bug fix was to
> simply not notify them of those addresses.
> 
> Let us now add the 'is_local' bit to bridge FDB entries, and make all
> drivers ignore these entries by their own choice.
> 
> Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>   drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c        | 4 ++--
>   drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 +-
>   drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c   | 5 +++--
>   drivers/net/ethernet/rocker/rocker_main.c                  | 4 ++--
>   drivers/net/ethernet/ti/am65-cpsw-switchdev.c              | 4 ++--
>   drivers/net/ethernet/ti/cpsw_switchdev.c                   | 4 ++--
>   include/net/switchdev.h                                    | 1 +
>   net/bridge/br_switchdev.c                                  | 3 +--
>   net/dsa/slave.c                                            | 2 +-
>   9 files changed, 15 insertions(+), 14 deletions(-)

For cpsw:
Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

Thank you.

-- 
Best regards,
grygorii

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify
  2021-04-14 16:52 ` [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify Vladimir Oltean
@ 2021-04-16 15:19   ` Ido Schimmel
  2021-04-19  8:50   ` Nikolay Aleksandrov
  1 sibling, 0 replies; 9+ messages in thread
From: Ido Schimmel @ 2021-04-16 15:19 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Jakub Kicinski, David S. Miller, netdev, Ioana Ciornei,
	Vadym Kochan, Taras Chornyi, Jiri Pirko, Ido Schimmel,
	Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Nikolay Aleksandrov, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Vignesh Raghavendra, Linus Walleij, linux-omap,
	Tobias Waldekranz, Vladimir Oltean

On Wed, Apr 14, 2021 at 07:52:55PM +0300, Vladimir Oltean wrote:
> From: Tobias Waldekranz <tobias@waldekranz.com>
> 
> Instead of having to add more and more arguments to
> br_switchdev_fdb_call_notifiers, get rid of it and build the info
> struct directly in br_switchdev_fdb_notify.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications
  2021-04-14 16:52 ` [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications Vladimir Oltean
  2021-04-16 10:11   ` Grygorii Strashko
@ 2021-04-16 15:22   ` Ido Schimmel
  2021-04-16 17:33     ` Vladimir Oltean
  2021-04-19  8:51   ` Nikolay Aleksandrov
  2 siblings, 1 reply; 9+ messages in thread
From: Ido Schimmel @ 2021-04-16 15:22 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Jakub Kicinski, David S. Miller, netdev, Ioana Ciornei,
	Vadym Kochan, Taras Chornyi, Jiri Pirko, Ido Schimmel,
	Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Nikolay Aleksandrov, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Vignesh Raghavendra, Linus Walleij, linux-omap,
	Vladimir Oltean, Tobias Waldekranz

On Wed, Apr 14, 2021 at 07:52:56PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify
> switchdev for local FDB addresses") as well as in this discussion:
> https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/
> 
> the switchdev notifiers for FDB entries managed to have a zero-day bug,
> which was that drivers would not know what to do with local FDB entries,
> because they were not told that they are local. The bug fix was to
> simply not notify them of those addresses.
> 
> Let us now add the 'is_local' bit to bridge FDB entries, and make all
> drivers ignore these entries by their own choice.
> 
> Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

One comment below

> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index c390f84adea2..a5e601e41cb9 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -114,13 +114,12 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
>  		.addr = fdb->key.addr.addr,
>  		.vid = fdb->key.vlan_id,
>  		.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags),
> +		.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags),
>  		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
>  	};
>  
>  	if (!fdb->dst)
>  		return;

Do you plan to eventually remove this check so that entries pointing to
the bridge device itself will be notified? For example:

# bridge fdb add 00:01:02:03:04:05 dev br0 self local

> -	if (test_bit(BR_FDB_LOCAL, &fdb->flags))
> -		return;
>  
>  	switch (type) {
>  	case RTM_DELNEIGH:

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications
  2021-04-16 15:22   ` Ido Schimmel
@ 2021-04-16 17:33     ` Vladimir Oltean
  0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Oltean @ 2021-04-16 17:33 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Jakub Kicinski, David S. Miller, netdev, Ioana Ciornei,
	Vadym Kochan, Taras Chornyi, Jiri Pirko, Ido Schimmel,
	Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Nikolay Aleksandrov, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Vignesh Raghavendra, Linus Walleij, linux-omap,
	Vladimir Oltean, Tobias Waldekranz

On Fri, Apr 16, 2021 at 06:22:08PM +0300, Ido Schimmel wrote:
> On Wed, Apr 14, 2021 at 07:52:56PM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > 
> > As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify
> > switchdev for local FDB addresses") as well as in this discussion:
> > https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/
> > 
> > the switchdev notifiers for FDB entries managed to have a zero-day bug,
> > which was that drivers would not know what to do with local FDB entries,
> > because they were not told that they are local. The bug fix was to
> > simply not notify them of those addresses.
> > 
> > Let us now add the 'is_local' bit to bridge FDB entries, and make all
> > drivers ignore these entries by their own choice.
> > 
> > Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
> > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Thanks!

> One comment below
> 
> > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> > index c390f84adea2..a5e601e41cb9 100644
> > --- a/net/bridge/br_switchdev.c
> > +++ b/net/bridge/br_switchdev.c
> > @@ -114,13 +114,12 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
> >  		.addr = fdb->key.addr.addr,
> >  		.vid = fdb->key.vlan_id,
> >  		.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags),
> > +		.is_local = test_bit(BR_FDB_LOCAL, &fdb->flags),
> >  		.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags),
> >  	};
> >  
> >  	if (!fdb->dst)
> >  		return;
> 
> Do you plan to eventually remove this check so that entries pointing to
> the bridge device itself will be notified? For example:
> 
> # bridge fdb add 00:01:02:03:04:05 dev br0 self local
> 
> > -	if (test_bit(BR_FDB_LOCAL, &fdb->flags))
> > -		return;
> >  
> >  	switch (type) {
> >  	case RTM_DELNEIGH:

Yes I do, it's this patch over here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210224114350.2791260-10-olteanv@gmail.com/

One at a time though.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify
  2021-04-14 16:52 ` [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify Vladimir Oltean
  2021-04-16 15:19   ` Ido Schimmel
@ 2021-04-19  8:50   ` Nikolay Aleksandrov
  1 sibling, 0 replies; 9+ messages in thread
From: Nikolay Aleksandrov @ 2021-04-19  8:50 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, David S. Miller, netdev
  Cc: Ioana Ciornei, Vadym Kochan, Taras Chornyi, Jiri Pirko,
	Ido Schimmel, Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vignesh Raghavendra, Linus Walleij, linux-omap,
	Tobias Waldekranz, Vladimir Oltean

On 14/04/2021 19:52, Vladimir Oltean wrote:
> From: Tobias Waldekranz <tobias@waldekranz.com>
> 
> Instead of having to add more and more arguments to
> br_switchdev_fdb_call_notifiers, get rid of it and build the info
> struct directly in br_switchdev_fdb_notify.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_switchdev.c | 41 +++++++++++----------------------------
>  1 file changed, 11 insertions(+), 30 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications
  2021-04-14 16:52 ` [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications Vladimir Oltean
  2021-04-16 10:11   ` Grygorii Strashko
  2021-04-16 15:22   ` Ido Schimmel
@ 2021-04-19  8:51   ` Nikolay Aleksandrov
  2 siblings, 0 replies; 9+ messages in thread
From: Nikolay Aleksandrov @ 2021-04-19  8:51 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, David S. Miller, netdev
  Cc: Ioana Ciornei, Vadym Kochan, Taras Chornyi, Jiri Pirko,
	Ido Schimmel, Grygorii Strashko, Ivan Vecera, Roopa Prabhu,
	Andrew Lunn, Vivien Didelot, Florian Fainelli,
	Vignesh Raghavendra, Linus Walleij, linux-omap, Vladimir Oltean,
	Tobias Waldekranz

On 14/04/2021 19:52, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> As explained in bugfix commit 6ab4c3117aec ("net: bridge: don't notify
> switchdev for local FDB addresses") as well as in this discussion:
> https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/
> 
> the switchdev notifiers for FDB entries managed to have a zero-day bug,
> which was that drivers would not know what to do with local FDB entries,
> because they were not told that they are local. The bug fix was to
> simply not notify them of those addresses.
> 
> Let us now add the 'is_local' bit to bridge FDB entries, and make all
> drivers ignore these entries by their own choice.
> 
> Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c        | 4 ++--
>  drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 +-
>  drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c   | 5 +++--
>  drivers/net/ethernet/rocker/rocker_main.c                  | 4 ++--
>  drivers/net/ethernet/ti/am65-cpsw-switchdev.c              | 4 ++--
>  drivers/net/ethernet/ti/cpsw_switchdev.c                   | 4 ++--
>  include/net/switchdev.h                                    | 1 +
>  net/bridge/br_switchdev.c                                  | 3 +--
>  net/dsa/slave.c                                            | 2 +-
>  9 files changed, 15 insertions(+), 14 deletions(-)
> 

For the bridge change:
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 16:52 [PATCH resend net-next 0/2] Pass the BR_FDB_LOCAL information to switchdev drivers Vladimir Oltean
2021-04-14 16:52 ` [PATCH resend net-next 1/2] net: bridge: switchdev: refactor br_switchdev_fdb_notify Vladimir Oltean
2021-04-16 15:19   ` Ido Schimmel
2021-04-19  8:50   ` Nikolay Aleksandrov
2021-04-14 16:52 ` [PATCH resend net-next 2/2] net: bridge: switchdev: include local flag in FDB notifications Vladimir Oltean
2021-04-16 10:11   ` Grygorii Strashko
2021-04-16 15:22   ` Ido Schimmel
2021-04-16 17:33     ` Vladimir Oltean
2021-04-19  8:51   ` Nikolay Aleksandrov

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git