| 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: <87tsyf58ro.fsf@oracle.com>
Date: Thu, 27 Nov 2025 15:57:15 -0800
From: Ankur Arora <ankur.a.arora@...cle.com>
To: Ankur Arora <ankur.a.arora@...cle.com>
Cc: "David Hildenbrand (Red Hat)" <david@...nel.org>,
"Christophe Leroy (CS
GROUP)" <chleroy@...nel.org>,
linux-kernel@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org,
akpm@...ux-foundation.org, bp@...en8.de, dave.hansen@...ux.intel.com,
hpa@...or.com, mingo@...hat.com, mjguzik@...il.com, luto@...nel.org,
peterz@...radead.org, tglx@...utronix.de, willy@...radead.org,
raghavendra.kt@....com, boris.ostrovsky@...cle.com,
konrad.wilk@...cle.com
Subject: Re: [PATCH v9 1/7] treewide: provide a generic clear_user_page()
variant
Ankur Arora <ankur.a.arora@...cle.com> writes:
> David Hildenbrand (Red Hat) <david@...nel.org> writes:
>
>> On 11/23/25 12:53, Christophe Leroy (CS GROUP) wrote:
>>>
>>> Le 21/11/2025 à 21:23, Ankur Arora a écrit :
>>>> From: David Hildenbrand <david@...hat.com>
>>>>
>>>> Let's drop all variants that effectively map to clear_page() and
>>>> provide it in a generic variant instead.
>>>>
>>>> We'll use the macro clear_user_page to indicate whether an architecture
>>>> provides it's own variant.
>>>>
>>>> We have to be a bit careful if an architecture provides a custom
>>>> clear_user_highpage(), because then it's very likely that some special
>>>> flushing magic is happening behind the scenes.
>>>>
>>>> Maybe at some point these should be CONFIG_ options.
>>>>
>>>> Note that for parisc, clear_page() and clear_user_page() map to
>>>> clear_page_asm(), so we can just get rid of the custom clear_user_page()
>>>> implementation. There is a clear_user_page_asm() function on parisc,
>>>> that seems to be unused. Not sure what's up with that.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@...hat.com>
>>>> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.com>
>>> ...
>>>
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index 7c79b3369b82..6fa6c188f99a 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -3879,6 +3879,28 @@ static inline void clear_page_guard(struct zone *zone, struct page *page,
>>>> unsigned int order) {}
>>>> #endif /* CONFIG_DEBUG_PAGEALLOC */
>>>> +#ifndef clear_user_page
>>>> +/**
>>>> + * clear_user_page() - clear a page to be mapped to user space
>>>> + * @addr: the address of the page
>>>> + * @vaddr: the address of the user mapping
>>>> + * @page: the page
>>>> + */
>>>> +static inline void clear_user_page(void *addr, unsigned long vaddr, struct page *page)
>>>> +{
>>>> +#ifdef clear_user_highpage
>>>> + /*
>>>> + * If an architecture defines its own clear_user_highpage() variant,
>>>> + * then we have to be a bit more careful here and cannot simply
>>>> + * rely on clear_page().
>>>> + */
>>>> + clear_user_highpage(page, vaddr);
>>>> +#else
>>>> + clear_page(addr);
>>>> +#endif
>>>> +}
>>>> +#endif
>>>> +
>>>> #ifdef __HAVE_ARCH_GATE_AREA
>>>> extern struct vm_area_struct *get_gate_vma(struct mm_struct *mm);
>>>> extern int in_gate_area_no_mm(unsigned long addr);
>>>
>>> Isn't it chicken and egg with clear_user_highpage() in linux/highmem.h ? :
>>
>> No really, because we make use of clear_user_highpage() only when the arch
>> defines it, so the highmem.h variant is ignored?
>>
>> Not that I particularly enjoy this way of handling it, so something cleaner
>> would be nice :)
>>
>> (in particular, relying on highmem.h defines in mm.h is a bit suboptimal)
>>
>>> #ifndef clear_user_highpage
>>> static inline void clear_user_highpage(struct page *page, unsigned long
>>> vaddr)
>>> {
>>> void *addr = kmap_local_page(page);
>>> clear_user_page(addr, vaddr, page);
>>> kunmap_local(addr);
>>> }
>>> #endif
>>> And at the end this function is the only caller of clear_user_page() so
>>> there is apparently no need for a generic clear_user_page(), at least
>>> not when clear_user_highpage() is defined.
>>> I think is would be simpler and cleaner to instead add the following in
>>> linux/highmem.c:
>>
>> I assume you mean highmem.h
>>
>> It's not really highmem.h material, but if it makes things cleaner, sure.
>>
>> Might be that the compiler will not be happy about that.
>>
>> @Ankur can you play with that and see if we can make compilers happy one way or
>> the other?
>
> This looks like a good change (and from my tests on a couple of
> configs seems to build fine.)
>
> Though clear_user_pages() is also only called from clear_user_highpages().
> Would be good to treat that similarly.
>
> (I don't think it can be done because how the arch code treats
> both quite differently.)
>
> Anyway I'll play with that a bit more.
How about something like this for clear_user_page() (though maybe I
should be always defining clear_user_page() and not conditioning it on
the existence of the generic clear_user_highpage()):
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index abc20f9810fd..ca9d28aa29b2 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -197,6 +197,22 @@ static inline void invalidate_kernel_vmap_range(void *vaddr, int size)
}
#endif
+#if !defined(clear_user_page) && !defined(clear_user_highpage)
+/**
+ * clear_user_page() - clear a page to be mapped to user space
+ * @addr: the address of the page
+ * @vaddr: the address of the user mapping
+ * @page: the page
+ *
+ * The sole user of clear_user_page() is clear_user_highpage().
+ * Define it if the arch does not and only if needed.
+ */
+static inline void clear_user_page(void *addr, unsigned long vaddr, struct page *page)
+{
+ clear_page(addr);
+}
+#endif
+
/* when CONFIG_HIGHMEM is not set these will be plain clear/copy_page */
#ifndef clear_user_highpage
static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
And for clear_user_pages():
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index ca9d28aa29b2..b9b3cc76a91a 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -223,6 +223,35 @@ static inline void clear_user_highpage(struct page *page, unsigned long vaddr)
}
#endif
+/**
+ * clear_user_pages() - clear a page range to be mapped to user space
+ * @addr: start address
+ * @vaddr: start address of the user mapping
+ * @page: start page
+ * @npages: number of pages
+ *
+ * Assumes that the region (@addr, +@...ges) has been validated
+ * already so this does no exception handling.
+ */
+static inline void clear_user_pages(void *addr, unsigned long vaddr,
+ struct page *page, unsigned int npages)
+{
+#ifdef clear_user_page
+ do {
+ clear_user_page(addr, vaddr, page);
+ addr += PAGE_SIZE;
+ vaddr += PAGE_SIZE;
+ page++;
+ } while (--npages);
+#else
+ /*
+ * Prefer clear_pages() to allow for architectural optimizations
+ * when operations on contiguous page ranges.
+ */
+ clear_pages(addr, npages);
+#endif
+}
+
Tree with changes: github.com/terminus/linux clear-pages.v10-rc2
This seems to be fine on a variety of configurations.
David, could you take a quick look at it?
Thanks
--
ankur
Powered by blists - more mailing lists