Re: [v2.6.26] what's brewing in x86.git for v2.6.26

From: Vegard Nossum
Date: Thu Apr 17 2008 - 14:47:24 EST

Next message: PEUGEOT ONLINE: "AWARD!!!"
Previous message: Alan Stern: "Re: Semphore -> mutex in the device tree"
In reply to: Pekka Enberg: "Re: [v2.6.26] what's brewing in x86.git for v2.6.26"
Next in thread: Ingo Molnar: "Re: [v2.6.26] what's brewing in x86.git for v2.6.26"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Apr 17, 2008 at 11:36 AM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 17 Apr 2008 11:30:25 +0200 Ingo Molnar <mingo@xxxxxxx> wrote:
> > you mean kmemcheck? Yes, that's planned. We've been working 4 months
> > non-stop on kmemcheck to make it mergeable and usable, it's at version 7
> > right now, and it caught a handful of real bugs already (such as
> > 63a7138671c - unfortunately not credited in the log to kmemcheck). But
> > because it touches SLUB (because it has to - and they are acked by
> > Pekka) i never had the chance to move it into the for-akpm branch.
>
> Does it really really really need to consume one of our few remaining page
> flags? We'll be in a mess when we run out.

Actually it doesn't. I attach a patch which gets rid of the page flag,
and we rely instead on the PTE flag for page-trackedness.

The reason we didn't do this at once is that the making of kmemcheck
has been pretty much my first introduction to SLUB, x86, page flags,
etc., and the actual semantics of the various introduced flags have
varied since the first version of kmemcheck. At this point, the struct
page flags weren't actually needed anymore, but they were convenient.

My apologies for not inlining the patch -- I don't have a mail client
that won't mess up whitespace. It can also be downloaded at:
http://folk.uio.no/vegardno/linux/0001-kmemcheck-remove-use-of-tracked-page-flag.patch

The patch has received minimal amount of testing, but I've
double-checked the logic. It boots fine on my laptop, boot log at:
http://folk.uio.no/vegardno/linux/kmemcheck-20080417.txt

Ingo, will you take this for some additional testing?

Vegard

--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
From 9860cfa3a38f72b82a83024b765d2aab39a917e9 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@xxxxxxxxx>
Date: Thu, 17 Apr 2008 19:53:48 +0200
Subject: [PATCH] kmemcheck: remove use of "tracked" page flag

This patch replaces the use of the PG_tracked page flag and rely instead
on the PTE flag PAGE_HIDDEN to determine whether a page is tracked.

Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxx>
---
arch/x86/kernel/kmemcheck.c | 105 +++++++++++++++++-------------------------
include/linux/kmemcheck.h | 2 +
include/linux/page-flags.h | 5 --
mm/slub.c | 2 +-
4 files changed, 46 insertions(+), 68 deletions(-)

diff --git a/arch/x86/kernel/kmemcheck.c b/arch/x86/kernel/kmemcheck.c
index 16dce10..d82f35d 100644
--- a/arch/x86/kernel/kmemcheck.c
+++ b/arch/x86/kernel/kmemcheck.c
@@ -233,12 +233,27 @@ param_kmemcheck(char *str)
if (!str)
return -EINVAL;

- sscanf("%d", str, &kmemcheck_enabled);
+ sscanf(str, "%d", &kmemcheck_enabled);
return 0;
}

early_param("kmemcheck", param_kmemcheck);

+static pte_t *
+address_get_pte(unsigned int address)
+{
+ pte_t *pte;
+ int level;
+
+ pte = lookup_address(address, &level);
+ if (!pte)
+ return NULL;
+ if (!pte_hidden(*pte))
+ return NULL;
+
+ return pte;
+}
+
/*
* Return the shadow address for the given address. Returns NULL if the
* address is not tracked.
@@ -249,88 +264,53 @@ early_param("kmemcheck", param_kmemcheck);
static void *
address_get_shadow(unsigned long address)
{
+ pte_t *pte;
struct page *page;
struct page *head;

if (!virt_addr_valid(address))
return NULL;

+ pte = address_get_pte(address);
+ if (!pte)
+ return NULL;
+
/* The accessed page */
page = virt_to_page(address);
- if (!PageCompound(page))
- return NULL;
+ BUG_ON(!PageCompound(page));

/* The head page */
head = compound_head(page);
- if (!PageTracked(head))
- return NULL;
+ BUG_ON(compound_order(head) == 0);

return (void *) address + (PAGE_SIZE << (compound_order(head) - 1));
}

static int
-show_addr(uint32_t addr)
+show_addr(uint32_t address)
{
pte_t *pte;
- int level;
-
- if (!address_get_shadow(addr))
- return 0;
-
- pte = lookup_address(addr, &level);
- BUG_ON(!pte);
-
- if (level != PG_LEVEL_4K)
- return 0;
-
- set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT));
- __flush_tlb_one(addr);
- return 1;
-}

-/*
- * In case there's something seriously wrong with kmemcheck (like a recursive
- * or looping page fault), we should disable tracking for the page as a last
- * attempt to not hang the machine.
- */
-static void
-emergency_show_addr(uint32_t address)
-{
- pte_t *pte;
- int level;
-
- pte = lookup_address(address, &level);
+ pte = address_get_pte(address);
if (!pte)
- return;
- if (level != PG_LEVEL_4K)
- return;
-
- /* Don't change pages that weren't hidden in the first place -- they
- * aren't ours to modify. */
- if (!(pte_val(*pte) & _PAGE_HIDDEN))
- return;
+ return 0;

set_pte(pte, __pte(pte_val(*pte) | _PAGE_PRESENT));
__flush_tlb_one(address);
+ return 1;
}

static int
-hide_addr(uint32_t addr)
+hide_addr(uint32_t address)
{
pte_t *pte;
- int level;
-
- if (!address_get_shadow(addr))
- return 0;

- pte = lookup_address(addr, &level);
- BUG_ON(!pte);
-
- if (level != PG_LEVEL_4K)
+ pte = address_get_pte(address);
+ if (!pte)
return 0;

set_pte(pte, __pte(pte_val(*pte) & ~_PAGE_PRESENT));
- __flush_tlb_one(addr);
+ __flush_tlb_one(address);
return 1;
}

@@ -365,8 +345,8 @@ kmemcheck_show(struct pt_regs *regs)
BUG_ON(!irqs_disabled());

if (unlikely(data->balance != 0)) {
- emergency_show_addr(data->addr1);
- emergency_show_addr(data->addr2);
+ show_addr(data->addr1);
+ show_addr(data->addr2);
error_save_bug(regs);
data->balance = 0;
return;
@@ -414,8 +394,8 @@ kmemcheck_hide(struct pt_regs *regs)
return;

if (unlikely(data->balance != 1)) {
- emergency_show_addr(data->addr1);
- emergency_show_addr(data->addr2);
+ show_addr(data->addr1);
+ show_addr(data->addr2);
error_save_bug(regs);
data->addr1 = 0;
data->addr2 = 0;
@@ -456,9 +436,6 @@ kmemcheck_show_pages(struct page *p, unsigned int n)
{
unsigned int i;

- BUG_ON(!PageCompound(p));
- ClearPageTracked(compound_head(p));
-
for (i = 0; i < n; ++i) {
unsigned long address;
pte_t *pte;
@@ -477,14 +454,18 @@ kmemcheck_show_pages(struct page *p, unsigned int n)
}
}

+bool
+kmemcheck_page_is_tracked(struct page *p)
+{
+ /* This will also check the "hidden" flag of the PTE. */
+ return address_get_pte((unsigned long) page_address(p));
+}
+
void
kmemcheck_hide_pages(struct page *p, unsigned int n)
{
unsigned int i;

- BUG_ON(!PageCompound(p));
- SetPageTracked(compound_head(p));
-
for (i = 0; i < n; ++i) {
unsigned long address;
pte_t *pte;
@@ -762,7 +743,7 @@ kmemcheck_access(struct pt_regs *regs,

/* Recursive fault -- ouch. */
if (data->busy) {
- emergency_show_addr(fallback_address);
+ show_addr(fallback_address);
error_save_bug(regs);
return;
}
diff --git a/include/linux/kmemcheck.h b/include/linux/kmemcheck.h
index 801da50..8d129eb 100644
--- a/include/linux/kmemcheck.h
+++ b/include/linux/kmemcheck.h
@@ -9,6 +9,8 @@ void kmemcheck_init(void);
void kmemcheck_show_pages(struct page *p, unsigned int n);
void kmemcheck_hide_pages(struct page *p, unsigned int n);

+bool kmemcheck_page_is_tracked(struct page *p);
+
void kmemcheck_mark_unallocated(void *address, unsigned int n);
void kmemcheck_mark_uninitialized(void *address, unsigned int n);
void kmemcheck_mark_initialized(void *address, unsigned int n);
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 63f5fd8..3696889 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -89,7 +89,6 @@
#define PG_mappedtodisk 16 /* Has blocks allocated on-disk */
#define PG_reclaim 17 /* To be reclaimed asap */
#define PG_buddy 19 /* Page is free, on buddy lists */
-#define PG_tracked 20 /* Tracked by kmemcheck */

/* PG_readahead is only used for file reads; PG_reclaim is only for writes */
#define PG_readahead PG_reclaim /* Reminder to do async read-ahead */
@@ -297,10 +296,6 @@ static inline void __ClearPageTail(struct page *page)
#define SetPageUncached(page) set_bit(PG_uncached, &(page)->flags)
#define ClearPageUncached(page) clear_bit(PG_uncached, &(page)->flags)

-#define PageTracked(page) test_bit(PG_tracked, &(page)->flags)
-#define SetPageTracked(page) set_bit(PG_tracked, &(page)->flags)
-#define ClearPageTracked(page) clear_bit(PG_tracked, &(page)->flags)
-

struct page; /* forward declaration */

diff --git a/mm/slub.c b/mm/slub.c
index 9b58979..7a544e6 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1125,7 +1125,7 @@ static void __free_slab(struct kmem_cache *s, struct page *page)
ClearSlabDebug(page);
}

- if (PageTracked(page) && !(s->flags & SLAB_NOTRACK)) {
+ if (kmemcheck_page_is_tracked(page) && !(s->flags & SLAB_NOTRACK)) {
kmemcheck_free_slab(s, page, pages);
return;
}
--
1.5.4.1

Next message: PEUGEOT ONLINE: "AWARD!!!"
Previous message: Alan Stern: "Re: Semphore -> mutex in the device tree"
In reply to: Pekka Enberg: "Re: [v2.6.26] what's brewing in x86.git for v2.6.26"
Next in thread: Ingo Molnar: "Re: [v2.6.26] what's brewing in x86.git for v2.6.26"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]