linux-kernel - Re: [PATCH v2] cpufreq: powernv: fix stack bloat and NR

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: <87d0e5wuc6.fsf@mpe.ellerman.id.au>
Date:   Wed, 06 Nov 2019 14:35:05 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     John Hubbard <jhubbard@...dia.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
Cc:     linux-pm@...r.kernel.org, "Rafael J . Wysocki" <rjw@...ysocki.net>,
        LKML <linux-kernel@...r.kernel.org>,
        Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2] cpufreq: powernv: fix stack bloat and NR_CPUS limitation

John Hubbard <jhubbard@...dia.com> writes:
> On 10/30/19 7:39 PM, Michael Ellerman wrote:
>> Hi John,
>> 
>> Sorry I didn't reply to this sooner, too many patches :/
>> 
>> John Hubbard <jhubbard@...dia.com> writes:
>>> The following build warning occurred on powerpc 64-bit builds:
>>>
>>> drivers/cpufreq/powernv-cpufreq.c: In function 'init_chip_info':
>>> drivers/cpufreq/powernv-cpufreq.c:1070:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>> 
>> Oddly I don't see that warning in my builds, eg with GCC9:
>> 
>>    https://travis-ci.org/linuxppc/linux/jobs/604870722
>
> This is with a cross-compiler based on gcc 8.1.0, which I got from:
>    https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/
>
> I'll put that in the v3 commit description.
>
>> 
>>> This is due to putting 1024 bytes on the stack:
>>>
>>>      unsigned int chip[256];
>>>
>>> ...and while looking at this, it also has a bug: it fails with a stack
>>> overrun, if CONFIG_NR_CPUS > 256.
>> 
>> It _probably_ doesn't, because it only increments the index when the
>> chip_id of the CPU changes, ie. it doesn't create a chip for every CPU.
>> But I agree it's flaky the way it's written.
>
> I'll soften up the wording accordingly.
>
>> 
>>> Fix both problems by dynamically allocating based on CONFIG_NR_CPUS.
>> 
>> Shouldn't it use num_possible_cpus() ?
>> 
>> Given the for loop is over possible CPUs that seems like the upper
>> bound. In practice it should be lower because some CPUs will share a
>> chip.
>> 
>
> OK, I see, that's more consistent with the code, I'll change to that.

Thanks.

cheers