x86/execmem: fix ROX cache usage in Xen PV guests

[PATCH] x86/execmem: fix ROX cache usage in Xen PV guests

Posted by Juergen Gross 1 year, 1 month ago

The recently introduced ROX cache for modules is assuming large page
support in 64-bit mode without testing the related feature bit. This
results in breakage when running as a Xen PV guest, as in this mode
large pages are not supported.

Fix that by testing the X86_FEATURE_PSE capability when deciding
whether to enable the ROX cache.

Fixes: 2e45474ab14f ("execmem: add support for cache of large ROX pages")
Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm/init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index c6d29f283001..62aa4d66a032 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -1080,7 +1080,8 @@ struct execmem_info __init *execmem_arch_setup(void)
 
 	start = MODULES_VADDR + offset;
 
-	if (IS_ENABLED(CONFIG_ARCH_HAS_EXECMEM_ROX)) {
+	if (IS_ENABLED(CONFIG_ARCH_HAS_EXECMEM_ROX) &&
+	    cpu_feature_enabled(X86_FEATURE_PSE)) {
 		pgprot = PAGE_KERNEL_ROX;
 		flags = EXECMEM_KASAN_SHADOW | EXECMEM_ROX_CACHE;
 	} else {
-- 
2.43.0

Re: [PATCH] x86/execmem: fix ROX cache usage in Xen PV guests

Posted by Borislav Petkov 1 year, 1 month ago

Adding the author in Fixes to Cc

On Fri, Jan 03, 2025 at 07:56:31AM +0100, Juergen Gross wrote:
> The recently introduced ROX cache for modules is assuming large page
> support in 64-bit mode without testing the related feature bit. This
> results in breakage when running as a Xen PV guest, as in this mode
> large pages are not supported.
> 
> Fix that by testing the X86_FEATURE_PSE capability when deciding
> whether to enable the ROX cache.
> 
> Fixes: 2e45474ab14f ("execmem: add support for cache of large ROX pages")
> Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/mm/init.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index c6d29f283001..62aa4d66a032 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -1080,7 +1080,8 @@ struct execmem_info __init *execmem_arch_setup(void)
>  
>  	start = MODULES_VADDR + offset;
>  
> -	if (IS_ENABLED(CONFIG_ARCH_HAS_EXECMEM_ROX)) {
> +	if (IS_ENABLED(CONFIG_ARCH_HAS_EXECMEM_ROX) &&
> +	    cpu_feature_enabled(X86_FEATURE_PSE)) {
>  		pgprot = PAGE_KERNEL_ROX;
>  		flags = EXECMEM_KASAN_SHADOW | EXECMEM_ROX_CACHE;
>  	} else {
> -- 
> 2.43.0
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/execmem: fix ROX cache usage in Xen PV guests

Posted by Mike Rapoport 1 year, 1 month ago

On Fri, Jan 03, 2025 at 02:00:44PM +0100, Borislav Petkov wrote:
> Adding the author in Fixes to Cc

Thanks, Boris!
 
> On Fri, Jan 03, 2025 at 07:56:31AM +0100, Juergen Gross wrote:
> > The recently introduced ROX cache for modules is assuming large page
> > support in 64-bit mode without testing the related feature bit. This
> > results in breakage when running as a Xen PV guest, as in this mode
> > large pages are not supported.

The ROX cache does not assume support for large pages, it just had a bug
when dealing with base pages and the patch below should fix it.

Restricting ROX cache only for configurations that support large pages
makes sense on it's own because there's no real benefit from the cache on
such systems, but it does not fix the issue but rather covers it up.

diff --git a/mm/execmem.c b/mm/execmem.c
index be6b234c032e..0090a6f422aa 100644
--- a/mm/execmem.c
+++ b/mm/execmem.c
@@ -266,6 +266,7 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
 	unsigned long vm_flags = VM_ALLOW_HUGE_VMAP;
 	struct execmem_area *area;
 	unsigned long start, end;
+	unsigned int page_shift;
 	struct vm_struct *vm;
 	size_t alloc_size;
 	int err = -ENOMEM;
@@ -296,8 +297,9 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
 	if (err)
 		goto err_free_mem;
 
+	page_shift = get_vm_area_page_order(vm) + PAGE_SHIFT;
 	err = vmap_pages_range_noflush(start, end, range->pgprot, vm->pages,
-				       PMD_SHIFT);
+				       page_shift);
 	if (err)
 		goto err_free_mem;
 
-- 
2.45.2

 
> > Fix that by testing the X86_FEATURE_PSE capability when deciding
> > whether to enable the ROX cache.
> > 
> > Fixes: 2e45474ab14f ("execmem: add support for cache of large ROX pages")
> > Reported-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> > ---
> >  arch/x86/mm/init.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > index c6d29f283001..62aa4d66a032 100644
> > --- a/arch/x86/mm/init.c
> > +++ b/arch/x86/mm/init.c
> > @@ -1080,7 +1080,8 @@ struct execmem_info __init *execmem_arch_setup(void)
> >  
> >  	start = MODULES_VADDR + offset;
> >  
> > -	if (IS_ENABLED(CONFIG_ARCH_HAS_EXECMEM_ROX)) {
> > +	if (IS_ENABLED(CONFIG_ARCH_HAS_EXECMEM_ROX) &&
> > +	    cpu_feature_enabled(X86_FEATURE_PSE)) {
> >  		pgprot = PAGE_KERNEL_ROX;
> >  		flags = EXECMEM_KASAN_SHADOW | EXECMEM_ROX_CACHE;
> >  	} else {
> > -- 
> > 2.43.0
> > 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

-- 
Sincerely yours,
Mike.

Re: [PATCH] x86/execmem: fix ROX cache usage in Xen PV guests

Posted by Borislav Petkov 1 year, 1 month ago

On Fri, Jan 10, 2025 at 12:02:38PM +0200, Mike Rapoport wrote:
> On Fri, Jan 03, 2025 at 02:00:44PM +0100, Borislav Petkov wrote:
> > Adding the author in Fixes to Cc
> 
> Thanks, Boris!
>  
> > On Fri, Jan 03, 2025 at 07:56:31AM +0100, Juergen Gross wrote:
> > > The recently introduced ROX cache for modules is assuming large page
> > > support in 64-bit mode without testing the related feature bit. This
> > > results in breakage when running as a Xen PV guest, as in this mode
> > > large pages are not supported.
> 
> The ROX cache does not assume support for large pages, it just had a bug
> when dealing with base pages and the patch below should fix it.
> 
> Restricting ROX cache only for configurations that support large pages
> makes sense on it's own because there's no real benefit from the cache on
> such systems, but it does not fix the issue but rather covers it up.
> 
> diff --git a/mm/execmem.c b/mm/execmem.c
> index be6b234c032e..0090a6f422aa 100644
> --- a/mm/execmem.c
> +++ b/mm/execmem.c
> @@ -266,6 +266,7 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
>  	unsigned long vm_flags = VM_ALLOW_HUGE_VMAP;
>  	struct execmem_area *area;
>  	unsigned long start, end;
> +	unsigned int page_shift;
>  	struct vm_struct *vm;
>  	size_t alloc_size;
>  	int err = -ENOMEM;
> @@ -296,8 +297,9 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
>  	if (err)
>  		goto err_free_mem;
>  
> +	page_shift = get_vm_area_page_order(vm) + PAGE_SHIFT;
>  	err = vmap_pages_range_noflush(start, end, range->pgprot, vm->pages,
> -				       PMD_SHIFT);
> +				       page_shift);
>  	if (err)
>  		goto err_free_mem;
>  
> -- 

So this patch is still being discussed here.

akpm has already picked up the original fix from Jürgen:

59f59108475e ("x86/execmem: fix ROX cache usage in Xen PV guests")

and the patch is already in Linus' tree.

How much of a fiasco is this execmem thing going to become?

Andrew, is there any chance we can synchronize on what you pick up for
arch/x86/ or?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/execmem: fix ROX cache usage in Xen PV guests

Posted by Marek Marczykowski-Górecki 1 year, 1 month ago

On Mon, Jan 13, 2025 at 06:55:52PM +0100, Borislav Petkov wrote:
> On Fri, Jan 10, 2025 at 12:02:38PM +0200, Mike Rapoport wrote:
> > On Fri, Jan 03, 2025 at 02:00:44PM +0100, Borislav Petkov wrote:
> > > Adding the author in Fixes to Cc
> > 
> > Thanks, Boris!
> >  
> > > On Fri, Jan 03, 2025 at 07:56:31AM +0100, Juergen Gross wrote:
> > > > The recently introduced ROX cache for modules is assuming large page
> > > > support in 64-bit mode without testing the related feature bit. This
> > > > results in breakage when running as a Xen PV guest, as in this mode
> > > > large pages are not supported.
> > 
> > The ROX cache does not assume support for large pages, it just had a bug
> > when dealing with base pages and the patch below should fix it.
> > 
> > Restricting ROX cache only for configurations that support large pages
> > makes sense on it's own because there's no real benefit from the cache on
> > such systems, but it does not fix the issue but rather covers it up.
> > 
> > diff --git a/mm/execmem.c b/mm/execmem.c
> > index be6b234c032e..0090a6f422aa 100644
> > --- a/mm/execmem.c
> > +++ b/mm/execmem.c
> > @@ -266,6 +266,7 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
> >  	unsigned long vm_flags = VM_ALLOW_HUGE_VMAP;
> >  	struct execmem_area *area;
> >  	unsigned long start, end;
> > +	unsigned int page_shift;
> >  	struct vm_struct *vm;
> >  	size_t alloc_size;
> >  	int err = -ENOMEM;
> > @@ -296,8 +297,9 @@ static int execmem_cache_populate(struct execmem_range *range, size_t size)
> >  	if (err)
> >  		goto err_free_mem;
> >  
> > +	page_shift = get_vm_area_page_order(vm) + PAGE_SHIFT;
> >  	err = vmap_pages_range_noflush(start, end, range->pgprot, vm->pages,
> > -				       PMD_SHIFT);
> > +				       page_shift);
> >  	if (err)
> >  		goto err_free_mem;
> >  
> > -- 
> 
> So this patch is still being discussed here.
> 
> akpm has already picked up the original fix from Jürgen:
> 
> 59f59108475e ("x86/execmem: fix ROX cache usage in Xen PV guests")
> 
> and the patch is already in Linus' tree.
> 
> How much of a fiasco is this execmem thing going to become?
> 
> Andrew, is there any chance we can synchronize on what you pick up for
> arch/x86/ or?

I was running some tests today with the above patch on top of -rc7
(and without Jürgen's one). Some tests are still running, and there are
still some crashes I need to take a look at (could be completely
unrelated), but generally it looks _much_ better, especially I don't see
the wall of crashes in HVM domU that I've seen before
(https://lore.kernel.org/xen-devel/Z3cyhdKu6M1vdBe_@mail-itl/).
The latter could be an effect of the above fix, or could be some other
fix that happened between -rc5 and -rc7. If that would be interesting,
I can also re-test with -rc5 + the above patch, or something else. Let
me know.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Re: [PATCH] x86/execmem: fix ROX cache usage in Xen PV guests

Posted by Borislav Petkov 1 year, 1 month ago

On Mon, Jan 13, 2025 at 09:10:03PM +0100, Marek Marczykowski-Górecki wrote:
> I was running some tests today with the above patch on top of -rc7
> (and without Jürgen's one). Some tests are still running, and there are
> still some crashes I need to take a look at (could be completely
> unrelated), but generally it looks _much_ better, especially I don't see
> the wall of crashes in HVM domU that I've seen before
> (https://lore.kernel.org/xen-devel/Z3cyhdKu6M1vdBe_@mail-itl/).
> The latter could be an effect of the above fix, or could be some other
> fix that happened between -rc5 and -rc7. If that would be interesting,
> I can also re-test with -rc5 + the above patch, or something else. Let
> me know.

Yeah, we disabled that EXECMEM_ROX thing for now on x86, that close to the
merge window to avoid the fiasco:

https://git.kernel.org/tip/a9bbe341333109465605e8733bab0b573cddcc8c

and so this should fix all issues for you, I presume.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette