linux-kernel - Re: [PATCH v9 1/7] treewide: provide a generic clear_user

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6d2605e8-409d-4b4a-9729-599dd2526b74@kernel.org>
Date: Fri, 28 Nov 2025 08:39:44 +0100
From: "Christophe Leroy (CS GROUP)" <chleroy@...nel.org>
To: Ankur Arora <ankur.a.arora@...cle.com>
Cc: "David Hildenbrand (Red Hat)" <david@...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

Hi Ankur,

Le 28/11/2025 à 00:57, Ankur Arora a écrit :
> 
> Ankur Arora <ankur.a.arora@...cle.com> writes:
> 
> 
> 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

WOuld be more obvious if you enclose that inside the same #ifdef as 
clear_user_highpage(), something like:

#ifndef clear_user_highpage

#ifndef clear_user_page
static inline void clear_user_page(void *addr, unsigned long vaddr, 
struct page *page)
{
	clear_page(addr);
}
#endif

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

> +
>   /* 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
> +}

Not sure to understand the logic. You say this is not expected to be 
overriden by architectures in the near future, then why do we need that 
? Can't we do everything inside clear_user_highpages() for clarity ?

At the time being clear_user_page() is used exclusively by 
clear_user_highpage() so I expect clear_user_page() to only exist when 
CONFIG_HIGHMEM is enabled. And in that case clear_user_highpages() 
doesn't call clear_user_pages() so at the end only the else part 
remains, which is a simple call to clear_pages(). Why not just call 
clear_pages() directly from clear_user_highpages() and drop 
clear_user_pages() ?

Christophe