| Message ID | 20230601143332.255312-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sunrpc: fix clang-17 warning | expand |
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 <dan.carpenter@linaro.org> 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 <dmantipov@yandex.ru> 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
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))
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 <dmantipov@yandex.ru> --- include/linux/sunrpc/xdr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)