[bug] Fix UBSAN out-of-bounds warning in drv/map

[xutao323]

Hello, this looks like a very interesting project!

While running test_read_write, I saw warnings in dmesg:

[96095.827497] Adding controller device: 1c:00.0
[96095.827868] Character device /dev/ugds_drv0 created (509.0)
[96095.828365] ugds_drv allocated 16 MSI-X vectors
[96095.828457] ugds_drv loaded
[96506.065045] tee (28483): drop_caches: 3
[96830.202386] ------------[ cut here ]------------
[96830.202395] UBSAN: array-index-out-of-bounds in /home/tao.xu/xio/uGDS/drv/map.c:139:19
[96830.202399] index 1 is out of range for type 'uint64_t [1]'
[96830.202401] CPU: 9 PID: 28527 Comm: test_read_write Tainted: G           OE      6.8.0-139-generic #139-Ubuntu
[96830.202405] Hardware name: System manufacturer System Product Name/Pro WS X299 SAGE II, BIOS 1601 08/28/2023

This is triggered by strict UBSAN array bounds checking on my Ubuntu Linux 6.8 for uint64_t addrs[1]. Changing it to a flexible array addrs[] resolves the warning (using struct_size() is optional):

diff --git a/drv/map.c b/drv/map.c
index 9625608..55711c1 100644
--- a/drv/map.c
+++ b/drv/map.c
@@ -120 +120 @@ static struct map* create_descriptor(const struct ctrl* ctrl, u64 vaddr, unsigne
-    map = kvmalloc(sizeof(struct map) + (n_pages - 1) * sizeof(uint64_t), GFP_KERNEL);
+    map = kvmalloc(struct_size(map, addrs, n_pages), GFP_KERNEL);
diff --git a/drv/map.h b/drv/map.h
index 4d1e793..29af10f 100644
--- a/drv/map.h
+++ b/drv/map.h
@@ -44 +44 @@ struct map
-    uint64_t            addrs[1];       /* Bus addresses */
+    uint64_t            addrs[];        /* Bus addresses */

BTW, my setup is a desktop with previous-generation hardware: Nvidia and AMD GPUs, and QLC NVMe SSDs. I have already set up libhipfile with good fio results (from fio #2113):

fio libhipfile	1MiB sync read	H2D memcpy	BW	CPU util%
rocm_io = posix	430us	270us	1.3 GiB/s	90%
rocm_io = hipfile	350us	N/A	2.2 GiB/s	25%

I plan to do more testing and profiling, and hopefully will share more results and findings soon.

[silence-breaker]

Thank you for your advise ! I have fixed it in #47 and you can take a look. Looking forward to your further testing and profiling😀

[xutao323]

Thanks for the fix, just realized the module is from libnvme here: https://github.com/enfiskutensykkel/ssd-gpu-dma, which makes sense to me as it's originally based on 4.x kernels.

I then continued to get some uGDS results, and found that benchmarking with size larger than 4G (like -l 8G) results in uGDSBufRegister faliure in my setup (fio hipfile is fine with larger size). I'll look into it, meanwhile, below is the perf data for 4GiB space:

benchmark	1MiB sync read	H2D memcpy	BW	CPU util%
fio libhipfile/posix	430us	270us	1.3 GiB/s	70%
fio libhipfile/hipfile	370us	N/A	2.6 GiB/s	40%
bench_ugds_hip	340us	N/A	2.8 GiB/s	96%

The above preliminary results confirm the benefits of uGDS. The extra CPU util% is an expected trade-off due to default user-space polling. Typical workloads for GPU-initiated IO scenario will get better performance and efficiency from the reduced wall-clock time when GPU is idle waiting for IO completion (mostly for KV cache).