linux-kernel - Re: [PATCH 5/9] ipv4: tcp_output: avoid warning about NET_ADD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <62cf1c3c-29b7-48cc-9d52-cd47a6c07aa4@app.fastmail.com>
Date: Thu, 28 Mar 2024 17:39:36 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Eric Dumazet" <edumazet@...gle.com>, "Arnd Bergmann" <arnd@...nel.org>
Cc: linux-kernel@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
 "David Ahern" <dsahern@...nel.org>, "Jakub Kicinski" <kuba@...nel.org>,
 "Paolo Abeni" <pabeni@...hat.com>, "Nathan Chancellor" <nathan@...nel.org>,
 "Nick Desaulniers" <ndesaulniers@...gle.com>,
 "Bill Wendling" <morbo@...gle.com>, "Justin Stitt" <justinstitt@...gle.com>,
 "Dmitry Safonov" <0x7f454c46@...il.com>,
 "Neal Cardwell" <ncardwell@...gle.com>,
 "mfreemon@...udflare.com" <mfreemon@...udflare.com>,
 "Yan Zhai" <yan@...udflare.com>, Netdev <netdev@...r.kernel.org>,
 llvm@...ts.linux.dev
Subject: Re: [PATCH 5/9] ipv4: tcp_output: avoid warning about NET_ADD_STATS

On Thu, Mar 28, 2024, at 15:38, Eric Dumazet wrote:
> On Thu, Mar 28, 2024 at 3:31 PM Arnd Bergmann <arnd@...nel.org> 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