Re: [PATCH] cpumask: fix checking valid cpu range

[Guenter Roeck]

On Mon, Sep 19, 2022 at 02:05:53PM -0700, Yury Norov wrote:
> The range of valid CPUs is [0, nr_cpu_ids). Some cpumask functions are
> passed with a shifted CPU index, and for them, the valid range is
> [-1, nr_cpu_ids-1). Currently for those functions, we check the index
> against [-1, nr_cpu_ids), which is wrong.
> 
> Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx>

This patch results in warnings when booting riscv images.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at include/linux/cpumask.h:110 smp_call_function_many_cond+0x462/0x46a
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.0.0-12198-gde62603c1e8c #1
Hardware name: riscv-virtio,qemu (DT)
epc : smp_call_function_many_cond+0x462/0x46a
ra : smp_call_function_many_cond+0x108/0x46a
epc : c00b7c36 ra : c00b78dc sp : c2515c00
gp : c1d8e778 tp : c2578040 t0 : 00001fff
t1 : 00000001 t2 : 00001fff s0 : c2515c60
s1 : c1d8e378 a0 : 00000000 a1 : c1d911e0
a2 : 00000001 a3 : 00000001 a4 : 00000000
a5 : 00000000 a6 : c1d911dc a7 : c1d8f304
s2 : c0239310 s3 : 00000000 s4 : c2843ac0
s5 : 00000000 s6 : 00000000 s7 : 00000001
s8 : c31c3468 s9 : 00000001 s10: 00000001
s11: 00000000 t3 : 5b9996c6 t4 : 5b152795
t5 : 00000005 t6 : 00000064
status: 00000120 badaddr: 00000000 cause: 00000003
[<c00b7ca8>] on_each_cpu_cond_mask+0x1e/0x30
[<c0239472>] invalidate_bh_lrus+0x2c/0x34
[<c051666c>] blkdev_flush_mapping+0x52/0x102
[<c0516bac>] blkdev_put+0x1a6/0x1c8
[<c05339fa>] disk_scan_partitions+0x4a/0x64
[<c0533cb8>] device_add_disk+0x2a4/0x2c4
[<c06a8070>] virtblk_probe+0xa8e/0xb2a
[<c063ce72>] virtio_dev_probe+0x1d6/0x354
[<c0683d46>] really_probe+0x82/0x214
[<c0683f30>] __driver_probe_device+0x58/0xa2
[<c0683faa>] driver_probe_device+0x30/0xaa
[<c068456e>] __driver_attach+0x56/0x11c
[<c0681efe>] bus_for_each_dev+0x52/0x90
[<c0683798>] driver_attach+0x1a/0x22
[<c06832f2>] bus_add_driver+0x148/0x1b6
[<c0684d48>] driver_register+0x52/0xea
[<c063c8fc>] register_virtio_driver+0x1a/0x28
[<c0c22862>] virtio_blk_init+0x68/0x9c
[<c0002824>] do_one_initcall+0x5e/0x2e2
[<c0c01130>] kernel_init_freeable+0x298/0x306
[<c0aa0a7a>] kernel_init+0x1e/0x10e
[<c0003ad0>] ret_from_exception+0x0/0x10
irq event stamp: 103724
hardirqs last  enabled at (103723): [<c0007518>] __trace_hardirqs_on+0xc/0x14
hardirqs last disabled at (103724): [<c000752c>] __trace_hardirqs_off+0xc/0x14
softirqs last  enabled at (103620): [<c0195420>] bdi_register_va.part.0+0x168/0x27a
softirqs last disabled at (103618): [<c0195396>] bdi_register_va.part.0+0xde/0x27a
---[ end trace 0000000000000000 ]---

Guenter

> ---
>  include/linux/cpumask.h | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 13b32dd9803b..286804bfe3b7 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -174,9 +174,8 @@ static inline unsigned int cpumask_last(const struct cpumask *srcp)
>  static inline
>  unsigned int cpumask_next(int n, const struct cpumask *srcp)
>  {
> -	/* -1 is a legal arg here. */
> -	if (n != -1)
> -		cpumask_check(n);
> +	/* n is a prior cpu */
> +	cpumask_check(n + 1);
>  	return find_next_bit(cpumask_bits(srcp), nr_cpumask_bits, n + 1);
>  }
>  
> @@ -189,9 +188,8 @@ unsigned int cpumask_next(int n, const struct cpumask *srcp)
>   */
>  static inline unsigned int cpumask_next_zero(int n, const struct cpumask *srcp)
>  {
> -	/* -1 is a legal arg here. */
> -	if (n != -1)
> -		cpumask_check(n);
> +	/* n is a prior cpu */
> +	cpumask_check(n + 1);
>  	return find_next_zero_bit(cpumask_bits(srcp), nr_cpumask_bits, n+1);
>  }
>  
> @@ -231,9 +229,8 @@ static inline
>  unsigned int cpumask_next_and(int n, const struct cpumask *src1p,
>  		     const struct cpumask *src2p)
>  {
> -	/* -1 is a legal arg here. */
> -	if (n != -1)
> -		cpumask_check(n);
> +	/* n is a prior cpu */
> +	cpumask_check(n + 1);
>  	return find_next_and_bit(cpumask_bits(src1p), cpumask_bits(src2p),
>  		nr_cpumask_bits, n + 1);
>  }
> @@ -263,8 +260,8 @@ static inline
>  unsigned int cpumask_next_wrap(int n, const struct cpumask *mask, int start, bool wrap)
>  {
>  	cpumask_check(start);
> -	if (n != -1)
> -		cpumask_check(n);
> +	/* n is a prior cpu */
> +	cpumask_check(n + 1);
>  
>  	/*
>  	 * Return the first available CPU when wrapping, or when starting before cpu0,
> -- 
> 2.36.2