netdev - Re: [net 01/15] net/mlx5e: E-switch, Fix rate calculation for overflow

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]

Message-ID: <47a3455638c908e3dd7301de3ff41c896bdb765e.camel@kernel.org>
Date:   Mon, 01 Mar 2021 16:52:35 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Networking <netdev@...r.kernel.org>,
        Parav Pandit <parav@...dia.com>, Eli Cohen <elic@...dia.com>
Subject: Re: [net 01/15] net/mlx5e: E-switch, Fix rate calculation for
 overflow

On Sat, 2021-02-27 at 13:14 +0100, Arnd Bergmann wrote:
> On Fri, Feb 12, 2021 at 3:59 AM Saeed Mahameed <saeed@...nel.org>
> wrote:
> > 
> > From: Parav Pandit <parav@...dia.com>
> > 
> > rate_bytes_ps is a 64-bit field. It passed as 32-bit field to
> > apply_police_params(). Due to this when police rate is higher
> > than 4Gbps, 32-bit calculation ignores the carry. This results
> > in incorrect rate configurationn the device.
> > 
> > Fix it by performing 64-bit calculation.
> 
> I just stumbled over this commit while looking at an unrelated
> problem.
> 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index dd0bfbacad47..717fbaa6ce73 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -5040,7 +5040,7 @@ static int apply_police_params(struct
> > mlx5e_priv *priv, u64 rate,
> >          */
> >         if (rate) {
> >                 rate = (rate * BITS_PER_BYTE) + 500000;
> > -               rate_mbps = max_t(u32, do_div(rate, 1000000), 1);
> > +               rate_mbps = max_t(u64, do_div(rate, 1000000), 1);
> 
> I think there are still multiple issues with this line:
> 
> - Before commit 1fe3e3166b35 ("net/mlx5e: E-switch, Fix rate
> calculation for
>   overflow"), it was trying to calculate rate divided by 1000000, but
> now
>   it uses the remainder of the division rather than the quotient. I
> assume
>   this was meant to use div_u64() instead of do_div().
> 

Yes, I already have a patch lined up to fix this issue.

Thanks for spotting this.

> - Both div_u64() and do_div() return a 32-bit number, and '1' is a
> constant
>   that also comfortably fits into a 32-bit number, so changing the
> max_t
>   to return a 64-bit type has no effect on the result
> 

as of the above comment, we shouldn't be using the return value of
do_div().


> - The maximum of an arbitrary unsigned integer and '1' is either one
> or zero,
>    so there doesn't need to be an expensive division here at all.
> From the
>    comment it sounds like the intention was to use 'min_t()' instead
> of 'max_t()'.
>    It has however used 'max_t' since the code was first introduced.
> 

if the input rate is less that 1mbps then the quotient will be 0,
otherwise we want the quotient, and we don't allow 0, so max_t(rate, 1)
should be used, what am I missing ?