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

[Rusty Russell <rusty@rustcorp.com.au>]

Steven Rostedt <rostedt@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 <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