Re: [PATCH v3 net-next 07/11] net: prep switchdev drivers for concurrent SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS

2021-02-10 Thread David Miller
From: Vladimir Oltean 
Date: Wed, 10 Feb 2021 11:14:41 +0200

> From: Vladimir Oltean 
> 
> Because the bridge will start offloading SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
> while not serialized by any lock such as the br->lock spinlock, existing
> drivers that treat that attribute and cache the brport flags might no
> longer work correctly.
> 
> The issue is that the brport flags are a single unsigned long bitmask,
> and the bridge only guarantees the validity of the changed bits, not the
> full state. So when offloading two concurrent switchdev attributes, such
> as one for BR_LEARNING and another for BR_FLOOD, it might happen that
> the flags having BR_FLOOD are written into the cached value, and this in
> turn disables the BR_LEARNING bit which was set previously.
> 
> We can fix this across the board by keeping individual boolean variables
> for each brport flag. Note that mlxsw and prestera were setting the
> BR_LEARNING_SYNC flag too, but that appears to be just dead code, so I
> removed it.
> 
> Signed-off-by: Vladimir Oltean 


This needs updating because, as discussed, there is no race.


Re: [PATCH v3 net-next 07/11] net: prep switchdev drivers for concurrent SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS

2021-02-10 Thread Vladimir Oltean
On Wed, Feb 10, 2021 at 12:12:57PM +0200, Ido Schimmel wrote:
> On Wed, Feb 10, 2021 at 11:14:41AM +0200, Vladimir Oltean wrote:
> > From: Vladimir Oltean 
> > 
> > Because the bridge will start offloading SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
> > while not serialized by any lock such as the br->lock spinlock, existing
> > drivers that treat that attribute and cache the brport flags might no
> > longer work correctly.
> 
> Can you explain the race? This notification is sent from sysfs/netlink
> call path, both of which take rtnl.

Replying here to both you and Nikolay: there isn't any race, sorry, I
missed the fact that brport_store takes the rtnl_mutex and by extension
I thought that RTM_SETLINK runs unlocked too, without really checking.
Well, at least that's good news, the implementation can be a lot more
straightforward then...


Re: [PATCH v3 net-next 07/11] net: prep switchdev drivers for concurrent SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS

2021-02-10 Thread Ido Schimmel
On Wed, Feb 10, 2021 at 11:14:41AM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean 
> 
> Because the bridge will start offloading SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
> while not serialized by any lock such as the br->lock spinlock, existing
> drivers that treat that attribute and cache the brport flags might no
> longer work correctly.

Can you explain the race? This notification is sent from sysfs/netlink
call path, both of which take rtnl.


[PATCH v3 net-next 07/11] net: prep switchdev drivers for concurrent SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS

2021-02-10 Thread Vladimir Oltean
From: Vladimir Oltean 

Because the bridge will start offloading SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
while not serialized by any lock such as the br->lock spinlock, existing
drivers that treat that attribute and cache the brport flags might no
longer work correctly.

The issue is that the brport flags are a single unsigned long bitmask,
and the bridge only guarantees the validity of the changed bits, not the
full state. So when offloading two concurrent switchdev attributes, such
as one for BR_LEARNING and another for BR_FLOOD, it might happen that
the flags having BR_FLOOD are written into the cached value, and this in
turn disables the BR_LEARNING bit which was set previously.

We can fix this across the board by keeping individual boolean variables
for each brport flag. Note that mlxsw and prestera were setting the
BR_LEARNING_SYNC flag too, but that appears to be just dead code, so I
removed it.

Signed-off-by: Vladimir Oltean 
---
Changes in v3:
Patch is new.

 .../marvell/prestera/prestera_switchdev.c | 32 ++--
 .../mellanox/mlxsw/spectrum_switchdev.c   | 38 ---
 drivers/net/ethernet/rocker/rocker.h  |  2 +-
 drivers/net/ethernet/rocker/rocker_main.c |  2 +-
 drivers/net/ethernet/rocker/rocker_ofdpa.c| 26 +++--
 net/bridge/br_switchdev.c |  3 +-
 6 files changed, 62 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c 
b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
index a797a7ff0cfe..76030c4c1546 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
@@ -49,7 +49,9 @@ struct prestera_bridge_port {
struct prestera_bridge *bridge;
struct list_head vlan_list;
refcount_t ref_count;
-   unsigned long flags;
+   bool learning;
+   bool ucast_flood;
+   bool mcast_flood;
u8 stp_state;
 };
 
@@ -339,8 +341,9 @@ prestera_bridge_port_create(struct prestera_bridge *bridge,
if (!br_port)
return NULL;
 
-   br_port->flags = BR_LEARNING | BR_FLOOD | BR_LEARNING_SYNC |
-   BR_MCAST_FLOOD;
+   br_port->learning = true;
+   br_port->ucast_flood = true;
+   br_port->mcast_flood = true;
br_port->stp_state = BR_STATE_DISABLED;
refcount_set(_port->ref_count, 1);
br_port->bridge = bridge;
@@ -404,11 +407,11 @@ prestera_bridge_1d_port_join(struct prestera_bridge_port 
*br_port)
if (err)
return err;
 
-   err = prestera_hw_port_flood_set(port, br_port->flags & BR_FLOOD);
+   err = prestera_hw_port_flood_set(port, br_port->ucast_flood);
if (err)
goto err_port_flood_set;
 
-   err = prestera_hw_port_learning_set(port, br_port->flags & BR_LEARNING);
+   err = prestera_hw_port_learning_set(port, br_port->learning);
if (err)
goto err_port_learning_set;
 
@@ -594,19 +597,24 @@ static int prestera_port_attr_br_flags_set(struct 
prestera_port *port,
return 0;
 
if (flags.mask & BR_FLOOD) {
-   err = prestera_hw_port_flood_set(port, flags.val & BR_FLOOD);
+   bool ucast_flood = !!(flags.val & BR_FLOOD);
+
+   err = prestera_hw_port_flood_set(port, ucast_flood);
if (err)
return err;
+
+   br_port->ucast_flood = ucast_flood;
}
 
if (flags.mask & BR_LEARNING) {
-   err = prestera_hw_port_learning_set(port,
-   flags.val & BR_LEARNING);
+   bool learning = !!(flags.val & BR_LEARNING);
+
+   err = prestera_hw_port_learning_set(port, learning);
if (err)
return err;
-   }
 
-   memcpy(_port->flags, , sizeof(flags.val));
+   br_port->learning = learning;
+   }
 
return 0;
 }
@@ -899,11 +907,11 @@ prestera_port_vlan_bridge_join(struct prestera_port_vlan 
*port_vlan,
if (port_vlan->br_port)
return 0;
 
-   err = prestera_hw_port_flood_set(port, br_port->flags & BR_FLOOD);
+   err = prestera_hw_port_flood_set(port, br_port->ucast_flood);
if (err)
return err;
 
-   err = prestera_hw_port_learning_set(port, br_port->flags & BR_LEARNING);
+   err = prestera_hw_port_learning_set(port, br_port->learning);
if (err)
goto err_port_learning_set;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c 
b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 7af79e3a3c7a..0c00754e0cb5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -62,9 +62,11 @@ struct mlxsw_sp_bridge_port {
struct list_head vlans_list;
unsigned