netdev - Re: [PATCH net] bonding: fix lockdep splat in bond_miimon

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Y6J+pOX5hAupkge2@Laptop-X1>
Date:   Wed, 21 Dec 2022 11:33:56 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        eric.dumazet@...il.com, syzbot <syzkaller@...glegroups.com>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH net] bonding: fix lockdep splat in bond_miimon_commit()

On Tue, Dec 20, 2022 at 01:08:31PM +0000, Eric Dumazet wrote:
> bond_miimon_commit() is run while RTNL is held, not RCU.
> 
> WARNING: suspicious RCU usage
> 6.1.0-syzkaller-09671-g89529367293c #0 Not tainted
> -----------------------------
> drivers/net/bonding/bond_main.c:2704 suspicious rcu_dereference_check() usage!
> 
> Fixes: e95cc44763a4 ("bonding: do failover when high prio link up")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Cc: Hangbin Liu <liuhangbin@...il.com>
> Cc: Jay Vosburgh <j.vosburgh@...il.com>
> Cc: Veaceslav Falico <vfalico@...il.com>
> Cc: Andy Gospodarek <andy@...yhouse.net>
> ---
>  drivers/net/bonding/bond_main.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b4c65783960a5aa14de5d64aeea190f02a04be44..0363ce597661422b82a7d33ef001151b275f9ada 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2654,10 +2654,12 @@ static void bond_miimon_link_change(struct bonding *bond,
>  
>  static void bond_miimon_commit(struct bonding *bond)
>  {
> -	struct slave *slave, *primary;
> +	struct slave *slave, *primary, *active;
>  	bool do_failover = false;
>  	struct list_head *iter;
>  
> +	ASSERT_RTNL();
> +
>  	bond_for_each_slave(bond, slave, iter) {
>  		switch (slave->link_new_state) {
>  		case BOND_LINK_NOCHANGE:
> @@ -2700,8 +2702,8 @@ static void bond_miimon_commit(struct bonding *bond)
>  
>  			bond_miimon_link_change(bond, slave, BOND_LINK_UP);
>  
> -			if (!rcu_access_pointer(bond->curr_active_slave) || slave == primary ||
> -			    slave->prio > rcu_dereference(bond->curr_active_slave)->prio)
> +			active = rtnl_dereference(bond->curr_active_slave);
> +			if (!active || slave == primary || slave->prio > active->prio)
>  				do_failover = true;

Hi Eric,

Thanks for the fix. I have some silly questions.

Is there an easy way or tool that could find if the functions is holding via
RTNL lock or RCU lock, except review all the call chains? I have faced
this issue in commit 9b80ccda233f ("bonding: fix missed rcu protection"),
which we though the function is under RTNL, while there is a call chain that
not hold rcu lock. Adding ASSERT_RTNL() could find it during running. I just
want to know if there is another way that we could find it in code review.


Another questions is, I'm still a little confused with the mixing usage of
rcu_access_pointer() and rtnl_dereference() under RTNL. e.g.

In bond_miimon_commit() we use rcu_access_pointer() to check the pointers.
                case BOND_LINK_DOWN:
                        if (slave == rcu_access_pointer(bond->curr_active_slave))
                                do_failover = true;

In bond_ab_arp_commit() we use rtnl_dereference() to check the pointer

                case BOND_LINK_DOWN:
                        if (slave == rtnl_dereference(bond->curr_active_slave)) {
                                RCU_INIT_POINTER(bond->current_arp_slave, NULL);
                                do_failover = true;
                        }
                case BOND_LINK_FAIL:
                        if (rtnl_dereference(bond->curr_active_slave))
                                RCU_INIT_POINTER(bond->current_arp_slave, NULL);

Does it matter to use which one? Should we change to rcu_access_pointer()
if there is no dereference?

Thanks
Hangbin