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
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")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
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;)
/**
* for_each_cpu_not - iterate over every cpu in a complemented mask
- * @cpu: the (optionally unsigned) integer iterator
+ * @cpu: the integer iterator
* @mask: the cpumask pointer
*
* After the loop, cpu is >= nr_cpu_ids.
@@ -252,13 +252,13 @@ int cpumask_any_distribute(const struct cpumask *srcp);
#define for_each_cpu_not(cpu, mask) \
for ((cpu) = -1; \
(cpu) = cpumask_next_zero((cpu), (mask)), \
- (cpu) < nr_cpu_ids;)
+ (cpu) < nr_cpu_ids && (cpu) >= 0;)
extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap);
/**
* for_each_cpu_wrap - iterate over every cpu in a mask, starting at a specified location
- * @cpu: the (optionally unsigned) integer iterator
+ * @cpu: the integer iterator
* @mask: the cpumask poiter
* @start: the start location
*
@@ -268,12 +268,12 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
*/
#define for_each_cpu_wrap(cpu, mask, start) \
for ((cpu) = cpumask_next_wrap((start)-1, (mask), (start), false); \
- (cpu) < nr_cpumask_bits; \
+ (cpu) < nr_cpumask_bits && (cpu) >= 0; \
(cpu) = cpumask_next_wrap((cpu), (mask), (start), true))
/**
* for_each_cpu_and - iterate over every cpu in both masks
- * @cpu: the (optionally unsigned) integer iterator
+ * @cpu: the integer iterator
* @mask1: the first cpumask pointer
* @mask2: the second cpumask pointer
*
@@ -288,7 +288,7 @@ extern int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool
#define for_each_cpu_and(cpu, mask1, mask2) \
for ((cpu) = -1; \
(cpu) = cpumask_next_and((cpu), (mask1), (mask2)), \
- (cpu) < nr_cpu_ids;)
+ (cpu) < nr_cpu_ids && (cpu) >= 0;)
#endif /* SMP */
#define CPU_BITS_NONE \
--
2.7.4
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 <[email protected]>
> ---
> 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?