Date:   Fri, 19 Aug 2022 10:28:09 +0200
From:   netdev@...io-technology.com
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>, Jiri Pirko <jiri@...nulli.us>,
        Ivan Vecera <ivecera@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Shuah Khan <shuah@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Ido Schimmel <idosch@...dia.com>, linux-kernel@...r.kernel.org,
        bridge@...ts.linux-foundation.org, linux-kselftest@...r.kernel.org
Subject: 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);
>> 

Powered by blists - more mailing lists