Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] powerpc/32s: Fix random crashes by adding isync() after
 locking/unlocking KUEP
To:     Segher Boessenkool <segher@kernel.crashing.org>
Cc:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Michael Ellerman <mpe@ellerman.id.au>, userm57@yahoo.com,
        fthain@linux-m68k.org, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
References: <1d28441dd80845e6428d693c0724cb6457247466.1629211378.git.christophe.leroy@csgroup.eu>
 <20210817162239.GF1583@gate.crashing.org>
From:   Christophe Leroy <christophe.leroy@csgroup.eu>
Message-ID: <0426a0d3-bdc6-1a34-1018-71b34282a6c6@csgroup.eu>
Date:   Tue, 17 Aug 2021 19:13:44 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <20210817162239.GF1583@gate.crashing.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Precedence: bulk


Le 17/08/2021 à 18:22, Segher Boessenkool a écrit :
> On Tue, Aug 17, 2021 at 02:43:15PM +0000, Christophe Leroy wrote:
>> Commit b5efec00b671 ("powerpc/32s: Move KUEP locking/unlocking in C")
>> removed the 'isync' instruction after adding/removing NX bit in user
>> segments. The reasoning behind this change was that when setting the
>> NX bit we don't mind it taking effect with delay as the kernel never
>> executes text from userspace, and when clearing the NX bit this is
>> to return to userspace and then the 'rfi' should synchronise the
>> context.
>>
>> However, it looks like on book3s/32 having a hash page table, at least
>> on the G3 processor, we get an unexpected fault from userspace, then
>> this is followed by something wrong in the verification of MSR_PR
>> at end of another interrupt.
>>
>> This is fixed by adding back the removed isync() following update
>> of NX bit in user segment registers. Only do it for cores with an
>> hash table, as 603 cores don't exhibit that problem and the two isync
>> increase ./null_syscall selftest by 6 cycles on an MPC 832x.
>>
>> First problem: unexpected PROTFAULT
>>
>> 	[   62.896426] WARNING: CPU: 0 PID: 1660 at arch/powerpc/mm/fault.c:354 do_page_fault+0x6c/0x5b0
>> 	[   62.918111] Modules linked in:
>> 	[   62.923350] CPU: 0 PID: 1660 Comm: Xorg Not tainted 5.13.0-pmac-00028-gb3c15b60339a #40
>> 	[   62.943476] NIP:  c001b5c8 LR: c001b6f8 CTR: 00000000
>> 	[   62.954714] REGS: e2d09e40 TRAP: 0700   Not tainted  (5.13.0-pmac-00028-gb3c15b60339a)
> 
> That is not a protection fault.  What causes this?

That's the WARN_ON(error_code & DSISR_PROTFAULT) at

https://elixir.bootlin.com/linux/v5.13/source/arch/powerpc/mm/fault.c#L354


> 
> A CSI (like isync) is required both before and after mtsr.  It may work
> on some cores without -- what part of that is luck, if there is anything
> that guarantees it, is anyone's guess :-/

kuep_lock() is called when entering interrupts, it means we recently got an 'rfi' to re-enable MMU.
kuep_unlock() is called when exit interrupts, it means we are soon going to call 'rfi' to go back to 
user.

In between, nobody is going to exec any userspace code, so who minds that the 'mtsr' changing user 
segments is not completely finished ?

> 
>> @@ -28,6 +30,8 @@ static inline void kuep_lock(void)
>>   		return;
>>   
>>   	update_user_segments(mfsr(0) | SR_NX);
>> +	if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
>> +		isync();	/* Context sync required after mtsr() */
>>   }
> 
> This needs a comment why you are not doing this for systems without
> hardware page table walk, at the least?

Ok, will add a comment tomorrow.

Christophe