Message-ID: <CAOnJCUJLKz8gUc7VPnUG4mULQhGD0g_ztxPGYYD7BO6HJBCqcg@mail.gmail.com>
Date:   Wed, 21 Oct 2020 18:31:30 -0700
From:   Atish Patra <atishp@...shpatra.org>
To:     Jim Wilson <jimw@...ive.com>, Palmer Dabbelt <palmer@...belt.com>
Cc:     Greentime Hu <greentime.hu@...ive.com>,
        Kito Cheng <kito.cheng@...il.com>,
        Atish Patra <atish.patra@....com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Kees Cook <keescook@...omium.org>,
        Anup Patel <anup@...infault.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Guo Ren <guoren@...ux.alibaba.com>,
        Zong Li <zong.li@...ive.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Borislav Petkov <bp@...e.de>,
        Michel Lespinasse <walken@...gle.com>,
        Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH 4/5] RISC-V: Protect .init.text & .init.data

On Fri, Oct 16, 2020 at 11:24 AM Atish Patra <atishp@...shpatra.org> wrote:
>
> On Tue, Oct 13, 2020 at 10:24 PM Atish Patra <atishp@...shpatra.org> wrote:
> >
> > On Tue, Oct 13, 2020 at 6:21 PM Jim Wilson <jimw@...ive.com> wrote:
> > >
> > > On Tue, Oct 13, 2020 at 3:25 PM Atish Patra <atishp@...shpatra.org> wrote:
> > > > This happens only when copy_from_user is called from function that is
> > > > annotated with __init.
> > > > Adding Kito & Jim for their input
> > > >
> > > > @kito, @Jim: Please let me know if I should create a issue in
> > > > riscv-gnu-toolchain repo or somewhere else.
> > >
> > > I can't do anything useful without a testcase that I can use to
> > > reproduce the problem.  The interactions here are complex, so pointing
> > > at lines of code or kernel config options doesn't give me any useful
> > > info.
> > >
> > > Relaxation can convert calls to a jal.  I don't know of any open bugs
> > > in this area that can generate relocation errors.  if it is a
> > > relaxation error then turning off relaxation should work around the
> > > problem as you suggested.
> > >
> > > A kernel build problem is serious.  I think this is worth a bug
> > > report.  FSF binutils or riscv-gnu-toolchain is fine.
> > >
> >
> > I have created an issue with detailed descriptions and reproduction steps.
> > Please let me know if you need anything else.
> >
>
> It may be a toolchain issue. Here is the ongoing discussion in case
> anybody else is interested.
>
> https://github.com/riscv/riscv-gnu-toolchain/issues/738
>
> > > Jim
> >
> >
> >
> > --
> > Regards,
> > Atish
>
>
>
> --
> Regards,
> Atish

Thanks to Jim, we know the cause now. Jim has provided an excellent
analysis of the issue in the github issue report.
https://github.com/riscv/riscv-gnu-toolchain/issues/738

To summarize, the linker relaxation code is not aware of the
alignments between sections.
That's why it relaxes the calls from .text to .init.text and  converts
a auipc+jalr pair to jal even if the address can't be fit +/- 1MB.

There are few ways we can solve this problem.

1. As per Jim's suggestion, linker relaxation code is aware of the
section alignments. We can mark .init.text as a 2MB aligned section.
   For calls within a section, section's alignment will be used in the
calculation. For calls across sections, e.g. from .init.text to .text,
the maximum
   section alignment of every section will be used. Thus, all
relaxation within .init.text and between any sections will be
impacted.
   Thus, it avoids the error but results in the following increase in
size of various sections.
     section           change in size (in bytes)
     .head.text      +4
     .text               +40
     .init.text.        +6530
     .exit.text        +84

The only significant increase is .init.text but it is freed after
boot. Thus, I don't see any significant performance degradation due to
that.

Here is the diff
--- a/arch/riscv/kernel/vmlinux.lds.S
+++ b/arch/riscv/kernel/vmlinux.lds.S
@@ -51,7 +51,13 @@ SECTIONS
        . = ALIGN(SECTION_ALIGN);
        __init_begin = .;
        __init_text_begin = .;
-       INIT_TEXT_SECTION(PAGE_SIZE)
+       . = ALIGN(PAGE_SIZE);                                   \
+       .init.text : AT(ADDR(.init.text) - LOAD_OFFSET)
ALIGN(SECTION_ALIGN) {  \
+               _sinittext = .;                                         \
+               INIT_TEXT                                               \
+               _einittext = .;                                         \
+       }
+
        . = ALIGN(8);
        __soc_early_init_table : {
                __soc_early_init_table_start = .;

2. We will continue to keep head.txt & .init.text together before
.text. However, we will map the pages that contain head & init.text at
page
    granularity so that .head.text and init.text can have different
permissions. I have not measured the performance impact of this but it
won't
    too bad given that the combined size of sections .head.txt &
.init.text is 200K. So we are talking about page level permission only
for
    ~50 pages during boot.

3. Keep head.text in a separate 2MB aligned section. .init.text will
follow .head.text in its own section as well. This increases the
kernel
    size by 2MB for MMU kernels. For nommu case, it will only increase
by 64 bytes due to smaller section alignment for nommu kernels.

Both solutions 1 & 2 come at minimal performance on boot time while
solution 3 comes at increased kernel size.

Any preference ?

-- 
Regards,
Atish

Powered by blists - more mailing lists