netdev - Re: bond link state mismatch, rtnl_trylock() vs rtnl

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]

Date:   Tue, 23 May 2017 14:30:47 -0700
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     Nithin Sujir <nsujir@...tri.com>
Cc:     linux-netdev <netdev@...r.kernel.org>
Subject: Re: bond link state mismatch, rtnl_trylock() vs rtnl_lock()

On Tue, May 23, 2017 at 12:32 PM, Nithin Sujir <nsujir@...tri.com> wrote:
> Hi,
> We're encountering a problem in 4.4 LTS where, rarely, the bond link state
> is not updated when the slave link changes.
>
> I've traced the issue to the arp monitor unable to get the rtnl lock. The
> sequence resulting in failure is as below.
>
> bond_loadbalance_arp_mon() periodically called, if slave link is _down_, it
> checks if the slave is sending/receiving packets. If it is, it sets flags to
> be processed later down the function for bond link update. However, it sets
> the slave->link right away.
>
>                 if (slave->link != BOND_LINK_UP) {
>                         if (bond_time_in_interval(bond, trans_start, 1) &&
>                             bond_time_in_interval(bond, slave->last_rx, 1))
> {
>
>                                 slave->link  = BOND_LINK_UP;
>                                 slave_state_changed = 1;
>
>
> Later down the function, it tries to get the rtnl_lock. If it doesn't get
> it, it rearms and returns.
>
>         if (do_failover || slave_state_changed) {
>                 if (!rtnl_trylock())
>                         goto re_arm; <-- returns here
>
>                 if (slave_state_changed) {
>                         bond_slave_state_change(bond);
>
> This is the problem. The next time this function is called, the slave->link
> is already marked UP. And we will never update the bond link state to UP.
>
> Changing the rtnl_trylock() -> rtnl_lock() _does_ fix the issue.
>
> Is this the right way to fix it? If it is, I can submit this formally.
>
> What are the guidelines around using rtnl_lock() vs rtnl_trylock()? Some
> places are using rtnl_lock() and other rtnl_trylock(). Sorry, I couldn't
> find much via a google search or in Documentation/.
>
> Thanks,
> Nithin.
>
> --------------------
>
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index 5dca77e..1f60503 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2614,8 +2614,7 @@ static void bond_loadbalance_arp_mon(struct
> work_struct *work)
>         rcu_read_unlock();
>
>         if (do_failover || slave_state_changed) {
> -               if (!rtnl_trylock())
> -                       goto re_arm;
> +               rtnl_lock();

Nitin, you can't do this. The tryRTNL code is to prevent deadlock
during work-cancellation during bond_close().

>
>                 if (slave_state_changed) {
>                         bond_slave_state_change(bond);
>
>