linux-kernel - Re: Kernel broken on processors without performance counters

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]

Date:	Wed, 08 Jul 2015 16:04:32 -0400
From:	Jason Baron <jasonbaron0@...il.com>
To:	Andy Lutomirski <luto@...capital.net>,
	Peter Zijlstra <peterz@...radead.org>
CC:	Mikulas Patocka <mpatocka@...hat.com>,
	Paul Mackerras <paulus@...ba.org>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Kees Cook <keescook@...omium.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Vince Weaver <vince@...ter.net>,
	"hillf.zj" <hillf.zj@...baba-inc.com>,
	Valdis Kletnieks <Valdis.Kletnieks@...edu>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Kernel broken on processors without performance counters

On 07/08/2015 01:37 PM, Andy Lutomirski wrote:
> On Wed, Jul 8, 2015 at 9:07 AM, Peter Zijlstra <peterz@...radead.org> wrote:
>> On Wed, Jul 08, 2015 at 11:17:38AM -0400, Mikulas Patocka wrote:
>>> Hi
>>>
>>> I found out that the patch a66734297f78707ce39d756b656bfae861d53f62 breaks
>>> the kernel on processors without performance counters, such as AMD K6-3.
>>> Reverting the patch fixes the problem.
>>>
>>> The static key rdpmc_always_available somehow gets set (I couldn't really
>>> find out what is setting it, the function set_attr_rdpmc is not executed),
>>> cr4_set_bits(X86_CR4_PCE) is executed and that results in a crash on boot
>>> when attempting to execute init, because the proecssor doesn't support
>>> that bit in CR4.
>> Urgh, the static key trainwreck bites again.
>>
>> One is not supposed to mix static_key_true() and STATIC_KEY_INIT_FALSE.
>>
>> Does this make it go again?
>>
>> ---
>>  arch/x86/include/asm/mmu_context.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>> index 5e8daee7c5c9..804a3a6030ca 100644
>> --- a/arch/x86/include/asm/mmu_context.h
>> +++ b/arch/x86/include/asm/mmu_context.h
>> @@ -23,7 +23,7 @@ extern struct static_key rdpmc_always_available;
>>
>>  static inline void load_mm_cr4(struct mm_struct *mm)
>>  {
>> -       if (static_key_true(&rdpmc_always_available) ||
>> +       if (static_key_false(&rdpmc_always_available) ||
> In what universe is "static_key_false" a reasonable name for a
> function that returns true if a static key is true?
>
> Can we rename that function?  And could we maybe make static keys type
> safe?  I.e. there would be a type that starts out true and a type that
> starts out false.

So the 'static_key_false' is really branch is initially false. We had
a naming discussion before, but if ppl think its confusing,
'static_key_init_false', or 'static_key_default_false' might be better,
or other ideas.... I agree its confusing.

In terms of getting the type to match so we don't have these
mismatches, I think we could introduce 'struct static_key_false'
and 'struct static_key_true' with proper initializers. However,
'static_key_slow_inc()/dec()'  would also have to add the
true/false modifier. Or maybe we do:

struct static_key_false {
    struct static_key key;
} random_key;

and then the 'static_key_sloc_inc()/dec()' would just take
a &random_key.key....

If we were to change this, I don't think it would be too hard to
introduce the new API, convert subsystems over time and then
drop the old one.

Thanks,

-Jason


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/