Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220221084911.895146879@linuxfoundation.org> <20220221084912.825972694@linuxfoundation.org>
 <20220222153528.GA27262@amd>
In-Reply-To: <20220222153528.GA27262@amd>
From:   Eric Dumazet <edumazet@google.com>
Date:   Tue, 22 Feb 2022 08:27:07 -0800
Message-ID: <CANn89iLOiaan+CG94+6-RsMWH5pPLJ6snfAGQ3UJfRBLijTbsQ@mail.gmail.com>
Subject: Re: [PATCH 4.19 29/58] bonding: fix data-races around agg_select_timer
To:     Pavel Machek <pavel@ucw.cz>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-stable <stable@vger.kernel.org>,
        syzbot <syzkaller@googlegroups.com>,
        Jay Vosburgh <j.vosburgh@gmail.com>,
        Veaceslav Falico <vfalico@gmail.com>,
        "David S. Miller" <davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Feb 22, 2022 at 7:35 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > syzbot reported that two threads might write over agg_select_timer
> > at the same time. Make agg_select_timer atomic to fix the races.
>
> Ok, but:
>
> > --- a/drivers/net/bonding/bond_3ad.c
> > +++ b/drivers/net/bonding/bond_3ad.c
> > @@ -249,7 +249,7 @@ static inline int __check_agg_selection_
> >       if (bond == NULL)
> >               return 0;
> >
> > -     return BOND_AD_INFO(bond).agg_select_timer ? 1 : 0;
> > +     return atomic_read(&BOND_AD_INFO(bond).agg_select_timer) ? 1 : 0;
> >  }
>
> This could probably use !!.

Probably... I chose to not change code style in a bug fix.

>
> > +static bool bond_agg_timer_advance(struct bonding *bond)
> > +{
> > +     int val, nval;
> > +
> > +     while (1) {
> > +             val = atomic_read(&BOND_AD_INFO(bond).agg_select_timer);
> > +             if (!val)
> > +                     return false;
> > +             nval = val - 1;
> > +             if (atomic_cmpxchg(&BOND_AD_INFO(bond).agg_select_timer,
> > +                                val, nval) == val)
> > +                     break;
> > +     }
> > +     return nval == 0;
> > +}
>
> This should really be atomic_dec_if_positive, no?

SGTM, please send a patch, thank you.