Re: [RFC PATCH 1/3] mm: make persistent huge zero folio read-only

[Dave Hansen]

On 5/26/26 20:56, Xueyuan chen wrote:> +#ifdef
CONFIG_READONLY_HUGE_ZERO_FOLIO
> +bool __init arch_make_huge_zero_folio_readonly(struct folio *folio);
> +#endif

All of the #ifdeffery needs to die, IMNHO.

This function is also a bad idea. There is nothing "huge zero" specific
about it. It takes any old folio and tries to make it read only.

Just make it:

	bool __init arch_make_folio_readonly(struct folio *folio)

Make it a weak symbol with a stub in some mm/foo.c file, and then the
architectures can override it if they want.

>  static inline struct folio *get_persistent_huge_zero_folio(void)
>  {
>  	if (!IS_ENABLED(CONFIG_PERSISTENT_HUGE_ZERO_FOLIO))
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 776b67c66e82..f31200816646 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -787,6 +787,23 @@ config PERSISTENT_HUGE_ZERO_FOLIO
>  	  Say Y if your system has lots of memory. Say N if you are
>  	  memory constrained.
>  
> +config ARCH_HAS_READONLY_HUGE_ZERO_FOLIO
> +	bool
> +
> +config READONLY_HUGE_ZERO_FOLIO
> +	bool "Map the huge zero folio read-only in the direct map"
> +	depends on PERSISTENT_HUGE_ZERO_FOLIO
> +	depends on ARCH_HAS_READONLY_HUGE_ZERO_FOLIO
> +	help
> +	  The persistent huge zero folio is shared globally, and nothing
> +	  should ever change its contents after initialization.
> +
> +	  When supported, mark the folio read-only in the direct map so such
> +	  writes trigger a fault instead of silently corrupting the zero contents.
> +
> +	  If the permission change is not supported, the kernel keeps using
> +	  the writable persistent huge zero folio.

I vote for no Kconfig options here. Why? This adds "security" with
_basically_ no extra runtime cost. The runtime cost is, what, usually
one kernel TLB invalidation during boot?

If you desperately need one, you can add it without a prompt so folks
need to edit .config files to change the values.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index bf9b480bb3b0..c568755dd58e 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -75,7 +75,11 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>  static bool split_underused_thp = true;
>  
>  static atomic_t huge_zero_refcount;
> +#ifdef CONFIG_READONLY_HUGE_ZERO_FOLIO
> +struct folio *huge_zero_folio __ro_after_init;
> +#else
>  struct folio *huge_zero_folio __read_mostly;
> +#endif

#ifdefs bad. Bad. Bad. Bad. ;)

Seriously, just pick one. Lowest common denominator is more important
that being precisely correct all the time.

>  unsigned long huge_zero_pfn __read_mostly = ~0UL;
>  unsigned long huge_anon_orders_always __read_mostly;
>  unsigned long huge_anon_orders_madvise __read_mostly;
> @@ -305,6 +309,18 @@ static unsigned long shrink_huge_zero_folio_scan(struct shrinker *shrink,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_READONLY_HUGE_ZERO_FOLIO
> +static bool __init make_huge_zero_folio_readonly(void)
> +{
> +	return arch_make_huge_zero_folio_readonly(READ_ONCE(huge_zero_folio));
> +}
> +#else
> +static bool __init make_huge_zero_folio_readonly(void)
> +{
> +	return false;
> +}
> +#endif
> +
>  static struct shrinker *huge_zero_folio_shrinker;
>  
>  #ifdef CONFIG_SYSFS
> @@ -965,8 +981,15 @@ static int __init thp_shrinker_init(void)
>  		 * that get_huge_zero_folio() will most likely not fail as
>  		 * thp_shrinker_init() is invoked early on during boot.
>  		 */
> -		if (!get_huge_zero_folio())
> +		if (!get_huge_zero_folio()) {
>  			pr_warn("Allocating persistent huge zero folio failed\n");
> +			return 0;
> +		}
> +
> +		if (IS_ENABLED(CONFIG_READONLY_HUGE_ZERO_FOLIO) &&
> +		    !make_huge_zero_folio_readonly())
> +			pr_warn("Making persistent huge zero folio read-only failed\n");
> +
>  		return 0;

The IS_ENABLED() doesn't do anything from what I can tell. This is all
in one .c file. The compiler can see that
make_huge_zero_folio_readonly() is always false. It should optimize out
the pr_warn() for you.