linux-kernel - Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions

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: <20170820031805.GF11771@tardis>
Date:   Sun, 20 Aug 2017 11:18:05 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Byungchul Park <byungchul.park@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Michel Lespinasse <walken@...gle.com>, kirill@...temov.name,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>, willy@...radead.org,
        Nicholas Piggin <npiggin@...il.com>, kernel-team@....com
Subject: Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions

On Sat, Aug 19, 2017 at 02:51:17PM +0200, Arnd Bergmann wrote:
[...]
> > Those two "rep movsq"s are very suspicious, because
> > COMPLETION_INITIALIZER_ONSTACK() should initialize the data in-place,
> > rather than move it to some temporary variable and copy it back.
> 
> Right. I've seen this behavior before when using c99 compound
> literals, but I was surprised to see it here.
> 
> I also submitted a patch for the one driver that turned up a new
> warning because of this behavior:
> 
> https://www.spinics.net/lists/raid/msg58766.html
> 

This solution also came up into my mind but then I found there are
several callsites of COMPLETION_INITIALIZER_ONSTACK(), so I then tried
to find a way to fix the macro itself. But your patch looks good to me
;-)

> In case of the mmc driver, the behavior was as expected, it was
> just a little too large and I sent the obvious workaround for it
> 
> https://patchwork.kernel.org/patch/9902063/
> 

Yep.

> > I tried to reduce the size of completion struct, and the "rep movsq" did
> > go away, however it seemed the compiler still allocated the memory for
> > the temporary variables on the stack, because whenever I
> > increased/decreased  the size of completion, the stack size of
> > write_journal() got increased/decreased *7* times, but there are only
> > 3 journal_completion structures in write_journal(). So the *4* callsites
> > of COMPLETION_INITIALIZER_ONSTACK() looked very suspicous.
> >
> > So I come up with the following patch, trying to teach the compiler not
> > to do the unnecessary allocation, could you give it a try?
> >
> > Besides, I could also observe the stack size reduction of
> > write_journal() even for !LOCKDEP kernel.
> 
> Ok.
> 
> > -------------------
> > Reported-by: Arnd Bergmann <arnd@...db.de>
> > Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> > ---
> >  include/linux/completion.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/completion.h b/include/linux/completion.h
> > index 791f053f28b7..cae5400022a3 100644
> > --- a/include/linux/completion.h
> > +++ b/include/linux/completion.h
> > @@ -74,7 +74,7 @@ static inline void complete_release_commit(struct completion *x) {}
> >  #endif
> >
> >  #define COMPLETION_INITIALIZER_ONSTACK(work) \
> > -       ({ init_completion(&work); work; })
> > +       (*({ init_completion(&work); &work; }))
> >
> >  /**
> >   * DECLARE_COMPLETION - declare and initialize a completion structure
> 
> Nice hack. Any idea why that's different to the compiler?
> 

So *I think* the block {init_completion(&work); &work;} now will return
a pointer rather than a whole structure, and a pointer could fit in a
register, so the compiler won't bother to allocate the memory for it.

> I've applied that one to my test tree now, and reverted my own patch,
> will let you know if anything else shows up. I think we probably want

Thanks ;-)

> to merge both patches to mainline.
> 

Agreed! Unless we want to remove COMPLETION_INITIALIZER_ONSTACK() for
some reason, then my patch is not needed.

Regards,
Boqun

>       Arnd

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)