Steven Rostedt <rost...@goodmis.org> 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 <rost...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
