| Message ID | 9cec2686-c2dc-2750-7e07-42e450ab6081@o2.pl (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Linux kernel: "mm: uninline copy_overflow()" breaks i386 build in Mellanox MLX4 | expand |
On Thu, 9 Jun 2022, Mateusz Jończyk wrote: > W dniu 26.04.2022 o 01:13, Jason Gunthorpe pisze: > > On Thu, Apr 21, 2022 at 10:47:01PM +0200, Mateusz Jończyk wrote: > >> Hello, > >> > >> commit ad7489d5262d ("mm: uninline copy_overflow()") > >> > >> breaks for me a build for i386 in the Mellanox MLX4 driver: > >> > >> In file included from ./arch/x86/include/asm/preempt.h:7, > >> from ./include/linux/preempt.h:78, > >> from ./include/linux/percpu.h:6, > >> from ./include/linux/context_tracking_state.h:5, > >> from ./include/linux/hardirq.h:5, > >> from drivers/net/ethernet/mellanox/mlx4/cq.c:37: > >> In function ‘check_copy_size’, > >> inlined from ‘copy_to_user’ at ./include/linux/uaccess.h:159:6, > >> inlined from ‘mlx4_init_user_cqes’ at drivers/net/ethernet/mellanox/mlx4/cq.c:317:9, > >> inlined from ‘mlx4_cq_alloc’ at drivers/net/ethernet/mellanox/mlx4/cq.c:394:10: > >> ./include/linux/thread_info.h:228:4: error: call to ‘__bad_copy_from’ declared with attribute error: copy source size is too small > >> 228 | __bad_copy_from(); > >> | ^~~~~~~~~~~~~~~~~ > >> make[5]: *** [scripts/Makefile.build:288: drivers/net/ethernet/mellanox/mlx4/cq.o] Błąd 1 > >> make[4]: *** [scripts/Makefile.build:550: drivers/net/ethernet/mellanox/mlx4] Błąd 2 > >> make[3]: *** [scripts/Makefile.build:550: drivers/net/ethernet/mellanox] Błąd 2 > >> make[2]: *** [scripts/Makefile.build:550: drivers/net/ethernet] Błąd 2 > >> make[1]: *** [scripts/Makefile.build:550: drivers/net] Błąd 2 > >> > >> Reverting this commit fixes the build. Disabling Mellanox Ethernet drivers > >> in Kconfig (tested only with also disabling of all Infiniband support) also fixes the build. > >> > >> It appears that uninlining of copy_overflow() causes GCC to analyze the code deeper. > > This looks like a compiler bug to me, array_size(entries, cqe_size) > > cannot be known at compile time, so the __builtin_constant_p(bytes) > > should be compile time false meaning the other two bad branches should > > have been eliminated. > > > > Jason > > Hello, > > This problem also exists in Linux v5.19-rc1. Compiling with GCC 8 and GCC 9 fails, > but with GCC10 it compiles successfully. > > I have extracted a standalone code snippet that triggers this bug, attaching > it at the bottom of this e-mail. > > This indeed looks like a compiler bug for me, as cqe_size cannot be known at compile > time. What is interesting, replacing > > err = copy_to_user2((void *)buf, init_ents, > size_mul2(4096, cqe_size)) ? -14 : 0; > > with > > err = copy_to_user2((void *)buf, init_ents, > 4096 * cqe_size) ? -14 : 0; > > makes the code compile successfully. > > I have bisected GCC to find which commit in GCC fixes this problem and > obtained this: > > 46dfa8ad6c18feb45d35734eae38798edb7c38cd is the first fixed commit > commit 46dfa8ad6c18feb45d35734eae38798edb7c38cd > Author: Richard Biener <rguenther@suse.de> > Date: Wed Sep 11 11:16:54 2019 +0000 > > re PR tree-optimization/90387 (__builtin_constant_p and -Warray-bounds warnings) > > 2019-09-11 Richard Biener <rguenther@suse.de> > > PR tree-optimization/90387 > * vr-values.c (vr_values::extract_range_basic): After inlining > simplify non-constant __builtin_constant_p to false. > > * gcc.dg/Warray-bounds-44.c: New testcase. > > From-SVN: r275639 > > gcc/ChangeLog | 6 ++++++ > gcc/testsuite/ChangeLog | 5 +++++ > gcc/testsuite/gcc.dg/Warray-bounds-44.c | 23 +++++++++++++++++++++++ > gcc/vr-values.c | 11 ++--------- > 4 files changed, 36 insertions(+), 9 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/Warray-bounds-44.c > > Applying this patch on top of releases/gcc-9.5.0 fixes the build (of the attached snippet and of > drivers/net/ethernet/mellanox/mlx4/cq.c ). > > Ccing Mr Richard Biener, as he is the author of this patch. Note the patch simply avoids some instances of path isolation with __builtin_constant_p by committing to constant/non-constant earlier. Think of for (i = 0; i < n; ++i) if (__builtin_constant_p (i)) foo (); else bar (); and GCC peeling the first iteration - then to GCC i is constant zero in this first iteration but the remaining iterations will have i non-constant. The semantics of __builtin_constant_p are not strongly enough defined to say whether that's "correct" or not given the documentation suggests it is evaluated after optimization (inlining given as example). Richard. > I'm on Ubuntu 20.04, which ships with gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.1). > It was with this compiler version that I have found the problem. > > It looks unlikely that GCC in Ubuntu 20.04 will be updated meaningfully. > Would a following workaround for Linux be acceptable? > > ==================== > > diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c > index 4d4f9cf9facb..a40701859721 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/cq.c > +++ b/drivers/net/ethernet/mellanox/mlx4/cq.c > @@ -314,8 +314,11 @@ static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size) > buf += PAGE_SIZE; > } > } else { > + /* Don't use array_size() as this triggers a bug in GCC < 10 > + * for i386. (entries * cqe_size) is guaranteed to be small. > + */ > err = copy_to_user((void __user *)buf, init_ents, > - array_size(entries, cqe_size)) ? > + entries * cqe_size) ? > -EFAULT : 0; > } > > ==================== > > Greetings, > > Mateusz Jończyk > > -------------------------------------------------- > > /* Compile with > * gcc -Wall -std=gnu11 -m32 -march=i686 -O2 -c -o bugtrigger.o bugtrigger.c > */ > > #include <stddef.h> > #include <stdbool.h> > #include <stdint.h> > > void *__kmalloc2(size_t size) __attribute__((alloc_size(1))); > extern unsigned long > _copy_to_user(void *, const void *, unsigned long); > > extern void __attribute__((__error__("copy source size is too small"))) > __bad_copy_from2(void); > extern void __attribute__((__error__("copy destination size is too small"))) > __bad_copy_to2(void); > > void copy_overflow2(int size, unsigned long count); > > static bool > check_copy_size2(const void *addr, size_t bytes, bool is_source) > { > int sz = __builtin_object_size(addr, 0); > if (sz >= 0 && sz < bytes) { > if (!__builtin_constant_p(bytes)) > copy_overflow2(sz, bytes); > else if (is_source) > __bad_copy_from2(); > else > __bad_copy_to2(); > return false; > } > return true; > } > > static unsigned long > copy_to_user2(void *to, const void *from, unsigned long n) > { > if (check_copy_size2(from, n, true)) > n = _copy_to_user(to, from, n); > return n; > } > > static inline size_t size_mul2(size_t factor1, size_t factor2) > { > size_t bytes; > if (__builtin_mul_overflow(factor1, factor2, &bytes)) > return SIZE_MAX; > > return bytes; > } > > int foobar(void *buf, int cqe_size) > { > int err; > void *init_ents = __kmalloc2(4096); > err = copy_to_user2((void *)buf, init_ents, > size_mul2(4096, cqe_size)) ? -14 : 0; > > return err; > } > >
diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c index 4d4f9cf9facb..a40701859721 100644 --- a/drivers/net/ethernet/mellanox/mlx4/cq.c +++ b/drivers/net/ethernet/mellanox/mlx4/cq.c @@ -314,8 +314,11 @@ static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size) buf += PAGE_SIZE; } } else { + /* Don't use array_size() as this triggers a bug in GCC < 10 + * for i386. (entries * cqe_size) is guaranteed to be small. + */ err = copy_to_user((void __user *)buf, init_ents, - array_size(entries, cqe_size)) ? + entries * cqe_size) ? -EFAULT : 0; }