[ adding Dan Carpenter ]
> On Jun 1, 2023, at 10:33 AM, Dmitry Antipov <[email protected]> wrote:
>
> Fix the following warning observed when building 64-bit (actually arm64)
> kernel with clang-17 (make LLVM=1 W=1):
>
> include/linux/sunrpc/xdr.h:779:10: warning: result of comparison of constant
> 4611686018427387903 with expression of type '__u32' (aka 'unsigned int') is
> always false [-Wtautological-constant-out-of-range-compare]
> 779 | if (len > SIZE_MAX / sizeof(*p))
>
> That is, an overflow check makes sense for 32-bit kernel only.
>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> include/linux/sunrpc/xdr.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 72014c9216fc..b2d5dc89cf7b 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -776,7 +776,7 @@ xdr_stream_decode_uint32_array(struct xdr_stream *xdr,
>
> if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0))
> return -EBADMSG;
> - if (len > SIZE_MAX / sizeof(*p))
> + if (unlikely(SIZE_MAX == U32_MAX ? (len > U32_MAX / sizeof(*p)) : 0))
> return -EBADMSG;
> p = xdr_inline_decode(xdr, len * sizeof(*p));
> if (unlikely(!p))
> --
> 2.40.1
>
--
Chuck Lever
On Thu, Jun 01, 2023 at 02:38:58PM +0000, Chuck Lever III wrote:
> > - if (len > SIZE_MAX / sizeof(*p))
> > + if (unlikely(SIZE_MAX == U32_MAX ? (len > U32_MAX / sizeof(*p)) : 0))
>
This is a bug in Clang.
Generally the rule, is that if there is a bug in the static checker then
you should fix the checker instead of the code. Smatch used to have
this same bug but I fixed it. So it's not something which is
unfixable. This doesn't cause a problem for normal Clang builds, only
for W=1, right?
But, here is a nicer way to fix it. You can send this, or I can send
it tomorrow with your Reported-by?
regards,
dan carpenter
Fix the following warning observed when building 64-bit (actually arm64)
kernel with clang-17 (make LLVM=1 W=1):
include/linux/sunrpc/xdr.h:779:10: warning: result of comparison of constant
4611686018427387903 with expression of type '__u32' (aka 'unsigned int') is
always false [-Wtautological-constant-out-of-range-compare]
779 | if (len > SIZE_MAX / sizeof(*p))
That is, an overflow check makes sense for 32-bit kernel only. Silence
the Clang warning and make the code nicer by using the size_mul()
function to prevent integer overflows.
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index f89ec4b5ea16..dbf7620a2853 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -775,9 +775,7 @@ xdr_stream_decode_uint32_array(struct xdr_stream *xdr,
if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0))
return -EBADMSG;
- if (len > SIZE_MAX / sizeof(*p))
- return -EBADMSG;
- p = xdr_inline_decode(xdr, len * sizeof(*p));
+ p = xdr_inline_decode(xdr, size_mul(len, sizeof(*p)));
if (unlikely(!p))
return -EBADMSG;
if (array == NULL)
I'd kind of like to make some other changes as well like...
I think I looked at this and it wraps to zero which is harmless but I
just hate that it has an integer overflow at all. Gotta run though.
Will look at this tomorrow.
regards,
dan carpenter
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index f89ec4b5ea16..3550dea95420 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -29,7 +29,7 @@ struct rpc_rqst;
/*
* Buffer adjustment
*/
-#define XDR_QUADLEN(l) (((l) + 3) >> 2)
+#define XDR_QUADLEN(l) (size_add(l, 3) >> 2)
/*
* Generic opaque `network object.'
On 6/1/23 18:34, Dan Carpenter wrote:
> This is a bug in Clang.
Is it confirmed by LLVM/clang developers? The compiler says that
<any unsigned 32-bit> can't be larger than <max unsigned 64-bit> / 8
(assuming LP64). Why this is wrong?
Dmitry
On Thu, Jun 1, 2023 at 11:46 AM Dan Carpenter <[email protected]> wrote:
>
> On Thu, Jun 01, 2023 at 02:38:58PM +0000, Chuck Lever III wrote:
> > > - if (len > SIZE_MAX / sizeof(*p))
> > > + if (unlikely(SIZE_MAX == U32_MAX ? (len > U32_MAX / sizeof(*p)) : 0))
> >
>
> This is a bug in Clang.
>
> Generally the rule, is that if there is a bug in the static checker then
> you should fix the checker instead of the code. Smatch used to have
> this same bug but I fixed it. So it's not something which is
> unfixable. This doesn't cause a problem for normal Clang builds, only
> for W=1, right?
>
> But, here is a nicer way to fix it. You can send this, or I can send
> it tomorrow with your Reported-by?
>
> regards,
> dan carpenter
>
> Fix the following warning observed when building 64-bit (actually arm64)
> kernel with clang-17 (make LLVM=1 W=1):
>
> include/linux/sunrpc/xdr.h:779:10: warning: result of comparison of constant
> 4611686018427387903 with expression of type '__u32' (aka 'unsigned int') is
> always false [-Wtautological-constant-out-of-range-compare]
> 779 | if (len > SIZE_MAX / sizeof(*p))
>
> That is, an overflow check makes sense for 32-bit kernel only. Silence
> the Clang warning and make the code nicer by using the size_mul()
> function to prevent integer overflows.
>
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index f89ec4b5ea16..dbf7620a2853 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -775,9 +775,7 @@ xdr_stream_decode_uint32_array(struct xdr_stream *xdr,
>
> if (unlikely(xdr_stream_decode_u32(xdr, &len) < 0))
> return -EBADMSG;
> - if (len > SIZE_MAX / sizeof(*p))
> - return -EBADMSG;
> - p = xdr_inline_decode(xdr, len * sizeof(*p));
> + p = xdr_inline_decode(xdr, size_mul(len, sizeof(*p)));
I personally prefer this approach, and I agree that it makes the code
look nicer.
Anna
> if (unlikely(!p))
> return -EBADMSG;
> if (array == NULL)
>
On Thu, Jun 01, 2023 at 06:52:03PM +0300, Dmitry Antipov wrote:
> On 6/1/23 18:34, Dan Carpenter wrote:
>
> > This is a bug in Clang.
>
> Is it confirmed by LLVM/clang developers? The compiler says that
> <any unsigned 32-bit> can't be larger than <max unsigned 64-bit> / 8
> (assuming LP64). Why this is wrong?
It's a false positive because the test is obviously intended for 32-bit
longs.
regards,
dan carpenter
On 6/1/23 19:39, Dan Carpenter wrote:
> It's a false positive because the test is obviously intended for 32-bit
> longs.
I'm not an expert in compiler development, but I do not understand
"obviously intended" in this context. An input literally compares
<any unsigned 32-bit> > <max unsigned 64-bit> / 8, which is always
false, and so the compiler complains. If "obviously intended" means
"the compiler should silently optimize away this check for LP64",
I would disagree, and that's why I would like to see the confirmation
from LLVM/clang developers.
Dmitry
> On Jun 1, 2023, at 1:06 PM, Dmitry Antipov <[email protected]> wrote:
>
> On 6/1/23 19:39, Dan Carpenter wrote:
>
>> It's a false positive because the test is obviously intended for 32-bit
>> longs.
>
> I'm not an expert in compiler development, but I do not understand
> "obviously intended" in this context. An input literally compares
> <any unsigned 32-bit> > <max unsigned 64-bit> / 8, which is always
> false, and so the compiler complains. If "obviously intended" means
> "the compiler should silently optimize away this check for LP64",
> I would disagree, and that's why I would like to see the confirmation
> from LLVM/clang developers.
Dan, Dmitry, has there been any resolution of this issue?
--
Chuck Lever