Christophe Leroy's on June 7, 2019 3:34 pm:
>
>
> Le 07/06/2019 à 05:56, Nicholas Piggin a écrit :
>> Commit 1b2443a547f9 ("powerpc/book3s64: Avoid multiple endian conversion
>> in pte helpers") changed the actual bitwise tests in pte_access_permitted
>> by using pte_write() and pte_present() helpers rather than raw bitwise
>> testing _PAGE_WRITE and _PAGE_PRESENT bits.
>>
>> The pte_present change now returns true for ptes which are !_PAGE_PRESENT
>> and _PAGE_INVALID, which is the combination used by pmdp_invalidate to
>> synchronize access from lock-free lookups. pte_access_permitted is used by
>> pmd_access_permitted, so allowing GUP lock free access to proceed with
>> such PTEs breaks this synchronisation.
>>
>> This bug has been observed on HPT host, with random crashes and corruption
>> in guests, usually together with bad PMD messages in the host.
>>
>> Fix this by adding an explicit check in pmd_access_permitted, and
>> documenting the condition explicitly.
>>
>> The pte_write() change should be okay, and would prevent GUP from falling
>> back to the slow path when encountering savedwrite ptes, which matches
>> what x86 (that does not implement savedwrite) does.
>>
>> Fixes: 1b2443a547f9 ("powerpc/book3s64: Avoid multiple endian conversion in
>> pte helpers")
>> Cc: Aneesh Kumar K.V <aneesh.ku...@linux.ibm.com>
>> Cc: Christophe Leroy <christophe.le...@c-s.fr>
>> Signed-off-by: Nicholas Piggin <npig...@gmail.com>
>> ---
>>
>> I accounted for Aneesh's and Christophe's feedback, except I couldn't
>> find a good way to replace the ifdef with IS_ENABLED because of
>> _PAGE_INVALID etc., but at least cleaned that up a bit nicer.
>
> I guess the standard way is to add a pmd_is_serializing() which return
> always false in book3s/32/pgtable.h and in nohash/pgtable.h
>
>>
>> Patch 1 solves a problem I can hit quite reliably running HPT/HPT KVM.
>> Patch 2 was noticed by Aneesh when inspecting code for similar bugs.
>> They should probably both be merged in stable kernels after upstream.
>>
>> arch/powerpc/include/asm/book3s/64/pgtable.h | 30 ++++++++++++++++++++
>> arch/powerpc/mm/book3s64/pgtable.c | 3 ++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> index 7dede2e34b70..ccf00a8b98c6 100644
>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> @@ -876,6 +876,23 @@ static inline int pmd_present(pmd_t pmd)
>> return false;
>> }
>>
>> +static inline int pmd_is_serializing(pmd_t pmd)
>
> should be static inline bool instead of int ?
I think just about all the p?d_blah boolean functions in the tree are
int at the moment, so I followed that pattern.
May be a good tree wide change to make at some point.
Thanks,
Nick
