linux-kernel - Re: [PATCH 3/4] base/drivers/topology: Move instructions in the error path

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: <CAOh2x==N=YM+4WE8wouM9mFd0h0L+npwww97e2HAf2HsEm5J0w@mail.gmail.com>
Date:   Tue, 30 Oct 2018 11:42:09 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH 3/4] base/drivers/topology: Move instructions in the error path

On Mon, Oct 29, 2018 at 9:56 PM Daniel Lezcano
<daniel.lezcano@...aro.org> wrote:
>
> When the function topology_parse_cpu_capacity() fails, we set the boolean
> cap_parsing_failed to true and we free the raw_capacity. This is correct as
> the function begins with a check against cap_parsing_failed thus protecting
> the function to be re-entered.
>
> However, even it is impossible that can happen with the current code, let's

Why impossible ?

> move in the instructions:
>
>  - cap_parsing_failed = true;
>  - free_raw_capacity();
>
>  ... in the 'else' block when the error is detected, that is more semantically
>  correct.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> ---
>  drivers/base/arch_topology.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index b19d6d4..7311641 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -155,9 +155,9 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
>                         pr_err("cpu_capacity: missing %pOF raw capacity\n",
>                                 cpu_node);
>                         pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
> +                       cap_parsing_failed = true;
> +                       free_raw_capacity();
>                 }
> -               cap_parsing_failed = true;
> -               free_raw_capacity();

While it is fine to move free_raw_capacity(), it is not to move the
other line. With your
patch what will happen if the first CPU in DT doesn't have the
"capacity-dmips-mhz"
property set ? We will never set cap_parsing_failed and keep on
re-entering this routine
which wasn't required.

Note that the current implementation isn't written to always print an
error where this
property is only partially filled and the same wouldn't happen with
your patch as well.

--
viresh