[RFC][PATCH] Add __GFP_ZERO to alloc_cpumask_var_node() if ptr is zero

* [RFC][PATCH] Add __GFP_ZERO to alloc_cpumask_var_node() if ptr is zero
@ 2015-12-03 22:24 Steven Rostedt
  2015-12-04  1:35 ` Rusty Russell
  0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2015-12-03 22:24 UTC (permalink / raw)
  To: LKML
  Cc: Ingo Molnar, Peter Zijlstra, Andrew Morton, Rusty Russell,
	Sergey Senozhatsky, Xunlei Pang

Xunlei Pang reported a bug in the scheduler code when
CONFIG_CPUMASK_OFFSTACK is set, several of the cpumasks used by the
root domains can contain garbage. The code does the following:

        memset(rd, 0, sizeof(*rd));

        if (!alloc_cpumask_var(&rd->span, GFP_KERNEL))
                goto out;
        if (!alloc_cpumask_var(&rd->online, GFP_KERNEL))
                goto free_span;
        if (!alloc_cpumask_var(&rd->dlo_mask, GFP_KERNEL))
                goto free_online;
        if (!alloc_cpumask_var(&rd->rto_mask, GFP_KERNEL))
                goto free_dlo_mask;

When CONFIG_CPUMASK_OFFSTACK is not defined, the four cpumasks are part
of the 'rd' structure, and the memset() will zero them all out. But
when CONFIG_CPUMASK_OFFSTACK is enabled, those cpumasks are no longer
set by the memset() but are allocated independently. That allocation
may contain garbage.

In order to make alloc_cpumask_var() and friends behave the same with
respect to being zero or not whether or not CONFIG_CPUMASK_OFFSTACK is
defined, a check is made to the contents of the mask pointer. If the
contents of the mask pointer is zero, it is assumed that the value was
zeroed out before and __GFP_ZERO is added to the flags for allocation
to make the returned cpumasks already zeroed.

Calls to alloc_cpumask_var() are not done in performance critical
paths, and even if they are, zeroing them out shouldn't add much
overhead to it. The up side to this change is that we remove subtle
bugs when enabling CONFIG_CPUMASK_OFFSTACK with cpumask logic that
worked fined when that config was not enabled.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---

diff --git a/lib/cpumask.c b/lib/cpumask.c
index 5a70f6196f57..c0d68752a8b9 100644
--- a/lib/cpumask.c
+++ b/lib/cpumask.c
@@ -60,6 +60,19 @@ int cpumask_any_but(const struct cpumask *mask, unsigned int cpu)
  */
 bool alloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags, int node)
 {
+	/*
+	 * When CONFIG_CPUMASK_OFFSTACK is not set, the cpumask may
+	 * be zeroed by a memset of the structure that contains the
+	 * mask. But if CONFIG_CPUMASK_OFFSTACK is then enabled,
+	 * the mask may end up containing garbage. By checking
+	 * if the pointer of the mask is already zero, we can assume
+	 * that the mask itself should be allocated to contain all
+	 * zeros as well. This will prevent subtle bugs by the
+	 * inconsistency of the config being set or not.
+	 */
+	if ((long)*mask == 0)
+		flags |= __GFP_ZERO;
+
 	*mask = kmalloc_node(cpumask_size(), flags, node);
 
 #ifdef CONFIG_DEBUG_PER_CPU_MAPS

^ permalink raw reply related	[flat|nested] 8+ messages in thread