From: Arnd Bergmann <[email protected]>
The warning option was introduced a few years ago but left disabled
by default. All of the actual bugs that this has found have been
fixed in the meantime, and this series should address the remaining
false-positives, as tested on arm/arm64/x86 randconfigs as well as
allmodconfig builds for all architectures supported by clang.
Please apply the patches individually to subsystem maintainer trees.
Arnd
Arnd Bergmann (9):
dm integrity: fix out-of-range warning
libceph: avoid clang out-of-range warning
rbd: avoid out-of-range warning
kcov: avoid clang out-of-range warning
ipv4: tcp_output: avoid warning about NET_ADD_STATS
nilfs2: fix out-of-range warning
infiniband: uverbs: avoid out-of-range warnings
mlx5: stop warning for 64KB pages
kbuild: enable tautological-constant-out-of-range-compare
drivers/block/rbd.c | 2 +-
drivers/infiniband/core/uverbs_ioctl.c | 4 ++--
drivers/md/dm-integrity.c | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 6 ++++--
fs/ceph/snap.c | 2 +-
fs/nilfs2/ioctl.c | 2 +-
kernel/kcov.c | 3 ++-
net/ceph/osdmap.c | 4 ++--
net/ipv4/tcp_output.c | 2 +-
scripts/Makefile.extrawarn | 1 -
10 files changed, 15 insertions(+), 13 deletions(-)
--
2.39.2
Cc: Ilya Dryomov <[email protected]>
Cc: Dongsheng Yang <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Leon Romanovsky <[email protected]>
Cc: Alasdair Kergon <[email protected]>
Cc: Mike Snitzer <[email protected]>
Cc: Mikulas Patocka <[email protected]>
Cc: [email protected]
Cc: Saeed Mahameed <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Xiubo Li <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: Ryusuke Konishi <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nicolas Schier <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Bill Wendling <[email protected]>
Cc: Justin Stitt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Tariq Toukan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
From: Arnd Bergmann <[email protected]>
Depending on the value of CONFIG_HZ, clang compares about a pointless
comparison:
drivers/md/dm-integrity.c:4085:12: error: result of comparison of constant 42949672950 with expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (val >= (uint64_t)UINT_MAX * 1000 / HZ) {
As the check remains useful for other configurations, shut up the
warning by adding a second type cast to uint64_t.
Fixes: 468dfca38b1a ("dm integrity: add a bitmap mode")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/md/dm-integrity.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 37b9f8f1ae1a..7f3dc8ee6ab8 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -4221,7 +4221,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned int argc, char **argv
} else if (sscanf(opt_string, "sectors_per_bit:%llu%c", &llval, &dummy) == 1) {
log2_sectors_per_bitmap_bit = !llval ? 0 : __ilog2_u64(llval);
} else if (sscanf(opt_string, "bitmap_flush_interval:%u%c", &val, &dummy) == 1) {
- if (val >= (uint64_t)UINT_MAX * 1000 / HZ) {
+ if ((uint64_t)val >= (uint64_t)UINT_MAX * 1000 / HZ) {
r = -EINVAL;
ti->error = "Invalid bitmap_flush_interval argument";
goto bad;
--
2.39.2
From: Arnd Bergmann <[email protected]>
clang-14 points out that on 64-bit architectures, a u32
is never larger than constant based on SIZE_MAX:
net/ceph/osdmap.c:1425:10: error: result of comparison of constant 4611686018427387891 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/ceph/osdmap.c:1608:10: error: result of comparison of constant 2305843009213693945 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The code is correct anyway, so just shut up that warning.
Fixes: 6f428df47dae ("libceph: pg_upmap[_items] infrastructure")
Signed-off-by: Arnd Bergmann <[email protected]>
---
fs/ceph/snap.c | 2 +-
net/ceph/osdmap.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index c65f2b202b2b..521507ea8260 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -374,7 +374,7 @@ static int build_snap_context(struct ceph_mds_client *mdsc,
/* alloc new snap context */
err = -ENOMEM;
- if (num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
+ if ((size_t)num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
goto fail;
snapc = ceph_create_snap_context(num, GFP_NOFS);
if (!snapc)
diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
index 295098873861..8e7cb2fde6f1 100644
--- a/net/ceph/osdmap.c
+++ b/net/ceph/osdmap.c
@@ -1438,7 +1438,7 @@ static struct ceph_pg_mapping *__decode_pg_temp(void **p, void *end,
ceph_decode_32_safe(p, end, len, e_inval);
if (len == 0 && incremental)
return NULL; /* new_pg_temp: [] to remove */
- if (len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
+ if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
return ERR_PTR(-EINVAL);
ceph_decode_need(p, end, len * sizeof(u32), e_inval);
@@ -1621,7 +1621,7 @@ static struct ceph_pg_mapping *__decode_pg_upmap_items(void **p, void *end,
u32 len, i;
ceph_decode_32_safe(p, end, len, e_inval);
- if (len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
+ if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
return ERR_PTR(-EINVAL);
ceph_decode_need(p, end, 2 * len * sizeof(u32), e_inval);
--
2.39.2
From: Arnd Bergmann <[email protected]>
clang-14 points out that the range check is always true on 64-bit
architectures since a u32 is not greater than the allowed size:
drivers/block/rbd.c:6079:17: error: result of comparison of constant 2305843009213693948 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is harmless, so just change the type of the temporary to size_t
to shut up that warning.
Fixes: bb23e37acb2a ("rbd: refactor rbd_header_from_disk()")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/block/rbd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 26ff5cd2bf0a..cb25ee513ada 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -6062,7 +6062,7 @@ static int rbd_dev_v2_snap_context(struct rbd_device *rbd_dev,
void *p;
void *end;
u64 seq;
- u32 snap_count;
+ size_t snap_count;
struct ceph_snap_context *snapc;
u32 i;
--
2.39.2
From: Arnd Bergmann <[email protected]>
The area_size is never larger than the maximum on 64-bit architectutes:
kernel/kcov.c:634:29: error: result of comparison of constant 1152921504606846975 with expression of type '__u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (remote_arg->area_size > LONG_MAX / sizeof(unsigned long))
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The compiler can correctly optimize the check away and the code appears
correct to me, so just add a cast to avoid the warning.
Signed-off-by: Arnd Bergmann <[email protected]>
---
kernel/kcov.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/kcov.c b/kernel/kcov.c
index f9ac2e9e460f..c3124f6d5536 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -627,7 +627,8 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
mode = kcov_get_mode(remote_arg->trace_mode);
if (mode < 0)
return mode;
- if (remote_arg->area_size > LONG_MAX / sizeof(unsigned long))
+ if ((unsigned long)remote_arg->area_size >
+ LONG_MAX / sizeof(unsigned long))
return -EINVAL;
kcov->mode = mode;
t->kcov = kcov;
--
2.39.2
From: Arnd Bergmann <[email protected]>
Clang warns about a range check in percpu_add_op() being impossible
to hit for an u8 variable:
net/ipv4/tcp_output.c:188:3: error: result of comparison of constant -1 with expression of type 'u8' (aka 'unsigned char') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/net/ip.h:291:41: note: expanded from macro 'NET_ADD_STATS'
#define NET_ADD_STATS(net, field, adnd) SNMP_ADD_STATS((net)->mib.net_statistics, field, adnd)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/net/snmp.h:143:4: note: expanded from macro 'SNMP_ADD_STATS'
this_cpu_add(mib->mibs[field], addend)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/percpu-defs.h:509:33: note: expanded from macro 'this_cpu_add'
#define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
<scratch space>:187:1: note: expanded from here
this_cpu_add_8
^
arch/x86/include/asm/percpu.h:326:35: note: expanded from macro 'this_cpu_add_8'
#define this_cpu_add_8(pcp, val) percpu_add_op(8, volatile, (pcp), val)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
((val) == 1 || (val) == -1)) ? \
~~~~~ ^ ~~
Avoid this warning with a cast to a signed 'int'.
Signed-off-by: Arnd Bergmann <[email protected]>
---
net/ipv4/tcp_output.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e3167ad96567..dbe54fceee08 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -183,7 +183,7 @@ static inline void tcp_event_ack_sent(struct sock *sk, u32 rcv_nxt)
if (unlikely(tp->compressed_ack)) {
NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
- tp->compressed_ack);
+ (int)tp->compressed_ack);
tp->compressed_ack = 0;
if (hrtimer_try_to_cancel(&tp->compressed_ack_timer) == 1)
__sock_put(sk);
--
2.39.2
From: Arnd Bergmann <[email protected]>
clang-14 points out that v_size is always smaller than a 64KB
page size if that is configured by the CPU architecture:
fs/nilfs2/ioctl.c:63:19: error: result of comparison of constant 65536 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (argv->v_size > PAGE_SIZE)
~~~~~~~~~~~~ ^ ~~~~~~~~~
This is ok, so just shut up that warning with a cast.
Signed-off-by: Arnd Bergmann <[email protected]>
---
fs/nilfs2/ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index f1a01c191cf5..8be471ce4f19 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -60,7 +60,7 @@ static int nilfs_ioctl_wrap_copy(struct the_nilfs *nilfs,
if (argv->v_nmembs == 0)
return 0;
- if (argv->v_size > PAGE_SIZE)
+ if ((size_t)argv->v_size > PAGE_SIZE)
return -EINVAL;
/*
--
2.39.2
From: Arnd Bergmann <[email protected]>
clang warns for comparisons that are always true, which is the case
for these two page size checks on architectures with 64KB pages:
drivers/infiniband/core/uverbs_ioctl.c:90:39: error: result of comparison of constant 65536 with expression of type 'u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
WARN_ON_ONCE(method_elm->bundle_size > PAGE_SIZE);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
include/asm-generic/bug.h:104:25: note: expanded from macro 'WARN_ON_ONCE'
int __ret_warn_on = !!(condition); \
^~~~~~~~~
drivers/infiniband/core/uverbs_ioctl.c:621:17: error: result of comparison of constant 65536 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (hdr.length > PAGE_SIZE ||
~~~~~~~~~~ ^ ~~~~~~~~~
Add a cast to u32 in both cases, so it never warns about this.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/infiniband/core/uverbs_ioctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index f80da6a67e24..e0cc3ddae71b 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -90,7 +90,7 @@ void uapi_compute_bundle_size(struct uverbs_api_ioctl_method *method_elm,
ALIGN(bundle_size + 256, sizeof(*pbundle->internal_buffer));
/* Do not want order-2 allocations for this. */
- WARN_ON_ONCE(method_elm->bundle_size > PAGE_SIZE);
+ WARN_ON_ONCE((u32)method_elm->bundle_size > PAGE_SIZE);
}
/**
@@ -636,7 +636,7 @@ long ib_uverbs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
if (err)
return -EFAULT;
- if (hdr.length > PAGE_SIZE ||
+ if ((u32)hdr.length > PAGE_SIZE ||
hdr.length != struct_size(&hdr, attrs, hdr.num_attrs))
return -EINVAL;
--
2.39.2
From: Arnd Bergmann <[email protected]>
When building with 64KB pages, clang points out that xsk->chunk_size
can never be PAGE_SIZE:
drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c:19:22: error: result of comparison of constant 65536 with expression of type 'u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
if (xsk->chunk_size > PAGE_SIZE ||
~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
In older versions of this code, using PAGE_SIZE was the only
possibility, so this would have never worked on 64KB page kernels,
but the patch apparently did not address this case completely.
As Maxim Mikityanskiy suggested, 64KB chunks are really not all that
useful, so just shut up the warning by adding a cast.
Fixes: 282c0c798f8e ("net/mlx5e: Allow XSK frames smaller than a page")
Link: https://lore.kernel.org/netdev/[email protected]/
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
index 06592b9f0424..9240cfe25d10 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
@@ -28,8 +28,10 @@ bool mlx5e_validate_xsk_param(struct mlx5e_params *params,
struct mlx5e_xsk_param *xsk,
struct mlx5_core_dev *mdev)
{
- /* AF_XDP doesn't support frames larger than PAGE_SIZE. */
- if (xsk->chunk_size > PAGE_SIZE || xsk->chunk_size < MLX5E_MIN_XSK_CHUNK_SIZE) {
+ /* AF_XDP doesn't support frames larger than PAGE_SIZE,
+ * and xsk->chunk_size is limited to 65535 bytes.
+ */
+ if ((size_t)xsk->chunk_size > PAGE_SIZE || xsk->chunk_size < MLX5E_MIN_XSK_CHUNK_SIZE) {
mlx5_core_err(mdev, "XSK chunk size %u out of bounds [%u, %lu]\n", xsk->chunk_size,
MLX5E_MIN_XSK_CHUNK_SIZE, PAGE_SIZE);
return false;
--
2.39.2
From: Arnd Bergmann <[email protected]>
All the previously reported warnings from this option have been addressed,
so the option can now be left default-enabled, rather than disabled without
W=1. There are not too many actual bugs found by this, but it can help
detect a silly mistake earlier, and it's usually trivial to work around
the false-positives.
Signed-off-by: Arnd Bergmann <[email protected]>
---
scripts/Makefile.extrawarn | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 5a25f133d0e9..24d29e477644 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -120,7 +120,6 @@ KBUILD_CFLAGS += -Wformat-insufficient-args
endif
endif
KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
-KBUILD_CFLAGS += -Wno-tautological-constant-out-of-range-compare
KBUILD_CFLAGS += $(call cc-disable-warning, unaligned-access)
KBUILD_CFLAGS += -Wno-enum-compare-conditional
KBUILD_CFLAGS += -Wno-enum-enum-conversion
--
2.39.2
On Thu, Mar 28, 2024 at 3:31 PM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> Clang warns about a range check in percpu_add_op() being impossible
> to hit for an u8 variable:
>
> net/ipv4/tcp_output.c:188:3: error: result of comparison of constant -1 with expression of type 'u8' (aka 'unsigned char') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/net/ip.h:291:41: note: expanded from macro 'NET_ADD_STATS'
> #define NET_ADD_STATS(net, field, adnd) SNMP_ADD_STATS((net)->mib.net_statistics, field, adnd)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/net/snmp.h:143:4: note: expanded from macro 'SNMP_ADD_STATS'
> this_cpu_add(mib->mibs[field], addend)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/percpu-defs.h:509:33: note: expanded from macro 'this_cpu_add'
> #define this_cpu_add(pcp, val) __pcpu_size_call(this_cpu_add_, pcp, val)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
> <scratch space>:187:1: note: expanded from here
> this_cpu_add_8
> ^
> arch/x86/include/asm/percpu.h:326:35: note: expanded from macro 'this_cpu_add_8'
> #define this_cpu_add_8(pcp, val) percpu_add_op(8, volatile, (pcp), val)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
> ((val) == 1 || (val) == -1)) ? \
> ~~~~~ ^ ~~
>
This seems like a bug in the macro or the compiler, because val is not
a constant ?
__builtin_constant_p(val) should return false ???
+#define percpu_add_op(size, qual, var, val) \
+do { \
+ const int pao_ID__ = (__builtin_constant_p(val) && \
+ ((val) == 1 || (val) == -1)) ? \
+ (int)(val) : 0; \
+ if (0) { \
+ typeof(var) pao_tmp__; \
+ pao_tmp__ = (val); \
+ (void)pao_tmp__; \
+ } \
+ if (pao_ID__ == 1) \
+ percpu_unary_op(size, qual, "inc", var); \
+ else if (pao_ID__ == -1) \
+ percpu_unary_op(size, qual, "dec", var); \
+ else \
+ percpu_to_op(size, qual, "add", var, val); \
+} while (0)
+
> Avoid this warning with a cast to a signed 'int'.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> net/ipv4/tcp_output.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index e3167ad96567..dbe54fceee08 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -183,7 +183,7 @@ static inline void tcp_event_ack_sent(struct sock *sk, u32 rcv_nxt)
>
> if (unlikely(tp->compressed_ack)) {
> NET_ADD_STATS(sock_net(sk), LINUX_MIB_TCPACKCOMPRESSED,
> - tp->compressed_ack);
> + (int)tp->compressed_ack);
> tp->compressed_ack = 0;
> if (hrtimer_try_to_cancel(&tp->compressed_ack_timer) == 1)
> __sock_put(sk);
> --
> 2.39.2
>
On 3/28/24 9:30 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> clang-14 points out that the range check is always true on 64-bit
> architectures since a u32 is not greater than the allowed size:
>
> drivers/block/rbd.c:6079:17: error: result of comparison of constant 2305843009213693948 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
w
> ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This is harmless, so just change the type of the temporary to size_t
> to shut up that warning.
This fixes the warning, but then the now size_t value is passed
to ceph_decode_32_safe(), which implies a different type conversion.
That too is not harmful, but...
Could we just cast the value in the comparison instead?
if ((size_t)snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
You could drop the space between sizeof and ( while
you're at it (I always used the space back then).
-Alex
>
> Fixes: bb23e37acb2a ("rbd: refactor rbd_header_from_disk()")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/block/rbd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 26ff5cd2bf0a..cb25ee513ada 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -6062,7 +6062,7 @@ static int rbd_dev_v2_snap_context(struct rbd_device *rbd_dev,
> void *p;
> void *end;
> u64 seq;
> - u32 snap_count;
> + size_t snap_count;
> struct ceph_snap_context *snapc;
> u32 i;
>
On Thu, 2024-03-28 at 15:30 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> clang-14 points out that v_size is always smaller than a 64KB
> page size if that is configured by the CPU architecture:
>
> fs/nilfs2/ioctl.c:63:19: error: result of comparison of constant
> 65536 with expression of type '__u16' (aka 'unsigned short') is
> always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (argv->v_size > PAGE_SIZE)
> ~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> This is ok, so just shut up that warning with a cast.
nit:
It's not a warning, but actually a compile error, right?
(no idea why they make that an error btw. Warning would be perfectly
fine)
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Should / could that be backported to stable kernels in case people
start building those with clang-14?
Regards,
P.
> ---
> fs/nilfs2/ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index f1a01c191cf5..8be471ce4f19 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -60,7 +60,7 @@ static int nilfs_ioctl_wrap_copy(struct the_nilfs
> *nilfs,
> if (argv->v_nmembs == 0)
> return 0;
>
> - if (argv->v_size > PAGE_SIZE)
> + if ((size_t)argv->v_size > PAGE_SIZE)
> return -EINVAL;
>
> /*
On Thu, 28 Mar 2024 at 15:30:46 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> When building with 64KB pages, clang points out that xsk->chunk_size
> can never be PAGE_SIZE:
>
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c:19:22: error: result of comparison of constant 65536 with expression of type 'u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (xsk->chunk_size > PAGE_SIZE ||
> ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> In older versions of this code, using PAGE_SIZE was the only
> possibility, so this would have never worked on 64KB page kernels,
> but the patch apparently did not address this case completely.
>
> As Maxim Mikityanskiy suggested, 64KB chunks are really not all that
> useful, so just shut up the warning by adding a cast.
>
> Fixes: 282c0c798f8e ("net/mlx5e: Allow XSK frames smaller than a page")
> Link: https://lore.kernel.org/netdev/[email protected]/
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> index 06592b9f0424..9240cfe25d10 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> @@ -28,8 +28,10 @@ bool mlx5e_validate_xsk_param(struct mlx5e_params *params,
> struct mlx5e_xsk_param *xsk,
> struct mlx5_core_dev *mdev)
> {
> - /* AF_XDP doesn't support frames larger than PAGE_SIZE. */
> - if (xsk->chunk_size > PAGE_SIZE || xsk->chunk_size < MLX5E_MIN_XSK_CHUNK_SIZE) {
> + /* AF_XDP doesn't support frames larger than PAGE_SIZE,
> + * and xsk->chunk_size is limited to 65535 bytes.
> + */
> + if ((size_t)xsk->chunk_size > PAGE_SIZE || xsk->chunk_size < MLX5E_MIN_XSK_CHUNK_SIZE) {
Acked-by: Maxim Mikityanskiy <[email protected]>
> mlx5_core_err(mdev, "XSK chunk size %u out of bounds [%u, %lu]\n", xsk->chunk_size,
> MLX5E_MIN_XSK_CHUNK_SIZE, PAGE_SIZE);
> return false;
> --
> 2.39.2
>
On Thu, Mar 28, 2024, at 16:21, Philipp Stanner wrote:
> On Thu, 2024-03-28 at 15:30 +0100, Arnd Bergmann wrote:
>>
>> This is ok, so just shut up that warning with a cast.
>
> nit:
> It's not a warning, but actually a compile error, right?
I build with CONFIG_WERROR=y, which turns all warnings
into errors. It's just a warning without that, and is
currently only enabled when building with 'make W=1',
though the point of my series is to have it always enabled.
> (no idea why they make that an error btw. Warning would be perfectly
> fine)
>
>>
>> Signed-off-by: Arnd Bergmann <[email protected]>
>
> Should / could that be backported to stable kernels in case people
> start building those with clang-14?
It's clearly harmless and could be backported, but it
is not needed either since older kernels will keep the
option as part of W=1, not the default.
Arnd
On Thu, Mar 28, 2024, at 15:38, Eric Dumazet wrote:
> On Thu, Mar 28, 2024 at 3:31 PM Arnd Bergmann <[email protected]> wrote:
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/x86/include/asm/percpu.h:127:31: note: expanded from macro 'percpu_add_op'
>> ((val) == 1 || (val) == -1)) ? \
>> ~~~~~ ^ ~~
>>
>
> This seems like a bug in the macro or the compiler, because val is not
> a constant ?
>
> __builtin_constant_p(val) should return false ???
>
> +#define percpu_add_op(size, qual, var, val) \
> +do { \
> + const int pao_ID__ = (__builtin_constant_p(val) && \
> + ((val) == 1 || (val) == -1)) ? \
> + (int)(val) : 0; \
It looks like gcc does the same thing, with the broader and
still disabled -Wtype-limits, see: https://godbolt.org/z/3EPTGx68n
As far as I can tell, it does not matter that the comparison
against -1 is never actually evaluated, since the warning
is already printed before it simplifies the condition.
This is the only such warning I got from percpu, but
I guess we could also add the cast inside of the macro,
such as
diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpuh
index 44958ebaf626..5923d786e67a 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -181,12 +181,14 @@ do { \
*/
#define percpu_add_op(size, qual, var, val) \
do { \
- const int pao_ID__ = (__builtin_constant_p(val) && \
- ((val) == 1 || (val) == -1)) ? \
- (int)(val) : 0; \
+ __auto_type __val = (val); \
+ const int pao_ID__ = (__builtin_constant_p(__val) && \
+ ((__val) == (typeof(__val))1 || \
+ (__val) == (typeof(__val))-1)) ? \
+ (int)(__val) : 0; \
if (0) { \
typeof(var) pao_tmp__; \
- pao_tmp__ = (val); \
+ pao_tmp__ = (__val); \
(void)pao_tmp__; \
} \
if (pao_ID__ == 1) \
@@ -194,7 +196,7 @@ do { \
else if (pao_ID__ == -1) \
percpu_unary_op(size, qual, "dec", var); \
else \
- percpu_to_op(size, qual, "add", var, val); \
+ percpu_to_op(size, qual, "add", var, __val); \
} while (0)
#define percpu_from_op(size, qual, op, _var) \
I added a temporary variable there to avoid expanding
the argument too many times.
Arnd
On Thu, 28 Mar 2024, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Depending on the value of CONFIG_HZ, clang compares about a pointless
> comparison:
>
> drivers/md/dm-integrity.c:4085:12: error: result of comparison of constant 42949672950 with expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (val >= (uint64_t)UINT_MAX * 1000 / HZ) {
>
> As the check remains useful for other configurations, shut up the
> warning by adding a second type cast to uint64_t.
>
> Fixes: 468dfca38b1a ("dm integrity: add a bitmap mode")
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Mikulas Patocka <[email protected]>
> ---
> drivers/md/dm-integrity.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 37b9f8f1ae1a..7f3dc8ee6ab8 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -4221,7 +4221,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned int argc, char **argv
> } else if (sscanf(opt_string, "sectors_per_bit:%llu%c", &llval, &dummy) == 1) {
> log2_sectors_per_bitmap_bit = !llval ? 0 : __ilog2_u64(llval);
> } else if (sscanf(opt_string, "bitmap_flush_interval:%u%c", &val, &dummy) == 1) {
> - if (val >= (uint64_t)UINT_MAX * 1000 / HZ) {
> + if ((uint64_t)val >= (uint64_t)UINT_MAX * 1000 / HZ) {
> r = -EINVAL;
> ti->error = "Invalid bitmap_flush_interval argument";
> goto bad;
> --
> 2.39.2
>
On Thu, Mar 28, 2024 at 7:31 AM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> Depending on the value of CONFIG_HZ, clang compares about a pointless
> comparison:
>
> drivers/md/dm-integrity.c:4085:12: error: result of comparison of constant 42949672950 with expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (val >= (uint64_t)UINT_MAX * 1000 / HZ) {
>
> As the check remains useful for other configurations, shut up the
> warning by adding a second type cast to uint64_t.
Yeah, nice.
>
> Fixes: 468dfca38b1a ("dm integrity: add a bitmap mode")
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Justin Stitt <[email protected]>
> ---
> drivers/md/dm-integrity.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 37b9f8f1ae1a..7f3dc8ee6ab8 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -4221,7 +4221,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned int argc, char **argv
> } else if (sscanf(opt_string, "sectors_per_bit:%llu%c", &llval, &dummy) == 1) {
> log2_sectors_per_bitmap_bit = !llval ? 0 : __ilog2_u64(llval);
> } else if (sscanf(opt_string, "bitmap_flush_interval:%u%c", &val, &dummy) == 1) {
> - if (val >= (uint64_t)UINT_MAX * 1000 / HZ) {
> + if ((uint64_t)val >= (uint64_t)UINT_MAX * 1000 / HZ) {
> r = -EINVAL;
> ti->error = "Invalid bitmap_flush_interval argument";
> goto bad;
> --
> 2.39.2
>
On Thu, Mar 28, 2024 at 7:32 AM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> clang-14 points out that v_size is always smaller than a 64KB
> page size if that is configured by the CPU architecture:
Is this only with clang-14?
>
> fs/nilfs2/ioctl.c:63:19: error: result of comparison of constant 65536 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (argv->v_size > PAGE_SIZE)
> ~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> This is ok, so just shut up that warning with a cast.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
My question out of curiosity aside,
Reviewed-by: Justin Stitt <[email protected]>
> ---
> fs/nilfs2/ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index f1a01c191cf5..8be471ce4f19 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -60,7 +60,7 @@ static int nilfs_ioctl_wrap_copy(struct the_nilfs *nilfs,
> if (argv->v_nmembs == 0)
> return 0;
>
> - if (argv->v_size > PAGE_SIZE)
> + if ((size_t)argv->v_size > PAGE_SIZE)
> return -EINVAL;
>
> /*
> --
> 2.39.2
>
On Thu, Mar 28, 2024 at 7:32 AM Arnd Bergmann <[email protected]> wrote:
> Add a cast to u32 in both cases, so it never warns about this.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Justin Stitt <[email protected]>
> ---
> drivers/infiniband/core/uverbs_ioctl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
> index f80da6a67e24..e0cc3ddae71b 100644
> --- a/drivers/infiniband/core/uverbs_ioctl.c
> +++ b/drivers/infiniband/core/uverbs_ioctl.c
> @@ -90,7 +90,7 @@ void uapi_compute_bundle_size(struct uverbs_api_ioctl_method *method_elm,
> ALIGN(bundle_size + 256, sizeof(*pbundle->internal_buffer));
>
> /* Do not want order-2 allocations for this. */
> - WARN_ON_ONCE(method_elm->bundle_size > PAGE_SIZE);
> + WARN_ON_ONCE((u32)method_elm->bundle_size > PAGE_SIZE);
> }
>
> /**
> @@ -636,7 +636,7 @@ long ib_uverbs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> if (err)
> return -EFAULT;
>
> - if (hdr.length > PAGE_SIZE ||
> + if ((u32)hdr.length > PAGE_SIZE ||
> hdr.length != struct_size(&hdr, attrs, hdr.num_attrs))
> return -EINVAL;
>
> --
> 2.39.2
>
On Thu, Mar 28, 2024, at 23:09, Justin Stitt wrote:
> On Thu, Mar 28, 2024 at 7:32 AM Arnd Bergmann <[email protected]> wrote:
>>
>> From: Arnd Bergmann <[email protected]>
>>
>> When building with 64KB pages, clang points out that xsk->chunk_size
>> can never be PAGE_SIZE:
>
> This is under W=1 right? Otherwise this is a mighty annoying warning.
At the moment yes. I'm fairly sure that I've covered all the
common cases with thousands of randconfig builds, so we should
be able to make it the default when this series is fully merged.
Arnd
On Thu, Mar 28, 2024, at 23:04, Justin Stitt wrote:
> On Thu, Mar 28, 2024 at 7:32 AM Arnd Bergmann <[email protected]> wrote:
>>
>> From: Arnd Bergmann <[email protected]>
>>
>> clang-14 points out that v_size is always smaller than a 64KB
>> page size if that is configured by the CPU architecture:
>
> Is this only with clang-14?
No, it should be every version since then. I checked my
randconfig tree and found that I originally committed this
patch in September 2021 when I was first testing prerelease
clang-14 builds. I sent a couple of fixes for this class
of warnings then, but for some reason not this one.
Arnd
On Thu, Mar 28, 2024 at 7:31 AM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> The area_size is never larger than the maximum on 64-bit architectutes:
>
> kernel/kcov.c:634:29: error: result of comparison of constant 1152921504606846975 with expression of type '__u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (remote_arg->area_size > LONG_MAX / sizeof(unsigned long))
> ~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The compiler can correctly optimize the check away and the code appears
> correct to me, so just add a cast to avoid the warning.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Justin Stitt <[email protected]>
> ---
> kernel/kcov.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index f9ac2e9e460f..c3124f6d5536 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -627,7 +627,8 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
> mode = kcov_get_mode(remote_arg->trace_mode);
> if (mode < 0)
> return mode;
> - if (remote_arg->area_size > LONG_MAX / sizeof(unsigned long))
> + if ((unsigned long)remote_arg->area_size >
> + LONG_MAX / sizeof(unsigned long))
> return -EINVAL;
> kcov->mode = mode;
> t->kcov = kcov;
> --
> 2.39.2
>
On Thu, Mar 28, 2024 at 7:31 AM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> clang-14 points out that on 64-bit architectures, a u32
> is never larger than constant based on SIZE_MAX:
>
> net/ceph/osdmap.c:1425:10: error: result of comparison of constant 4611686018427387891 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
> ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> net/ceph/osdmap.c:1608:10: error: result of comparison of constant 2305843009213693945 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The code is correct anyway, so just shut up that warning.
OK.
>
> Fixes: 6f428df47dae ("libceph: pg_upmap[_items] infrastructure")
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Justin Stitt <[email protected]>
> ---
> fs/ceph/snap.c | 2 +-
> net/ceph/osdmap.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index c65f2b202b2b..521507ea8260 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -374,7 +374,7 @@ static int build_snap_context(struct ceph_mds_client *mdsc,
>
> /* alloc new snap context */
> err = -ENOMEM;
> - if (num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
> + if ((size_t)num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
> goto fail;
> snapc = ceph_create_snap_context(num, GFP_NOFS);
> if (!snapc)
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index 295098873861..8e7cb2fde6f1 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -1438,7 +1438,7 @@ static struct ceph_pg_mapping *__decode_pg_temp(void **p, void *end,
> ceph_decode_32_safe(p, end, len, e_inval);
> if (len == 0 && incremental)
> return NULL; /* new_pg_temp: [] to remove */
> - if (len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
> + if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
> return ERR_PTR(-EINVAL);
>
> ceph_decode_need(p, end, len * sizeof(u32), e_inval);
> @@ -1621,7 +1621,7 @@ static struct ceph_pg_mapping *__decode_pg_upmap_items(void **p, void *end,
> u32 len, i;
>
> ceph_decode_32_safe(p, end, len, e_inval);
> - if (len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> + if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> return ERR_PTR(-EINVAL);
>
> ceph_decode_need(p, end, 2 * len * sizeof(u32), e_inval);
> --
> 2.39.2
>
On 3/28/24 22:53, Alex Elder wrote:
> On 3/28/24 9:30 AM, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> clang-14 points out that the range check is always true on 64-bit
>> architectures since a u32 is not greater than the allowed size:
>>
>> drivers/block/rbd.c:6079:17: error: result of comparison of constant
>> 2305843009213693948 with expression of type 'u32' (aka 'unsigned
>> int') is always false
>> [-Werror,-Wtautological-constant-out-of-range-compare]
> w
>> ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> This is harmless, so just change the type of the temporary to size_t
>> to shut up that warning.
>
> This fixes the warning, but then the now size_t value is passed
> to ceph_decode_32_safe(), which implies a different type conversion.
> That too is not harmful, but...
>
> Could we just cast the value in the comparison instead?
>
> if ((size_t)snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
>
> You could drop the space between sizeof and ( while
> you're at it (I always used the space back then).
>
Agree.
- Xiubo
> -Alex
>
>>
>> Fixes: bb23e37acb2a ("rbd: refactor rbd_header_from_disk()")
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> ---
>> drivers/block/rbd.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 26ff5cd2bf0a..cb25ee513ada 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -6062,7 +6062,7 @@ static int rbd_dev_v2_snap_context(struct
>> rbd_device *rbd_dev,
>> void *p;
>> void *end;
>> u64 seq;
>> - u32 snap_count;
>> + size_t snap_count;
>> struct ceph_snap_context *snapc;
>> u32 i;
>
>
On 3/28/24 22:30, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> clang-14 points out that on 64-bit architectures, a u32
> is never larger than constant based on SIZE_MAX:
>
> net/ceph/osdmap.c:1425:10: error: result of comparison of constant 4611686018427387891 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
> ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> net/ceph/osdmap.c:1608:10: error: result of comparison of constant 2305843009213693945 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The code is correct anyway, so just shut up that warning.
>
> Fixes: 6f428df47dae ("libceph: pg_upmap[_items] infrastructure")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> fs/ceph/snap.c | 2 +-
> net/ceph/osdmap.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index c65f2b202b2b..521507ea8260 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -374,7 +374,7 @@ static int build_snap_context(struct ceph_mds_client *mdsc,
>
> /* alloc new snap context */
> err = -ENOMEM;
> - if (num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
> + if ((size_t)num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
> goto fail;
> snapc = ceph_create_snap_context(num, GFP_NOFS);
> if (!snapc)
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index 295098873861..8e7cb2fde6f1 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -1438,7 +1438,7 @@ static struct ceph_pg_mapping *__decode_pg_temp(void **p, void *end,
> ceph_decode_32_safe(p, end, len, e_inval);
> if (len == 0 && incremental)
> return NULL; /* new_pg_temp: [] to remove */
> - if (len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
> + if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / sizeof(u32))
> return ERR_PTR(-EINVAL);
>
> ceph_decode_need(p, end, len * sizeof(u32), e_inval);
> @@ -1621,7 +1621,7 @@ static struct ceph_pg_mapping *__decode_pg_upmap_items(void **p, void *end,
> u32 len, i;
>
> ceph_decode_32_safe(p, end, len, e_inval);
> - if (len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> + if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> return ERR_PTR(-EINVAL);
>
> ceph_decode_need(p, end, 2 * len * sizeof(u32), e_inval);
Reviewed-by: Xiubo Li <[email protected]>
Thanks
- Xiubo
On Thu, Mar 28, 2024 at 11:32 PM Arnd Bergmann wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> clang-14 points out that v_size is always smaller than a 64KB
> page size if that is configured by the CPU architecture:
>
> fs/nilfs2/ioctl.c:63:19: error: result of comparison of constant 65536 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (argv->v_size > PAGE_SIZE)
> ~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> This is ok, so just shut up that warning with a cast.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> fs/nilfs2/ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
> index f1a01c191cf5..8be471ce4f19 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -60,7 +60,7 @@ static int nilfs_ioctl_wrap_copy(struct the_nilfs *nilfs,
> if (argv->v_nmembs == 0)
> return 0;
>
> - if (argv->v_size > PAGE_SIZE)
> + if ((size_t)argv->v_size > PAGE_SIZE)
> return -EINVAL;
>
> /*
> --
> 2.39.2
>
Andrew, could you please apply this to the -mm tree along with the
following tags and Justin's reviewed-by tag?
Acked-by: Ryusuke Konishi <[email protected]>
Fixes: 3358b4aaa84f ("nilfs2: fix problems of memory allocation in ioctl")
Thank you, Arnd.
I didn't notice this warning existed depending on the environment.
Ryusuke Konishi
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Thu, 28 Mar 2024 15:30:38 +0100 you wrote:
> From: Arnd Bergmann <[email protected]>
>
> The warning option was introduced a few years ago but left disabled
> by default. All of the actual bugs that this has found have been
> fixed in the meantime, and this series should address the remaining
> false-positives, as tested on arm/arm64/x86 randconfigs as well as
> allmodconfig builds for all architectures supported by clang.
>
> [...]
Here is the summary with links:
- [2/9] libceph: avoid clang out-of-range warning
(no matching commit)
- [5/9] ipv4: tcp_output: avoid warning about NET_ADD_STATS
(no matching commit)
- [8/9] mlx5: stop warning for 64KB pages
https://git.kernel.org/netdev/net-next/c/a5535e533694
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Thu, 28 Mar 2024 15:30:44 +0100, Arnd Bergmann wrote:
> clang-14 points out that v_size is always smaller than a 64KB
> page size if that is configured by the CPU architecture:
>
> fs/nilfs2/ioctl.c:63:19: error: result of comparison of constant 65536 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (argv->v_size > PAGE_SIZE)
> ~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> [...]
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[6/9] nilfs2: fix out-of-range warning
https://git.kernel.org/vfs/vfs/c/d20180d5dd10
On 28/03/2024 16:30, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> When building with 64KB pages, clang points out that xsk->chunk_size
> can never be PAGE_SIZE:
>
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c:19:22: error: result of comparison of constant 65536 with expression of type 'u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (xsk->chunk_size > PAGE_SIZE ||
> ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> In older versions of this code, using PAGE_SIZE was the only
> possibility, so this would have never worked on 64KB page kernels,
> but the patch apparently did not address this case completely.
>
> As Maxim Mikityanskiy suggested, 64KB chunks are really not all that
> useful, so just shut up the warning by adding a cast.
>
> Fixes: 282c0c798f8e ("net/mlx5e: Allow XSK frames smaller than a page")
> Link: https://lore.kernel.org/netdev/[email protected]/
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Arnd Bergmann <[email protected]>
Thanks for your patch.
Reviewed-by: Tariq Toukan <[email protected]>
On Thu, Mar 28, 2024 at 7:32 AM Arnd Bergmann <[email protected]> wrote:
>
> From: Arnd Bergmann <[email protected]>
>
> When building with 64KB pages, clang points out that xsk->chunk_size
> can never be PAGE_SIZE:
This is under W=1 right? Otherwise this is a mighty annoying warning.
>
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c:19:22: error: result of comparison of constant 65536 with expression of type 'u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (xsk->chunk_size > PAGE_SIZE ||
> ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
>
> In older versions of this code, using PAGE_SIZE was the only
> possibility, so this would have never worked on 64KB page kernels,
> but the patch apparently did not address this case completely.
>
> As Maxim Mikityanskiy suggested, 64KB chunks are really not all that
> useful, so just shut up the warning by adding a cast.
>
> Fixes: 282c0c798f8e ("net/mlx5e: Allow XSK frames smaller than a page")
> Link: https://lore.kernel.org/netdev/20211013150232.2942146-1-arnd@kernelorg/
> Link: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Arnd Bergmann <[email protected]>
Reviewed-by: Justin Stitt <[email protected]>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> index 06592b9f0424..9240cfe25d10 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> @@ -28,8 +28,10 @@ bool mlx5e_validate_xsk_param(struct mlx5e_params *params,
> struct mlx5e_xsk_param *xsk,
> struct mlx5_core_dev *mdev)
> {
> - /* AF_XDP doesn't support frames larger than PAGE_SIZE. */
> - if (xsk->chunk_size > PAGE_SIZE || xsk->chunk_size < MLX5E_MIN_XSK_CHUNK_SIZE) {
> + /* AF_XDP doesn't support frames larger than PAGE_SIZE,
> + * and xsk->chunk_size is limited to 65535 bytes.
> + */
> + if ((size_t)xsk->chunk_size > PAGE_SIZE || xsk->chunk_size < MLX5E_MIN_XSK_CHUNK_SIZE) {
> mlx5_core_err(mdev, "XSK chunk size %u out of bounds [%u, %lu]\n", xsk->chunk_size,
> MLX5E_MIN_XSK_CHUNK_SIZE, PAGE_SIZE);
> return false;
> --
> 2.39.2
>
On Thu, Mar 28, 2024 at 03:30:45PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> clang warns for comparisons that are always true, which is the case
> for these two page size checks on architectures with 64KB pages:
>
> drivers/infiniband/core/uverbs_ioctl.c:90:39: error: result of comparison of constant 65536 with expression of type 'u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> WARN_ON_ONCE(method_elm->bundle_size > PAGE_SIZE);
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> include/asm-generic/bug.h:104:25: note: expanded from macro 'WARN_ON_ONCE'
> int __ret_warn_on = !!(condition); \
> ^~~~~~~~~
> drivers/infiniband/core/uverbs_ioctl.c:621:17: error: result of comparison of constant 65536 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> if (hdr.length > PAGE_SIZE ||
> ~~~~~~~~~~ ^ ~~~~~~~~~
>
> Add a cast to u32 in both cases, so it never warns about this.
But doesn't that hurt the codegen?
Jason
On Wed, Apr 3, 2024, at 17:45, Jason Gunthorpe wrote:
> On Thu, Mar 28, 2024 at 03:30:45PM +0100, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> clang warns for comparisons that are always true, which is the case
>> for these two page size checks on architectures with 64KB pages:
>>
>> drivers/infiniband/core/uverbs_ioctl.c:90:39: error: result of comparison of constant 65536 with expression of type 'u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>> WARN_ON_ONCE(method_elm->bundle_size > PAGE_SIZE);
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
>> include/asm-generic/bug.h:104:25: note: expanded from macro 'WARN_ON_ONCE'
>> int __ret_warn_on = !!(condition); \
>> ^~~~~~~~~
>> drivers/infiniband/core/uverbs_ioctl.c:621:17: error: result of comparison of constant 65536 with expression of type '__u16' (aka 'unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>> if (hdr.length > PAGE_SIZE ||
>> ~~~~~~~~~~ ^ ~~~~~~~~~
>>
>> Add a cast to u32 in both cases, so it never warns about this.
>
> But doesn't that hurt the codegen?
I just double-checked in the compiler explorer to confirm that
this works as I expected: both gcc and clang are still able
to optimize out the comparison for 64K pages, but clang no
longer complains after my change that this is an obvious
case.
I also see that gcc still produces a -Wtype-limits warning, but that
likely has to stay disabled because it produces too much output
elsewhere and I don't see an easy way to shut it up.
Arnd