[PATCH] x86/mtrr: Correct the returned MTRR type of mtrr_type_lookup.

[PATCH] x86/mtrr: Correct the returned MTRR type of mtrr_type_lookup.

[Ying-Tsun Huang]

In mtrr_type_lookup, if the input memory address region is not in the
MTRR, over 4GB, and not over the top of memory, write-back attribute
is returned. These condition checks are for ensuring the input memory
address region is mapped to the physical memory actually.

However, if the end address is just aligned with the top of memory,
the condition check treats the address is over the top of memory, and
write-back attribute is not returned.

There is a real case of NVDIMM. The nd_pmem module tries to map
NVDIMMs as cacheable memories when NVDIMMs are connected. If a NVDIMM
is the last of the DIMMs, the performance of this NVDIMM becomes very
low since it aligned with the top of memory and its memory type is
uncached-minus.

To check the top of memory should use "<=" instead of "<" since both the
input end address and the value of top of memory are actually the start
of next region.

Fixes: b73522e0c1be ("x86/mm/mtrr: Enhance MTRR checks in kernel mapping
helpers")
Signed-off-by: Ying-Tsun Huang <ying-tsun.huang@amd.com>
---
 arch/x86/kernel/cpu/mtrr/generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 23ad8e953dfb..b13e87dba27d 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -303,7 +303,7 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
 			goto out;
 	}
 
-	if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
+	if (mtrr_tom2 && (start >= (1ULL<<32)) && (end <= mtrr_tom2))
 		type = MTRR_TYPE_WRBACK;
 
 out:
-- 
2.25.1

Re: [PATCH] x86/mtrr: Correct the returned MTRR type of mtrr_type_lookup.

[Borislav Petkov]

On Mon, Dec 07, 2020 at 02:12:26PM +0800, Ying-Tsun Huang wrote:
> In mtrr_type_lookup, if the input memory address region is not in the
> MTRR, over 4GB, and not over the top of memory, write-back attribute
> is returned. These condition checks are for ensuring the input memory
> address region is mapped to the physical memory actually.
> 
> However, if the end address is just aligned with the top of memory,
> the condition check treats the address is over the top of memory, and
> write-back attribute is not returned.

Oh fun. So to make sure I understand this correctly end ends up equal to
TOM2?

> There is a real case of NVDIMM. The nd_pmem module tries to map
> NVDIMMs as cacheable memories when NVDIMMs are connected. If a NVDIMM
> is the last of the DIMMs, the performance of this NVDIMM becomes very
> low since it aligned with the top of memory and its memory type is
> uncached-minus.
> 
> To check the top of memory should use "<=" instead of "<" since both the
> input end address and the value of top of memory are actually the start
> of next region.

Right, so looking at that function, it calls
mtrr_type_lookup_variable(), among others, and that does:

        /* Make end inclusive instead of exclusive */
        end--;

which sounds to me like it expects ranges with exclusive end.

So maybe it would be better to do something like:

	/*
	 * Blurb about end address being == tom2, perhaps give your example
	 */
	end--;

above the check so that it is absolutely obvious why this is done.

But but, looking at this more, the PPR says about TOM2:

"This value is normally placed above 4G. From 4G to TOM2 - 1 is DRAM;
TOM2 and above is MMIO."

So the check is *actually* correct - TOM2 - 1 is DRAM so you need '<'.

Unless you do end-- before, which would make sense and suggest the end
decrement to be the proper fix.

Hmm?

> Fixes: b73522e0c1be ("x86/mm/mtrr: Enhance MTRR checks in kernel mapping
> helpers")

I think you mean:

35605a1027ac ("x86: enable PAT for amd k8 and fam10h")

which first added that check.

Btw, while doing git archeology, I saw that mtrr_type_lookup() used to do end--
on function entry, see

2e5d9c857d4e ("x86: PAT infrastructure patch")

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/mtrr: Correct the returned MTRR type of mtrr_type_lookup.

[Huang, Ying-Tsun]

On Fri, 11 Dec 2020, Borislav Petkov wrote:

> [CAUTION: External Email]
> 
> On Mon, Dec 07, 2020 at 02:12:26PM +0800, Ying-Tsun Huang wrote:
> > In mtrr_type_lookup, if the input memory address region is not in the
> > MTRR, over 4GB, and not over the top of memory, write-back attribute
> > is returned. These condition checks are for ensuring the input memory
> > address region is mapped to the physical memory actually.
> >
> > However, if the end address is just aligned with the top of memory,
> > the condition check treats the address is over the top of memory, and
> > write-back attribute is not returned.
> 
> Oh fun. So to make sure I understand this correctly end ends up equal to
> TOM2?
> 

Yes, it's equal to TOM2.

> > There is a real case of NVDIMM. The nd_pmem module tries to map
> > NVDIMMs as cacheable memories when NVDIMMs are connected. If a NVDIMM
> > is the last of the DIMMs, the performance of this NVDIMM becomes very
> > low since it aligned with the top of memory and its memory type is
> > uncached-minus.
> >
> > To check the top of memory should use "<=" instead of "<" since both the
> > input end address and the value of top of memory are actually the start
> > of next region.
> 
> Right, so looking at that function, it calls
> mtrr_type_lookup_variable(), among others, and that does:
> 
>         /* Make end inclusive instead of exclusive */
>         end--;
> 
> which sounds to me like it expects ranges with exclusive end.
> 
> So maybe it would be better to do something like:
> 
>         /*
>          * Blurb about end address being == tom2, perhaps give your example
>          */
>         end--;
> 
> above the check so that it is absolutely obvious why this is done.
> 
> But but, looking at this more, the PPR says about TOM2:
> 
> "This value is normally placed above 4G. From 4G to TOM2 - 1 is DRAM;
> TOM2 and above is MMIO."
> 
> So the check is *actually* correct - TOM2 - 1 is DRAM so you need '<'.
> 
> Unless you do end-- before, which would make sense and suggest the end
> decrement to be the proper fix.
> 
> Hmm?
> 

I think your comment makes sense, "TOM2 - 1" is the last DRAM address, we 
should make the end be included in the DRAM range, and the condition check 
becomes more precise to use "<", even the both codes are with the same
result.

> > Fixes: b73522e0c1be ("x86/mm/mtrr: Enhance MTRR checks in kernel mapping
> > helpers")
> 
> I think you mean:
> 
> 35605a1027ac ("x86: enable PAT for amd k8 and fam10h")
> 
> which first added that check.
> 
> Btw, while doing git archeology, I saw that mtrr_type_lookup() used to do end--
> on function entry, see
> 
> 2e5d9c857d4e ("x86: PAT infrastructure patch")
> 

With your above comments, the bug should come from
0cc705f56e40 ("x86/mm/mtrr: Clean up mtrr_type_lookup()"

Before this commit, the "end--" is in __mtrr_type_lookup(), and the end 
address is not used in mtrr_type_lookup, so the condition check is 
correct.

After the commit, the end address is started to be used in 
mtrr_type_lookup(), but the "end--" is in the mtrr_type_lookup_variable(), 
it makes the check mtrr_type_lookup() become incorrect.

> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cying-tsun.huang%40amd.com%7C2e57de535baf40ff480f08d89dc2e51d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432807709717408%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tM5ww%2F96JirsAsDZyImHHtsR3L8TQyr%2B5xWeaGbdwH4%3D&amp;reserved=0
>