Re: Linux kernel: "mm: uninline copy_overflow()" breaks i386 build in Mellanox MLX4
From: Richard Biener
Date: Mon Jun 13 2022 - 04:54:10 EST
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@xxxxxxx>
> 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@xxxxxxx>
>
> 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;
> }
>
>
--
Richard Biener <rguenther@xxxxxxx>
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)