[PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Janusz Krzysztofik]

Visible glitches have been observed when running graphics applications on
Linux under Xen hypervisor.  Those observations have been confirmed with
failures from kms_pwrite_crc Intel GPU test that verifies data coherency
of DRM frame buffer objects using hardware CRC checksums calculated by
display controllers, exposed to userspace via debugfs.  Affected
processing paths have then been identified with new IGT test variants that
mmap the objects using different methods and caching modes [1].

When running as a Xen PV guest, Linux uses Xen provided PAT configuration
which is different from its native one.  In particular, Xen specific PTE
encoding of write-combining caching, likely used by graphics applications,
differs from the Linux default one found among statically defined minimal
set of supported modes.  Since Xen defines PTE encoding of the WC mode as
_PAGE_PAT, it no longer belongs to the minimal set, depends on correct
handling of _PAGE_PAT bit, and can be mismatched with write-back caching.

When a user calls mmap() for a DRM buffer object, DRM device specific
.mmap file operation, called from mmap_region(), takes care of setting PTE
encoding bits in a vm_page_prot field of an associated virtual memory area
structure.  Unfortunately, _PAGE_PAT bit is not preserved when the vma's
.vm_flags are then applied to .vm_page_prot via vm_set_page_prot().  Bits
to be preserved are determined with _PAGE_CHG_MASK symbol that doesn't
cover _PAGE_PAT.  As a consequence, WB caching is requested instead of WC
when running under Xen (also, WP is silently changed to WT, and UC
downgraded to UC_MINUS).  When running on bare metal, WC is not affected,
but WP and WT extra modes are unintentionally replaced with WC and UC,
respectively.

WP and WT modes, encoded with _PAGE_PAT bit set, were introduced by commit
281d4078bec3 ("x86: Make page cache mode a real type").  Care was taken
to extend _PAGE_CACHE_MASK symbol with that additional bit, but that
symbol has never been used for identification of bits preserved when
applying page protection flags.  Support for all cache modes under Xen,
including the problematic WC mode, was then introduced by commit
47591df50512 ("xen: Support Xen pv-domains using PAT").

Extend bitmask used by pgprot_modify() for selecting bits to be preserved
with _PAGE_PAT bit.  However, since that bit can be reused as _PAGE_PSE,
and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely
intentionally defined with that bit not set, keep that symbol unchanged.

v2: Keep pgprot_modify() untouched, make _PAGE_PAT part of _PAGE_CHG_MASK
    instead (Borislav),
  - also add _PAGE_PAT_LARGE to _HPAGE_CHG_MASK (Juergen).

[1] https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/0f0754413f14

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7648
Fixes: 281d4078bec3 ("x86: Make page cache mode a real type")
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: stable@vger.kernel.org # v3.19+
---
 arch/x86/include/asm/pgtable_types.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 447d4bee25c48..0432ed9e59e3e 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -125,11 +125,11 @@
  * instance, and is *not* included in this mask since
  * pte_modify() does modify it.
  */
-#define _PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |		\
-			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	\
-			 _PAGE_SOFT_DIRTY | _PAGE_DEVMAP | _PAGE_ENC |  \
+#define _PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |	_PAGE_PAT | \
+			 _PAGE_SPECIAL | _PAGE_ACCESSED | _PAGE_DIRTY |	    \
+			 _PAGE_SOFT_DIRTY | _PAGE_DEVMAP | _PAGE_ENC |	    \
 			 _PAGE_UFFD_WP)
-#define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE)
+#define _HPAGE_CHG_MASK (_PAGE_CHG_MASK | _PAGE_PSE | _PAGE_PAT_LARGE)
 
 /*
  * The cache modes defined here are used to translate between pure SW usage
-- 
2.41.0

Re: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Dave Hansen]

On 6/7/23 08:23, Janusz Krzysztofik wrote:
> 
> Extend bitmask used by pgprot_modify() for selecting bits to be preserved
> with _PAGE_PAT bit.  However, since that bit can be reused as _PAGE_PSE,
> and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely
> intentionally defined with that bit not set, keep that symbol unchanged.

I'm really having a hard time parsing what that last sentence is saying.

Could you try again, please?

Re: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Janusz Krzysztofik]

On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote:
> On 6/7/23 08:23, Janusz Krzysztofik wrote:
> > 
> > Extend bitmask used by pgprot_modify() for selecting bits to be preserved
> > with _PAGE_PAT bit.  However, since that bit can be reused as _PAGE_PSE,
> > and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is likely
> > intentionally defined with that bit not set, keep that symbol unchanged.
> 
> I'm really having a hard time parsing what that last sentence is saying.
> 
> Could you try again, please?

OK, but then I need to get my doubts addressed by someone first, otherwise I'm 
not able to provide a better justification from my heart.

The issue needs to be fixed by including _PAGE_PAT bit into a bitmask used 
by pgprot_modify() for selecting bits to be preserved.  We can either do 
that internally to pgprot_modify() (my initial proposal, which my poorly 
worded paragraph was still trying to describe and justify), or by making 
_PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and reflected in 
my v2 changelog.  But for the latter, I think we need to make sure that we 
don't break other users of _PAGE_CHG_MASK.  Maybe Borislav can confirm that's 
the case.

Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge pmds' 
counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c ("thp: add 
pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no longer differ 
from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter.  If such 
modification of _PAGE_CHG_MASK was irrelevant to its users then one may ask 
why a new symbol was introduced instead of reusing the existing one with that 
otherwise irrelevant bit (_PAGE_PSE in that case) added.  I've initially 
assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT) included 
into it was intentional for some reason.  Maybe Johannes Weiner, the author of 
that patch (adding him to Cc:), could shed more light on that.

Thanks,
Janusz

Re: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Edgecombe, Rick P]

On Wed, 2023-06-07 at 19:11 +0200, Janusz Krzysztofik wrote:
> On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote:
> > On 6/7/23 08:23, Janusz Krzysztofik wrote:
> > > 
> > > Extend bitmask used by pgprot_modify() for selecting bits to be
> > > preserved
> > > with _PAGE_PAT bit.  However, since that bit can be reused as
> > > _PAGE_PSE,
> > > and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is
> > > likely
> > > intentionally defined with that bit not set, keep that symbol
> > > unchanged.
> > 
> > I'm really having a hard time parsing what that last sentence is
> > saying.
> > 
> > Could you try again, please?
> 
> OK, but then I need to get my doubts addressed by someone first,
> otherwise I'm 
> not able to provide a better justification from my heart.
> 
> The issue needs to be fixed by including _PAGE_PAT bit into a bitmask
> used 
> by pgprot_modify() for selecting bits to be preserved.  We can either
> do 
> that internally to pgprot_modify() (my initial proposal, which my
> poorly 
> worded paragraph was still trying to describe and justify), or by
> making 
> _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and
> reflected in 
> my v2 changelog.  But for the latter, I think we need to make sure
> that we 
> don't break other users of _PAGE_CHG_MASK.  Maybe Borislav can
> confirm that's 
> the case.
> 
> Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge
> pmds' 
> counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c
> ("thp: add 
> pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no
> longer differ 
> from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter. 
> If such 
> modification of _PAGE_CHG_MASK was irrelevant to its users then one
> may ask 
> why a new symbol was introduced instead of reusing the existing one
> with that 
> otherwise irrelevant bit (_PAGE_PSE in that case) added.  I've
> initially 
> assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT)
> included 
> into it was intentional for some reason.  Maybe Johannes Weiner, the
> author of 
> that patch (adding him to Cc:), could shed more light on that.

So since _PAGE_PSE is actually the same value as _PAGE_PAT, you don't
actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get
functional correctness. Is that right?

I think it is still a little hidden (even before this) and I wonder
about separating out the common bits into, like, _COMMON_PAGE_CHG_MASK
or something. Then setting specific PAGE and HPAGE bits (like
_PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define.
Would it be more readable that way?

Re: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Janusz Krzysztofik]

On Wednesday, 7 June 2023 23:12:13 CEST Edgecombe, Rick P wrote:
> On Wed, 2023-06-07 at 19:11 +0200, Janusz Krzysztofik wrote:
> > On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote:
> > > On 6/7/23 08:23, Janusz Krzysztofik wrote:
> > > > 
> > > > Extend bitmask used by pgprot_modify() for selecting bits to be
> > > > preserved
> > > > with _PAGE_PAT bit.  However, since that bit can be reused as
> > > > _PAGE_PSE,
> > > > and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is
> > > > likely
> > > > intentionally defined with that bit not set, keep that symbol
> > > > unchanged.
> > > 
> > > I'm really having a hard time parsing what that last sentence is
> > > saying.
> > > 
> > > Could you try again, please?
> > 
> > OK, but then I need to get my doubts addressed by someone first,
> > otherwise I'm 
> > not able to provide a better justification from my heart.
> > 
> > The issue needs to be fixed by including _PAGE_PAT bit into a bitmask
> > used 
> > by pgprot_modify() for selecting bits to be preserved.  We can either
> > do 
> > that internally to pgprot_modify() (my initial proposal, which my
> > poorly 
> > worded paragraph was still trying to describe and justify), or by
> > making 
> > _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and
> > reflected in 
> > my v2 changelog.  But for the latter, I think we need to make sure
> > that we 
> > don't break other users of _PAGE_CHG_MASK.  Maybe Borislav can
> > confirm that's 
> > the case.
> > 
> > Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge
> > pmds' 
> > counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c
> > ("thp: add 
> > pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no
> > longer differ 
> > from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter. 
> > If such 
> > modification of _PAGE_CHG_MASK was irrelevant to its users then one
> > may ask 
> > why a new symbol was introduced instead of reusing the existing one
> > with that 
> > otherwise irrelevant bit (_PAGE_PSE in that case) added.  I've
> > initially 
> > assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT)
> > included 
> > into it was intentional for some reason.  Maybe Johannes Weiner, the
> > author of 
> > that patch (adding him to Cc:), could shed more light on that.
> 
> So since _PAGE_PSE is actually the same value as _PAGE_PAT, you don't
> actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get
> functional correctness. Is that right?

As soon as we add _PAGE_PAT to _PAGE_CHG_MASK -- yes, that's right.  But we 
may still want to add _PAGE_PSE to _HPAGE_CHG_MASK to have the need for that 
bit explicitly documented.

> 
> I think it is still a little hidden (even before this) and I wonder
> about separating out the common bits into, like, _COMMON_PAGE_CHG_MASK
> or something. Then setting specific PAGE and HPAGE bits (like
> _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define.
> Would it be more readable that way?

Yes, I think that's a good idea, and I can use it in my patch.

The question if _PAGE_PAT vel _PAGE_PSE added to _PAGE_CHG_MASK is really 
harmless for pte_modify() and its users is still open for me though.

Thanks,
Janusz

Re: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Edgecombe, Rick P]

On Wed, 2023-06-07 at 23:33 +0200, Janusz Krzysztofik wrote:
> > So since _PAGE_PSE is actually the same value as _PAGE_PAT, you
> > don't
> > actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get
> > functional correctness. Is that right?
> 
> As soon as we add _PAGE_PAT to _PAGE_CHG_MASK -- yes, that's right. 
> But we 
> may still want to add _PAGE_PSE to _HPAGE_CHG_MASK to have the need
> for that 
> bit explicitly documented.

_PAGE_PSE is already in _HPAGE_CHG_MASK though, right? I'm confused.

> 
> > 
> > I think it is still a little hidden (even before this) and I wonder
> > about separating out the common bits into, like,
> > _COMMON_PAGE_CHG_MASK
> > or something. Then setting specific PAGE and HPAGE bits (like
> > _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define.
> > Would it be more readable that way?
> 
> Yes, I think that's a good idea, and I can use it in my patch.
> 
> The question if _PAGE_PAT vel _PAGE_PSE added to _PAGE_CHG_MASK is
> really 
> harmless for pte_modify() and its users is still open for me though.

When you say "vel", this is similar to the english acronym "AKA" I
think?

So I think you mean, when you add _PAGE_PAT to _PAGE_CHG_MASK, you are
also adding _PAGE_PSE to it. So does that cause any problems? I see,
good question... 

vm_page_prot is used when creating PTEs and huge PMDs, and the setter
only uses _PAGE_CHG_MASK, even though it won't actually know where that
prot is going to end up.

Having _PAGE_PAT/PSE in _PAGE_CHG_MASK certainly doesn't make it easier
to think about. One thing it's favor though is vm_page_prot is not
applied to page table entries that are pointing to other page table
entries (PSE = 0). So you shouldn't accidentally set PSE=1. And
_PAGE_PSE shouldn't be being set in there, so you also shouldn't
accidentally be setting PAT=1.

But yea, I see why you are concerned. I would /guess/ it would be ok
functionally. That probably doesn't help much...

Re: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Juergen Gross]

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

On 07.06.23 23:12, Edgecombe, Rick P wrote:
> On Wed, 2023-06-07 at 19:11 +0200, Janusz Krzysztofik wrote:
>> On Wednesday, 7 June 2023 17:31:24 CEST Dave Hansen wrote:
>>> On 6/7/23 08:23, Janusz Krzysztofik wrote:
>>>>
>>>> Extend bitmask used by pgprot_modify() for selecting bits to be
>>>> preserved
>>>> with _PAGE_PAT bit.  However, since that bit can be reused as
>>>> _PAGE_PSE,
>>>> and the _PAGE_CHG_MASK symbol, primarly used by pte_modify(), is
>>>> likely
>>>> intentionally defined with that bit not set, keep that symbol
>>>> unchanged.
>>>
>>> I'm really having a hard time parsing what that last sentence is
>>> saying.
>>>
>>> Could you try again, please?
>>
>> OK, but then I need to get my doubts addressed by someone first,
>> otherwise I'm
>> not able to provide a better justification from my heart.
>>
>> The issue needs to be fixed by including _PAGE_PAT bit into a bitmask
>> used
>> by pgprot_modify() for selecting bits to be preserved.  We can either
>> do
>> that internally to pgprot_modify() (my initial proposal, which my
>> poorly
>> worded paragraph was still trying to describe and justify), or by
>> making
>> _PAGE_PAT a part of _PAGE_CHG_MASK, as suggested by Borislav and
>> reflected in
>> my v2 changelog.  But for the latter, I think we need to make sure
>> that we
>> don't break other users of _PAGE_CHG_MASK.  Maybe Borislav can
>> confirm that's
>> the case.
>>
>> Since _PAGE_PAT is the same as _PAGE_PSE, _HPAGE_CHG_MASK -- a huge
>> pmds'
>> counterpart of _PAGE_CHG_MASK, introduced by commit c489f1257b8c
>> ("thp: add
>> pmd_modify"), defined as (_PAGE_CHG_MASK | _PAGE_PSE) -- will no
>> longer differ
>> from _PAGE_CHG_MASK as soon as we add _PAGE_PAT bit to the latter.
>> If such
>> modification of _PAGE_CHG_MASK was irrelevant to its users then one
>> may ask
>> why a new symbol was introduced instead of reusing the existing one
>> with that
>> otherwise irrelevant bit (_PAGE_PSE in that case) added.  I've
>> initially
>> assumed that keeping _PAGE_CHG_MASK without _PAGE_PSE (vel _PAGE_PAT)
>> included
>> into it was intentional for some reason.  Maybe Johannes Weiner, the
>> author of
>> that patch (adding him to Cc:), could shed more light on that.
> 
> So since _PAGE_PSE is actually the same value as _PAGE_PAT, you don't
> actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get
> functional correctness. Is that right?
> 
> I think it is still a little hidden (even before this) and I wonder
> about separating out the common bits into, like, _COMMON_PAGE_CHG_MASK
> or something. Then setting specific PAGE and HPAGE bits (like
> _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define.
> Would it be more readable that way?

I'd go that route. I don't think we should rely on _PAGE_PSE == _PAGE_PAT
here.


Juergen

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

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

Re: [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

[Janusz Krzysztofik]

On Thursday, 8 June 2023 00:47:36 CEST Edgecombe, Rick P wrote:
> On Wed, 2023-06-07 at 23:33 +0200, Janusz Krzysztofik wrote:
> > > So since _PAGE_PSE is actually the same value as _PAGE_PAT, you
> > > don't
> > > actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get
> > > functional correctness. Is that right?
> > 
> > As soon as we add _PAGE_PAT to _PAGE_CHG_MASK -- yes, that's right. 
> > But we 
> > may still want to add _PAGE_PSE to _HPAGE_CHG_MASK to have the need
> > for that 
> > bit explicitly documented.
> 
> _PAGE_PSE is already in _HPAGE_CHG_MASK though, right? I'm confused.

Yes, it is, sorry for confusion.  I should have said: we may still want to 
keep _PAGE_PSE explicitly added to _HPAGE_CHK_MASK to have the reason for 
including that bit documented.

Thanks,
Janusz


> 
> > 
> > > 
> > > I think it is still a little hidden (even before this) and I wonder
> > > about separating out the common bits into, like,
> > > _COMMON_PAGE_CHG_MASK
> > > or something. Then setting specific PAGE and HPAGE bits (like
> > > _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define.
> > > Would it be more readable that way?
> > 
> > Yes, I think that's a good idea, and I can use it in my patch.
> > 
> > The question if _PAGE_PAT vel _PAGE_PSE added to _PAGE_CHG_MASK is
> > really 
> > harmless for pte_modify() and its users is still open for me though.
> 
> When you say "vel", this is similar to the english acronym "AKA" I
> think?
> 
> So I think you mean, when you add _PAGE_PAT to _PAGE_CHG_MASK, you are
> also adding _PAGE_PSE to it. So does that cause any problems? I see,
> good question... 
> 
> vm_page_prot is used when creating PTEs and huge PMDs, and the setter
> only uses _PAGE_CHG_MASK, even though it won't actually know where that
> prot is going to end up.
> 
> Having _PAGE_PAT/PSE in _PAGE_CHG_MASK certainly doesn't make it easier
> to think about. One thing it's favor though is vm_page_prot is not
> applied to page table entries that are pointing to other page table
> entries (PSE = 0). So you shouldn't accidentally set PSE=1. And
> _PAGE_PSE shouldn't be being set in there, so you also shouldn't
> accidentally be setting PAT=1.
> 
> But yea, I see why you are concerned. I would /guess/ it would be ok
> functionally. That probably doesn't help much...
>