lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 27 May 2020 10:57:51 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>, davem@...emloft.net
Cc:     andrew@...n.ch, vivien.didelot@...il.com, xiyou.wangcong@...il.com,
        ap420073@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: declare lockless TX feature for slave ports



On 5/27/2020 10:45 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> Be there a platform with the following layout:
> 
>       Regular NIC
>        |
>        +----> DSA master for switch port
>                |
>                +----> DSA master for another switch port
> 
> After changing DSA back to static lockdep class keys in commit
> 1a33e10e4a95 ("net: partially revert dynamic lockdep key changes"), this
> kernel splat can be seen:
> 
> [   13.361198] ============================================
> [   13.366524] WARNING: possible recursive locking detected
> [   13.371851] 5.7.0-rc4-02121-gc32a05ecd7af-dirty #988 Not tainted
> [   13.377874] --------------------------------------------
> [   13.383201] swapper/0/0 is trying to acquire lock:
> [   13.388004] ffff0000668ff298 (&dsa_slave_netdev_xmit_lock_key){+.-.}-{2:2}, at: __dev_queue_xmit+0x84c/0xbe0
> [   13.397879]
> [   13.397879] but task is already holding lock:
> [   13.403727] ffff0000661a1698 (&dsa_slave_netdev_xmit_lock_key){+.-.}-{2:2}, at: __dev_queue_xmit+0x84c/0xbe0
> [   13.413593]
> [   13.413593] other info that might help us debug this:
> [   13.420140]  Possible unsafe locking scenario:
> [   13.420140]
> [   13.426075]        CPU0
> [   13.428523]        ----
> [   13.430969]   lock(&dsa_slave_netdev_xmit_lock_key);
> [   13.435946]   lock(&dsa_slave_netdev_xmit_lock_key);
> [   13.440924]
> [   13.440924]  *** DEADLOCK ***
> [   13.440924]
> [   13.446860]  May be due to missing lock nesting notation
> [   13.446860]
> [   13.453668] 6 locks held by swapper/0/0:
> [   13.457598]  #0: ffff800010003de0 ((&idev->mc_ifc_timer)){+.-.}-{0:0}, at: call_timer_fn+0x0/0x400
> [   13.466593]  #1: ffffd4d3fb478700 (rcu_read_lock){....}-{1:2}, at: mld_sendpack+0x0/0x560
> [   13.474803]  #2: ffffd4d3fb478728 (rcu_read_lock_bh){....}-{1:2}, at: ip6_finish_output2+0x64/0xb10
> [   13.483886]  #3: ffffd4d3fb478728 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x6c/0xbe0
> [   13.492793]  #4: ffff0000661a1698 (&dsa_slave_netdev_xmit_lock_key){+.-.}-{2:2}, at: __dev_queue_xmit+0x84c/0xbe0
> [   13.503094]  #5: ffffd4d3fb478728 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x6c/0xbe0
> [   13.512000]
> [   13.512000] stack backtrace:
> [   13.516369] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc4-02121-gc32a05ecd7af-dirty #988
> [   13.530421] Call trace:
> [   13.532871]  dump_backtrace+0x0/0x1d8
> [   13.536539]  show_stack+0x24/0x30
> [   13.539862]  dump_stack+0xe8/0x150
> [   13.543271]  __lock_acquire+0x1030/0x1678
> [   13.547290]  lock_acquire+0xf8/0x458
> [   13.550873]  _raw_spin_lock+0x44/0x58
> [   13.554543]  __dev_queue_xmit+0x84c/0xbe0
> [   13.558562]  dev_queue_xmit+0x24/0x30
> [   13.562232]  dsa_slave_xmit+0xe0/0x128
> [   13.565988]  dev_hard_start_xmit+0xf4/0x448
> [   13.570182]  __dev_queue_xmit+0x808/0xbe0
> [   13.574200]  dev_queue_xmit+0x24/0x30
> [   13.577869]  neigh_resolve_output+0x15c/0x220
> [   13.582237]  ip6_finish_output2+0x244/0xb10
> [   13.586430]  __ip6_finish_output+0x1dc/0x298
> [   13.590709]  ip6_output+0x84/0x358
> [   13.594116]  mld_sendpack+0x2bc/0x560
> [   13.597786]  mld_ifc_timer_expire+0x210/0x390
> [   13.602153]  call_timer_fn+0xcc/0x400
> [   13.605822]  run_timer_softirq+0x588/0x6e0
> [   13.609927]  __do_softirq+0x118/0x590
> [   13.613597]  irq_exit+0x13c/0x148
> [   13.616918]  __handle_domain_irq+0x6c/0xc0
> [   13.621023]  gic_handle_irq+0x6c/0x160
> [   13.624779]  el1_irq+0xbc/0x180
> [   13.627927]  cpuidle_enter_state+0xb4/0x4d0
> [   13.632120]  cpuidle_enter+0x3c/0x50
> [   13.635703]  call_cpuidle+0x44/0x78
> [   13.639199]  do_idle+0x228/0x2c8
> [   13.642433]  cpu_startup_entry+0x2c/0x48
> [   13.646363]  rest_init+0x1ac/0x280
> [   13.649773]  arch_call_rest_init+0x14/0x1c
> [   13.653878]  start_kernel+0x490/0x4bc
> 
> Lockdep keys themselves were added in commit ab92d68fc22f ("net: core:
> add generic lockdep keys"), and it's very likely that this splat existed
> since then, but I have no real way to check, since this stacked platform
> wasn't supported by mainline back then.
> 
> From Taehee's own words:
> 
>   This patch was considered that all stackable devices have LLTX flag.
>   But the dsa doesn't have LLTX, so this splat happened.
>   After this patch, dsa shares the same lockdep class key.
>   On the nested dsa interface architecture, which you illustrated,
>   the same lockdep class key will be used in __dev_queue_xmit() because
>   dsa doesn't have LLTX.
>   So that lockdep detects deadlock because the same lockdep class key is
>   used recursively although actually the different locks are used.
>   There are some ways to fix this problem.
> 
>   1. using NETIF_F_LLTX flag.
>   If possible, using the LLTX flag is a very clear way for it.
>   But I'm so sorry I don't know whether the dsa could have LLTX or not.
> 
>   2. using dynamic lockdep again.
>   It means that each interface uses a separate lockdep class key.
>   So, lockdep will not detect recursive locking.
>   But this way has a problem that it could consume lockdep class key
>   too many.
>   Currently, lockdep can have 8192 lockdep class keys.
>    - you can see this number with the following command.
>      cat /proc/lockdep_stats
>      lock-classes:                         1251 [max: 8192]
>      ...
>      The [max: 8192] means that the maximum number of lockdep class keys.
>   If too many lockdep class keys are registered, lockdep stops to work.
>   So, using a dynamic(separated) lockdep class key should be considered
>   carefully.
>   In addition, updating lockdep class key routine might have to be existing.
>   (lockdep_register_key(), lockdep_set_class(), lockdep_unregister_key())
> 
>   3. Using lockdep subclass.
>   A lockdep class key could have 8 subclasses.
>   The different subclass is considered different locks by lockdep
>   infrastructure.
>   But "lock-classes" is not counted by subclasses.
>   So, it could avoid stopping lockdep infrastructure by an overflow of
>   lockdep class keys.
>   This approach should also have an updating lockdep class key routine.
>   (lockdep_set_subclass())
> 
>   4. Using nonvalidate lockdep class key.
>   The lockdep infrastructure supports nonvalidate lockdep class key type.
>   It means this lockdep is not validated by lockdep infrastructure.
>   So, the splat will not happen but lockdep couldn't detect real deadlock
>   case because lockdep really doesn't validate it.
>   I think this should be used for really special cases.
>   (lockdep_set_novalidate_class())
> 
> Further discussion here:
> https://patchwork.ozlabs.org/project/netdev/patch/20200503052220.4536-2-xiyou.wangcong@gmail.com/
> 
> There appears to be no negative side-effect to declaring lockless TX for
> the DSA virtual interfaces, which means they handle their own locking.
> So that's what we do to make the splat go away.
> 
> Patch tested in a wide variety of cases: unicast, multicast, PTP, etc.
> 
> Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
> Suggested-by: Taehee Yoo <ap420073@...il.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

This looks fine to me and an appropriate fix, just one nit below:

> ---
>  net/dsa/slave.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 886490fb203d..4188290f8edd 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1746,6 +1746,8 @@ int dsa_slave_create(struct dsa_port *port)
>  	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
>  		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>  	slave_dev->hw_features |= NETIF_F_HW_TC;
> +	slave_dev->features |= NETIF_F_LLTX;
> +	slave_dev->hw_features |= NETIF_F_LLTX;

I do not believe this feature needs to propagate to hw_features though,
does it? net/8021q/vlan_dev.c does not seem to concern itself with that.

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian

Powered by blists - more mailing lists