Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

[Jann Horn]

On Mon, Apr 27, 2026 at 5:44 PM Ard Biesheuvel <ardb+git@google.com> wrote:
> The empty zero page is used to back any kernel or user space mapping
> that is supposed to remain cleared, and so the page itself is never
> supposed to be modified.
>
> So make it __ro_after_init rather than __page_aligned_bss: on most
> architectures, this ensures that both the kernel's mapping of it and any
> aliases that are accessible via the kernel direct (linear) map are
> mapped read-only, and cannot be used (inadvertently or maliciously) to
> corrupt the contents of the zero page.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Jann Horn <jannh@google.com>

Sorry, I should have looked at this properly earlier instead of ending
up duplicating this patch with
<https://lore.kernel.org/all/20260508-ro-zeropage-v1-1-9808abc20b49@google.com/>.

> ---
>  mm/mm_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mm_init.c b/mm/mm_init.c
> index f9f8e1af921c..6ca01ed2a5a4 100644
> --- a/mm/mm_init.c
> +++ b/mm/mm_init.c
> @@ -57,7 +57,7 @@ unsigned long zero_page_pfn __ro_after_init;
>  EXPORT_SYMBOL(zero_page_pfn);
>
>  #ifndef __HAVE_COLOR_ZERO_PAGE
> -uint8_t empty_zero_page[PAGE_SIZE] __page_aligned_bss;
> +uint8_t empty_zero_page[PAGE_SIZE] __ro_after_init __aligned(PAGE_SIZE);

I think this is fine as-is; but FWIW:
"__ro_after_init __aligned(PAGE_SIZE)" means that this will land
in the middle of the .data..ro_after_init section, with padding in
front of it to create 4K alignment. So this probably wastes some
RAM on padding.

Looking at "nm ../linux-out/vmlinux | sort" with this patch applied
(from a build without any LTO or such), I see this:
```
[...]
ffffffff8473d378 d shmem_inode_cachep
ffffffff8473d380 d user_buckets
ffffffff8473e000 D zero_page_pfn
ffffffff8473f000 D empty_zero_page
ffffffff84740000 D __zero_page
ffffffff84740008 D pcpu_reserved_chunk
[...]
```
So I think there are almost 4K of padding between zero_page_pfn and
empty_zero_page for alignment; and I think when the linker linked
mm-init.o with the rest of the kernel, it also had to align the
compilation unit's entire .data..ro_after_init section to 4K, which is
why I also got ~3K of padding before zero_page_pfn, resulting in a
total of ~7K of padding.

If you want to change this:
I searched through the arch-specific linker scripts, and I think they
all rely on the generic RO_DATA() macro for emitting the rodata
section; so creating an analogous page-aligned rodata section should
be as simple as adding "*(.rodata..page_aligned)" directly after
"__start_rodata = .;", as I did in my duplicate patch.

Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

[Ard Biesheuvel]

On Fri, 8 May 2026, at 19:02, Jann Horn wrote:
> On Mon, Apr 27, 2026 at 5:44 PM Ard Biesheuvel <ardb+git@google.com> wrote:
>> The empty zero page is used to back any kernel or user space mapping
>> that is supposed to remain cleared, and so the page itself is never
>> supposed to be modified.
>>
>> So make it __ro_after_init rather than __page_aligned_bss: on most
>> architectures, this ensures that both the kernel's mapping of it and any
>> aliases that are accessible via the kernel direct (linear) map are
>> mapped read-only, and cannot be used (inadvertently or maliciously) to
>> corrupt the contents of the zero page.
>>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> Reviewed-by: Jann Horn <jannh@google.com>
>

Thanks

> Sorry, I should have looked at this properly earlier instead of ending
> up duplicating this patch with
> <https://lore.kernel.org/all/20260508-ro-zeropage-v1-1-9808abc20b49@google.com/>.
>

No worries. I might borrow some of that rationale btw

>> ---
>>  mm/mm_init.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>> index f9f8e1af921c..6ca01ed2a5a4 100644
>> --- a/mm/mm_init.c
>> +++ b/mm/mm_init.c
>> @@ -57,7 +57,7 @@ unsigned long zero_page_pfn __ro_after_init;
>>  EXPORT_SYMBOL(zero_page_pfn);
>>
>>  #ifndef __HAVE_COLOR_ZERO_PAGE
>> -uint8_t empty_zero_page[PAGE_SIZE] __page_aligned_bss;
>> +uint8_t empty_zero_page[PAGE_SIZE] __ro_after_init __aligned(PAGE_SIZE);
>
> I think this is fine as-is; but FWIW:
> "__ro_after_init __aligned(PAGE_SIZE)" means that this will land
> in the middle of the .data..ro_after_init section, with padding in
> front of it to create 4K alignment. So this probably wastes some
> RAM on padding.
>
> Looking at "nm ../linux-out/vmlinux | sort" with this patch applied
> (from a build without any LTO or such), I see this:
> ```
> [...]
> ffffffff8473d378 d shmem_inode_cachep
> ffffffff8473d380 d user_buckets
> ffffffff8473e000 D zero_page_pfn
> ffffffff8473f000 D empty_zero_page
> ffffffff84740000 D __zero_page
> ffffffff84740008 D pcpu_reserved_chunk
> [...]
> ```
> So I think there are almost 4K of padding between zero_page_pfn and
> empty_zero_page for alignment; and I think when the linker linked
> mm-init.o with the rest of the kernel, it also had to align the
> compilation unit's entire .data..ro_after_init section to 4K, which is
> why I also got ~3K of padding before zero_page_pfn, resulting in a
> total of ~7K of padding.
>
> If you want to change this:
> I searched through the arch-specific linker scripts, and I think they
> all rely on the generic RO_DATA() macro for emitting the rodata
> section; so creating an analogous page-aligned rodata section should
> be as simple as adding "*(.rodata..page_aligned)" directly after
> "__start_rodata = .;", as I did in my duplicate patch.

I think we should simply do something along the lines of the below,
considering that the size of a data object tends to correlate with
its minimum alignment.

I do find it rather puzzling that the compiler emits empty_zero_page
*after* zero_page_pfn - ideally, we'd combine the below with
-fdata-sections so that the linker sees all individual objects, but
I suspect that would create some problems elsewhere.


--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -452,7 +452,7 @@
 #define RO_AFTER_INIT_DATA                                   \
        . = ALIGN(8);                                         \
        __start_ro_after_init = .;                            \
-       *(.data..ro_after_init)                               \
+       *(SORT_BY_ALIGNMENT(.data..ro_after_init))            \
        JUMP_TABLE_DATA                                       \
        STATIC_CALL_DATA                                      \
        __end_ro_after_init = .;

Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

[Jann Horn]

On Mon, May 11, 2026 at 10:59 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> I think we should simply do something along the lines of the below,
> considering that the size of a data object tends to correlate with
> its minimum alignment.
>
> I do find it rather puzzling that the compiler emits empty_zero_page
> *after* zero_page_pfn - ideally, we'd combine the below with
> -fdata-sections so that the linker sees all individual objects, but
> I suspect that would create some problems elsewhere.
>
>
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -452,7 +452,7 @@
>  #define RO_AFTER_INIT_DATA                                   \
>         . = ALIGN(8);                                         \
>         __start_ro_after_init = .;                            \
> -       *(.data..ro_after_init)                               \
> +       *(SORT_BY_ALIGNMENT(.data..ro_after_init))            \

Oh, neat, I didn't realize that's possible. That seems like a nicer
approach... (Assuming that it doesn't cause cache efficiency issues
somehow, I imagine it could be possible that source-code-adjacent
objects should also be located next to each other for cache/TLB
efficiency... but I have no concrete reason to think that, just a
thought.)

Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

[Kees Cook]

On Fri, May 08, 2026 at 07:02:51PM +0200, Jann Horn wrote:
> On Mon, Apr 27, 2026 at 5:44 PM Ard Biesheuvel <ardb+git@google.com> wrote:
> > The empty zero page is used to back any kernel or user space mapping
> > that is supposed to remain cleared, and so the page itself is never
> > supposed to be modified.
> >
> > So make it __ro_after_init rather than __page_aligned_bss: on most
> > architectures, this ensures that both the kernel's mapping of it and any
> > aliases that are accessible via the kernel direct (linear) map are
> > mapped read-only, and cannot be used (inadvertently or maliciously) to
> > corrupt the contents of the zero page.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> 
> Reviewed-by: Jann Horn <jannh@google.com>
> 
> Sorry, I should have looked at this properly earlier instead of ending
> up duplicating this patch with
> <https://lore.kernel.org/all/20260508-ro-zeropage-v1-1-9808abc20b49@google.com/>.

As you mention in your testing of the patch, could we add an LKDTM test
that does the same to catch any regressions?

-- 
Kees Cook

Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

[Jann Horn]

On Mon, May 11, 2026 at 8:45 PM Kees Cook <kees@kernel.org> wrote:
> On Fri, May 08, 2026 at 07:02:51PM +0200, Jann Horn wrote:
> > On Mon, Apr 27, 2026 at 5:44 PM Ard Biesheuvel <ardb+git@google.com> wrote:
> > > The empty zero page is used to back any kernel or user space mapping
> > > that is supposed to remain cleared, and so the page itself is never
> > > supposed to be modified.
> > >
> > > So make it __ro_after_init rather than __page_aligned_bss: on most
> > > architectures, this ensures that both the kernel's mapping of it and any
> > > aliases that are accessible via the kernel direct (linear) map are
> > > mapped read-only, and cannot be used (inadvertently or maliciously) to
> > > corrupt the contents of the zero page.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > Reviewed-by: Jann Horn <jannh@google.com>
> >
> > Sorry, I should have looked at this properly earlier instead of ending
> > up duplicating this patch with
> > <https://lore.kernel.org/all/20260508-ro-zeropage-v1-1-9808abc20b49@google.com/>.
>
> As you mention in your testing of the patch, could we add an LKDTM test
> that does the same to catch any regressions?

Shouldn't be too hard - LKDTM crashtypes don't accept arguments, so we
couldn't easily pass in a userspace pointer, I think, but we could
make a crashtype that allocates anon memory with do_mmap() (under
mmap_lock), then GUP on the allocated userspace address, then writes
into the obtained address with something like atomic_add(0, <pointer
from kmap(page)>) to trigger a write access without actually changing
memory contents...

Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

[Kiryl Shutsemau]

On Tue, Apr 28, 2026 at 09:51:09PM +0200, David Hildenbrand (Arm) wrote:
> On 4/27/26 17:34, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 
> > The empty zero page is used to back any kernel or user space mapping
> > that is supposed to remain cleared, and so the page itself is never
> > supposed to be modified.
> > 
> > So make it __ro_after_init rather than __page_aligned_bss: on most
> > architectures, this ensures that both the kernel's mapping of it and any
> > aliases that are accessible via the kernel direct (linear) map are
> > mapped read-only, and cannot be used (inadvertently or maliciously) to
> > corrupt the contents of the zero page.
> > 
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  mm/mm_init.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/mm_init.c b/mm/mm_init.c
> > index f9f8e1af921c..6ca01ed2a5a4 100644
> > --- a/mm/mm_init.c
> > +++ b/mm/mm_init.c
> > @@ -57,7 +57,7 @@ unsigned long zero_page_pfn __ro_after_init;
> >  EXPORT_SYMBOL(zero_page_pfn);
> >  
> >  #ifndef __HAVE_COLOR_ZERO_PAGE
> > -uint8_t empty_zero_page[PAGE_SIZE] __page_aligned_bss;
> > +uint8_t empty_zero_page[PAGE_SIZE] __ro_after_init __aligned(PAGE_SIZE);
> >  EXPORT_SYMBOL(empty_zero_page);
> >  
> >  struct page *__zero_page __ro_after_init;
> 
> I am no expert on BSS etc, but from what I understand, we'll still get zeroed
> page-aligned memory. I don't know if there is any other impact on not having it
> in bss.page_aligned. I assume no

IIUC, unlike BSS, it will be part of the kernel image. So kernel image
size will grow by PAGE_SIZE. But compressor will eat it as it is all
zeros, so it should be okay.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v4 02/15] mm: Make empty_zero_page __ro_after_init

[Feng Tang]

On Mon, Apr 27, 2026 at 05:34:19PM +0200, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> The empty zero page is used to back any kernel or user space mapping
> that is supposed to remain cleared, and so the page itself is never
> supposed to be modified.
> 
> So make it __ro_after_init rather than __page_aligned_bss: on most
> architectures, this ensures that both the kernel's mapping of it and any
> aliases that are accessible via the kernel direct (linear) map are
> mapped read-only, and cannot be used (inadvertently or maliciously) to
> corrupt the contents of the zero page.

Reviewed-by: Feng Tang <feng.tang@linux.alibaba.com>

We did hit the issue that zero page got corrupted due to non-kernel
reason earlier this year, and it took us weeks to track it down to
zero page corruption and the final root cause, as it randomly happened
to different user space tasks. Thanks for the patch!

> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  mm/mm_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/mm_init.c b/mm/mm_init.c
> index f9f8e1af921c..6ca01ed2a5a4 100644
> --- a/mm/mm_init.c
> +++ b/mm/mm_init.c
> @@ -57,7 +57,7 @@ unsigned long zero_page_pfn __ro_after_init;
>  EXPORT_SYMBOL(zero_page_pfn);
>  
>  #ifndef __HAVE_COLOR_ZERO_PAGE
> -uint8_t empty_zero_page[PAGE_SIZE] __page_aligned_bss;
> +uint8_t empty_zero_page[PAGE_SIZE] __ro_after_init __aligned(PAGE_SIZE);
>  EXPORT_SYMBOL(empty_zero_page);
>  
>  struct page *__zero_page __ro_after_init;
> -- 
> 2.54.0.rc2.544.gc7ae2d5bb8-goog
>