linux-kernel - Re: [PATCH] x86/mm: drop superfluous initialization

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: <1d376c1f-2f14-5c29-671e-ca59853fa4a4@intel.com>
Date:   Mon, 28 Sep 2020 10:59:36 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Lukas Bulwahn <lukas.bulwahn@...il.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-kernel@...r.kernel.org, clang-built-linux@...glegroups.com,
        kernel-janitors@...r.kernel.org, linux-safety@...ts.elisa.tech
Subject: Re: [PATCH] x86/mm: drop superfluous initialization

On 9/28/20 3:00 AM, Lukas Bulwahn wrote:
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index c7a47603537f..5632f02146ca 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
>  	step_size = PMD_SIZE;
>  	max_pfn_mapped = 0; /* will get exact value next */
>  	min_pfn_mapped = real_end >> PAGE_SHIFT;
> -	last_start = start = real_end;
> +	last_start = real_end;

Thanks for finding this.

This becomes even more obviously correct if we just move the 'start'
declaration into the while() loop.  If we do that, it puts the three
assignment locations right next to the definition, and its trivial to
spot that the initialization was not missed:

        while (last_start > map_start) {
		unsigned long start;

                if (last_start > step_size) {
                        start = round_down(last_start - 1, step_size);
                        if (start < map_start)
                                start = map_start;
                } else
                        start = map_start;
		...

Either way, your patch looks correct to me:

Acked-by: Dave Hansen <dave.hansen@...ux.intel.com>