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