On Tue, Feb 22, 2022 at 7:35 AM Pavel Machek <pavel@xxxxxx> 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.
