linux-kernel - Re: [Intel-gfx] [PATCH] drm/i915: Zero initialize this_cpu in busywait

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] [day] [month] [year] [list]

Message-ID: <CAKwvOd=mpKR2V9rOEDDytctxT7uSwREyqfDVrYEEessWL=DY8Q@mail.gmail.com>
Date:   Fri, 8 Mar 2019 11:26:10 -0800
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Chris Wilson <chris@...is-wilson.co.uk>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        intel-gfx@...ts.freedesktop.org,
        LKML <linux-kernel@...r.kernel.org>,
        dri-devel@...ts.freedesktop.org,
        clang-built-linux@...glegroups.com,
        Greg KH <gregkh@...uxfoundation.org>,
        Sandeep Patil <sspatil@...gle.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Zero initialize this_cpu in busywait_stop

On Fri, Mar 8, 2019 at 12:27 AM Chris Wilson <chris@...is-wilson.co.uk> wrote:
>
> Quoting Nathan Chancellor (2019-03-08 01:20:24)
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/gpu/drm/i915/i915_request.c:1032:6: warning: variable 'this_cpu'
> > is used uninitialized whenever '&&' condition is false
> > [-Wsometimes-uninitialized]
> >
> > time_after expands to use two typecheck with logical ANDs between them.
> > typecheck evaluates to 1 but Clang clearly gets confused with the logic
> > that as semantic analysis happens early in the pipeline. Fix this by
> > just zero initializing this_cpu as it will always be properly
> > initialized before the comparison below.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/415
> > Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> > ---
> >
> > Alternatively, this can be solved by having the return value of
> > local_clock_us(&this_cpu) be a local variable but this seems less
> > controversial.
>
> I'll just wait for clang to be fixed, as this severely undermines any
> respect I have for its semantic analysis.
> -Chris

I'm still playing around with this in Godbolt (my hunch is that GNU C
statement expressions are maybe inlined as part of GCC's early
inlining phase).  For example:
https://godbolt.org/z/G54s5z

If you change `typecheck(unsigned long, a)` and `typecheck(unsigned
long, b)` in `time_after()` both to `1` (what `typecheck` would
evaluate to), then the warning goes away.  But a further
simplification shows that GNU C statement expressions are not the
problem:
https://godbolt.org/z/KzCN8m

I need to keep investigating, but there may be more we can do on the
compiler side.

It seems that another workaround that avoid default initialization is
to just create another local for the temporary expression that
provably initialized this_cpu, ie.

diff --git a/drivers/gpu/drm/i915/i915_request.c
b/drivers/gpu/drm/i915/i915_request.c
index c2a5c48c7541..5b90b5c35c8b 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1028,8 +1028,9 @@ static unsigned long local_clock_us(unsigned int *cpu)
 static bool busywait_stop(unsigned long timeout, unsigned int cpu)
 {
        unsigned int this_cpu;
+       unsigned long local_clock = local_clock_us(&this_cpu);

-       if (time_after(local_clock_us(&this_cpu), timeout))
+       if (time_after(local_clock, timeout))
                return true;

        return this_cpu != cpu;

-- 
Thanks,
~Nick Desaulniers