Re: [PATCH v4 net-next 5/6] net: dsa: mv88e6xxx: mac-auth/MAB implementation

On 2022-07-17 02:47, Vladimir Oltean wrote:

...

@@ -1721,11 +1735,11 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid, return err; }

int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port, bool locked) { @@ -1257,13 +1270,18 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port, if (err) return err;

• err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,

&reg);

• if (err)

• return err;
  

• if (!locked) {

• err = mv88e6xxx_atu_locked_entry_flush(chip->ds, port);
  

Did you re-run the selftest with v4? Because this deadlocks due to the double reg_lock illustrated below:

I did some selftest, but I didn't experience the problem of the double chip lock as it only happens on unlock which is only called where the MAB test ends.

• vrf_name=vlan1
• ip vrf exec vlan1 ping 192.0.2.2 -c 10 -i 0.1 -w 5
• check_err 1 'Ping did not work after locking port and adding FDB

entry'

• local err=1
• local 'msg=Ping did not work after locking port and adding FDB entry'
• [[ 0 -eq 0 ]]
• [[ 1 -ne 0 ]]
• RET=1
• retmsg='Ping did not work after locking port and adding FDB entry'
• bridge link set dev lan2 locked off

[ 733.273994] [ 733.275515] ============================================ [ 733.280823] WARNING: possible recursive locking detected [ 733.286133] 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293 Not tainted [ 733.292311] -------------------------------------------- [ 733.297613] kworker/0:0/601 is trying to acquire lock: [ 733.302751] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at: mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4 [ 733.312524] [ 733.312524] but task is already holding lock: [ 733.318351] ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at: mv88e6xxx_port_bridge_flags+0x48/0x234 [ 733.327674] [ 733.327674] other info that might help us debug this: [ 733.334198] Possible unsafe locking scenario: [ 733.334198] [ 733.340109] CPU0 [ 733.342549] ---- [ 733.344990] lock(&chip->reg_lock); [ 733.348567] lock(&chip->reg_lock); [ 733.352149] [ 733.352149] *** DEADLOCK *** [ 733.352149] [ 733.358063] May be due to missing lock nesting notation [ 733.358063] [ 733.364846] 6 locks held by kworker/0:0/601: [ 733.369115] #0: ffff00000000b748 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4 [ 733.378541] #1: ffff80000c43bdc8 (deferred_process_work){+.+.}-{0:0}, at: process_one_work+0x1f4/0x6c4 [ 733.387966] #2: ffff80000b020cb0 (rtnl_mutex){+.+.}-{4:4}, at: rtnl_lock+0x1c/0x30 [ 733.395660] #3: ffff80000b073370 ((switchdev_blocking_notif_chain).rwsem){++++}-{4:4}, at: blocking_notifier_call_chain+0x34/0xa0 [ 733.407432] #4: ffff00000213c110 (&chip->reg_lock){+.+.}-{4:4}, at: mv88e6xxx_port_bridge_flags+0x48/0x234 [ 733.417202] #5: ffff00000213e0f0 (&p->ale_list_lock){+.+.}-{4:4}, at: mv88e6xxx_atu_locked_entry_flush+0x4c/0xc0 [ 733.427495] [ 733.427495] stack backtrace: [ 733.431858] CPU: 0 PID: 601 Comm: kworker/0:0 Not tainted 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3293 [ 733.440992] Hardware name: CZ.NIC Turris Mox Board (DT) [ 733.446219] Workqueue: events switchdev_deferred_process_work [ 733.451982] Call trace: [ 733.454424] dump_backtrace.part.0+0xcc/0xe0 [ 733.458702] show_stack+0x18/0x6c [ 733.462028] dump_stack_lvl+0x8c/0xb8 [ 733.465703] dump_stack+0x18/0x34 [ 733.469029] __lock_acquire+0x1074/0x1fd0 [ 733.473052] lock_acquire.part.0+0xe4/0x220 [ 733.477244] lock_acquire+0x68/0x8c [ 733.480744] __mutex_lock+0x9c/0x460 [ 733.484332] mutex_lock_nested+0x40/0x50 [ 733.488268] mv88e6xxx_atu_locked_entry_purge+0x70/0x1a4 [ 733.493592] mv88e6xxx_atu_locked_entry_flush+0x7c/0xc0 [ 733.498827] mv88e6xxx_port_set_lock+0xfc/0x10c [ 733.503374] mv88e6xxx_port_bridge_flags+0x200/0x234 [ 733.508351] dsa_port_bridge_flags+0x44/0xe0 [ 733.512635] dsa_slave_port_attr_set+0x1ec/0x230 [ 733.517268] __switchdev_handle_port_attr_set+0x58/0x100 [ 733.522594] switchdev_handle_port_attr_set+0x10/0x24 [ 733.527659] dsa_slave_switchdev_blocking_event+0x8c/0xd4 [ 733.533074] blocking_notifier_call_chain+0x6c/0xa0 [ 733.537968] switchdev_port_attr_notify.constprop.0+0x4c/0xb0 [ 733.543729] switchdev_port_attr_set_deferred+0x24/0x80 [ 733.548967] switchdev_deferred_process+0x90/0x164 [ 733.553774] switchdev_deferred_process_work+0x14/0x2c [ 733.558926] process_one_work+0x28c/0x6c4 [ 733.562949] worker_thread+0x74/0x450 [ 733.566623] kthread+0x118/0x11c [ 733.569860] ret_from_fork+0x10/0x20 ++ mac_get lan1 ++ local if_name=lan1 ++ jq -r '.[]["address"]' ++ ip -j link show dev lan1

I've tentatively fixed this in my tree by taking the register lock more localized to the sub-functions of mv88e6xxx_port_bridge_flags(), and not taking it at caller side for mv88e6xxx_port_set_lock(), but rather letting the callee take it:

Yes, the double chip lock came from calling mv88e6xxx_atu_locked_entry_flush() in mv88e6xxx_port_set_lock() that is called from mv88e6xxx_port_bridge_flags() whic has the lock taken. I have fixed this now, but when I unlock a port with locked entries, I cannot have notify the bridge to clear the same locked entries by having mv88e6xxx_atu_locked_entry_flush() calling with the notify on, thus taking the rtnl lock, as this also results in a double lock. To remove the locked entries in the bridge FDB it is then necessary as I see it, to take the link down or have a userspace daemon clear them. I hope that is okay if documented?

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 7b57ac121589..ec5954f32774 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -6557,41 +6557,47 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port, struct mv88e6xxx_chip *chip = ds->priv; int err = -EOPNOTSUPP;

• mv88e6xxx_reg_lock(chip);
• if (flags.mask & BR_LEARNING) { bool learning = !!(flags.val & BR_LEARNING); u16 pav = learning ? (1 << port) : 0;

• mv88e6xxx_reg_lock(chip);
  

  err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);

• mv88e6xxx_reg_unlock(chip);
  

  if (err)

• 	goto out;
  

• 	return err;
  

  }

  if (flags.mask & BR_FLOOD) { bool unicast = !!(flags.val & BR_FLOOD);

• mv88e6xxx_reg_lock(chip);
  

  err = chip->info->ops->port_set_ucast_flood(chip, port, unicast);

• mv88e6xxx_reg_unlock(chip);
  

  if (err)

• 	goto out;
  

• 	return err;
  

  }

  if (flags.mask & BR_MCAST_FLOOD) { bool multicast = !!(flags.val & BR_MCAST_FLOOD);

• mv88e6xxx_reg_lock(chip);
  

  err = chip->info->ops->port_set_mcast_flood(chip, port, multicast);

• mv88e6xxx_reg_unlock(chip);
  

  if (err)

• 	goto out;
  

• 	return err;
  

  }

  if (flags.mask & BR_BCAST_FLOOD) { bool broadcast = !!(flags.val & BR_BCAST_FLOOD);

• mv88e6xxx_reg_lock(chip);
  

  err = mv88e6xxx_port_broadcast_sync(chip, port, broadcast);

• mv88e6xxx_reg_unlock(chip);
  

  if (err)

• 	goto out;
  

• 	return err;
  

  }

  if (flags.mask & BR_PORT_LOCKED) {

@@ -6599,10 +6605,8 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,

err = mv88e6xxx_port_set_lock(chip, port, locked);
if (err)

• 	goto out;
  

• 	return err;
  

  }

-out:

• mv88e6xxx_reg_unlock(chip);

  return err;

} diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index 5e4d5e9265a4..2a60aded6fbe 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -1222,15 +1222,19 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port, u16 reg; int err;

• mv88e6xxx_reg_lock(chip); err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);

• if (err)

• if (err) {

• mv88e6xxx_reg_unlock(chip);
  

  return err;

• }

  reg &= ~MV88E6XXX_PORT_CTL0_SA_FILT_MASK; if (locked) reg |= MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_LOCK;

  err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);

• mv88e6xxx_reg_unlock(chip); if (err) return err;

@@ -1247,7 +1251,11 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port, MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT; }

• return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,

reg);

• mv88e6xxx_reg_lock(chip);
• err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,

reg);

• mv88e6xxx_reg_unlock(chip);
• return err;

}

int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,

...

• if (err)
  

• 	return err;
  

• }

• reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
• if (locked)

• reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
  

• reg = 0;

• if (locked) {

• reg = (1 << port);
  

• reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
  

• 	MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
  

• }

  return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,

reg); } diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index e0a705d82019..5c1d485e7442 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -231,6 +231,7 @@ #define MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT 0x2000 #define MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG 0x1000 #define MV88E6XXX_PORT_ASSOC_VECTOR_REFRESH_LOCKED 0x0800 +#define MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK 0x07ff

/* Offset 0x0C: Port ATU Control */ #define MV88E6XXX_PORT_ATU_CTL 0x0c @@ -374,6 +375,7 @@ int mv88e6xxx_port_set_fid(struct mv88e6xxx_chip *chip, int port, u16 fid); int mv88e6xxx_port_get_pvid(struct mv88e6xxx_chip *chip, int port, u16 *pvid); int mv88e6xxx_port_set_pvid(struct mv88e6xxx_chip *chip, int port, u16 pvid);

+bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip, int port); int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port, bool locked);