Skip to contentSkip to conversation

x86: Fix /dev/mem mmap breakage when PAT is disabled

PATCH Applied Ravikiran G Thirumalai 3 participants 8 messages Oct 30, 2008

Subsystems (2)
x86 x86 MM
lore
Share & tools
lore

Ravikiran G Thirumalai

author
#0 lore raw

PATCH 1/1 x86: Fix /dev/mem mmap breakage when PAT is disabled

Fix mmap to /dev/mem when CONFIG_X86_PAT is off and CONFIG_STRICT_DEVMEM is off

mmap to /dev/mem on kernel memory has been failing since the
introduction of PAT (CONFIG_STRICT_DEVMEM=n case). Seems like
the check to avoid cache aliasing with PAT is kicking in even when PAT is disabled. The bug seems to have crept in 2.6.26.

This patch makes sure that mmap to regular kernel memory succeeds if CONFIG_STRICT_DEVMEM=n and
PAT is disabled. The checks to avoid cache aliasing still happens if PAT is enabled.

Signed-off-by Ravikiran Thirumalai <[email protected]>
Tested-by Tim Sirianni <[email protected]>
arch/x86/mm/pat.c +3 -1
Index: git.tip/arch/x86/mm/pat.c
===================================================================
@@ -481,6 +481,7 @@
 	return 1;
 }
 #else
+/* This check is needed to avoid cache aliasing when PAT is enabled */
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
 	u64 from = ((u64)pfn) << PAGE_SHIFT;

@@ -508,7 +509,8 @@
 	unsigned long flags = -1;
 	int retval;
 
-	if (!range_is_allowed(pfn, size))
+	/* Ensure cache aliasing does not occur if and only if PAT is on */
+	if (pat_enabled && !range_is_allowed(pfn, size))
 		return 0;
 
 	if (file->f_flags & O_SYNC) {
#1 lore raw
Quoted 29 lines · Ravikiran G Thirumalai

Fix mmap to /dev/mem when CONFIG_X86_PAT is off and CONFIG_STRICT_DEVMEM is off

mmap to /dev/mem on kernel memory has been failing since the
introduction of PAT (CONFIG_STRICT_DEVMEM=n case). Seems like
the check to avoid cache aliasing with PAT is kicking in even when PAT is disabled. The bug seems to have crept in 2.6.26.

This patch makes sure that mmap to regular kernel memory succeeds if CONFIG_STRICT_DEVMEM=n and
PAT is disabled. The checks to avoid cache aliasing still happens if PAT is enabled.

Signed-off-by Ravikiran Thirumalai <[email protected]>
Tested-by Tim Sirianni <[email protected]>

Index: git.tip/arch/x86/mm/pat.c
===================================================================

arch/x86/mm/pat.c +1 -0
@@ -481,6 +481,7 @@
 	return 1;
 }
 #else
+/* This check is needed to avoid cache aliasing when PAT is enabled */
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
 	u64 from = ((u64)pfn) << PAGE_SHIFT;

arch/x86/mm/pat.c line 514

Quoted 7 lines
arch/x86/mm/pat.c +2 -1
@@ -508,7 +509,8 @@
 	unsigned long flags = -1;
 	int retval;
 
-	if (!range_is_allowed(pfn, size))
+	/* Ensure cache aliasing does not occur if and only if PAT is on */
+	if (pat_enabled && !range_is_allowed(pfn, size))
 		return 0;

I guess it might be cleaner to push this check into the range_is_allowed() function?

Ingo

author Tested-by Tim Sirianni
#2 lore raw
Quoted 4 lines · Ingo Molnar

I guess it might be cleaner to push this check into the range_is_allowed() function?

OK. Here it is.

Fix mmap to /dev/mem when CONFIG_X86_PAT is off and CONFIG_STRICT_DEVMEM is off

mmap to /dev/mem on kernel memory has been failing since the
introduction of PAT (CONFIG_STRICT_DEVMEM=n case). Seems like
the check to avoid cache aliasing with PAT is kicking in even when PAT is disabled. The bug seems to have crept in 2.6.26.

This patch makes sure that the mmap to regular kernel memory succeeds if CONFIG_STRICT_DEVMEM=n and PAT is disabled, and the checks to avoid cache aliasing still happens if PAT is enabled.

Signed-off-by Ravikiran Thirumalai <[email protected]>
Tested-by Tim Sirianni <[email protected]>
arch/x86/mm/pat.c +4 -0
Index: git.tip/arch/x86/mm/pat.c
===================================================================
@@ -481,12 +481,16 @@
 	return 1;
 }
 #else
+/* This check is needed to avoid cache aliasing when PAT is enabled */
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
 	u64 from = ((u64)pfn) << PAGE_SHIFT;
 	u64 to = from + size;
 	u64 cursor = from;
 
+	if (!pat_enabled)
+		return 1;
+
 	while (cursor < to) {
 		if (!devmem_is_allowed(pfn)) {
 			printk(KERN_INFO
Applied
#3 lore raw
Quoted 7 lines · Ravikiran G Thirumalai
Quoted 4 lines · Ingo Molnar

I guess it might be cleaner to push this check into the range_is_allowed() function?

OK. Here it is.

applied to tip/x86/urgent, thanks!

Ingo

#4 lore raw
In reply to Ravikiran G Thirumalai
Quoted 8 lines · On Wed, 29 Oct 2008 19:02:03 -0700 Ravikiran G Thirumalai <[email protected]>…

Fix mmap to /dev/mem when CONFIG_X86_PAT is off and CONFIG_STRICT_DEVMEM is off

mmap to /dev/mem on kernel memory has been failing since the
introduction of PAT (CONFIG_STRICT_DEVMEM=n case). Seems like
the check to avoid cache aliasing with PAT is kicking in even when PAT is disabled. The bug seems to have crept in 2.6.26.

Quoted 4 lines

This patch makes sure that mmap to regular kernel memory succeeds if CONFIG_STRICT_DEVMEM=n and
PAT is disabled. The checks to avoid cache aliasing still happens if PAT is enabled.

well... technically the aliases are bad without PAT as well...

Signature · 3 lines
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
author
#5 lore raw
Quoted 5 lines · Arjan van de Ven

On Wed, 29 Oct 2008 19:02:03 -0700 Ravikiran G Thirumalai <[email protected]> wrote:

well... technically the aliases are bad without PAT as well...

Hmm! Well, you mean if mtrr is used to change attributes?

I guess there is always a risk associated with luser space mmaping /dev/mem, and we have CONFIG_STRICT_DEVMEM to avoid this in production. However, for debugging tools, it is worth having the capability in _debug_ kernels and living with the risk IMHO.

Thanks,
Kiran

#6 lore raw
Quoted 8 lines · On Thu, 30 Oct 2008 14:21:21 -0700 Ravikiran G Thirumalai <[email protected]>…
Quoted 5 lines · Arjan van de Ven

On Wed, 29 Oct 2008 19:02:03 -0700 Ravikiran G Thirumalai <[email protected]> wrote:

well... technically the aliases are bad without PAT as well...

Hmm! Well, you mean if mtrr is used to change attributes?

you can also set uncached in the pagetables, like via ioremap_uncached and other ways.

and /dev/mem may or may not imply cached/uncached, depending on the open flags.

On memory... what would that mean?

Signature · 3 lines
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
author
#7 lore raw
Quoted 12 lines · Arjan van de Ven
Quoted 8 lines · On Thu, 30 Oct 2008 14:21:21 -0700 Ravikiran G Thirumalai <[email protected]>…
Quoted 5 lines · Arjan van de Ven

On Wed, 29 Oct 2008 19:02:03 -0700 Ravikiran G Thirumalai <[email protected]> wrote:

well... technically the aliases are bad without PAT as well...

Hmm! Well, you mean if mtrr is used to change attributes?

Quoted 8 lines

you can also set uncached in the pagetables, like via ioremap_uncached and other ways.

and /dev/mem may or may not imply cached/uncached, depending on the open flags.

On memory... what would that mean?

Well, I am not denying that an incorrectly written userspace program could cause cache aliasing/crash the system I am not arguing for or against this argument at all.

However, there is what seems like an unintended change in behavior from 2.6.25 to 2.6.26 when CONFIG_STRICT_DEVMEM is disabled and PAT is disabled, which is a bug, and this patch fixes it that's all. If this was intentional, then I don't see the reason for having CONFIG_STRICT_DEVMEM!!