Re: Linux kernel: "mm: uninline copy_overflow()" breaks i386 build in Mellanox MLX4

[Richard Biener <rguenther@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 9117 bytes --]

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;
> }
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)