Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Oct 2021 10:43:21 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Jiasheng Jiang <jiasheng@iscas.ac.cn>
Cc:     valentin.schneider@arm.com, mingo@kernel.org, namit@vmware.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cpumask: Fix implicit type conversion
Message-ID: <YXkRKe7ovUFVvzqd@hirez.programming.kicks-ass.net>
References: <1635317123-2716894-1-git-send-email-jiasheng@iscas.ac.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1635317123-2716894-1-git-send-email-jiasheng@iscas.ac.cn>
Precedence: bulk

On Wed, Oct 27, 2021 at 06:45:23AM +0000, Jiasheng Jiang wrote:
> Since the 'nr_cpu_ids' is unsigned int, then we can assume its
> value is (2^32 - 1).
> Also, the 'cpu' is unsigned int.
> As the number of cycles increases, the value of 'cpu' can be
> (2^31 - 1).
> Although in the cpumask_next() 'cpu' is implicitly casted to int,
> its actual value is also (2^31 - 1).
> However, the return value of cpumask_next(), the updated value of
> 'cpu', is (2^31).
> That means the restriction 'cpu < nr_cpu_ids' is still statisfied
> and in cpumask_next() when 'cpu' is implicitly casted to int, its
> actual value is (-2^31).
> Obviously, it is illegal and dangerous for cpumask_next(), as well

It is not illegal, police will not come for you.

> as others.
> Therefore, we should fix the macro description of 'cpu' that remove
> the '(optionally unsigned)' and restrict the value of 'cpu' to be
> non-negative integer.
> Moreover, all the existing issues should be dealed with.
> 
> Fixes: c743f0a ("sched/fair, cpumask: Export for_each_cpu_wrap()")
> Fixes: 8bd93a2 ("rcu: Accelerate grace period if last non-dynticked CPU")
> Fixes: 984f2f3 ("cpumask: introduce new API, without changing anything, v3")

There is no actual bug, Fixes tag is unwarranted.

> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>  include/linux/cpumask.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index bfc4690..8a8e59f 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -232,7 +232,7 @@ int cpumask_any_distribute(const struct cpumask *srcp);
>  
>  /**
>   * for_each_cpu - iterate over every cpu in a mask
> - * @cpu: the (optionally unsigned) integer iterator
> + * @cpu: the integer iterator
>   * @mask: the cpumask pointer
>   *
>   * After the loop, cpu is >= nr_cpu_ids.
> @@ -240,11 +240,11 @@ int cpumask_any_distribute(const struct cpumask *srcp);
>  #define for_each_cpu(cpu, mask)				\
>  	for ((cpu) = -1;				\
>  		(cpu) = cpumask_next((cpu), (mask)),	\
> -		(cpu) < nr_cpu_ids;)
> +		(cpu) < nr_cpu_ids && (cpu) >= 0;)

So now you're generating worse code for no actual gain?