Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754794AbbLDB7J (ORCPT ); Thu, 3 Dec 2015 20:59:09 -0500 Received: from ozlabs.org ([103.22.144.67]:33547 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754406AbbLDB7I (ORCPT ); Thu, 3 Dec 2015 20:59:08 -0500 From: Rusty Russell To: Steven Rostedt , LKML Cc: Ingo Molnar , Peter Zijlstra , Andrew Morton , Sergey Senozhatsky , Xunlei Pang Subject: Re: [RFC][PATCH] Add __GFP_ZERO to alloc_cpumask_var_node() if ptr is zero In-Reply-To: <20151203172428.600f380a@gandalf.local.home> References: <20151203172428.600f380a@gandalf.local.home> User-Agent: Notmuch/0.20.2 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Fri, 04 Dec 2015 12:05:12 +1030 Message-ID: <87lh9b573j.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3227 Lines: 79 Steven Rostedt writes: > 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. This is clever, but I would advise against such subtle code. We will never be able to remove this code once it is in. Would suggest making the non-CPUMASK_OFFSTACK stubs write garbage into the cpumasks instead, iff !(flags & __GFP_ZERO). Cheers, Rusty. > > Signed-off-by: Steven Rostedt > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/