add helper for highmem checks

I was auding the /dev/mem code for more questionable uses of __pa(), and ran across this.

My assumption is that if you use /dev/kmem, you expect to be able to read the kernel virtual mappings. However, those mappings _stop_ as soon as we hit high memory. The pfn_valid() check in here is good for memory holes, but since highmem pages are still valid, it does no good for those.

Also, since we are now checking that __pa() is being done on valid virtual addresses, this might have tripped the new check. Even with the new check, this code would have been broken with the NUMA remapping code had we not ripped it out:

#endif
diff -puN include/linux/mm.h~clean-up-highmem-checks include/linux/mm.h

include/linux/mm.h renamed from include/linux/mm.h~clean-up-highmem-checks +3 -0

@@ -1771,5 +1771,18 @@ static inline unsigned int debug_guardpa
  static inline bool page_is_guard(struct page *page) { return false; }
  #endif /* CONFIG_DEBUG_PAGEALLOC */
 
+static inline phys_addr_t last_lowmem_phys_addr(void)
+{
+	/*

Quoted 9 lines · Dave Hansen

include/linux/mm.h renamed from include/linux/mm.h~clean-up-highmem-checks +9 -0

+static inline phys_addr_t last_lowmem_phys_addr(void)
+{
+    /*
+     * 'high_memory' is not a pointer that can be dereferenced, so
+     * avoid calling __pa() on it directly.
+     */
+    return __pa(high_memory - 1);
+}
+static inline bool phys_addr_is_highmem(phys_addr_t addr)

include/linux/mm.h static inline unsigned int debug_guardpa · line 1786

Quoted 4 lines

include/linux/mm.h renamed from include/linux/mm.h~clean-up-highmem-checks +4 -0

+{
+    return addr > last_lowmem_paddr();
+}
+

Are we sure that high_memory - 1 is always a valid reference? Consider especially the case where there is MMIO beyond end of memory on a system which has less RAM than the HIGHMEM boundary...

Boris, could you check that this series also fixes the /dev/mem problem you were seeing?

--

We have a new debugging check on x86 that has caught a number of long-standing bugs. However, there is a _bit_ of collateral damage with things that call __pa(high_memory).

We are now checking that any addresses passed to __pa() are *valid* and can be dereferenced.

"high_memory", however, is not valid. It marks the start of highmem, and isn't itself a valid pointer. But, those users are really just asking "is this vaddr mapped"? So, give them a helper that does that, plus is also kind to our new debugging check.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/arch/x86/mm/pat.c     |   11 ++++++-----
 linux-2.6.git-dave/drivers/char/mem.c    |    4 ++--
 linux-2.6.git-dave/drivers/mtd/mtdchar.c |    2 +-
 linux-2.6.git-dave/include/linux/mm.h    |   13 +++++++++++++
 4 files changed, 22 insertions(+), 8 deletions(-)

diff -puN drivers/char/mem.c~clean-up-highmem-checks drivers/char/mem.c

drivers/char/mem.c renamed from drivers/char/mem.c~clean-up-highmem-checks +2 -2

@@ -51,7 +51,7 @@ static inline unsigned long size_inside_
 #ifndef ARCH_HAS_VALID_PHYS_ADDR_RANGE
 static inline int valid_phys_addr_range(phys_addr_t addr, size_t count)
 {
-	return addr + count <= __pa(high_memory);
+	return !phys_addr_is_highmem(addr + count);
 }
 
 static inline int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)

@@ -250,7 +250,7 @@ static int uncached_access(struct file *
 	 */
 	if (file->f_flags & O_DSYNC)
 		return 1;
-	return addr >= __pa(high_memory);
+	return phys_addr_is_highmem(addr);
 #endif
 }
 #endif

diff -puN include/linux/mm.h~clean-up-highmem-checks include/linux/mm.h

include/linux/mm.h renamed from include/linux/mm.h~clean-up-highmem-checks +1 -0

@@ -1771,5 +1771,18 @@ static inline unsigned int debug_guardpa
 static inline bool page_is_guard(struct page *page) { return false; }
 #endif /* CONFIG_DEBUG_PAGEALLOC */
 
+static inline phys_addr_t last_lowmem_phys_addr(void)

include/linux/mm.h static inline unsigned int debug_guardpa · line 1784

Quoted 3 lines

include/linux/mm.h renamed from include/linux/mm.h~clean-up-highmem-checks +3 -0

+static inline bool phys_addr_is_highmem(phys_addr_t addr)
+{
+	return addr > last_lowmem_paddr();

I think you mean last_lowmem_phys_addr() here:

include/linux/mm.h: In function a??phys_addr_is_highmema??:
include/linux/mm.h:1764:2: error: implicit declaration of function a??last_lowmem_paddra?? [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
make[1]: *** [arch/x86/kernel/asm-offsets.s] Error 1

Changed.

On Sat, Feb 09, 2013 at 10:41:21AM +0100, Borislav Petkov wrote:
With this change, they definitely fix something because I even get X on the box started. Previously, it would spit out the warning and wouldn't start X with the login window. And my suspicion is that wdm (WINGs display manager) I'm using, does /dev/mem accesses when it starts and it obviously failed. Now not so much :-)

Quoted 6 lines · Borislav Petkov

On Sat, Feb 09, 2013 at 10:41:21AM +0100, Borislav Petkov wrote:
With this change, they definitely fix something because I even get X on the box started. Previously, it would spit out the warning and wouldn't start X with the login window. And my suspicion is that wdm (WINGs display manager) I'm using, does /dev/mem accesses when it starts and it obviously failed. Now not so much :-)

That's crazy. Didn't expect that at all.

I guess X is happier getting an error than getting random pages back. I'm working on a set of patches now that should get it _working_ instead of just returning an error.

That's crazy. Didn't expect that at all.

I guess X is happier getting an error than getting random pages back.

I'm working on a set of patches now that should get it _working_ instead of just returning an error.

Quoted 3 lines · Dave Hansen

That's crazy. Didn't expect that at all.

I guess X is happier getting an error than getting random pages back.

Yeah, I think this is something special only this window manager wdm does. The line below has appeared repeatedly in the logs earlier:

Feb  5 23:02:02 a1 wdm: Cannot read randomFile "/dev/mem", errno = 14

This happens when wdm starts so I'm going to guess it uses it for something funny, "randomFile" it calls it??

With the WARN_ON check added and booting 3.8-rc6, it would choke wdm somehow and it wouldn't start properly so that even the error out above doesn't happen. Oh well ...

Quoted 2 lines

I'm working on a set of patches now that should get it _working_ instead of just returning an error.

Yeah, send them on and I'll run them.

Thanks.

Oh, craptastic. X used to hash /dev/mem to get a random seed. It should have stopped that long ago, and used /dev/[u]random.

Quoted 2 lines · H. Peter Anvin

Oh, craptastic. X used to hash /dev/mem to get a random seed. It should have stopped that long ago, and used /dev/[u]random.

That's because debian still has this WINGs window manager which hasn't seen any new releases since 2005: http://voins.program.ru/wdm/ and I'm using it because I don't want the pompous crap of the other display managers.

But this one uses /dev/mem as a randomFile only by default - there's a configuration variable DisplayManager.randomFile which can be pointed away from /dev/mem so that's easily fixable.

Mind you, I wouldnt've caught the issue if I wasn't using this ancient thing in its default settings :o).

The X server itself used to do that. Are you saying that wdm is a *privileged process*?

Quoted 2 lines · H. Peter Anvin

The X server itself used to do that. Are you saying that wdm is a *privileged process*?

Nah, it is a simple display manager you start with /etc/init.d/wdm init script. Like the other display managers gdm, kdm, etc.

But it looks like wdm has copied stuff from xdm (from the README):

"Wdm is a modification of XFree86's xdm package for graphically handling authentication and system login. Most of xdm has been preserved (XFree86 4.2.1.1) with the Login interface based on a WINGs implementation using Tom Rothamel's "external greet" interface (see AUTHORS)."

And from looking at the part in the source which does the /dev/mem accesses, it comes from XFree86's source apparently, this is at the beginning of src/wdm/genauth.c: