linux-kernel - Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask

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: <24253442-9878-c5ed-cbaa-05a5c4851a6e@csgroup.eu>
Date:   Thu, 1 Sep 2022 07:47:10 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Gabriel Paubert <paubert@...m.es>
CC:     Segher Boessenkool <segher@...nel.crashing.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Zhouyi Zhou <zhouzhouyi@...il.com>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nicholas Piggin <npiggin@...il.com>
Subject: Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and
 irq_soft_mask_return() with sanitizer



Le 01/09/2022 à 09:37, Gabriel Paubert a écrit :
> On Thu, Sep 01, 2022 at 05:22:32AM +0000, Christophe Leroy wrote:
>>
>>
>> Le 01/09/2022 à 00:45, Segher Boessenkool a écrit :
>>> Hi!
>>>
>>> On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
>>>> Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
>>>>> On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
>>>>>>> This is still slightly concerning to me. Is there any guarantee that the
>>>>>>> compiler would not use a different sequence for the address here?
>>>>>>>
>>>>>>> Maybe explicit r13 is required.
>>>>>>>
>>>>>>
>>>>>> local_paca is defined as:
>>>>>>
>>>>>> 	register struct paca_struct *local_paca asm("r13");
>>>
>>> And this is in global scope, making it a global register variable.
>>>
>>>>>> Why would the compiler use another register ?
>>>>>
>>>>> Hopefully it doesn't. Is it guaranteed that it won't?
>>>
>>> Yes, this is guaranteed.
>>>
>>> For a local register variable this is guaranteed only for operands to an
>>> extended inline asm; any other access to the variable does not have to
>>> put it in the specified register.
>>>
>>> But this is a global register variable.  The only thing that would make
>>> this crash and burn is if *any* translation unit did not see this
>>> declaration: it could then use r13 (if that was allowed by the ABI in
>>> effect, heh).
>>>
>>>>> I'm sure Segher will be delighted with the creative asm in __do_IRQ
>>>>> and call_do_irq :) *Grabs popcorn*
>>>
>>> All that %% is blinding, yes.
>>>
>>> Inline tabs are bad taste.
>>>
>>> Operand names instead of numbers are great for obfuscation, and nothing
>>> else -- unless you have more than four or five operands, in which case
>>> you have bigger problems already.
>>>
>>> Oh, this function is a good example of proper use of local register asm,
>>> btw.
>>>
>>> Comments like "// Inputs" are just harmful.  As is the "creative"
>>> indentation here.  Both harm readability and do not help understanding
>>> in any other way either.
>>>
>>> The thing about inline asm is the smallest details change meaning of the
>>> whole, it is a very harsh environment, you are programming both in C and
>>> directly assembler code as well, and things have to be valid for both,
>>> although on the other hand there is almost no error checking.  Keeping
>>> it small, simple, readable is paramount.
>>>
>>> The rules for using inline asm:
>>>
>>> 0) Do no use inline asm.
>>> 1) Use extended asm, unless you know all differences with basic asm, and
>>>      you know you want that.  And if you answer "yes I do" to the latter,
>>>      you answered wrong to the former.
>>> 2) Do not use toplevel asm.
>>> 3) Do no use inline asm.
>>> 4) Do no use inline asm.
>>> 5) Do no use inline asm.
>>>
>>> Inline asm is a very powerful escape hatch.  Like all emergency exits,
>>> you should not use them if you do not need them!  :-)
>>>
>>> But, you are talking about the function calling and the frame change I
>>> bet :-)  Both of these are only okay because everything is back as it
>>> was when this (single!) asm is done, and the state created is perfectly
>>> fine (this is very dependent on exact ABI used, etc.)
>>>
>>> I would have used real assembler code here (in a .s file).  But there
>>> probably are reasons to do things this way, performance probably?
>>
>> We changed it to inline asm in order to ... inline it in the caller.
> 
> And there is a single caller it seems. Typically GCC inlines function
> with a single call site, but it may be confused by asm statements.

Regardless, it is tagged __always_inline.

> 
>>
>> I also find that those operand names make it awull more difficult to
>> read that traditional numbering. I really dislike that new trend.
>> And same with those // comments, better use meaningfull C variable names.
> 
> Agree, but there is one thing which escapes me: why is r3 listed in the
> outputs section (actually as a read write operand with the "+"
> constraint modifier) but is not used after the asm which is the last
> statement of function returning void?
> 
> Do I miss something?

As far as I remember, that's to tell GCC that r3 register is modified by 
the callee. As it is an input, it couldn't be listed in the clobber list.

Christophe