linux-kernel - Re: [PATCH 3/6] tracing: Wrap section comparison in tracer_alloc_buffers with COMPARE

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: <CAKwvOdm-N1iX0SMxGDV5Vf=qS5uHPdH3S-TRs-065BuSOdKt1w@mail.gmail.com>
Date:   Wed, 19 Feb 2020 09:44:31 -0800
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Steven Rostedt <rostedt@...dmis.org>,
        Nathan Chancellor <natechancellor@...il.com>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Arnd Bergmann <arnd@...db.de>, Ingo Molnar <mingo@...hat.com>,
        Jason Baron <jbaron@...mai.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH 3/6] tracing: Wrap section comparison in
 tracer_alloc_buffers with COMPARE_SECTIONS

On Wed, Feb 19, 2020 at 6:34 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Tue, 18 Feb 2020 21:54:20 -0700
> Nathan Chancellor <natechancellor@...il.com> wrote:
>
> > Clang warns:
> >
> > ../kernel/trace/trace.c:9335:33: warning: array comparison always
> > evaluates to true [-Wtautological-compare]
> >         if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
> >                                        ^
> > 1 warning generated.
> >
> > These are not true arrays, they are linker defined symbols, which are
> > just addresses so there is not a real issue here. Use the
> > COMPARE_SECTIONS macro to silence this warning by casting the linker
> > defined symbols to unsigned long, which keeps the logic the same.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/765
> > Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> > ---
> >  kernel/trace/trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index c797a15a1fc7..e1f3b16e457b 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -9332,7 +9332,7 @@ __init static int tracer_alloc_buffers(void)
> >               goto out_free_buffer_mask;
> >
> >       /* Only allocate trace_printk buffers if a trace_printk exists */
> > -     if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)
> > +     if (COMPARE_SECTIONS(__stop___trace_bprintk_fmt, !=, __start___trace_bprintk_fmt))
>
> Sorry, but this is really ugly and unreadable. Please find some other
> solution to fix this.
>
> NAK-by: Steven Rostedt
>

Hey Nathan,
Thanks for the series; enabling the warning will help us find more
bugs.  Revisiting what the warning is about, I checked on this
"referring to symbols defined in linker scripts from C" pattern.  This
document [0] (by no means definitive, but I think it has a good idea)
says:

Linker symbols that represent a data address: In C code, declare the
variable as an extern variable. Then, refer to the value of the linker
symbol using the & operator. Because the variable is at a valid data
address, we know that a data pointer can represent the value.
Linker symbols for an arbitrary address: In C code, declare this as an
extern symbol. The type does not matter. If you are using GCC
extensions, declare it as "extern void".

Indeed, it seems that Clang is happier with that pattern:
https://godbolt.org/z/sW3t5W

Looking at __stop___trace_bprintk_fmt in particular:

kernel/trace/trace.h
1923:extern const char *__stop___trace_bprintk_fmt[];

(Should be `extern const void __stop___trace_bprintk_fmt;` void since
we don't access any data or function from that symbol, just compare
its address)

kernel/trace/trace_printk.c
260: start_index = __stop___trace_bprintk_fmt - __start___trace_bprintk_fmt;

(Should be `start_index = (uintptr_t)__stop___trace_bprintk_fmt -
(uintptr_t)__start___trace_bprintk_fmt;`) (storing the result in an
int worries me a little, but maybe that refactoring can be saved for
another day)

kernel/trace/trace.c
9335: if (__stop___trace_bprintk_fmt != __start___trace_bprintk_fmt)

(Should be `if (&__stop___trace_bprintk_fmt -
&__start___trace_bprintk_fmt)`.  That's not a significant change to
the existing code, and is quite legible IMO)

Steven, thoughts?

[0] http://downloads.ti.com/docs/esd/SPRUI03/using-linker-symbols-in-c-c-applications-slau1318080.html
-- 
Thanks,
~Nick Desaulniers