linux-kernel - Re: [PATCH] cpumask: alloc blank cpumask left over

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]

Date:	Fri, 5 Jun 2009 10:34:35 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Rusty Russell <rusty@...tcorp.com.au>
cc:	Yinghai Lu <yinghai@...nel.org>, Avi Kivity <avi@...hat.com>,
	Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cpumask: alloc blank cpumask left over



On Fri, 5 Jun 2009, Rusty Russell wrote:
> 
> OK, here's what I've got in my tree.  Ingo, I think this should go in the
> current -rc to avoid nasty bugs.

Why is the fix not to simply clear it in alloc?

> BTW, the original alloc_cpumask_var did zero; that was dropped after arguments
> over efficiency and fitting with other interfaces, but I clearly had the old
> semantics in my head for a while.

How could this ever be a efficiency issue? If you allocate cpumasks so 
often that it's an efficiency problem, it would seem that you have bigger 
issues than the memset. You'll have to initialize them some other way 
anyway, so it's not like you can ever really avoid the dirty cachelines.

IOW, why isn't the fix just to clean up the horrible mess that is 
alloc_cpumask_var_node() once and for all. Why not something like this? 
The end result looks a lot simpler.

Or you could just add in that __GFP_ZERO there, and remove the memset. I 
don't care. But the current code just looks messy and crazy, and that 
FIXME is bogus. The end of the allocation needs to be cleared regardless.

		Linus
---
 lib/cpumask.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/lib/cpumask.c b/lib/cpumask.c
index 1f71b97..272b5e5 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c
@@ -101,21 +101,16 @@ bool alloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node)
 #endif
 		*mask = NULL;
 	}
-#ifdef CONFIG_DEBUG_PER_CPU_MAPS
 	if (!*mask) {
+#ifdef CONFIG_DEBUG_PER_CPU_MAPS
 		printk(KERN_ERR "=> alloc_cpumask_var: failed!\n");
 		dump_stack();
-	}
 #endif
-	/* FIXME: Bandaid to save us from old primitives which go to NR_CPUS. */
-	if (*mask) {
-		unsigned char *ptr = (unsigned char *)cpumask_bits(*mask);
-		unsigned int tail;
-		tail = BITS_TO_LONGS(NR_CPUS - nr_cpumask_bits) * sizeof(long);
-		memset(ptr + cpumask_size() - tail, 0, tail);
+		return 0;
 	}
 
-	return *mask != NULL;
+	memset(cpumask_bits(*mask), 0, cpumask_size());
+	return 1;
 }
 EXPORT_SYMBOL(alloc_cpumask_var_node);
 
--
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/