diff mbox series

[4/4] objtool: add UACCESS exceptions for __tsan_volatile_read/write

Message ID 20230208164011.2287122-4-arnd@kernel.org
State New
Headers
Series [1/4] kasan: mark addr_has_metadata __always_inline |

Commit Message

Arnd Bergmann Feb. 8, 2023, 4:39 p.m. UTC 
  From: Arnd Bergmann <arnd@arndb.de>

A lot of the tsan helpers are already excempt from the UACCESS warnings,
but some more functions were added that need the same thing:

kernel/kcsan/core.o: warning: objtool: __tsan_volatile_read16+0x0: call to __tsan_unaligned_read16() with UACCESS enabled
kernel/kcsan/core.o: warning: objtool: __tsan_volatile_write16+0x0: call to __tsan_unaligned_write16() with UACCESS enabled
vmlinux.o: warning: objtool: __tsan_unaligned_volatile_read16+0x4: call to __tsan_unaligned_read16() with UACCESS enabled
vmlinux.o: warning: objtool: __tsan_unaligned_volatile_write16+0x4: call to __tsan_unaligned_write16() with UACCESS enabled

Fixes: 75d75b7a4d54 ("kcsan: Support distinguishing volatile accesses")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 tools/objtool/check.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Marco Elver Feb. 8, 2023, 4:59 p.m. UTC | #1 
On Wed, 8 Feb 2023 at 17:40, Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> A lot of the tsan helpers are already excempt from the UACCESS warnings,
> but some more functions were added that need the same thing:
>
> kernel/kcsan/core.o: warning: objtool: __tsan_volatile_read16+0x0: call to __tsan_unaligned_read16() with UACCESS enabled
> kernel/kcsan/core.o: warning: objtool: __tsan_volatile_write16+0x0: call to __tsan_unaligned_write16() with UACCESS enabled
> vmlinux.o: warning: objtool: __tsan_unaligned_volatile_read16+0x4: call to __tsan_unaligned_read16() with UACCESS enabled
> vmlinux.o: warning: objtool: __tsan_unaligned_volatile_write16+0x4: call to __tsan_unaligned_write16() with UACCESS enabled

That's odd - this has never been needed, because all __tsan_unaligned
are aliases for the non-unaligned functions. And all those are in the
uaccess_safe_builtin list already.

So if suddenly the alias name becomes the symbol that objtool sees, we
might need to add all the other functions as well.

Is this a special build with a new compiler?

> Fixes: 75d75b7a4d54 ("kcsan: Support distinguishing volatile accesses")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  tools/objtool/check.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index e8fb3bf7a2e3..d89ef6957021 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1200,6 +1200,8 @@ static const char *uaccess_safe_builtin[] = {
>         "__tsan_atomic64_compare_exchange_val",
>         "__tsan_atomic_thread_fence",
>         "__tsan_atomic_signal_fence",
> +       "__tsan_unaligned_read16",
> +       "__tsan_unaligned_write16",
>         /* KCOV */
>         "write_comp_data",
>         "check_kcov_mode",
> --
> 2.39.1
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20230208164011.2287122-4-arnd%40kernel.org.
Arnd Bergmann Feb. 8, 2023, 7:53 p.m. UTC | #2 
On Wed, Feb 8, 2023, at 17:59, Marco Elver wrote:
> On Wed, 8 Feb 2023 at 17:40, Arnd Bergmann <arnd@kernel.org> wrote:
>>
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> A lot of the tsan helpers are already excempt from the UACCESS warnings,
>> but some more functions were added that need the same thing:
>>
>> kernel/kcsan/core.o: warning: objtool: __tsan_volatile_read16+0x0: call to __tsan_unaligned_read16() with UACCESS enabled
>> kernel/kcsan/core.o: warning: objtool: __tsan_volatile_write16+0x0: call to __tsan_unaligned_write16() with UACCESS enabled
>> vmlinux.o: warning: objtool: __tsan_unaligned_volatile_read16+0x4: call to __tsan_unaligned_read16() with UACCESS enabled
>> vmlinux.o: warning: objtool: __tsan_unaligned_volatile_write16+0x4: call to __tsan_unaligned_write16() with UACCESS enabled
>
> That's odd - this has never been needed, because all __tsan_unaligned
> are aliases for the non-unaligned functions. And all those are in the
> uaccess_safe_builtin list already.
>
> So if suddenly the alias name becomes the symbol that objtool sees, we
> might need to add all the other functions as well.
>
> Is this a special build with a new compiler?

I see this with gcc-12 and gcc-13 but not with clang-{14,15,16}, have
not tried any older versions recently.

What I see in the .s file for one of the affected configs is

        .globl  __tsan_unaligned_read16
        .set    __tsan_unaligned_read16,__tsan_read16
        .p2align 6
        .globl  __tsan_volatile_read16
        .type   __tsan_volatile_read16, @function
__tsan_volatile_read16:
        endbr64 
        jmp     __tsan_read16   # 
        .size   __tsan_volatile_read16, .-__tsan_volatile_read16
        .globl  __tsan_unaligned_volatile_read16
        .set    __tsan_unaligned_volatile_read16,__tsan_volatile_read16
...
        .set    __tsan_unaligned_write16,__tsan_write16
        .p2align 6
        .globl  __tsan_volatile_write16
        .type   __tsan_volatile_write16, @function
__tsan_volatile_write16:
        endbr64 
        jmp     __tsan_write16  # 
        .size   __tsan_volatile_write16, .-__tsan_volatile_write16
        .globl  __tsan_unaligned_volatile_write16
        .set    __tsan_unaligned_volatile_write16,__tsan_volatile_write16


In the object file that turns into:

0000000000004e80 <__tsan_unaligned_volatile_read16>:
    4e80:       f3 0f 1e fa             endbr64
    4e84:       e9 b7 fe ff ff          jmp    4d40 <__tsan_read16>
...
0000000000005000 <__tsan_unaligned_volatile_write16>:
    5000:       f3 0f 1e fa             endbr64
    5004:       e9 b7 fe ff ff          jmp    4ec0 <__tsan_unaligned_write16>


It appears like it picks randomly between the original name
and the alias here, no idea why. Using the clang integrated assembler
to build the .o file from the gcc generated .s file shows the same
code as

0000000000004e80 <__tsan_unaligned_volatile_read16>:
    4e80:       f3 0f 1e fa             endbr64
    4e84:       e9 00 00 00 00          jmp    4e89 <__tsan_unaligned_volatile_read16+0x9>
                        4e85: R_X86_64_PLT32    __tsan_read16-0x4
...
0000000000005000 <__tsan_unaligned_volatile_write16>:
    5000:       f3 0f 1e fa             endbr64
    5004:       e9 00 00 00 00          jmp    5009 <__tsan_unaligned_volatile_write16+0x9>
                        5005: R_X86_64_PLT32    __tsan_write16-0x4


Attaching the object file for reference.

      Arnd
Marco Elver Feb. 8, 2023, 8:10 p.m. UTC | #3 
On Wed, 8 Feb 2023 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Feb 8, 2023, at 17:59, Marco Elver wrote:
> > On Wed, 8 Feb 2023 at 17:40, Arnd Bergmann <arnd@kernel.org> wrote:
> >>
> >> From: Arnd Bergmann <arnd@arndb.de>
> >>
> >> A lot of the tsan helpers are already excempt from the UACCESS warnings,
> >> but some more functions were added that need the same thing:
> >>
> >> kernel/kcsan/core.o: warning: objtool: __tsan_volatile_read16+0x0: call to __tsan_unaligned_read16() with UACCESS enabled
> >> kernel/kcsan/core.o: warning: objtool: __tsan_volatile_write16+0x0: call to __tsan_unaligned_write16() with UACCESS enabled
> >> vmlinux.o: warning: objtool: __tsan_unaligned_volatile_read16+0x4: call to __tsan_unaligned_read16() with UACCESS enabled
> >> vmlinux.o: warning: objtool: __tsan_unaligned_volatile_write16+0x4: call to __tsan_unaligned_write16() with UACCESS enabled
> >
> > That's odd - this has never been needed, because all __tsan_unaligned
> > are aliases for the non-unaligned functions. And all those are in the
> > uaccess_safe_builtin list already.
> >
> > So if suddenly the alias name becomes the symbol that objtool sees, we
> > might need to add all the other functions as well.
> >
> > Is this a special build with a new compiler?
>
> I see this with gcc-12 and gcc-13 but not with clang-{14,15,16}, have
> not tried any older versions recently.
>
> What I see in the .s file for one of the affected configs is
>
>         .globl  __tsan_unaligned_read16
>         .set    __tsan_unaligned_read16,__tsan_read16
>         .p2align 6
>         .globl  __tsan_volatile_read16
>         .type   __tsan_volatile_read16, @function
> __tsan_volatile_read16:
>         endbr64
>         jmp     __tsan_read16   #
>         .size   __tsan_volatile_read16, .-__tsan_volatile_read16
>         .globl  __tsan_unaligned_volatile_read16
>         .set    __tsan_unaligned_volatile_read16,__tsan_volatile_read16
> ...
>         .set    __tsan_unaligned_write16,__tsan_write16
>         .p2align 6
>         .globl  __tsan_volatile_write16
>         .type   __tsan_volatile_write16, @function
> __tsan_volatile_write16:
>         endbr64
>         jmp     __tsan_write16  #
>         .size   __tsan_volatile_write16, .-__tsan_volatile_write16
>         .globl  __tsan_unaligned_volatile_write16
>         .set    __tsan_unaligned_volatile_write16,__tsan_volatile_write16
>
>
> In the object file that turns into:
>
> 0000000000004e80 <__tsan_unaligned_volatile_read16>:
>     4e80:       f3 0f 1e fa             endbr64
>     4e84:       e9 b7 fe ff ff          jmp    4d40 <__tsan_read16>
> ...
> 0000000000005000 <__tsan_unaligned_volatile_write16>:
>     5000:       f3 0f 1e fa             endbr64
>     5004:       e9 b7 fe ff ff          jmp    4ec0 <__tsan_unaligned_write16>
>
>
> It appears like it picks randomly between the original name
> and the alias here, no idea why. Using the clang integrated assembler
> to build the .o file from the gcc generated .s file shows the same
> code as
>
> 0000000000004e80 <__tsan_unaligned_volatile_read16>:
>     4e80:       f3 0f 1e fa             endbr64
>     4e84:       e9 00 00 00 00          jmp    4e89 <__tsan_unaligned_volatile_read16+0x9>
>                         4e85: R_X86_64_PLT32    __tsan_read16-0x4
> ...
> 0000000000005000 <__tsan_unaligned_volatile_write16>:
>     5000:       f3 0f 1e fa             endbr64
>     5004:       e9 00 00 00 00          jmp    5009 <__tsan_unaligned_volatile_write16+0x9>
>                         5005: R_X86_64_PLT32    __tsan_write16-0x4

Interesting - also note that in kernel/kcsan/core.c, these functions
don't even call each other explicitly. Although because sizeof(long) <
16 everywhere, the code for the volatile and non-volatile 16-byte
variants ends up the same. So the optimizer seems to think it's ok to
just "call" the other equivalent function, even though we didn't tell
it to do so - check_access() is __always_inline.

Whatever happens here isn't completely wrong, so if you just want to
silence the warning:

  Acked-by: Marco Elver <elver@google.com>

But I have a feeling the compiler is being a bit too clever here.
diff mbox series

Patch

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e8fb3bf7a2e3..d89ef6957021 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1200,6 +1200,8 @@  static const char *uaccess_safe_builtin[] = {
 	"__tsan_atomic64_compare_exchange_val",
 	"__tsan_atomic_thread_fence",
 	"__tsan_atomic_signal_fence",
+	"__tsan_unaligned_read16",
+	"__tsan_unaligned_write16",
 	/* KCOV */
 	"write_comp_data",
 	"check_kcov_mode",