linux-kernel - Re: [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary

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: <CAKwvOd=9o6B0NHEAZHjsDdQ+rwaCeni+2yk2WRX=NB2946VtgQ@mail.gmail.com>
Date:   Wed, 8 Jun 2022 14:16:47 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Justin Stitt <jstitt007@...il.com>
Cc:     linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
        nathan@...nel.org, trix@...hat.com,
        Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary

On Wed, Jun 8, 2022 at 1:09 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
>
> On Tue,  7 Jun 2022 17:14:22 -0700 Justin Stitt <jstitt007@...il.com> wrote:
>
> > A cast inside __builtin_constant_p doesn't do anything since it should evaluate
> > as constant at compile time irrespective of this cast. Instead, I moved this
> > cast outside the ternary to ensure the return type is as expected.
> >
> > For instance, if __HAVE_BUILTIN_BSWAP16__ was not defined then __swab16 is
> > actually returning an `int` not a `u16` due to integer promotion as described
> > by Nick in this thread. This has repercussions when building with clang
> > -Wformat. This fix should solve many of these warnings.
> >
>
> ARM allmodconfig:
>
> In file included from ./include/linux/swab.h:5,
>                  from ./arch/arm/include/asm/opcodes.h:86,
>                  from ./arch/arm/include/asm/bug.h:7,
>                  from ./include/linux/bug.h:5,
>                  from ./include/linux/mmdebug.h:5,
>                  from ./include/linux/gfp.h:5,
>                  from ./include/linux/slab.h:15,
>                  from ./fs/xfs/kmem.h:9,
>                  from ./fs/xfs/xfs_linux.h:24,
>                  from ./fs/xfs/xfs.h:22,
>                  from fs/xfs/scrub/agheader.c:6:
> fs/xfs/scrub/agheader.c: In function 'xchk_superblock':
> ./include/uapi/linux/byteorder/little_endian.h:42:52: error: unsigned conversion from 'int' to 'short unsigned int' changes value from '-49265' to '16271' [-Werror=overflow]
>    42 | #define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
>       |                                                    ^~~
> ./include/uapi/linux/swab.h:102:46: note: in definition of macro '__swab16'
>   102 | #define __swab16(x) (__u16)__builtin_bswap16(x)
>       |                                              ^
> ./include/linux/byteorder/generic.h:96:21: note: in expansion of macro '__cpu_to_be16'
>    96 | #define cpu_to_be16 __cpu_to_be16
>       |                     ^~~~~~~~~~~~~
> fs/xfs/scrub/agheader.c:158:23: note: in expansion of macro 'cpu_to_be16'
>   158 |         vernum_mask = cpu_to_be16(~XFS_SB_VERSION_OKBITS |
>       |                       ^~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[2]: *** [scripts/Makefile.build:249: fs/xfs/scrub/agheader.o] Error 1
> make[1]: *** [scripts/Makefile.build:466: fs/xfs] Error 2
> make: *** [Makefile:1839: fs] Error 2
>

Ah, I see what went wrong.

Justin added more than I requested.
Compare the diffstat between:
https://lore.kernel.org/llvm/CAKwvOd=-NXR5HoHwwEUZMsCt90oaADH6XHifje9n-8S8rj9SFw@mail.gmail.com/
with the v2:
https://lore.kernel.org/llvm/20220608001422.26383-1-jstitt007@gmail.com/

Justin, please send a v3 dropping the changes made to the
"__HAVE_BUILTIN_BSWAP*__ is defined" case; just make the changes I
suggested.

In addition, please add to the commit message this snippet from Al:
```
As Al Viro notes:

You *can't* get smaller-than-int out of ? :, same as you can't get it
out of addition, etc.
```

Finally, Justin, you can retest this by running:
$ ARCH=arm make LLVM=1 -j72 allmodconfig fs/xfs/scrub/agheader.o
with your patch applied to linux-next. (It wouldn't hurt to build the
whole thing...but it's next, which is red today for ARCH=arm so nvm:
https://github.com/ClangBuiltLinux/continuous-integration2/runs/6796317443?check_suite_focus=true).
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
-- 
Thanks,
~Nick Desaulniers