All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mike Rapoport <rppt@kernel.org>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Ingo Molnar <mingo@redhat.com>,
	Nathan Chancellor <nathan@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@kernel.org>,
	linux-kernel@vger.kernel.org, x86@kernel.org
Subject: Re: [PATCH] x86/mm/pat: skip RWX verification until kernel text is set to read only
Date: Wed, 9 Sep 2026 20:45:14 +0300	[thread overview]
Message-ID: <aqGbKuaVo5FVGG7l@kernel.org> (raw)
In-Reply-To: <c364b3b6-f8c1-45a6-af3e-8685d9044b0c@intel.com>

On Wed, Sep 09, 2026 at 07:22:31AM -0700, Dave Hansen wrote:
> On 9/9/26 02:40, Mike Rapoport wrote:
> > On Tue, Sep 08, 2026 at 05:15:46PM -0700, Dave Hansen wrote:
> ...
> >> How about we add a kernel_strict_rwx() helper:
> >>
> >> bool strict_kernel_rwx(void)
> >> {
> >> 	return IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled;
> >> }
> >>
> >> Have verify_rwx() check *that*.
> > 
> > Checking only that will bring the warning about ITS pages back :)
> > 
> > Both CONFIG_STRICT_KERNEL_RWX and rodata_enabled are set before the
> > alternatives patching. 
> > 
> > And rodata_enabled is kinda arm64 specific thingy :)
> 
> I think I was confusing rodata_enabled and kernel_set_to_readonly as I
> read through things.
> 
> But either way, I really don't like the idea of checking some random
> state bit. It's generally fragile.
> 
> > The check for (!kernel_set_to_readonly) has to stay to actually rule
> > out the silly checks at boot and it covers your strict_kernel_rwx() because
> > kernel_set_to_readonly is only set when strict_kernel_rwx() will be true.
> 
> Could we do something slightly more generic?
> 
> mark_readonly() is awfully close to setting SYSTEM_RUNNING:
> 
>         mark_readonly();
>         pti_finalize();
>         system_state = SYSTEM_RUNNING;

We could check SYSTEM_RUNNING, but OTOH using kernel_set_to_readonly is
consistent with the existing checks in protect_rodata() and
protect_kernel_text_ro() in the same file.

I think that it's better to use the consistent kernel_set_to_readonly now
and flipping them all to SYSTEM_RUNNING ...

> What if we (eventually) did something like:

... when something like this materializes. 

> /*
>  * Get the kernel page tables ready to run userspace.
>  * There should be no page table manipulation between this and
>  * settings SYSTEM_RUNNING.
>  */
> void boot_finalize_page_tables(void)
> {
> 	WARN_ON(system_state >= SYSTEM_RUNNING);
> 
> 	mark_readonly();
> 
> 	/*
>          * Kernel mappings are now finalized - update
> 	 * the userspace page-able to finalize PTI.
> 	 */
> 	pti_finalize();
> 
> 	/* Ensure the final kernel mappings have no W+X issues: */
> 	debug_checkwx();
> }
> 
> and then verify_rwx() can just check system_state?

and protect_rodata() and protect_kernel_text_ro() :)
 
> >> Also, make the DEBUG_WX functionality mandatory for STRICT_KERNEL_RWX
> >> (with appropriate renaming) so that it can be depended upon.  I assume
> >> all the distros are turning DEBUG_WX on already (Ubuntu seems to).
> > 
> > Just checked my Debian config, it does.
> >  
> >> Maybe just do the strict_kernel_rwx() to start and then circle back
> >> around to muck with making DEBUG_WX mandatory?
> > 
> > I though about making DEBUG_WX mandatory right away, but that's surely not a
> > oneliner, so I kept it for later. Maybe should have mentioned in the
> > changelog, though.
> 
> Yes, please. It would be nice to mention that the fix is a bit exposed
> without DEBUG_WX. It could honestly even pr_info_once() about it to make
> it less likely it somehow gets forgotten.

I like the pr_info_once() idea, will add it in v2.

-- 
Sincerely yours,
Mike.

  reply	other threads:[~2026-09-09 17:45 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-08  9:27 [PATCH] x86/mm/pat: skip RWX verification until kernel text is set to read only Mike Rapoport
2026-09-08 23:33 ` Nathan Chancellor
2026-09-09  0:15 ` Dave Hansen
2026-09-09  9:40   ` Mike Rapoport
2026-09-09 14:22     ` Dave Hansen
2026-09-09 17:45       ` Mike Rapoport [this message]
2026-09-09 17:38 ` Ihor Solodrai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aqGbKuaVo5FVGG7l@kernel.org \
    --to=rppt@kernel.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=nathan@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@kernel.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.