linux-kernel - Re: [PATCH 1/2] mm: Move page struct poisoning from CONFIG_DEBUG_VM to CONFIG_DEBUG_VM

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe84cdb4-7be7-8ad8-58ca-681f46e2e55c@intel.com>
Date:   Tue, 4 Sep 2018 12:25:49 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Alexander Duyck <alexander.duyck@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Cc:     alexander.h.duyck@...el.com, pavel.tatashin@...rosoft.com,
        mhocko@...e.com, akpm@...ux-foundation.org, mingo@...nel.org,
        kirill.shutemov@...ux.intel.com
Subject: Re: [PATCH 1/2] mm: Move page struct poisoning from CONFIG_DEBUG_VM
 to CONFIG_DEBUG_VM_PGFLAGS

On 09/04/2018 11:33 AM, Alexander Duyck wrote:
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1444,7 +1444,7 @@ void * __init memblock_virt_alloc_try_nid_raw(
>  
>  	ptr = memblock_virt_alloc_internal(size, align,
>  					   min_addr, max_addr, nid);
> -#ifdef CONFIG_DEBUG_VM
> +#ifdef CONFIG_DEBUG_VM_PGFLAGS
>  	if (ptr && size > 0)
>  		memset(ptr, PAGE_POISON_PATTERN, size);
>  #endif
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 10b07eea9a6e..0fd9ad5021b0 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -696,7 +696,7 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		goto out;
>  	}
>  
> -#ifdef CONFIG_DEBUG_VM
> +#ifdef CONFIG_DEBUG_VM_PGFLAGS
>  	/*
>  	 * Poison uninitialized struct pages in order to catch invalid flags
>  	 * combinations.

I think this is the wrong way to do this.  It keeps the setting and
checking still rather tenuously connected.  If you were to leave it this
way, it needs commenting.  It's also rather odd that we're memsetting
the entire 'struct page' for a config option that's supposedly dealing
with page->flags.  That deserves _some_ addressing in a comment or
changelog.

How about:

#ifdef CONFIG_DEBUG_VM_PGFLAGS
#define VM_BUG_ON_PGFLAGS(cond, page) VM_BUG_ON_PAGE(cond, page)
+static inline void poison_struct_pages(struct page *pages, int nr)
+{
+	memset(pages, PAGE_POISON_PATTERN, size * sizeof(...));
+}
#else
#define VM_BUG_ON_PGFLAGS(cond, page) BUILD_BUG_ON_INVALID(cond)
static inline void poison_struct_pages(struct page *pages, int nr) {}
#endif

That puts the setting and checking in one spot, and also removes a
couple of #ifdefs from .c files.