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

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

[Mateusz Jończyk]

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.

The code in mlx4_init_user_cqes, for reference:

        static int mlx4_init_user_cqes(void *buf, int entries, int cqe_size)
        {
                int entries_per_copy = PAGE_SIZE / cqe_size;
                void *init_ents;
                int err = 0;
                int i;

                init_ents = kmalloc(PAGE_SIZE, GFP_KERNEL);
                // ...
                if (entries_per_copy < entries) {
                        // ...
                } else {
                        // BUG here
                        err = copy_to_user((void __user *)buf, init_ents,
                                        array_size(entries, cqe_size)) ?
                                -EFAULT : 0;
                }

                // ...
        }

My setup: Ubuntu 20.04, gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.1)

I was using lightly modified Kconfig from Debian i386 Linux packages.

Greetings,

Mateusz Jończyk

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

[Jason Gunthorpe]

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

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

[Mateusz Jończyk]

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.

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

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

[Richard Biener]

[-- 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)