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

["Jürgen Groß" <jgross@suse.com>]

[-- 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 --]