On 20/12/16 20:32, Benjamin Herrenschmidt wrote:
> On Tue, 2016-12-20 at 20:03 +1100, Balbir Singh wrote:
>>
>> On 19/12/16 19:06, Madhavan Srinivasan wrote:
>>> Move set_soft_enabled() from powerpc/kernel/irq.c to
>>> asm/hw_irq.c, to force updates to paca-soft_enabled
>>> done via these access function. Add "memory" clobber
>>> to hint compiler since paca->soft_enabled memory is the target
>>> here
>>> +static inline notrace void soft_enabled_set(unsigned long enable)
>>> +{
>>> + __asm__ __volatile__("stb %0,%1(13)"
>>> + : : "r" (enable), "i" (offsetof(struct paca_struct,
>>> soft_enabled))
>>> + : "memory");
>>> +}
>>> +
>>
>> Can't we just rewrite this in "C"?
>>
>> local_paca->soft_enabled = enable
>
> Well, this is digging out another bloody can of baby eating worms...
>
> So the reason we wrote it like that is because we had gcc playing
> tricks to us.
>
> It has been proved to move around references to local_paca, even
> accross preempt_disable boundaries.
>
> The above prevents it.
>
> I'm about 100% sure we have a bunch of other issues related to PACA
> access and gcc playing games though. I remember grepping the kernel
> disassembly once for gcc doing funny things with r13 such as copying it
> into another register or even saving and restoring it and my grep
> didn't come up empty ...
>
I checked for arch_local_irq_restore before suggesting it, not all
functions. I tried a quick audit of disassembly not using load/stores
on r13 and most of it was in hand written disassembly in the exception
handling paths, secondary processor initialization, wake up from idle.
Having said that I audited with one version of gcc on a little endian
vmlinux.
> Something we need to seriously look into one day. I have some ideas on
> how to clean that up in the code to make hardening it easier, let's
> talk next time Nick is in town.
>
Sure lets leave it as is for now
Balbir
