Received-SPF: pass (google.com: domain of linux-kernel+bounces-170779-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240504114713.567164-1-pchelkin@ispras.ru> <20240504114713.567164-5-pchelkin@ispras.ru>
In-Reply-To: <20240504114713.567164-5-pchelkin@ispras.ru>
From: Barry Song <21cnbao@gmail.com>
Date: Tue, 7 May 2024 18:56:06 +1200
Message-ID: <CAGsJ_4xo6rxO=twFVL1w6D2ht+8BRmB5816zPaptE9v3SoYA-w@mail.gmail.com>
Subject: Re: [PATCH v2 4/4] dma-mapping: benchmark: handle NUMA_NO_NODE correctly
To: Fedor Pchelkin <pchelkin@ispras.ru>
Cc: Xiang Chen <chenxiang66@hisilicon.com>, Christoph Hellwig <hch@lst.de>, 
	Marek Szyprowski <m.szyprowski@samsung.com>, Robin Murphy <robin.murphy@arm.com>, iommu@lists.linux.dev, 
	linux-kernel@vger.kernel.org, Alexey Khoroshilov <khoroshilov@ispras.ru>, 
	lvc-project@linuxtesting.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, May 4, 2024 at 11:48=E2=80=AFPM Fedor Pchelkin <pchelkin@ispras.ru>=
 wrote:
>
> cpumask_of_node() can be called for NUMA_NO_NODE inside do_map_benchmark(=
)
> resulting in the following sanitizer report:
>
> UBSAN: array-index-out-of-bounds in ./arch/x86/include/asm/topology.h:72:=
28
> index -1 is out of range for type 'cpumask [64][1]'
> CPU: 1 PID: 990 Comm: dma_map_benchma Not tainted 6.9.0-rc6 #29
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> Call Trace:
>  <TASK>
> dump_stack_lvl (lib/dump_stack.c:117)
> ubsan_epilogue (lib/ubsan.c:232)
> __ubsan_handle_out_of_bounds (lib/ubsan.c:429)
> cpumask_of_node (arch/x86/include/asm/topology.h:72) [inline]
> do_map_benchmark (kernel/dma/map_benchmark.c:104)
> map_benchmark_ioctl (kernel/dma/map_benchmark.c:246)
> full_proxy_unlocked_ioctl (fs/debugfs/file.c:333)
> __x64_sys_ioctl (fs/ioctl.c:890)
> do_syscall_64 (arch/x86/entry/common.c:83)
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
>
> Use cpumask_of_node() in place when binding a kernel thread to a cpuset
> of a particular node.
>
> Note that the provided node id is checked inside map_benchmark_ioctl().
> It's just a NUMA_NO_NODE case which is not handled properly later.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> Fixes: 65789daa8087 ("dma-mapping: add benchmark support for streaming DM=
A APIs")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

We've never actually accessed the content of cpu_mask when node
equals NUMA_NO_NODE because we always have the condition if (node !=3D
NUMA_NO_NODE) before accessing it. Therefore, the reported failure isn't
actually an issue. However, the underlying concept is correct, hence,

Acked-by: Barry Song <baohua@kernel.org>

I also noticed some arch have fixed up cpumask_of_node itself.
#define cpumask_of_node(node)   ((node) =3D=3D -1 ?                        =
 \
                                 cpu_all_mask :                         \
                                 &hub_data(node)->h_cpus)

> ---
>  kernel/dma/map_benchmark.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> index 9f6c15f3f168..4950e0b622b1 100644
> --- a/kernel/dma/map_benchmark.c
> +++ b/kernel/dma/map_benchmark.c
> @@ -101,7 +101,6 @@ static int do_map_benchmark(struct map_benchmark_data=
 *map)
>         struct task_struct **tsk;
>         int threads =3D map->bparam.threads;
>         int node =3D map->bparam.node;
> -       const cpumask_t *cpu_mask =3D cpumask_of_node(node);
>         u64 loops;
>         int ret =3D 0;
>         int i;
> @@ -124,7 +123,7 @@ static int do_map_benchmark(struct map_benchmark_data=
 *map)
>                 }
>
>                 if (node !=3D NUMA_NO_NODE)
> -                       kthread_bind_mask(tsk[i], cpu_mask);
> +                       kthread_bind_mask(tsk[i], cpumask_of_node(node));
>         }
>
>         /* clear the old value in the previous benchmark */
> --
> 2.45.0
>