linux-kernel - Re: [PATCH] cpumask: Fix implicit type conversion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YXkRKe7ovUFVvzqd@hirez.programming.kicks-ass.net>
Date:   Wed, 27 Oct 2021 10:43:21 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Jiasheng Jiang <jiasheng@...as.ac.cn>
Cc:     valentin.schneider@....com, mingo@...nel.org, namit@...are.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpumask: Fix implicit type conversion

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@...as.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?