Re: [PATCH net] net/sched: taprio: fix duration_to_length()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, May 27, 2024 at 10:07:31AM +0200, Eric Dumazet wrote:
> On Fri, May 24, 2024 at 6:07 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > On Fri, May 24, 2024 at 05:52:17PM +0200, Eric Dumazet wrote:
> > > On Fri, May 24, 2024 at 5:50 PM Eric Dumazet <edumazet@google.com> wrote:
> > > >
> > > > On Fri, May 24, 2024 at 5:39 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> > > > >
> > > > > On Thu, May 23, 2024 at 01:45:49PM +0000, Eric Dumazet wrote:
> > > > > > duration_to_length() is incorrectly using div_u64()
> > > > > > instead of div64_u64().
> > > > > > ---
> > > > > >  net/sched/sch_taprio.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > > > > > index 1ab17e8a72605385280fad9b7f656a6771236acc..827fb81fc63a098304bad198fadd4aed55d1fec4 100644
> > > > > > --- a/net/sched/sch_taprio.c
> > > > > > +++ b/net/sched/sch_taprio.c
> > > > > > @@ -256,7 +256,8 @@ static int length_to_duration(struct taprio_sched *q, int len)
> > > > > >
> > > > > >  static int duration_to_length(struct taprio_sched *q, u64 duration)
> > > > > >  {
> > > > > > -     return div_u64(duration * PSEC_PER_NSEC, atomic64_read(&q->picos_per_byte));
> > > > > > +     return div64_u64(duration * PSEC_PER_NSEC,
> > > > > > +                      atomic64_read(&q->picos_per_byte));
> > > > > >  }
> > > > >
> > > > > There's a netdev_dbg() in taprio_set_picos_per_byte(). Could you turn
> > > > > that on? I'm curious what was the q->picos_per_byte value that triggered
> > > > > the 64-bit division fault. There are a few weird things about
> > > > > q->picos_per_byte's representation and use as an atomic64_t (s64) type.
> > > >
> > > >
> > > > No repro yet.
> > > >
> > > > Anything with 32 low order bits cleared would trigger a divide by 0.
> > > >
> > > > (1ULL << 32) picoseconds is only 4.294 ms
> > >
> > > BTW, just a reminder, div_u64() is a divide by a 32bit value...
> > >
> > > static inline u64 div_u64(u64 dividend, u32 divisor)
> > > ...
> >
> > The thing is that I don't see how q->picos_per_byte could take any sane
> > value of either 0 or a multiple of 2^32. Its formula is "(USEC_PER_SEC * 8) / speed"
> > where "speed" is the link speed: 10, 100, 1000 etc. The special cases
> > of speed=0 and speed=SPEED_UNKNOWN are handled by falling back to SPEED_10
> > in the picos_per_byte calculation.
> >
> > For q->picos_per_byte to be larger than 2^32, "speed" would have to be
> > smaller than 8000000 / U32_MAX (0.001862645).
> >
> > For q->picos_per_byte to be exactly 0, "speed" would have to be larger
> > than 8000000. But the largest defined speed in include/uapi/linux/ethtool.h
> > is precisely SPEED_800000, leading to an expected q->picos_per_byte of 1.
> 
> This suggests q->picos_per_byte should be a mere u32, and that
> taprio_set_picos_per_byte()
> should make sure to not set  0 in q->picos_per_byte

This is what I was hinting at, indeed. But we're getting farther away
from the problem, which is the fact that syzbot _was_ able to trigger a
division by zero somehow, when zero was not a valid value that I can see.

> Presumably some devices must get a speed bigger than SPEED_800000
> 
> team driver could do that, according to team_ethtool_get_link_ksettings()

I misspoke in the earlier email. SPEED_800000 is still 1 order of
magnitude lower than the maximum representable speed (picos_per_byte
should be 10 for it, not 1). So, we should still be good.

> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 1ab17e8a72605385280fad9b7f656a6771236acc..71087a53630362863cc6c5e462b29dbef8cd5d74
> 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -89,9 +89,9 @@ struct taprio_sched {
>         bool offloaded;
>         bool detected_mqprio;
>         bool broken_mqprio;
> -       atomic64_t picos_per_byte; /* Using picoseconds because for 10Gbps+
> -                                   * speeds it's sub-nanoseconds per byte
> -                                   */
> +       atomic_t picos_per_byte; /* Using picoseconds because for 10Gbps+
> +                                 * speeds it's sub-nanoseconds per byte
> +                                 */
> 
>         /* Protects the update side of the RCU protected current_entry */
>         spinlock_t current_entry_lock;
> @@ -251,12 +251,12 @@ static ktime_t get_interval_end_time(struct
> sched_gate_list *sched,
> 
>  static int length_to_duration(struct taprio_sched *q, int len)
>  {
> -       return div_u64(len * atomic64_read(&q->picos_per_byte), PSEC_PER_NSEC);
> +       return div_u64((u64)len * atomic_read(&q->picos_per_byte),
> PSEC_PER_NSEC);
>  }
> 
>  static int duration_to_length(struct taprio_sched *q, u64 duration)
>  {
> -       return div_u64(duration * PSEC_PER_NSEC,
> atomic64_read(&q->picos_per_byte));
> +       return div_u64(duration * PSEC_PER_NSEC,
> atomic_read(&q->picos_per_byte));
>  }
> 
>  /* Sets sched->max_sdu[] and sched->max_frm_len[] to the minimum between the
> @@ -666,8 +666,8 @@ static void taprio_set_budgets(struct taprio_sched *q,
>                 if (entry->gate_duration[tc] == sched->cycle_time)
>                         budget = INT_MAX;
>                 else
> -                       budget =
> div64_u64((u64)entry->gate_duration[tc] * PSEC_PER_NSEC,
> -                                          atomic64_read(&q->picos_per_byte));
> +                       budget = div_u64((u64)entry->gate_duration[tc]
> * PSEC_PER_NSEC,
> +                                        atomic_read(&q->picos_per_byte));
> 
>                 atomic_set(&entry->budget[tc], budget);
>         }
> @@ -1291,7 +1291,7 @@ static void taprio_set_picos_per_byte(struct
> net_device *dev,
>  {
>         struct ethtool_link_ksettings ecmd;
>         int speed = SPEED_10;
> -       int picos_per_byte;
> +       u32 picos_per_byte;
>         int err;
> 
>         err = __ethtool_get_link_ksettings(dev, &ecmd);
> @@ -1303,11 +1303,11 @@ static void taprio_set_picos_per_byte(struct
> net_device *dev,
> 
>  skip:
>         picos_per_byte = (USEC_PER_SEC * 8) / speed;
> -
> -       atomic64_set(&q->picos_per_byte, picos_per_byte);
> -       netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %lld,
> linkspeed: %d\n",
> -                  dev->name, (long long)atomic64_read(&q->picos_per_byte),
> -                  ecmd.base.speed);
> +       if (!picos_per_byte)
> +               picos_per_byte = 1U;
> +       atomic_set(&q->picos_per_byte, picos_per_byte);
> +       netdev_dbg(dev, "taprio: set %s's picos_per_byte to: %u,
> linkspeed: %d\n",
> +                  dev->name, picos_per_byte, ecmd.base.speed);
>  }

I would be cautious about making this change not having certainty what
was the picos_per_byte value (and associated speed) that triggered the fault.
I'm hoping we're not masking some larger issue about how the speed is
retrieved or processed.