| 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
| ||
|
Message-ID: <acc75c62-b919-12da-25b4-2b65ecf89ab6@linux.alibaba.com>
Date: Fri, 10 Dec 2021 12:48:17 +0800
From: Yu Xu <xuyu@...ux.alibaba.com>
To: Ankur Arora <ankur.a.arora@...cle.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org,
mingo@...nel.org, bp@...en8.de, luto@...nel.org,
akpm@...ux-foundation.org, mike.kravetz@...cle.com,
jon.grimm@....com, kvm@...r.kernel.org, konrad.wilk@...cle.com,
boris.ostrovsky@...cle.com
Subject: Re: [PATCH v2 07/14] x86/clear_page: add clear_page_uncached()
On 12/10/21 12:37 PM, Ankur Arora wrote:
>
> Yu Xu <xuyu@...ux.alibaba.com> writes:
>
>> On 10/21/21 1:02 AM, Ankur Arora wrote:
>>> Expose the low-level uncached primitives (clear_page_movnt(),
>>> clear_page_clzero()) as alternatives via clear_page_uncached().
>>> Also fallback to clear_page(), if X86_FEATURE_MOVNT_SLOW is set
>>> and the CPU does not have X86_FEATURE_CLZERO.
>>> Both the uncached primitives use stores which are weakly ordered
>>> with respect to other instructions accessing the memory hierarchy.
>>> To ensure that callers don't mix accesses to different types of
>>> address_spaces, annotate clear_user_page_uncached(), and
>>> clear_page_uncached() as taking __incoherent pointers as arguments.
>>> Also add clear_page_uncached_make_coherent() which provides the
>>> necessary store fence to flush out the uncached regions.
>>> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.com>
>>> ---
>>> Notes:
>>> This patch adds the fallback definitions of clear_user_page_uncached()
>>> etc in include/linux/mm.h which is likely not the right place for it.
>>> I'm guessing these should be moved to include/asm-generic/page.h
>>> (or maybe a new include/asm-generic/page_uncached.h) and for
>>> architectures that do have arch/$arch/include/asm/page.h (which
>>> seems like all of them), also replicate there?
>>> Anyway, wanted to first check if that's the way to do it, before
>>> doing that.
>>> arch/x86/include/asm/page.h | 10 ++++++++++
>>> arch/x86/include/asm/page_32.h | 9 +++++++++
>>> arch/x86/include/asm/page_64.h | 32 ++++++++++++++++++++++++++++++++
>>> include/linux/mm.h | 14 ++++++++++++++
>>> 4 files changed, 65 insertions(+)
>>> diff --git a/arch/x86/include/asm/page_32.h b/arch/x86/include/asm/page_32.h
>>> index 94dbd51df58f..163be03ac422 100644
>>> --- a/arch/x86/include/asm/page_32.h
>>> +++ b/arch/x86/include/asm/page_32.h
>>> @@ -39,6 +39,15 @@ static inline void clear_page(void *page)
>>> memset(page, 0, PAGE_SIZE);
>>> }
>>> +static inline void clear_page_uncached(__incoherent void *page)
>>> +{
>>> + clear_page((__force void *) page);
>>> +}
>>> +
>>> +static inline void clear_page_uncached_make_coherent(void)
>>> +{
>>> +}
>>> +
>>> static inline void copy_page(void *to, void *from)
>>> {
>>> memcpy(to, from, PAGE_SIZE);
>>> diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
>>> index 3c53f8ef8818..d7946047c70f 100644
>>> --- a/arch/x86/include/asm/page_64.h
>>> +++ b/arch/x86/include/asm/page_64.h
>>> @@ -56,6 +56,38 @@ static inline void clear_page(void *page)
>>> : "cc", "memory", "rax", "rcx");
>>> }
>>> +/*
>>> + * clear_page_uncached: only allowed on __incoherent memory regions.
>>> + */
>>> +static inline void clear_page_uncached(__incoherent void *page)
>>> +{
>>> + alternative_call_2(clear_page_movnt,
>>> + clear_page, X86_FEATURE_MOVNT_SLOW,
>>> + clear_page_clzero, X86_FEATURE_CLZERO,
>>> + "=D" (page),
>>> + "0" (page)
>>> + : "cc", "memory", "rax", "rcx");
>>> +}
>>> +
>>> +/*
>>> + * clear_page_uncached_make_coherent: executes the necessary store
>>> + * fence after which __incoherent regions can be safely accessed.
>>> + */
>>> +static inline void clear_page_uncached_make_coherent(void)
>>> +{
>>> + /*
>>> + * Keep the sfence for oldinstr and clzero separate to guard against
>>> + * the possibility that a cpu-model both has X86_FEATURE_MOVNT_SLOW
>>> + * and X86_FEATURE_CLZERO.
>>> + *
>>> + * The alternatives need to be in the same order as the ones
>>> + * in clear_page_uncached().
>>> + */
>>> + alternative_2("sfence",
>>> + "", X86_FEATURE_MOVNT_SLOW,
>>> + "sfence", X86_FEATURE_CLZERO);
>>> +}
>>> +
>>> void copy_page(void *to, void *from);
>>> #ifdef CONFIG_X86_5LEVEL
>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>> index 73a52aba448f..b88069d1116c 100644
>>> --- a/include/linux/mm.h
>>> +++ b/include/linux/mm.h
>>> @@ -3192,6 +3192,20 @@ static inline bool vma_is_special_huge(const struct vm_area_struct *vma)
>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_HUGETLBFS */
>>> +#ifndef clear_user_page_uncached
>>
>> Hi Ankur Arora,
>>
>> I've been looking for where clear_user_page_uncached is defined in this
>> patchset, but failed.
>>
>> There should be something like follows in arch/x86, right?
>>
>> static inline void clear_user_page_uncached(__incoherent void *page,
>> unsigned long vaddr, struct page *pg)
>> {
>> clear_page_uncached(page);
>> }
>>
>>
>> Did I miss something?
>>
> Hi Yu Xu,
>
> Defined in include/linux/mm.h. Just below :).
Thanks for your reply :)
This is the version when #ifndef clear_user_page_uncached, i.e., fall
back to standard clear_user_page.
But where is the uncached version of clear_user_page? I am looking for
this.
>
>>> +/*
>>> + * clear_user_page_uncached: fallback to the standard clear_user_page().
>>> + */
>>> +static inline void clear_user_page_uncached(__incoherent void *page,
>>> + unsigned long vaddr, struct page *pg)
>>> +{
>>> + clear_user_page((__force void *)page, vaddr, pg);
>>> +}
>
> That said, as this note in the patch mentions, this isn't really a great
> place for this definition. As you also mention, the right place for this
> would be somewhere in the arch/.../include and include/asm-generic hierarchy.
>
>>> This patch adds the fallback definitions of clear_user_page_uncached()
>>> etc in include/linux/mm.h which is likely not the right place for it.
>>> I'm guessing these should be moved to include/asm-generic/page.h
>>> (or maybe a new include/asm-generic/page_uncached.h) and for
>>> architectures that do have arch/$arch/include/asm/page.h (which
>>> seems like all of them), also replicate there?
>>> Anyway, wanted to first check if that's the way to do it, before
>>> doing that.
>
> Recommendations on how to handle this, welcome.
>
> Thanks
>
> --
> ankur
>
--
Thanks,
Yu
Powered by blists - more mailing lists