linux-kernel - Re: [PATCH 1/2] asm-generic/atomic: Prefer __always

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: <CANpmjNMLEYdW0kaLAiO9fQN1uC7bW6K08zZRG=GG7vq4fBn+WA@mail.gmail.com>
Date:   Mon, 25 Nov 2019 19:22:33 +0100
From:   Marco Elver <elver@...gle.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH 1/2] asm-generic/atomic: Prefer __always_inline for wrappers

On Mon, 25 Nov 2019 at 18:38, Mark Rutland <mark.rutland@....com> wrote:
>
> On Fri, Nov 22, 2019 at 04:42:20PM +0100, Marco Elver wrote:
> > Prefer __always_inline for atomic wrappers. When building for size
> > (CC_OPTIMIZE_FOR_SIZE), some compilers appear to be less inclined to
> > inline even relatively small static inline functions that are assumed to
> > be inlinable such as atomic ops. This can cause problems, for example in
> > UACCESS regions.
>
> From looking at the link below, the problem is tat objtool isn't happy
> about non-whiteliested calls within UACCESS regions.
>
> Is that a problem here? are the kasan/kcsan calls whitelisted?

We whitelisted all the relevant functions.

The problem it that small static inline functions private to the
compilation unit do not get inlined when CC_OPTIMIZE_FOR_SIZE=y (they
do get inlined when CC_OPTIMIZE_FOR_PERFORMANCE=y).

For the runtime this is easy to fix, by just making these small
functions __always_inline (also avoiding these function call overheads
in the runtime when CC_OPTIMIZE_FOR_SIZE).

I stumbled upon the issue for the atomic ops, because the runtime uses
atomic_long_try_cmpxchg outside a user_access_save() region (and it
should not be moved inside). Essentially I fixed up the runtime, but
then objtool still complained about the access to
atomic64_try_cmpxchg. Hence this patch.

I believe it is the right thing to do, because the final inlining
decision should *not* be made by wrappers. I would think this patch is
the right thing to do irrespective of KCSAN or not.

> > By using __always_inline, we let the real implementation and not the
> > wrapper determine the final inlining preference.
>
> That sounds reasonable to me, assuming that doesn't end up significantly
> bloating the kernel text. What impact does this have on code size?

It actually seems to make it smaller.

x86 tinyconfig:
- vmlinux baseline: 1316204
- vmlinux with patches: 1315988 (-216 bytes)

> > This came up when addressing UACCESS warnings with CC_OPTIMIZE_FOR_SIZE
> > in the KCSAN runtime:
> > http://lkml.kernel.org/r/58708908-84a0-0a81-a836-ad97e33dbb62@infradead.org
> >
> > Reported-by: Randy Dunlap <rdunlap@...radead.org>
> > Signed-off-by: Marco Elver <elver@...gle.com>
> > ---
> >  include/asm-generic/atomic-instrumented.h | 334 +++++++++++-----------
> >  include/asm-generic/atomic-long.h         | 330 ++++++++++-----------
> >  scripts/atomic/gen-atomic-instrumented.sh |   6 +-
> >  scripts/atomic/gen-atomic-long.sh         |   2 +-
> >  4 files changed, 336 insertions(+), 336 deletions(-)
>
> Do we need to do similar for gen-atomic-fallback.sh and the fallbacks
> defined in scripts/atomic/fallbacks/ ?

I think they should be, but I think that's debatable. Some of them do
a little more than just wrap things. If we want to make this
__always_inline, I would do it in a separate patch independent from
this series to not stall the fixes here.

What do you prefer?

> [...]
>
> > diff --git a/scripts/atomic/gen-atomic-instrumented.sh b/scripts/atomic/gen-atomic-instrumented.sh
> > index 8b8b2a6f8d68..68532d4f36ca 100755
> > --- a/scripts/atomic/gen-atomic-instrumented.sh
> > +++ b/scripts/atomic/gen-atomic-instrumented.sh
> > @@ -84,7 +84,7 @@ gen_proto_order_variant()
> >       [ ! -z "${guard}" ] && printf "#if ${guard}\n"
> >
> >  cat <<EOF
> > -static inline ${ret}
> > +static __always_inline ${ret}
>
> We should add an include of <linux/compiler.h> to the preamble if we're
> explicitly using __always_inline.

Will add in v2.

> > diff --git a/scripts/atomic/gen-atomic-long.sh b/scripts/atomic/gen-atomic-long.sh
> > index c240a7231b2e..4036d2dd22e9 100755
> > --- a/scripts/atomic/gen-atomic-long.sh
> > +++ b/scripts/atomic/gen-atomic-long.sh
> > @@ -46,7 +46,7 @@ gen_proto_order_variant()
> >       local retstmt="$(gen_ret_stmt "${meta}")"
> >
> >  cat <<EOF
> > -static inline ${ret}
> > +static __always_inline ${ret}
>
> Likewise here

Will add in v2.

Thanks,
-- Marco