From: Atish Patra <atishp@atishpatra.org>
To: Anup Patel <anup@brainfault.org>
Cc: Albert Ou <aou@eecs.berkeley.edu>,
Kees Cook <keescook@chromium.org>,
Ard Biesheuvel <ardb@kernel.org>,
Kito Cheng <kito.cheng@gmail.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Atish Patra <atish.patra@wdc.com>,
Guo Ren <guoren@linux.alibaba.com>,
Palmer Dabbelt <palmer@dabbelt.com>, Zong Li <zong.li@sifive.com>,
Paul Walmsley <paul.walmsley@sifive.com>,
Greentime Hu <greentime.hu@sifive.com>,
linux-riscv <linux-riscv@lists.infradead.org>,
Borislav Petkov <bp@suse.de>,
Michel Lespinasse <walken@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Jim Wilson <jimw@sifive.com>
Subject: Re: [PATCH 4/5] RISC-V: Protect .init.text & .init.data
Date: Thu, 22 Oct 2020 10:13:58 -0700 [thread overview]
Message-ID: <CAOnJCUJai8T3uw1E8o_wHiXf27Q2BGm+x_c8quSThUcO9Fdd=Q@mail.gmail.com> (raw)
In-Reply-To: <CAAhSdy2M+uWufbqtYOjyd8wUXg7a5jO6J5o5NC=duaTBsnyHsA@mail.gmail.com>
On Thu, Oct 22, 2020 at 12:22 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Oct 22, 2020 at 10:33 AM Anup Patel <anup@brainfault.org> wrote:
> >
> > On Thu, Oct 22, 2020 at 7:01 AM Atish Patra <atishp@atishpatra.org> wrote:
> > >
> > > On Fri, Oct 16, 2020 at 11:24 AM Atish Patra <atishp@atishpatra.org> wrote:
> > > >
> > > > On Tue, Oct 13, 2020 at 10:24 PM Atish Patra <atishp@atishpatra.org> wrote:
> > > > >
> > > > > On Tue, Oct 13, 2020 at 6:21 PM Jim Wilson <jimw@sifive.com> wrote:
> > > > > >
> > > > > > On Tue, Oct 13, 2020 at 3:25 PM Atish Patra <atishp@atishpatra.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 ?
> >
> > I prefer solution#3 because:
> > 1. This will help us avoid special handling of static objects
> > 2. This will make RISC-V linker script more aligned with other
> > major architectures
> >
> > I don't think solution#3 will increase the size of the kernel by 2MB. We
> > can make head.text part of text section. There will be only one section
> > alignment between text section and init section but the existing section
> > alignment between init section and text section will be removed. In other
> > words, number of section alignments will remain same.
>
> I think we will need to incorporate Jim's suggestion irrespective of the
> solution we choose because without Jim's changes we can hit the
> linker relaxation issue in solution#2 as well.
>
Reconsidering all the possible approaches, I think you are right.
With approach 2 & 3, I just hit the issue now. However, there is an
alignment between
sections which may throw the same linker error in future when text or
init.text gets bigger.
I will follow approach 1 suggested by Jim to revise the series.
> Regards,
> Anup
--
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2020-10-22 17:14 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-09 21:13 [PATCH 0/5] Improve kernel section protections Atish Patra
2020-10-09 21:13 ` [PATCH 1/5] RISC-V: Move __start_kernel to .head.text Atish Patra
2020-10-09 21:13 ` [PATCH 2/5] RISC-V: Initialize SBI early Atish Patra
2020-10-09 21:13 ` [PATCH 3/5] RISC-V: Enforce protections for kernel sections early Atish Patra
2020-10-09 21:13 ` [PATCH 4/5] RISC-V: Protect .init.text & .init.data Atish Patra
2020-10-12 13:14 ` Greentime Hu
2020-10-12 23:26 ` Atish Patra
2020-10-13 1:28 ` Atish Patra
2020-10-13 3:08 ` Greentime Hu
2020-10-13 22:25 ` Atish Patra
2020-10-14 1:20 ` Jim Wilson
2020-10-14 5:24 ` Atish Patra
2020-10-16 18:24 ` Atish Patra
2020-10-22 1:31 ` Atish Patra
2020-10-22 5:03 ` Anup Patel
2020-10-22 7:22 ` Anup Patel
2020-10-22 17:13 ` Atish Patra [this message]
2020-10-09 21:13 ` [PATCH 5/5] RISC-V: Move dynamic relocation section under __init Atish Patra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOnJCUJai8T3uw1E8o_wHiXf27Q2BGm+x_c8quSThUcO9Fdd=Q@mail.gmail.com' \
--to=atishp@atishpatra.org \
--cc=akpm@linux-foundation.org \
--cc=anup@brainfault.org \
--cc=aou@eecs.berkeley.edu \
--cc=ardb@kernel.org \
--cc=atish.patra@wdc.com \
--cc=bp@suse.de \
--cc=greentime.hu@sifive.com \
--cc=guoren@linux.alibaba.com \
--cc=jimw@sifive.com \
--cc=keescook@chromium.org \
--cc=kito.cheng@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=walken@google.com \
--cc=zong.li@sifive.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).