linux-kernel - Re: [PATCH][next] net: dsa: sja1105: fix check on while loop exit

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] [day] [month] [year] [list]

Message-ID: <CA+h21hoEVYKTeGxSxPi0E1bWp1rBE=sG5T8MzGOBkranpdGhjQ@mail.gmail.com>
Date:   Tue, 7 May 2019 16:59:10 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Colin King <colin.king@...onical.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH][next] net: dsa: sja1105: fix check on while loop exit

On Tue, 7 May 2019 at 13:34, Colin King <colin.king@...onical.com> wrote:
>
> From: Colin Ian King <colin.king@...onical.com>
>
> The while-loop exit condition check is not correct; the
> loop should continue if the returns from the function calls are
> negative or the CRC status returns are invalid.  Currently it
> is ignoring the returns from the function calls.  Fix this by
> removing the status return checks and only break from the loop
> at the very end when we know that all the success condtions have
> been met.
>
> Kudos to Dan Carpenter for describing the correct fix.
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch")
> Signed-off-by: Colin Ian King <colin.king@...onical.com>
> ---
>
> V2: Discard my broken origina fix. Use correct fix as described by
>     Dan Carpenter.
> ---
>  drivers/net/dsa/sja1105/sja1105_spi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
> index 244a94ccfc18..40ac696adf63 100644
> --- a/drivers/net/dsa/sja1105/sja1105_spi.c
> +++ b/drivers/net/dsa/sja1105/sja1105_spi.c
> @@ -465,9 +465,11 @@ int sja1105_static_config_upload(struct sja1105_private *priv)
>                         dev_err(dev, "Switch reported that configuration is "
>                                 "invalid, retrying...\n");
>                         continue;
> +
>                 }
> -       } while (--retries && (status.crcchkl == 1 || status.crcchkg == 1 ||
> -                status.configs == 0 || status.ids == 1));
> +               /* Success! */
> +               break;
> +       } while (--retries);
>
>         if (!retries) {
>                 rc = -EIO;
> --
> 2.20.1
>

Hi Colin, Dan,

Thanks for the fix.
This portion below needs to be changed as well: "else if (retries !=
RETRIES - 1)" should become "else if (retries != RETRIES)" because on
success, it now does not decrement retries due to the early break.
Otherwise this message gets printed: "Succeeded after 0 tried" which
was not the original intention.
Please also remove the extraneous newline introduced at 468.

Tested-by: Vladimir Oltean <olteanv@...il.com>

-Vladimir