[PATCH v7 0/3] mm: make persistent huge zero folio read-only

[PATCH v7 0/3] mm: make persistent huge zero folio read-only

[Xueyuan Chen]

The persistent huge zero folio is shared globally and must remain zero
after initialization. As Jann Horn pointed out [1], kernel bugs can write
to pages that are intended to be read-only, including in security-sensitive
paths. Protecting the folio's direct-map mapping turns such writes into
faults instead of silently corrupting the shared zero page.

This series makes that protection available to MM code and applies it to
the persistent huge zero folio. It is best-effort: arm64 and x86 protect
the permanent direct-map mapping, while highmem folios and architectures
without support retain the existing behavior.

The interface remains page-based to match the existing direct-map helpers.
The wider address-based conversion discussed in [2] can be handled
separately. Permission changes and TLB invalidation stay together in the
architecture code, avoiding a noflush interface that is easy to misuse.

Patches 2 and 3 add arm64 and x86 support. The approach follows the
huge-zero-folio discussion in [3].

Link: https://lore.kernel.org/linux-mm/20260508-ro-zeropage-v1-1-9808abc20b49@google.com/ [1]
Link: https://lore.kernel.org/linux-mm/0e5b23a6-4895-454a-9dfa-6dc21adc2991@kernel.org/ [2]
Link: https://lore.kernel.org/linux-mm/CAHbLzkrXXe7r3n3jXgDKtwZhRqj=jDx9E6dLOULohnhBguvi9A@mail.gmail.com/ [3]

v6 -> v7:
- Rebase onto the latest mm-unstable and adapt to huge_zero_init()
  (per David).
- Switch to a page-based interface and move TLB flushing into the
  architecture implementations (per Will, Dave and David).
- https://lore.kernel.org/r/20260730090647.2401252-1-xueyuan.chen21@gmail.com/

v5 -> v6:
- Patch #01: Skip the direct-map permission change and TLB flush for
  highmem folios, which have no permanent direct-map mapping.
- https://lore.kernel.org/all/20260727143426.1077133-1-xueyuan.chen21@gmail.com/

RFC v4 -> v5:
- Drop the RFC tag.
- No code changes.
- https://lore.kernel.org/all/20260718095647.182592-1-xueyuan.chen21@gmail.com/

RFC v3 -> RFC v4:
- Patch #01: Flush the direct-map range after changing it read-only, since
  the folio was cleared through writable mappings after SMP initialization
  (per Usama, thanks!).
- Patch #01: Keep the flush in the caller to preserve the
  set_direct_map_ro_noflush() contract and make the flushed range explicit.
- Patch #01: Clarify the noflush API contract and the reason stale writable
  translations must be invalidated.
- https://lore.kernel.org/linux-mm/20260706130440.9295-1-xueyuan.chen21@gmail.com/

RFC v2 -> RFC v3:
- Patch #01: Replace arch_make_pages_readonly() with
  set_direct_map_ro_noflush() in the existing set_direct_map* family
  (per Mike and David, thanks!).
- Patch #01: Use a direct-map address and number of pages, and document the
  direct-map-only and no-TLB-flush semantics (per David, thanks!).
- Patch #02 and #03: Update the arm64 and x86 implementations for
  set_direct_map_ro_noflush().
- https://lore.kernel.org/linux-mm/20260609143801.7917-1-xueyuan.chen21@gmail.com/

RFC v1 -> RFC v2:
- Patch #01: Drop the READONLY_HUGE_ZERO_FOLIO Kconfig option
  (per Dave, thanks!).
- Patch #01: Replace the huge-zero-folio-specific hook with a generic
  page-range hook (per David, thanks!).
- Patch #02 and #03: Update the arm64 and x86 implementations for the new
  hook.
- https://lore.kernel.org/linux-mm/20260527035607.14919-1-xueyuan.chen21@gmail.com/

Xueyuan Chen (3):
  mm: make persistent huge zero folio read-only
  arm64/mm: add set_direct_map_ro()
  x86/mm: add set_direct_map_ro()

 arch/arm64/include/asm/set_memory.h |  2 ++
 arch/arm64/mm/pageattr.c            | 12 ++++++++++++
 arch/x86/include/asm/set_memory.h   |  2 ++
 arch/x86/mm/pat/set_memory.c        | 10 ++++++++++
 include/linux/set_memory.h          | 17 +++++++++++++++++
 mm/huge_memory.c                    | 13 ++++++++++---
 6 files changed, 53 insertions(+), 3 deletions(-)


base-commit: 88297631d4d42f6004cb39c0ba3da7d2d10a616f
-- 
2.47.3

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

[Xueyuan Chen]

The persistent huge zero folio is shared globally and should stay zero
after initialization. As Jann Horn pointed out [1], kernel bugs have
ended up writing to pages that were meant to be read-only, including in
security-sensitive cases. Making the folio read-only in the direct map
turns such writes into faults instead of silent zero-page corruption.

Add a page-based helper consistent with the existing direct-map interfaces.
Handle TLB invalidation in the architecture implementation; unsupported
architectures retain their current behavior.

Protect the folio after initialization. Skip highmem folios, which have no
permanent direct-map mapping.

Inspired by Jann Horn's read-only zero page work [1] and follow-up
discussion [3] with Yang Shi.

Link: https://lore.kernel.org/r/20260508-ro-zeropage-v1-1-9808abc20b49@google.com [1]
Link: https://lore.kernel.org/r/0e5b23a6-4895-454a-9dfa-6dc21adc2991@kernel.org [2]
Link: https://lore.kernel.org/r/CAHbLzkrXXe7r3n3jXgDKtwZhRqj=jDx9E6dLOULohnhBguvi9A@mail.gmail.com [3]

Suggested-by: David Hildenbrand <david@kernel.org>
Suggested-by: Usama Arif <usama.arif@linux.dev>
Co-developed-by: Lance Yang <lance.yang@linux.dev>
Signed-off-by: Lance Yang <lance.yang@linux.dev>
Signed-off-by: Xueyuan Chen <xueyuan.chen21@gmail.com>
---
 include/linux/set_memory.h | 17 +++++++++++++++++
 mm/huge_memory.c           | 13 ++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h
index 3fe293cfed8c..ed9ce04b18a1 100644
--- a/include/linux/set_memory.h
+++ b/include/linux/set_memory.h
@@ -54,6 +54,23 @@ static inline bool can_set_direct_map(void)
 #endif
 #endif /* CONFIG_ARCH_HAS_SET_DIRECT_MAP */
 
+#ifndef set_direct_map_ro
+/**
+ * set_direct_map_ro - make a direct-map range read-only
+ * @page: first page in the direct-map range
+ * @nr: number of pages in the range
+ *
+ * Make the direct-map range starting at @page read-only and invalidate stale
+ * writable translations before returning.
+ *
+ * Return: 0 on success, or a negative error code on failure.
+ */
+static inline int set_direct_map_ro(struct page *page, unsigned int nr)
+{
+	return 0;
+}
+#endif
+
 #ifdef CONFIG_X86_64
 int set_mce_nospec(unsigned long pfn);
 int clear_mce_nospec(unsigned long pfn);
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 54494c3fa983..742283b36d74 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -42,6 +42,7 @@
 #include <linux/pgalloc_tag.h>
 #include <linux/pagewalk.h>
 #include <linux/cleanup.h>
+#include <linux/set_memory.h>
 
 #include <asm/tlb.h>
 #include "internal.h"
@@ -291,10 +292,16 @@ static int __init huge_zero_init(void)
 	huge_zero_folio = alloc_huge_zero_folio();
 	if (!huge_zero_folio) {
 		pr_warn("Allocating persistent huge zero folio failed\n");
-	} else {
-		huge_zero_pfn = folio_pfn(huge_zero_folio);
-		count_vm_event(THP_ZERO_PAGE_ALLOC);
+		return 0;
 	}
+
+	huge_zero_pfn = folio_pfn(huge_zero_folio);
+	count_vm_event(THP_ZERO_PAGE_ALLOC);
+
+	/* Highmem folios have no permanent direct-map mapping to protect. */
+	if (!folio_test_highmem(huge_zero_folio))
+		set_direct_map_ro(folio_page(huge_zero_folio, 0), HPAGE_PMD_NR);
+
 	return 0;
 }
 
-- 
2.47.3

[PATCH v7 2/3] arm64/mm: add set_direct_map_ro()

[Xueyuan Chen]

Implement set_direct_map_ro() for arm64 and flush the TLB internally.

Co-developed-by: Lance Yang <lance.yang@linux.dev>
Signed-off-by: Lance Yang <lance.yang@linux.dev>
Signed-off-by: Xueyuan Chen <xueyuan.chen21@gmail.com>
---
 arch/arm64/include/asm/set_memory.h |  2 ++
 arch/arm64/mm/pageattr.c            | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/arch/arm64/include/asm/set_memory.h b/arch/arm64/include/asm/set_memory.h
index 0091ba12200e..1be86dc11543 100644
--- a/arch/arm64/include/asm/set_memory.h
+++ b/arch/arm64/include/asm/set_memory.h
@@ -13,6 +13,8 @@ int set_memory_valid(unsigned long addr, int numpages, int enable);
 
 int set_direct_map_invalid_noflush(struct page *page, unsigned int numpages);
 int set_direct_map_default_noflush(struct page *page, unsigned int numpages);
+int set_direct_map_ro(struct page *page, unsigned int nr);
+#define set_direct_map_ro set_direct_map_ro
 bool kernel_page_present(struct page *page);
 
 int set_memory_encrypted(unsigned long addr, int numpages);
diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 132938b32eb1..0e95cd904ccb 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -355,6 +355,18 @@ int realm_register_memory_enc_ops(void)
 	return arm64_mem_crypt_ops_register(&realm_crypt_ops);
 }
 
+int set_direct_map_ro(struct page *page, unsigned int nr)
+{
+	unsigned long addr = (unsigned long)page_address(page);
+
+	if (!can_set_direct_map())
+		return 0;
+
+	return __change_memory_common(addr, PAGE_SIZE * nr,
+				      __pgprot(PTE_RDONLY),
+				      __pgprot(PTE_WRITE));
+}
+
 #ifdef CONFIG_DEBUG_PAGEALLOC
 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
-- 
2.47.3

[PATCH v7 3/3] x86/mm: add set_direct_map_ro()

[Xueyuan Chen]

Implement set_direct_map_ro() for x86 with a page-based interface. Clear
_PAGE_RW and _PAGE_DIRTY through the CPA path and let the common CPA
wrapper invalidate stale writable translations before returning.

Co-developed-by: Lance Yang <lance.yang@linux.dev>
Signed-off-by: Lance Yang <lance.yang@linux.dev>
Signed-off-by: Xueyuan Chen <xueyuan.chen21@gmail.com>
---
 arch/x86/include/asm/set_memory.h |  2 ++
 arch/x86/mm/pat/set_memory.c      | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
index 39271a5ea925..710164ea8258 100644
--- a/arch/x86/include/asm/set_memory.h
+++ b/arch/x86/include/asm/set_memory.h
@@ -88,6 +88,8 @@ int set_pages_rw(struct page *page, int numpages);
 
 int set_direct_map_invalid_noflush(struct page *page, unsigned int nr);
 int set_direct_map_default_noflush(struct page *page, unsigned int nr);
+int set_direct_map_ro(struct page *page, unsigned int nr);
+#define set_direct_map_ro set_direct_map_ro
 bool kernel_page_present(struct page *page);
 
 extern int kernel_set_to_readonly;
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index a1a061d995b3..7c0cfa7acd17 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -2666,6 +2666,16 @@ int set_direct_map_default_noflush(struct page *page, unsigned int nr)
 	return __set_pages_p(page, nr, 0);
 }
 
+int set_direct_map_ro(struct page *page, unsigned int nr)
+{
+	unsigned long addr = (unsigned long)page_address(page);
+
+	/* Direct-map callers are expected to pass pages without aliases. */
+	return change_page_attr_set_clr(&addr, nr, __pgprot(0),
+					__pgprot(_PAGE_RW | _PAGE_DIRTY), 0,
+					CPA_NO_CHECK_ALIAS, NULL);
+}
+
 #ifdef CONFIG_DEBUG_PAGEALLOC
 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
-- 
2.47.3

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

[Dave Hansen]

On 9/1/26 08:18, Xueyuan Chen wrote:
> +static inline int set_direct_map_ro(struct page *page, unsigned int nr)
> +{
> +	return 0;
> +}

I still really think the stub here needs to return an error *or* the
thing needs to just be void and tell folks that they can't know whether
it worked or not.

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

[Mike Rapoport]

Hi Dave,

On Wed, Sep 02, 2026 at 01:59:15PM -0700, Dave Hansen wrote:
> On 9/1/26 08:18, Xueyuan Chen wrote:
> > +static inline int set_direct_map_ro(struct page *page, unsigned int nr)
> > +{
> > +	return 0;
> > +}
> 
> I still really think the stub here needs to return an error *or* the
> thing needs to just be void and tell folks that they can't know whether
> it worked or not.

All other stubs return 0 here when an arch does not support set_direct_map.
I'm for keeping them all consistent and fixing that technical debt as a
whole rather than creating a one-off here.

-- 
Sincerely yours,
Mike.

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

[Mike Rapoport]

On Tue, Sep 01, 2026 at 11:18:16PM +0800, Xueyuan Chen wrote:
> The persistent huge zero folio is shared globally and should stay zero
> after initialization. As Jann Horn pointed out [1], kernel bugs have
> ended up writing to pages that were meant to be read-only, including in
> security-sensitive cases. Making the folio read-only in the direct map
> turns such writes into faults instead of silent zero-page corruption.
> 
> Add a page-based helper consistent with the existing direct-map interfaces.
> Handle TLB invalidation in the architecture implementation; unsupported
> architectures retain their current behavior.
> 
> Protect the folio after initialization. Skip highmem folios, which have no
> permanent direct-map mapping.
> 
> Inspired by Jann Horn's read-only zero page work [1] and follow-up
> discussion [3] with Yang Shi.
> 
> Link: https://lore.kernel.org/r/20260508-ro-zeropage-v1-1-9808abc20b49@google.com [1]
> Link: https://lore.kernel.org/r/0e5b23a6-4895-454a-9dfa-6dc21adc2991@kernel.org [2]
> Link: https://lore.kernel.org/r/CAHbLzkrXXe7r3n3jXgDKtwZhRqj=jDx9E6dLOULohnhBguvi9A@mail.gmail.com [3]
> 
> Suggested-by: David Hildenbrand <david@kernel.org>
> Suggested-by: Usama Arif <usama.arif@linux.dev>
> Co-developed-by: Lance Yang <lance.yang@linux.dev>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> Signed-off-by: Xueyuan Chen <xueyuan.chen21@gmail.com>
> ---
>  include/linux/set_memory.h | 17 +++++++++++++++++
>  mm/huge_memory.c           | 13 ++++++++++---
>  2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h
> index 3fe293cfed8c..ed9ce04b18a1 100644
> --- a/include/linux/set_memory.h
> +++ b/include/linux/set_memory.h
> @@ -54,6 +54,23 @@ static inline bool can_set_direct_map(void)
>  #endif
>  #endif /* CONFIG_ARCH_HAS_SET_DIRECT_MAP */
>  
> +#ifndef set_direct_map_ro
> +/**
> + * set_direct_map_ro - make a direct-map range read-only
> + * @page: first page in the direct-map range
> + * @nr: number of pages in the range
> + *
> + * Make the direct-map range starting at @page read-only and invalidate stale
> + * writable translations before returning.
> + *
> + * Return: 0 on success, or a negative error code on failure.
> + */
> +static inline int set_direct_map_ro(struct page *page, unsigned int nr)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #ifdef CONFIG_X86_64
>  int set_mce_nospec(unsigned long pfn);
>  int clear_mce_nospec(unsigned long pfn);
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 54494c3fa983..742283b36d74 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -42,6 +42,7 @@
>  #include <linux/pgalloc_tag.h>
>  #include <linux/pagewalk.h>
>  #include <linux/cleanup.h>
> +#include <linux/set_memory.h>
>  
>  #include <asm/tlb.h>
>  #include "internal.h"
> @@ -291,10 +292,16 @@ static int __init huge_zero_init(void)
>  	huge_zero_folio = alloc_huge_zero_folio();
>  	if (!huge_zero_folio) {
>  		pr_warn("Allocating persistent huge zero folio failed\n");
> -	} else {
> -		huge_zero_pfn = folio_pfn(huge_zero_folio);
> -		count_vm_event(THP_ZERO_PAGE_ALLOC);
> +		return 0;
>  	}
> +
> +	huge_zero_pfn = folio_pfn(huge_zero_folio);
> +	count_vm_event(THP_ZERO_PAGE_ALLOC);
> +
> +	/* Highmem folios have no permanent direct-map mapping to protect. */
> +	if (!folio_test_highmem(huge_zero_folio))
> +		set_direct_map_ro(folio_page(huge_zero_folio, 0), HPAGE_PMD_NR);

Sorry, I don't remember if it was discussed previously, but why can't we
use the existing set_memory_ro() here?

> +
>  	return 0;
>  }
>  
> -- 
> 2.47.3
> 

-- 
Sincerely yours,
Mike.

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

[Xueyuan Chen]

On Thu, Sep 3, 2026 at 5:22 PM Mike Rapoport <rppt@kernel.org> wrote:
>
> On Tue, Sep 01, 2026 at 11:18:16PM +0800, Xueyuan Chen wrote:
> > The persistent huge zero folio is shared globally and should stay zero
> > after initialization. As Jann Horn pointed out [1], kernel bugs have
> > ended up writing to pages that were meant to be read-only, including in
> > security-sensitive cases. Making the folio read-only in the direct map
> > turns such writes into faults instead of silent zero-page corruption.
> >
> > Add a page-based helper consistent with the existing direct-map interfaces.
> > Handle TLB invalidation in the architecture implementation; unsupported
> > architectures retain their current behavior.
> >
> > Protect the folio after initialization. Skip highmem folios, which have no
> > permanent direct-map mapping.
> >
> > Inspired by Jann Horn's read-only zero page work [1] and follow-up
> > discussion [3] with Yang Shi.
> >
> > Link: https://lore.kernel.org/r/20260508-ro-zeropage-v1-1-9808abc20b49@google.com [1]
> > Link: https://lore.kernel.org/r/0e5b23a6-4895-454a-9dfa-6dc21adc2991@kernel.org [2]
> > Link: https://lore.kernel.org/r/CAHbLzkrXXe7r3n3jXgDKtwZhRqj=jDx9E6dLOULohnhBguvi9A@mail.gmail.com [3]
> >
> > Suggested-by: David Hildenbrand <david@kernel.org>
> > Suggested-by: Usama Arif <usama.arif@linux.dev>
> > Co-developed-by: Lance Yang <lance.yang@linux.dev>
> > Signed-off-by: Lance Yang <lance.yang@linux.dev>
> > Signed-off-by: Xueyuan Chen <xueyuan.chen21@gmail.com>
> > ---
> >  include/linux/set_memory.h | 17 +++++++++++++++++
> >  mm/huge_memory.c           | 13 ++++++++++---
> >  2 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h
> > index 3fe293cfed8c..ed9ce04b18a1 100644
> > --- a/include/linux/set_memory.h
> > +++ b/include/linux/set_memory.h
> > @@ -54,6 +54,23 @@ static inline bool can_set_direct_map(void)
> >  #endif
> >  #endif /* CONFIG_ARCH_HAS_SET_DIRECT_MAP */
> >
> > +#ifndef set_direct_map_ro
> > +/**
> > + * set_direct_map_ro - make a direct-map range read-only
> > + * @page: first page in the direct-map range
> > + * @nr: number of pages in the range
> > + *
> > + * Make the direct-map range starting at @page read-only and invalidate stale
> > + * writable translations before returning.
> > + *
> > + * Return: 0 on success, or a negative error code on failure.
> > + */
> > +static inline int set_direct_map_ro(struct page *page, unsigned int nr)
> > +{
> > +     return 0;
> > +}
> > +#endif
> > +
> >  #ifdef CONFIG_X86_64
> >  int set_mce_nospec(unsigned long pfn);
> >  int clear_mce_nospec(unsigned long pfn);
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 54494c3fa983..742283b36d74 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -42,6 +42,7 @@
> >  #include <linux/pgalloc_tag.h>
> >  #include <linux/pagewalk.h>
> >  #include <linux/cleanup.h>
> > +#include <linux/set_memory.h>
> >
> >  #include <asm/tlb.h>
> >  #include "internal.h"
> > @@ -291,10 +292,16 @@ static int __init huge_zero_init(void)
> >       huge_zero_folio = alloc_huge_zero_folio();
> >       if (!huge_zero_folio) {
> >               pr_warn("Allocating persistent huge zero folio failed\n");
> > -     } else {
> > -             huge_zero_pfn = folio_pfn(huge_zero_folio);
> > -             count_vm_event(THP_ZERO_PAGE_ALLOC);
> > +             return 0;
> >       }
> > +
> > +     huge_zero_pfn = folio_pfn(huge_zero_folio);
> > +     count_vm_event(THP_ZERO_PAGE_ALLOC);
> > +
> > +     /* Highmem folios have no permanent direct-map mapping to protect. */
> > +     if (!folio_test_highmem(huge_zero_folio))
> > +             set_direct_map_ro(folio_page(huge_zero_folio, 0), HPAGE_PMD_NR);
>
> Sorry, I don't remember if it was discussed previously, but why can't we
> use the existing set_memory_ro() here?

Hi Mike,

We want to change the linear map here, but arm64 set_memory_ro() only
works on vmalloc addresses.
So a new helper is needed on arm64, and x86 implements the same
helper to keep the two architectures consistent. On x86 it is
basically set_memory_ro() minus the alias check.

Thanks,
Xueyuan

>
> > +
> >       return 0;
> >  }
> >
> > --
> > 2.47.3
> >
>
> --
> Sincerely yours,
> Mike.

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

[Dave Hansen]

On 9/3/26 02:20, Mike Rapoport wrote:
> On Wed, Sep 02, 2026 at 01:59:15PM -0700, Dave Hansen wrote:
>> On 9/1/26 08:18, Xueyuan Chen wrote:
>>> +static inline int set_direct_map_ro(struct page *page, unsigned int nr)
>>> +{
>>> +	return 0;
>>> +}
>> I still really think the stub here needs to return an error *or* the
>> thing needs to just be void and tell folks that they can't know whether
>> it worked or not.
> All other stubs return 0 here when an arch does not support set_direct_map.
> I'm for keeping them all consistent and fixing that technical debt as a
> whole rather than creating a one-off here.

Oof. I didn't realize the set_direct_map_*() functions all did this.

I do think it's generally a bad idea, and I'm also OK if it is not fixed
up here.