linux-kernel - Re: [PATCH V2] mm: add a new command-line kmemcheck value

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] [day] [month] [year] [list]

Message-ID: <5303D4EF.7040906@intel.com>
Date:	Tue, 18 Feb 2014 13:47:27 -0800
From:	Dave Hansen <dave.hansen@...el.com>
To:	Xishi Qiu <qiuxishi@...wei.com>,
	Vegard Nossum <vegard.nossum@...il.com>
CC:	Ingo Molnar <mingo@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
	Vegard Nossum <vegardno@....uio.no>,
	Pekka Enberg <penberg@...nel.org>,
	Mel Gorman <mgorman@...e.de>,
	the arch/x86 maintainers <x86@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux MM <linux-mm@...ck.org>, Li Zefan <lizefan@...wei.com>
Subject: Re: [PATCH V2] mm: add a new command-line kmemcheck value

On 02/16/2014 06:34 PM, Xishi Qiu wrote:
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index f971306..cd7d75f 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -135,6 +135,15 @@ static void __init probe_page_size_mask(void)
>  		page_size_mask |= 1 << PG_LEVEL_2M;
>  #endif
>  
> +#if defined(CONFIG_KMEMCHECK)
> +	if (!kmemcheck_on) {
> +		if (direct_gbpages)
> +			page_size_mask |= 1 << PG_LEVEL_1G;
> +		if (cpu_has_pse)
> +			page_size_mask |= 1 << PG_LEVEL_2M;
> +	}
> +#endif

This is a copy-n-paste from just above which is inside a:

#if !defined(CONFIG_DEBUG_PAGEALLOC) && !defined(CONFIG_KMEMCHECK)

This gets really confusing to figure out which one of these options will
rule.  Maybe it's just time to add a kmemcheck_active() function which
gets #ifdef'd to 0 if the config option is off.

>  	/* Enable PSE if available */
>  	if (cpu_has_pse)
>  		set_in_cr4(X86_CR4_PSE);
> @@ -331,6 +340,8 @@ bool pfn_range_is_mapped(unsigned long start_pfn, unsigned long end_pfn)
>  	return false;
>  }
>  
> +extern int kmemcheck_on;

Didn't you _just_ reference this?  Either it's unnecessary, or this code
doesn't compile.

>  /*
>   * Setup the direct mapping of the physical memory at PAGE_OFFSET.
>   * This runs before bootmem is initialized and gets pages directly from
> diff --git a/arch/x86/mm/kmemcheck/kmemcheck.c b/arch/x86/mm/kmemcheck/kmemcheck.c
> index d87dd6d..d686ee0 100644
> --- a/arch/x86/mm/kmemcheck/kmemcheck.c
> +++ b/arch/x86/mm/kmemcheck/kmemcheck.c
> @@ -44,30 +44,35 @@
>  #ifdef CONFIG_KMEMCHECK_ONESHOT_BY_DEFAULT
>  #  define KMEMCHECK_ENABLED 2
>  #endif
> +#define KMEMCHECK_CLOSED 3
>  
> -int kmemcheck_enabled = KMEMCHECK_ENABLED;
> +int kmemcheck_enabled = KMEMCHECK_CLOSED;
> +int kmemcheck_on = 0;

This is pretty confusing.  If I see "kmemcheck_on" and
"kmemcheck_enabled" in the code, it's hard to figure out which one to
trust and infer what they were _supposed_ to be doing.

Please add some documentation for these, at least.  The commit message
isn't enough.

I'd also suggest breaking this up in to at least two pieces: one which
adds the functions to check at runtime if we want to use kmemcheck, and
then a second one to actually add this tunable.
--
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/