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] [day] [month] [year] [list]
Message-ID: <CAMArcTX4DuSGeBBjWquDWgdxejdEN1s2gJ92-Nwp6_wkFTGU4Q@mail.gmail.com>
Date:   Fri, 14 Feb 2020 13:18:17 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, j.vosburgh@...il.com,
        vfalico@...il.com, Andy Gospodarek <andy@...yhouse.net>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net 2/2] bonding: do not collect slave's stats

On Fri, 14 Feb 2020 at 04:34, Eric Dumazet <eric.dumazet@...il.com> wrote:
>

Hi Eric,
Thank you for the review!

> 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.
>

I agree with this.

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

Yes, this patch reduces overheads of dev_get_stats(), but it
sadly increases overheads of datapath.
So, I agree with this.

> 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)

Thank you for the advice!

> 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.
>

I think fixing only lockdep issues would be good.
So, I will send a v2 patch, which will fix only lockdep issues.

Thanks a lot!
Taehee Yoo