Re: [PATCH] x86/mm/pat: fix effective RW computation in lookup_address_in_pgd_attr()

[David Laight]

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

> On 16.07.26 14:35, David Laight wrote:
> > On Thu, 16 Jul 2026 12:18:48 +0300
> > Mike Rapoport <rppt@xxxxxxxxxx> 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@xxxxxxxxxx>  
> >>>
> >>> Thanks for catching this.
> >>>
> >>> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
> >>>
> >>> 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