Re: [PATCH 4.19 29/58] bonding: fix data-races around agg_select_timer

[Eric Dumazet]

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.