linux-kernel - Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP

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: <CAKwvOdmDwO1U1sk=3-6zXFrVw6by1e_9VBx1WWMCx_rMqYSw-A@mail.gmail.com>
Date:   Fri, 3 Aug 2018 11:11:28 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     dave.anglin@...l.net
Cc:     deller@....de, jejb@...isc-linux.org,
        Nathan Chancellor <natechancellor@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Pravin Shedge <pravin.shedge4linux@...il.com>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-parisc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

On Fri, Aug 3, 2018 at 10:57 AM John David Anglin <dave.anglin@...l.net> wrote:
>
> On 2018-08-02 4:31 PM, Nick Desaulniers wrote:
> > If I understand your point correctly, is it that you're saying that
> > _THIS_IP_  should be implemented in terms of inline assembly (as in
> > what current_text_addr() is currently)?  If that's what you mean and
> > I'm understanding correctly, my point is that we should be preferring
> > the generic C implementation as that's what's being used in most
> > places currently, so if it was broken you'd likely already know about
> > it.  Unless unwinding is truly broken by the additional label, I don't
> > think we need an inline assembly implementation of current_text_addr()
> > for parisc (or any arch for that matter).  If we do, then it can be
> > localized to the parisc unwinding code, that way it can be
> > consolidated everywhere else for every other arch.
> The label breaks the unwind data, not the unwind code.  So, localizing
> the use of
> current_text_addr() to the parisc unwind code doesn't help.

But the kernel uses the generic _THIS_IP_ *everywhere*, not parisc's
custom current_text_addr().  So if this did actually break unwinding,
you should have noticed by now.  There's only one call site that uses
the custom current_text_addr(), which is what my patch is addressing.

Localizing the custom implementation would literally produce the same
binary as is produced today, but allow us to start removing all the
other custom implementations of current_text_addr() from
arch/*/include/asm/processor.h, which is why I proposed that as an
alternative to this patch.

> Personally, I prefer the implementation of current_text_addr() because:
>
> 1) The generated code is smaller, and

One instruction vs two, for a single call site that I bet is cold and
not inlined in multiple places.

> 2) it doesn't introduce any unnecessary labels into the text.
>
> As noted, these labels can cause issues with unwinding.

Can you confirm that applying this patch actually breaks unwinding?

-- 
Thanks,
~Nick Desaulniers