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: <CAK8P3a0b88NUZUebzxLx5J9Lz4r=s2AmxsWPRTvvQm6ieFnuww@mail.gmail.com>
Date:   Sat, 27 Feb 2021 13:14:28 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Saeed Mahameed <saeed@...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>,
        Saeed Mahameed <saeedm@...dia.com>
Subject: Re: [net 01/15] net/mlx5e: E-switch, Fix rate calculation for overflow

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().

- 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

- 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.

        Arnd