netdev - Re: [PATCH net 2/2] bonding: do not collect slave's stats

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a86bb353-f20c-115e-27f1-568773360496@gmail.com>
Date:   Thu, 13 Feb 2020 11:34:37 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Taehee Yoo <ap420073@...il.com>, davem@...emloft.net,
        kuba@...nel.org, j.vosburgh@...il.com, vfalico@...il.com,
        andy@...yhouse.net, netdev@...r.kernel.org
Subject: Re: [PATCH net 2/2] bonding: do not collect slave's stats



On 2/13/20 11:21 AM, Taehee Yoo wrote:
> When stat query is requested(dev_get_stats()), bonding interfaces
> collect stats of slave interfaces.
> Then, it prints delta value, which is "new_stats - old_stats" and
> updates bond->bond_stats.
> But, this mechanism has some problems.
> 
> 1. It needs a lock for protecting "bond->bond_stats".
> Bonding interfaces would be nested. So this lock would also be nested.
> So, spin_lock_nested() or dynamic lockdep class key was used.
> In the case of spin_lock_nested(), it needs correct nested level value
> and this value will be changed when master/nomaster operations
> (ip link set bond0 master bond1) are being executed.
> This value is protected by RTNL mutex lock, but "dev_get_stats()" would
> be called outside of RTNL mutex.
> So, imbalance lock/unlock would be happened.
> Another case, which is to use dynamic lockdep class key has same problem.
> dynamic lockdep class key is protected by RTNL mutex lock
> and if master/nomaster operations are executed, updating lockdep class
> key is needed.
> But, dev_get_stats() would be called outside of RTNL mutex, so imbalance
> lock/unlock would be happened too.
> 
> 2. Couldn't show correct stats value when slave interfaces are used
> directly.
> 
> Test commands:
>     ip netns add nst
>     ip link add veth0 type veth peer name veth1
>     ip link set veth1 netns nst
>     ip link add bond0 type bond
>     ip link set veth0 master bond0
>     ip netns exec nst ip link set veth1 up
>     ip netns exec nst ip a a 192.168.100.2/24 dev veth1
>     ip a a 192.168.100.1/24 dev bond0
>     ip link set veth0 up
>     ip link set bond0 up
>     ping 192.168.100.2 -I veth0 -c 10
>     ip -s link show bond0
>     ip -s link show veth0
> 
> Before:
> 26: bond0:
> RX: bytes  packets  errors  dropped overrun mcast
> 656        8        0       0       0       0
> TX: bytes  packets  errors  dropped carrier collsns
> 1340       22       0       0       0       0
> ~~~~~~~~~~~~
> 
> 25: veth0@...4:
> RX: bytes  packets  errors  dropped overrun mcast
> 656        8        0       0       0       0
> TX: bytes  packets  errors  dropped carrier collsns
> 1340       22       0       0       0       0
> ~~~~~~~~~~~~
> 
> After:
> 19: bond0:
> RX: bytes  packets  errors  dropped overrun mcast
> 544        8        0       0       0       8
> TX: bytes  packets  errors  dropped carrier collsns
> 746        9        0       0       0       0
> ~~~~~~~~~~~
> 
> 18: veth0@...7:
> link/ether 76:14:ee:f1:7d:8e brd ff:ff:ff:ff:ff:ff link-netns nst
> RX: bytes  packets  errors  dropped overrun mcast
> 656        8        0       0       0       0
> TX: bytes  packets  errors  dropped carrier collsns
> 1250       21       0       0       0       0
> ~~~~~~~~~~~~
> 
> Only veth0 interface is used by ping process directly. bond0 interface
> isn't used. So, bond0 stats should not be increased.
> But, bond0 collects stats value of slave interface.
> So bond0 stats will be increased.
> 
> In order to fix the above problems, this patch makes bonding interfaces
> record own stats data like other interfaces.
> This patch is made based on team interface stats logic.
> 
> There is another problem.
> When master/nomaster operations are being executed, updating a dynamic
> lockdep class key is needed.
> But, bonding doesn't update dynamic lockdep key.
> So, lockdep warning message occurs.
> But, this problem will be disappeared by this patch.
> Because this patch removes stats_lock and a dynamic lockdep class key
> for stats_lock, which is stats_lock_key.
> 
> Test commands:
>     ip link add bond0 type bond
>     ip link add bond1 type bond
>     ip link set bond0 master bond1
>     ip link set bond0 nomaster
>     ip link set bond1 master bond0
> 
> Splat looks like:

This is way too invasive patch IMO for net tree.

We do not want adding costs in bonding fast path, for stats accounting.

We do not care of glitches causes by slaves being added/deleted, this usually happens
when we do not need stats (boot time, and before reboot)

BTW, skb->len in RX path is different than the stats provided by hw usually
(because of things like GRO)

Maybe revert the prior patches instead, they have caused a lot of churn.

Or just fix the lockdep issue, and leave stats being what they are.

> Fixes: 089bca2caed0 ("bonding: use dynamic lockdep key instead of subclass")
> Signed-off-by: Taehee Yoo <ap420073@...il.com>
> ---