| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Apr 2008 12:43:53 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: "Vegard Nossum" <vegard.nossum@...il.com>
Cc: mingo@...e.hu, linux-kernel@...r.kernel.org,
torvalds@...ux-foundation.org, tglx@...utronix.de, hpa@...or.com
Subject: Re: [v2.6.26] what's brewing in x86.git for v2.6.26
On Thu, 17 Apr 2008 20:47:06 +0200
"Vegard Nossum" <vegard.nossum@...il.com> wrote:
> On Thu, Apr 17, 2008 at 11:36 AM, Andrew Morton
> <akpm@...ux-foundation.org> wrote:
> > On Thu, 17 Apr 2008 11:30:25 +0200 Ingo Molnar <mingo@...e.hu> 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?
>
If you're OK with doing it this way then it would be preferable.
> 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;
> }
whoops. Note to Ingo: unrelated bugfix in there.
> early_param("kmemcheck", param_kmemcheck);
kmemcheck= is documented in at least three places, which is nice, but it
isn't mentioned in the place where we document kernel-parameters:
Documentation/kernel-parameters.txt. A brief section there which directs
the user to the extended docs would be fine.
early_param() is unusual - we normally use __setup(). I assume there's a
reason for using early_param(), but that reason cannot be discerned from
reading the code. A /*comment*/ is the way to fix that.
> +static pte_t *
> +address_get_pte(unsigned int address)
This is not the preferred way of laying out function declarations but I've
basically given up on this one.
> +{
> + 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));
> }
(void *)address
is more common, but I'm close to giving up on that too.
> static int
> -show_addr(uint32_t addr)
> +show_addr(uint32_t address)
u32 is preferred, but ditto.
> {
> 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;
> -}
>
> ...
>
> --- 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)
> -
That's about 15 less rejects I have to fix ;)
> 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;
> }
Perhaps we should get all this code onto the list(s) for re-review. It's
been a while..
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists