The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
@ 2026-07-16  8:10 Mike Rapoport (Microsoft)
  2026-07-16  8:37 ` Jürgen Groß
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Rapoport (Microsoft) @ 2026-07-16  8:10 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Andy Lutomirski, Borislav Petkov, Ingo Molnar, Ingo Molnar,
	Juergen Gross, Mike Rapoport (Microsoft), H. Peter Anvin,
	Peter Zijlstra, Thomas Gleixner, x86, linux-kernel

lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
the walked page table levels so that verify_rwx() can detect mappings that
are both writable and executable.

The RW bits are folded into a bool with

	*rw &= pXd_flags(*pXd) & _PAGE_RW;

but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
always 0.  *rw becomes false at the first level walked, regardless of the
actual permissions, and verify_rwx() treats every mapping as non-writable
and never reports a W^X violation.

Accumulate NX and RW in unsigned long locals in their native bit positions
and store the result into the bool outputs once.

Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
Assisted-by: Copilot:claude-opus-4.8
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
---
 arch/x86/mm/pat/set_memory.c | 58 ++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index d023a40a1e03..05de76887fd3 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -716,57 +716,69 @@ static inline pgprot_t verify_rwx(pgprot_t old, pgprot_t new, unsigned long star
  * page table levels.
  */
 pte_t *lookup_address_in_pgd_attr(pgd_t *pgd, unsigned long address,
-				  unsigned int *level, bool *nx, bool *rw)
+				  unsigned int *level,
+				  bool *ret_nx, bool *ret_rw)
 {
+	unsigned long rw = _PAGE_RW;
+	unsigned long nx = 0;
+	pte_t *pte = NULL;
 	p4d_t *p4d;
 	pud_t *pud;
 	pmd_t *pmd;
 
 	*level = PG_LEVEL_256T;
-	*nx = false;
-	*rw = true;
-
 	if (pgd_none(*pgd))
-		return NULL;
+		goto out;
 
 	*level = PG_LEVEL_512G;
-	*nx |= pgd_flags(*pgd) & _PAGE_NX;
-	*rw &= pgd_flags(*pgd) & _PAGE_RW;
+	nx |= pgd_flags(*pgd) & _PAGE_NX;
+	rw &= pgd_flags(*pgd) & _PAGE_RW;
 
 	p4d = p4d_offset(pgd, address);
 	if (p4d_none(*p4d))
-		return NULL;
+		goto out;
 
-	if (p4d_leaf(*p4d) || !p4d_present(*p4d))
-		return (pte_t *)p4d;
+	if (p4d_leaf(*p4d) || !p4d_present(*p4d)) {
+		pte = (pte_t *)p4d;
+		goto out;
+	}
 
 	*level = PG_LEVEL_1G;
-	*nx |= p4d_flags(*p4d) & _PAGE_NX;
-	*rw &= p4d_flags(*p4d) & _PAGE_RW;
+	nx |= p4d_flags(*p4d) & _PAGE_NX;
+	rw &= p4d_flags(*p4d) & _PAGE_RW;
 
 	pud = pud_offset(p4d, address);
 	if (pud_none(*pud))
-		return NULL;
+		goto out;
 
-	if (pud_leaf(*pud) || !pud_present(*pud))
-		return (pte_t *)pud;
+	if (pud_leaf(*pud) || !pud_present(*pud)) {
+		pte = (pte_t *)pud;
+		goto out;
+	}
 
 	*level = PG_LEVEL_2M;
-	*nx |= pud_flags(*pud) & _PAGE_NX;
-	*rw &= pud_flags(*pud) & _PAGE_RW;
+	nx |= pud_flags(*pud) & _PAGE_NX;
+	rw &= pud_flags(*pud) & _PAGE_RW;
 
 	pmd = pmd_offset(pud, address);
 	if (pmd_none(*pmd))
-		return NULL;
+		goto out;
 
-	if (pmd_leaf(*pmd) || !pmd_present(*pmd))
-		return (pte_t *)pmd;
+	if (pmd_leaf(*pmd) || !pmd_present(*pmd)) {
+		pte = (pte_t *)pmd;
+		goto out;
+	}
 
 	*level = PG_LEVEL_4K;
-	*nx |= pmd_flags(*pmd) & _PAGE_NX;
-	*rw &= pmd_flags(*pmd) & _PAGE_RW;
+	nx |= pmd_flags(*pmd) & _PAGE_NX;
+	rw &= pmd_flags(*pmd) & _PAGE_RW;
+	pte = pte_offset_kernel(pmd, address);
+
+out:
+	*ret_nx = !!nx;
+	*ret_rw = !!rw;
 
-	return pte_offset_kernel(pmd, address);
+	return pte;
 }
 
 /*

---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20260716-verify-rwx-fix-3b0bd3e51244

--
Sincerely yours,
Mike.


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16  8:10 [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr() Mike Rapoport (Microsoft)
@ 2026-07-16  8:37 ` Jürgen Groß
  2026-07-16  8:46   ` Juergen Gross
  2026-07-16  9:18   ` Mike Rapoport
  0 siblings, 2 replies; 11+ messages in thread
From: Jürgen Groß @ 2026-07-16  8:37 UTC (permalink / raw)
  To: Mike Rapoport (Microsoft), Dave Hansen
  Cc: Andy Lutomirski, Borislav Petkov, Ingo Molnar, Ingo Molnar,
	H. Peter Anvin, Peter Zijlstra, Thomas Gleixner, x86,
	linux-kernel


[-- Attachment #1.1.1: Type: text/plain, Size: 1234 bytes --]

On 16.07.26 10:10, Mike Rapoport (Microsoft) wrote:
> lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
> the walked page table levels so that verify_rwx() can detect mappings that
> are both writable and executable.
> 
> The RW bits are folded into a bool with
> 
> 	*rw &= pXd_flags(*pXd) & _PAGE_RW;
> 
> but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
> always 0.  *rw becomes false at the first level walked, regardless of the
> actual permissions, and verify_rwx() treats every mapping as non-writable
> and never reports a W^X violation.
> 
> Accumulate NX and RW in unsigned long locals in their native bit positions
> and store the result into the bool outputs once.
> 
> Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
> Assisted-by: Copilot:claude-opus-4.8
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>

Thanks for catching this.

Reviewed-by: Juergen Gross <jgross@suse.com>

Just one remark: instead of using additional local variables the fix could
just look like:

     *rw |= !!(pXd_flags(*pXd) & _PAGE_RW);

I don't really care how the issue is fixed, but this would result in less
code.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16  8:37 ` Jürgen Groß
@ 2026-07-16  8:46   ` Juergen Gross
  2026-07-16  9:18   ` Mike Rapoport
  1 sibling, 0 replies; 11+ messages in thread
From: Juergen Gross @ 2026-07-16  8:46 UTC (permalink / raw)
  To: Mike Rapoport (Microsoft), Dave Hansen
  Cc: Andy Lutomirski, Borislav Petkov, Ingo Molnar, Ingo Molnar,
	H. Peter Anvin, Peter Zijlstra, Thomas Gleixner, x86,
	linux-kernel


[-- Attachment #1.1.1: Type: text/plain, Size: 1256 bytes --]

On 16.07.26 10:37, Jürgen Groß wrote:
> On 16.07.26 10:10, Mike Rapoport (Microsoft) wrote:
>> lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
>> the walked page table levels so that verify_rwx() can detect mappings that
>> are both writable and executable.
>>
>> The RW bits are folded into a bool with
>>
>>     *rw &= pXd_flags(*pXd) & _PAGE_RW;
>>
>> but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
>> always 0.  *rw becomes false at the first level walked, regardless of the
>> actual permissions, and verify_rwx() treats every mapping as non-writable
>> and never reports a W^X violation.
>>
>> Accumulate NX and RW in unsigned long locals in their native bit positions
>> and store the result into the bool outputs once.
>>
>> Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
>> Assisted-by: Copilot:claude-opus-4.8
>> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> 
> Thanks for catching this.
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Just one remark: instead of using additional local variables the fix could
> just look like:
> 
>      *rw |= !!(pXd_flags(*pXd) & _PAGE_RW);

&=, of course.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16  8:37 ` Jürgen Groß
  2026-07-16  8:46   ` Juergen Gross
@ 2026-07-16  9:18   ` Mike Rapoport
  2026-07-16 12:35     ` David Laight
  2026-07-16 13:27     ` Dave Hansen
  1 sibling, 2 replies; 11+ messages in thread
From: Mike Rapoport @ 2026-07-16  9:18 UTC (permalink / raw)
  To: Jürgen Groß
  Cc: Dave Hansen, Andy Lutomirski, Borislav Petkov, Ingo Molnar,
	Ingo Molnar, H. Peter Anvin, Peter Zijlstra, Thomas Gleixner, x86,
	linux-kernel

On Thu, Jul 16, 2026 at 10:37:00AM +0200, Jürgen Groß wrote:
> On 16.07.26 10:10, Mike Rapoport (Microsoft) wrote:
> > lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
> > the walked page table levels so that verify_rwx() can detect mappings that
> > are both writable and executable.
> > 
> > The RW bits are folded into a bool with
> > 
> > 	*rw &= pXd_flags(*pXd) & _PAGE_RW;
> > 
> > but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
> > always 0.  *rw becomes false at the first level walked, regardless of the
> > actual permissions, and verify_rwx() treats every mapping as non-writable
> > and never reports a W^X violation.
> > 
> > Accumulate NX and RW in unsigned long locals in their native bit positions
> > and store the result into the bool outputs once.
> > 
> > Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
> > Assisted-by: Copilot:claude-opus-4.8
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> 
> Thanks for catching this.
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> Just one remark: instead of using additional local variables the fix could
> just look like:
> 
>     *rw |= !!(pXd_flags(*pXd) & _PAGE_RW);
> 
> I don't really care how the issue is fixed, but this would result in less
> code.

But it will be slower :) 

I instrumented cpa-test and I see ~2% improvement with additional local
variables.
 
> Juergen






-- 
Sincerely yours,
Mike.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16  9:18   ` Mike Rapoport
@ 2026-07-16 12:35     ` David Laight
  2026-07-16 12:46       ` Juergen Gross
  2026-07-16 13:27     ` Dave Hansen
  1 sibling, 1 reply; 11+ messages in thread
From: David Laight @ 2026-07-16 12:35 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Jürgen Groß, Dave Hansen, Andy Lutomirski,
	Borislav Petkov, Ingo Molnar, Ingo Molnar, H. Peter Anvin,
	Peter Zijlstra, Thomas Gleixner, x86, linux-kernel

On Thu, 16 Jul 2026 12:18:48 +0300
Mike Rapoport <rppt@kernel.org> wrote:

> On Thu, Jul 16, 2026 at 10:37:00AM +0200, Jürgen Groß wrote:
> > On 16.07.26 10:10, Mike Rapoport (Microsoft) wrote:  
> > > lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
> > > the walked page table levels so that verify_rwx() can detect mappings that
> > > are both writable and executable.
> > > 
> > > The RW bits are folded into a bool with
> > > 
> > > 	*rw &= pXd_flags(*pXd) & _PAGE_RW;
> > > 
> > > but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
> > > always 0.  *rw becomes false at the first level walked, regardless of the
> > > actual permissions, and verify_rwx() treats every mapping as non-writable
> > > and never reports a W^X violation.
> > > 
> > > Accumulate NX and RW in unsigned long locals in their native bit positions
> > > and store the result into the bool outputs once.
> > > 
> > > Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
> > > Assisted-by: Copilot:claude-opus-4.8
> > > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>  
> > 
> > Thanks for catching this.
> > 
> > Reviewed-by: Juergen Gross <jgross@suse.com>
> > 
> > Just one remark: instead of using additional local variables the fix could
> > just look like:
> > 
> >     *rw |= !!(pXd_flags(*pXd) & _PAGE_RW);
> > 
> > I don't really care how the issue is fixed, but this would result in less
> > code.  
> 
> But it will be slower :) 
> 
> I instrumented cpa-test and I see ~2% improvement with additional local
> variables.

Does it improve further if you defer the '& _PAGE_NX' to the final assigment?

	David

>  
> > Juergen  
> 
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16 12:35     ` David Laight
@ 2026-07-16 12:46       ` Juergen Gross
  2026-07-16 12:49         ` David Laight
  0 siblings, 1 reply; 11+ messages in thread
From: Juergen Gross @ 2026-07-16 12:46 UTC (permalink / raw)
  To: David Laight, Mike Rapoport
  Cc: Dave Hansen, Andy Lutomirski, Borislav Petkov, Ingo Molnar,
	Ingo Molnar, H. Peter Anvin, Peter Zijlstra, Thomas Gleixner, x86,
	linux-kernel


[-- Attachment #1.1.1: Type: text/plain, Size: 1937 bytes --]

On 16.07.26 14:35, David Laight wrote:
> On Thu, 16 Jul 2026 12:18:48 +0300
> Mike Rapoport <rppt@kernel.org> wrote:
> 
>> On Thu, Jul 16, 2026 at 10:37:00AM +0200, Jürgen Groß wrote:
>>> On 16.07.26 10:10, Mike Rapoport (Microsoft) wrote:
>>>> lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
>>>> the walked page table levels so that verify_rwx() can detect mappings that
>>>> are both writable and executable.
>>>>
>>>> The RW bits are folded into a bool with
>>>>
>>>> 	*rw &= pXd_flags(*pXd) & _PAGE_RW;
>>>>
>>>> but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
>>>> always 0.  *rw becomes false at the first level walked, regardless of the
>>>> actual permissions, and verify_rwx() treats every mapping as non-writable
>>>> and never reports a W^X violation.
>>>>
>>>> Accumulate NX and RW in unsigned long locals in their native bit positions
>>>> and store the result into the bool outputs once.
>>>>
>>>> Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
>>>> Assisted-by: Copilot:claude-opus-4.8
>>>> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
>>>
>>> Thanks for catching this.
>>>
>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>>
>>> Just one remark: instead of using additional local variables the fix could
>>> just look like:
>>>
>>>      *rw |= !!(pXd_flags(*pXd) & _PAGE_RW);
>>>
>>> I don't really care how the issue is fixed, but this would result in less
>>> code.
>>
>> But it will be slower :)
>>
>> I instrumented cpa-test and I see ~2% improvement with additional local
>> variables.
> 
> Does it improve further if you defer the '& _PAGE_NX' to the final assigment?

If so, it would probably be beneficial to have one local variable for the
logical OR of all page table entries involved (used for *nx), and one for
the logical AND of all entries (used for *rw).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16 12:46       ` Juergen Gross
@ 2026-07-16 12:49         ` David Laight
  2026-07-16 12:55           ` Jürgen Groß
  0 siblings, 1 reply; 11+ messages in thread
From: David Laight @ 2026-07-16 12:49 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Mike Rapoport, Dave Hansen, Andy Lutomirski, Borislav Petkov,
	Ingo Molnar, Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Thomas Gleixner, x86, linux-kernel

On Thu, 16 Jul 2026 14:46:48 +0200
Juergen Gross <jgross@suse.com> wrote:

> On 16.07.26 14:35, David Laight wrote:
> > On Thu, 16 Jul 2026 12:18:48 +0300
> > Mike Rapoport <rppt@kernel.org> wrote:
> >   
> >> On Thu, Jul 16, 2026 at 10:37:00AM +0200, Jürgen Groß wrote:  
> >>> On 16.07.26 10:10, Mike Rapoport (Microsoft) wrote:  
> >>>> lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
> >>>> the walked page table levels so that verify_rwx() can detect mappings that
> >>>> are both writable and executable.
> >>>>
> >>>> The RW bits are folded into a bool with
> >>>>
> >>>> 	*rw &= pXd_flags(*pXd) & _PAGE_RW;
> >>>>
> >>>> but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
> >>>> always 0.  *rw becomes false at the first level walked, regardless of the
> >>>> actual permissions, and verify_rwx() treats every mapping as non-writable
> >>>> and never reports a W^X violation.
> >>>>
> >>>> Accumulate NX and RW in unsigned long locals in their native bit positions
> >>>> and store the result into the bool outputs once.
> >>>>
> >>>> Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
> >>>> Assisted-by: Copilot:claude-opus-4.8
> >>>> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>  
> >>>
> >>> Thanks for catching this.
> >>>
> >>> Reviewed-by: Juergen Gross <jgross@suse.com>
> >>>
> >>> Just one remark: instead of using additional local variables the fix could
> >>> just look like:
> >>>
> >>>      *rw |= !!(pXd_flags(*pXd) & _PAGE_RW);
> >>>
> >>> I don't really care how the issue is fixed, but this would result in less
> >>> code.  
> >>
> >> But it will be slower :)
> >>
> >> I instrumented cpa-test and I see ~2% improvement with additional local
> >> variables.  
> > 
> > Does it improve further if you defer the '& _PAGE_NX' to the final assigment?  
> 
> If so, it would probably be beneficial to have one local variable for the
> logical OR of all page table entries involved (used for *nx), and one for
> the logical AND of all entries (used for *rw).

Which is what the patch does...

	David

> 
> 
> Juergen


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16 12:49         ` David Laight
@ 2026-07-16 12:55           ` Jürgen Groß
  0 siblings, 0 replies; 11+ messages in thread
From: Jürgen Groß @ 2026-07-16 12:55 UTC (permalink / raw)
  To: David Laight
  Cc: Mike Rapoport, Dave Hansen, Andy Lutomirski, Borislav Petkov,
	Ingo Molnar, Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Thomas Gleixner, x86, linux-kernel


[-- Attachment #1.1.1: Type: text/plain, Size: 2647 bytes --]

On 16.07.26 14:49, David Laight wrote:
> On Thu, 16 Jul 2026 14:46:48 +0200
> Juergen Gross <jgross@suse.com> wrote:
> 
>> On 16.07.26 14:35, David Laight wrote:
>>> On Thu, 16 Jul 2026 12:18:48 +0300
>>> Mike Rapoport <rppt@kernel.org> wrote:
>>>    
>>>> On Thu, Jul 16, 2026 at 10:37:00AM +0200, Jürgen Groß wrote:
>>>>> On 16.07.26 10:10, Mike Rapoport (Microsoft) wrote:
>>>>>> lookup_address_in_pgd_attr() accumulates the effective NX and RW bits of
>>>>>> the walked page table levels so that verify_rwx() can detect mappings that
>>>>>> are both writable and executable.
>>>>>>
>>>>>> The RW bits are folded into a bool with
>>>>>>
>>>>>> 	*rw &= pXd_flags(*pXd) & _PAGE_RW;
>>>>>>
>>>>>> but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the AND is
>>>>>> always 0.  *rw becomes false at the first level walked, regardless of the
>>>>>> actual permissions, and verify_rwx() treats every mapping as non-writable
>>>>>> and never reports a W^X violation.
>>>>>>
>>>>>> Accumulate NX and RW in unsigned long locals in their native bit positions
>>>>>> and store the result into the bool outputs once.
>>>>>>
>>>>>> Fixes: ceb647b4b529 ("x86/pat: Introduce lookup_address_in_pgd_attr()")
>>>>>> Assisted-by: Copilot:claude-opus-4.8
>>>>>> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
>>>>>
>>>>> Thanks for catching this.
>>>>>
>>>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> Just one remark: instead of using additional local variables the fix could
>>>>> just look like:
>>>>>
>>>>>       *rw |= !!(pXd_flags(*pXd) & _PAGE_RW);
>>>>>
>>>>> I don't really care how the issue is fixed, but this would result in less
>>>>> code.
>>>>
>>>> But it will be slower :)
>>>>
>>>> I instrumented cpa-test and I see ~2% improvement with additional local
>>>> variables.
>>>
>>> Does it improve further if you defer the '& _PAGE_NX' to the final assigment?
>>
>> If so, it would probably be beneficial to have one local variable for the
>> logical OR of all page table entries involved (used for *nx), and one for
>> the logical AND of all entries (used for *rw).
> 
> Which is what the patch does...

Yes, of course, but you could delay masking the RW and NX bits until the final
assignment to *ret_nx and *ret_rw. At the same time I'd fetch the flags only
once per level. So something like:

unsigned long flags_ored = 0;
unsigned long flags_anded = ~0UL;
unsigned long flags;

flags = pgd_flags(*pgd)
flags_ored |= flags;
flags_anded &= flags;

...

*ret_nx = !!(flags_ored & _PAGE_NX);
*ret_rw = !!(flags_anded & _PAGE_RW);


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16  9:18   ` Mike Rapoport
  2026-07-16 12:35     ` David Laight
@ 2026-07-16 13:27     ` Dave Hansen
  2026-07-16 13:44       ` Mike Rapoport
  1 sibling, 1 reply; 11+ messages in thread
From: Dave Hansen @ 2026-07-16 13:27 UTC (permalink / raw)
  To: Mike Rapoport, Jürgen Groß
  Cc: Dave Hansen, Andy Lutomirski, Borislav Petkov, Ingo Molnar,
	Ingo Molnar, H. Peter Anvin, Peter Zijlstra, Thomas Gleixner, x86,
	linux-kernel

On 7/16/26 02:18, Mike Rapoport wrote:
>> I don't really care how the issue is fixed, but this would result in less
>> code.
> But it will be slower 🙂 

What does "slower" mean here?

Does it matter in _practice_? Does it measurably slow down something an
end user might see?

Also, I thought we had tests for this gunk. But maybe we're only testing
the leaf entry permissions or something and not the upper-level
permissions. I guess we don't often muck with those so this is
relatively unlikely to have hidden real bugs.

BTW, my pre-coffee brain struggled with the changelog here:

	but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the

Maybe it was a 0 vs. 1 bit thing, but it still took me way too long.
Could we do something like:

_PAGE_RW is 0x2. So consider the accumulation line:

	rw &= pXd_flags(*pXd) & _PAGE_RW;

where rw=0x1 and the right side evaluates down to 0x2. It'll end up doing:

	rw = 0x1 & 0x2

and rw always ends up 0.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16 13:27     ` Dave Hansen
@ 2026-07-16 13:44       ` Mike Rapoport
  2026-07-16 13:50         ` Dave Hansen
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Rapoport @ 2026-07-16 13:44 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Jürgen Groß, Dave Hansen, Andy Lutomirski,
	Borislav Petkov, Ingo Molnar, Ingo Molnar, H. Peter Anvin,
	Peter Zijlstra, Thomas Gleixner, x86, linux-kernel

On Thu, Jul 16, 2026 at 06:27:34AM -0700, Dave Hansen wrote:
> On 7/16/26 02:18, Mike Rapoport wrote:
> >> I don't really care how the issue is fixed, but this would result in less
> >> code.
> > But it will be slower 🙂 
> 
> What does "slower" mean here?
 
I've seen ~2% difference per set_memory call in instrumented cpa-test.

> Does it matter in _practice_? Does it measurably slow down something an
> end user might see?

Yes, if a user is loading/unloading a BPF program in a tight loop :)

We can also accumulate flags and push the masking to the final assignment
like Jürgen suggested earlier in this thread:

https://lore.kernel.org/all/f0d7dbe5-5472-409c-9f61-eeefe7d08591@suse.com/
 
I'd prefer to keep the current structure though as I'm planning to pull
lookup_address_in_pgd_attr() to generic code and replace bit checks with
pXd_write()/pXd_exec() helpers.

> Also, I thought we had tests for this gunk. But maybe we're only testing
> the leaf entry permissions or something and not the upper-level
> permissions. I guess we don't often muck with those so this is
> relatively unlikely to have hidden real bugs.
> 
> BTW, my pre-coffee brain struggled with the changelog here:
> 
> 	but _PAGE_RW is bit 1 while *rw only ever holds 0 or 1, so the
> 
> Maybe it was a 0 vs. 1 bit thing, but it still took me way too long.
> Could we do something like:
> 
> _PAGE_RW is 0x2. So consider the accumulation line:
> 
> 	rw &= pXd_flags(*pXd) & _PAGE_RW;
> 
> where rw=0x1 and the right side evaluates down to 0x2. It'll end up doing:
> 
> 	rw = 0x1 & 0x2
> 
> and rw always ends up 0.

Works for me.

-- 
Sincerely yours,
Mike.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()
  2026-07-16 13:44       ` Mike Rapoport
@ 2026-07-16 13:50         ` Dave Hansen
  0 siblings, 0 replies; 11+ messages in thread
From: Dave Hansen @ 2026-07-16 13:50 UTC (permalink / raw)
  To: Mike Rapoport
  Cc: Jürgen Groß, Dave Hansen, Andy Lutomirski,
	Borislav Petkov, Ingo Molnar, Ingo Molnar, H. Peter Anvin,
	Peter Zijlstra, Thomas Gleixner, x86, linux-kernel

On 7/16/26 06:44, Mike Rapoport wrote:
>> What does "slower" mean here?
>  
> I've seen ~2% difference per set_memory call in instrumented cpa-test.
> 
>> Does it matter in _practice_? Does it measurably slow down something an
>> end user might see?
> Yes, if a user is loading/unloading a BPF program in a tight loop 🙂

IMNHO, it may be worth optimizing if you can measure the difference in
BPF program load times (even in a tight loop).

But if set_memory() is the only thing you can measure a delta in, I'm
less convinced.

It's really sounding to me like we should do the simplest thing for a
bug fix and then arm wrestle later about how to micro-optimize the sucker.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-07-16 13:50 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-16  8:10 [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr() Mike Rapoport (Microsoft)
2026-07-16  8:37 ` Jürgen Groß
2026-07-16  8:46   ` Juergen Gross
2026-07-16  9:18   ` Mike Rapoport
2026-07-16 12:35     ` David Laight
2026-07-16 12:46       ` Juergen Gross
2026-07-16 12:49         ` David Laight
2026-07-16 12:55           ` Jürgen Groß
2026-07-16 13:27     ` Dave Hansen
2026-07-16 13:44       ` Mike Rapoport
2026-07-16 13:50         ` Dave Hansen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox