linux-kernel - Re: [PATCH v2] overflow: Add __must_check attribute to check

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: <20200818062736.GL7555@unreal>
Date:   Tue, 18 Aug 2020 09:27:36 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     dsterba@...e.cz, Rasmus Villemoes <linux@...musvillemoes.dk>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Matthew Wilcox <willy@...radead.org>,
        linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v2] overflow: Add __must_check attribute to check_*()
 helpers

On Mon, Aug 17, 2020 at 12:36:51PM -0700, Kees Cook wrote:
> On Mon, Aug 17, 2020 at 11:08:54AM +0200, David Sterba wrote:
> > On Sat, Aug 15, 2020 at 10:09:24AM -0700, Kees Cook wrote:
> > > +static inline bool __must_check __must_check_overflow(bool overflow)
> > > +{
> > > +	return unlikely(overflow);
> >
> > How does the 'unlikely' hint propagate through return? It is in a static
> > inline so compiler has complete information in order to use it, but I'm
> > curious if it actually does.
>
> It may not -- it depends on how the compiler decides to deal with it. :)

In theory yes, in practice, the compilers will ignore this macro.

And if you success to force compiler to use this macro, it won't give
any real performance advantage.

We (RDMA) tried very hard to see any performance gain by instrumenting
code with likely/unlikely in our performance critical data path both in
user space and kernel. The performance results were statistically equal.

If you are interested, we had a very intense discussion about it when
likely/unlikely can still be usable (hint random input).
https://lore.kernel.org/linux-rdma/20200807160956.GO4432@unreal

Thanks

>
> > In case the hint gets dropped, the fix would probably be
> >
> > #define check_add_overflow(a, b, d) unlikely(__must_check_overflow(({	\
> >  	typeof(a) __a = (a);			\
> >  	typeof(b) __b = (b);			\
> >  	typeof(d) __d = (d);			\
> >  	(void) (&__a == &__b);			\
> >  	(void) (&__a == __d);			\
> >  	__builtin_add_overflow(__a, __b, __d);	\
> > })))
>
> Unfortunately not, as the unlikely() ends up eating the __must_check
> attribute. :(
>
> --
> Kees Cook