Re: [PATCH 4/5] RISC-V: Protect .init.text & .init.data

[Atish Patra]

On Thu, Oct 22, 2020 at 12:22 AM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 22, 2020 at 10:33 AM Anup Patel <anup@xxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Oct 22, 2020 at 7:01 AM Atish Patra <atishp@xxxxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 16, 2020 at 11:24 AM Atish Patra <atishp@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Oct 13, 2020 at 10:24 PM Atish Patra <atishp@xxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, Oct 13, 2020 at 6:21 PM Jim Wilson <jimw@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Tue, Oct 13, 2020 at 3:25 PM Atish Patra <atishp@xxxxxxxxxxxxxx> 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