Re: [PATCH v2 4/4] dma-mapping: benchmark: handle NUMA_NO_NODE correctly

[Barry Song]

On Sat, May 4, 2024 at 11:48 PM Fedor Pchelkin <pchelkin@xxxxxxxxx> 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 DMA APIs")
> Signed-off-by: Fedor Pchelkin <pchelkin@xxxxxxxxx>

We've never actually accessed the content of cpu_mask when node
equals NUMA_NO_NODE because we always have the condition if (node !=
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@xxxxxxxxxx>

I also noticed some arch have fixed up cpumask_of_node itself.
#define cpumask_of_node(node)   ((node) == -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 = map->bparam.threads;
>         int node = map->bparam.node;
> -       const cpumask_t *cpu_mask = cpumask_of_node(node);
>         u64 loops;
>         int ret = 0;
>         int i;
> @@ -124,7 +123,7 @@ static int do_map_benchmark(struct map_benchmark_data *map)
>                 }
>
>                 if (node != 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
>