* [RFT PATCH 0/2] ARM/arm64: module-plt: split core and init PLT sections
@ 2017-02-20 22:00 Ard Biesheuvel
2017-02-20 22:00 ` [RFT PATCH 1/2] ARM: module: " Ard Biesheuvel
2017-02-20 22:00 ` [RFT PATCH 2/2] arm64: " Ard Biesheuvel
0 siblings, 2 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-20 22:00 UTC (permalink / raw)
To: linux-arm-kernel
This fixes a thinko on my part in both the ARM and the arm64 implementations
of module PLTs. What I failed to realise is that the core module sections and
the init sections are allocated independently, which means they could end up
further away from each other than the range of a branch instruction. This
implies that they cannot share a single array of PLT entries, and so this
series splits them into core and init PLT sections.
For ARM, this is actually a revert of commit 35fa91eed817 ("ARM: kernel:
merge core and init PLTs") [although the patch in this series is not a
straight revert.] This means that the issue is a regression, and the patch
should probably go to -stable.
For arm64, the likelihood of this issue ever occurring is very small, due
to the 128 MB range of its branch instructions. Also, the arm64 version of
the code was never correct, so it is not a regression. But for correctness,
it is fixed in the same way as for ARM.
For now, these are build tested only. I was hoping Angus could check whether
1/2 makes his problems go away.
Ard Biesheuvel (2):
ARM: module: split core and init PLT sections
arm64: module: split core and init PLT sections
arch/arm/include/asm/module.h | 9 ++-
arch/arm/kernel/module-plts.c | 62 ++++++++++++++------
arch/arm/kernel/module.lds | 1 +
arch/arm64/include/asm/module.h | 9 ++-
arch/arm64/kernel/module-plts.c | 56 ++++++++++++------
arch/arm64/kernel/module.c | 2 +-
arch/arm64/kernel/module.lds | 1 +
7 files changed, 99 insertions(+), 41 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFT PATCH 1/2] ARM: module: split core and init PLT sections
2017-02-20 22:00 [RFT PATCH 0/2] ARM/arm64: module-plt: split core and init PLT sections Ard Biesheuvel
@ 2017-02-20 22:00 ` Ard Biesheuvel
2017-02-21 14:26 ` Angus Clark
2017-02-23 15:45 ` Ard Biesheuvel
2017-02-20 22:00 ` [RFT PATCH 2/2] arm64: " Ard Biesheuvel
1 sibling, 2 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-20 22:00 UTC (permalink / raw)
To: linux-arm-kernel
Since commit 35fa91eed817 ("ARM: kernel: merge core and init PLTs"),
the ARM module PLT code allocates all PLT entries in a single core
section, since the overhead of having a separate init PLT section is
not justified by the small number of PLT entries usually required for
init code.
However, the core and init module regions are allocated independently,
and there is a corner case where the core region may be allocated from
the VMALLOC region if the dedicated module region is exhausted, but the
init region, being much smaller, can still be allocated from the module
region. This puts the PLT entries out of reach of the relocated branch
instructions, defeating the whole purpose of PLTs.
So split the core and init PLT regions, and name the latter ".init.plt"
so it gets allocated along with (and sufficiently close to) the .init
sections that it serves. This is not a straight revert, given that the
PLT code was heavily modified since the commit in question was merged.
Fixes: 35fa91eed817 ("ARM: kernel: merge core and init PLTs")
Reported-by: Angus Clark <angus@angusclark.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
arch/arm/include/asm/module.h | 9 ++-
arch/arm/kernel/module-plts.c | 62 ++++++++++++++------
arch/arm/kernel/module.lds | 1 +
3 files changed, 51 insertions(+), 21 deletions(-)
diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
index 464748b9fd7d..ed2319663a1e 100644
--- a/arch/arm/include/asm/module.h
+++ b/arch/arm/include/asm/module.h
@@ -18,13 +18,18 @@ enum {
};
#endif
+struct mod_plt_sec {
+ struct elf32_shdr *plt;
+ int plt_count;
+};
+
struct mod_arch_specific {
#ifdef CONFIG_ARM_UNWIND
struct unwind_table *unwind[ARM_SEC_MAX];
#endif
#ifdef CONFIG_ARM_MODULE_PLTS
- struct elf32_shdr *plt;
- int plt_count;
+ struct mod_plt_sec core;
+ struct mod_plt_sec init;
#endif
};
diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
index 3a5cba90c971..1a79f2789325 100644
--- a/arch/arm/kernel/module-plts.c
+++ b/arch/arm/kernel/module-plts.c
@@ -31,9 +31,17 @@ struct plt_entries {
u32 lit[PLT_ENT_COUNT];
};
+static bool in_init(const struct module *mod, void *loc)
+{
+ return (u32)loc - (u32)mod->init_layout.base < mod->init_layout.size;
+}
+
u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
{
- struct plt_entries *plt = (struct plt_entries *)mod->arch.plt->sh_addr;
+ struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
+ &mod->arch.init;
+
+ struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
int idx = 0;
/*
@@ -41,9 +49,9 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
* relocations are sorted, this will be the last entry we allocated.
* (if one exists).
*/
- if (mod->arch.plt_count > 0) {
- plt += (mod->arch.plt_count - 1) / PLT_ENT_COUNT;
- idx = (mod->arch.plt_count - 1) % PLT_ENT_COUNT;
+ if (pltsec->plt_count > 0) {
+ plt += (pltsec->plt_count - 1) / PLT_ENT_COUNT;
+ idx = (pltsec->plt_count - 1) % PLT_ENT_COUNT;
if (plt->lit[idx] == val)
return (u32)&plt->ldr[idx];
@@ -53,8 +61,8 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
plt++;
}
- mod->arch.plt_count++;
- BUG_ON(mod->arch.plt_count * PLT_ENT_SIZE > mod->arch.plt->sh_size);
+ pltsec->plt_count++;
+ BUG_ON(pltsec->plt_count * PLT_ENT_SIZE > pltsec->plt->sh_size);
if (!idx)
/* Populate a new set of entries */
@@ -174,7 +182,8 @@ static unsigned int count_plts(const Elf32_Sym *syms, Elf32_Addr base,
int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
char *secstrings, struct module *mod)
{
- unsigned long plts = 0;
+ unsigned long core_plts = 0;
+ unsigned long init_plts = 0;
Elf32_Shdr *s, *sechdrs_end = sechdrs + ehdr->e_shnum;
Elf32_Sym *syms = NULL;
@@ -184,13 +193,15 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
*/
for (s = sechdrs; s < sechdrs_end; ++s) {
if (strcmp(".plt", secstrings + s->sh_name) == 0)
- mod->arch.plt = s;
+ mod->arch.core.plt = s;
+ else if (strcmp(".init.plt", secstrings + s->sh_name) == 0)
+ mod->arch.init.plt = s;
else if (s->sh_type == SHT_SYMTAB)
syms = (Elf32_Sym *)s->sh_addr;
}
- if (!mod->arch.plt) {
- pr_err("%s: module PLT section missing\n", mod->name);
+ if (!mod->arch.core.plt || !mod->arch.init.plt) {
+ pr_err("%s: module PLT section(s) missing\n", mod->name);
return -ENOEXEC;
}
if (!syms) {
@@ -213,16 +224,29 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
/* sort by type and symbol index */
sort(rels, numrels, sizeof(Elf32_Rel), cmp_rel, NULL);
- plts += count_plts(syms, dstsec->sh_addr, rels, numrels);
+ if (strncmp(secstrings + dstsec->sh_name, ".init", 5) != 0)
+ core_plts += count_plts(syms, dstsec->sh_addr, rels,
+ numrels);
+ else
+ init_plts += count_plts(syms, dstsec->sh_addr, rels,
+ numrels);
}
- mod->arch.plt->sh_type = SHT_NOBITS;
- mod->arch.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
- mod->arch.plt->sh_addralign = L1_CACHE_BYTES;
- mod->arch.plt->sh_size = round_up(plts * PLT_ENT_SIZE,
- sizeof(struct plt_entries));
- mod->arch.plt_count = 0;
-
- pr_debug("%s: plt=%x\n", __func__, mod->arch.plt->sh_size);
+ mod->arch.core.plt->sh_type = SHT_NOBITS;
+ mod->arch.core.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
+ mod->arch.core.plt->sh_addralign = L1_CACHE_BYTES;
+ mod->arch.core.plt->sh_size = round_up(core_plts * PLT_ENT_SIZE,
+ sizeof(struct plt_entries));
+ mod->arch.core.plt_count = 0;
+
+ mod->arch.init.plt->sh_type = SHT_NOBITS;
+ mod->arch.init.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
+ mod->arch.init.plt->sh_addralign = L1_CACHE_BYTES;
+ mod->arch.init.plt->sh_size = round_up(init_plts * PLT_ENT_SIZE,
+ sizeof(struct plt_entries));
+ mod->arch.init.plt_count = 0;
+
+ pr_debug("%s: plt=%x, init.plt=%x\n", __func__,
+ mod->arch.core.plt->sh_size, mod->arch.init.plt->sh_size);
return 0;
}
diff --git a/arch/arm/kernel/module.lds b/arch/arm/kernel/module.lds
index 05881e2b414c..eacb5c67f61e 100644
--- a/arch/arm/kernel/module.lds
+++ b/arch/arm/kernel/module.lds
@@ -1,3 +1,4 @@
SECTIONS {
.plt : { BYTE(0) }
+ .init.plt : { BYTE(0) }
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFT PATCH 2/2] arm64: module: split core and init PLT sections
2017-02-20 22:00 [RFT PATCH 0/2] ARM/arm64: module-plt: split core and init PLT sections Ard Biesheuvel
2017-02-20 22:00 ` [RFT PATCH 1/2] ARM: module: " Ard Biesheuvel
@ 2017-02-20 22:00 ` Ard Biesheuvel
1 sibling, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-20 22:00 UTC (permalink / raw)
To: linux-arm-kernel
The arm64 module PLT code allocates all PLT entries in a single core
section, since the overhead of having a separate init PLT section is
not justified by the small number of PLT entries usually required for
init code.
However, the core and init module regions are allocated independently,
and there is a corner case where the core region may be allocated from
the VMALLOC region if the dedicated module region is exhausted, but the
init region, being much smaller, can still be allocated from the module
region. This leads to relocation failures if the distance between those
regions exceeds 128 MB. (In fact, this corner case is highly unlikely to
occur on arm64, but the issue has been observed on ARM, whose module
region is much smaller).
So split the core and init PLT regions, and name the latter ".init.plt"
so it gets allocated along with (and sufficiently close to) the .init
sections that it serves.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
arch/arm64/include/asm/module.h | 9 +++-
arch/arm64/kernel/module-plts.c | 56 ++++++++++++++------
arch/arm64/kernel/module.c | 2 +-
arch/arm64/kernel/module.lds | 1 +
4 files changed, 48 insertions(+), 20 deletions(-)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index 06ff7fd9e81f..b6c6fa29fe56 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -22,14 +22,19 @@
#define MODULE_ARCH_VERMAGIC "aarch64"
#ifdef CONFIG_ARM64_MODULE_PLTS
-struct mod_arch_specific {
+struct mod_plt_sec {
struct elf64_shdr *plt;
int plt_num_entries;
int plt_max_entries;
};
+
+struct mod_arch_specific {
+ struct mod_plt_sec core;
+ struct mod_plt_sec init;
+};
#endif
-u64 module_emit_plt_entry(struct module *mod, const Elf64_Rela *rela,
+u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym);
#ifdef CONFIG_RANDOMIZE_BASE
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index 1ce90d8450ae..0d396ffd3d08 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -26,11 +26,19 @@ struct plt_entry {
__le32 br; /* br x16 */
};
-u64 module_emit_plt_entry(struct module *mod, const Elf64_Rela *rela,
+static bool in_init(const struct module *mod, void *loc)
+{
+ return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
+}
+
+u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym)
{
- struct plt_entry *plt = (struct plt_entry *)mod->arch.plt->sh_addr;
- int i = mod->arch.plt_num_entries;
+ struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
+ &mod->arch.init;
+
+ struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
+ int i = pltsec->plt_num_entries;
u64 val = sym->st_value + rela->r_addend;
/*
@@ -51,8 +59,8 @@ u64 module_emit_plt_entry(struct module *mod, const Elf64_Rela *rela,
return sym->st_size;
}
- mod->arch.plt_num_entries++;
- BUG_ON(mod->arch.plt_num_entries > mod->arch.plt_max_entries);
+ pltsec->plt_num_entries++;
+ BUG_ON(pltsec->plt_num_entries > pltsec->plt_max_entries);
/*
* MOVK/MOVN/MOVZ opcode:
@@ -149,7 +157,8 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num)
int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
char *secstrings, struct module *mod)
{
- unsigned long plt_max_entries = 0;
+ unsigned long plt_max_core = 0;
+ unsigned long plt_max_init = 0;
Elf64_Sym *syms = NULL;
int i;
@@ -158,14 +167,16 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
* entries. Record the symtab address as well.
*/
for (i = 0; i < ehdr->e_shnum; i++) {
- if (strcmp(".plt", secstrings + sechdrs[i].sh_name) == 0)
- mod->arch.plt = sechdrs + i;
+ if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
+ mod->arch.core.plt = sechdrs + i;
+ else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt"))
+ mod->arch.init.plt = sechdrs + i;
else if (sechdrs[i].sh_type == SHT_SYMTAB)
syms = (Elf64_Sym *)sechdrs[i].sh_addr;
}
- if (!mod->arch.plt) {
- pr_err("%s: module PLT section missing\n", mod->name);
+ if (!mod->arch.core.plt || !mod->arch.init.plt) {
+ pr_err("%s: module PLT section(s) missing\n", mod->name);
return -ENOEXEC;
}
if (!syms) {
@@ -188,14 +199,25 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
/* sort by type, symbol index and addend */
sort(rels, numrels, sizeof(Elf64_Rela), cmp_rela, NULL);
- plt_max_entries += count_plts(syms, rels, numrels);
+ if (strncmp(secstrings + dstsec->sh_name, ".init", 5) != 0)
+ plt_max_core += count_plts(syms, rels, numrels);
+ else
+ plt_max_init += count_plts(syms, rels, numrels);
}
- mod->arch.plt->sh_type = SHT_NOBITS;
- mod->arch.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
- mod->arch.plt->sh_addralign = L1_CACHE_BYTES;
- mod->arch.plt->sh_size = plt_max_entries * sizeof(struct plt_entry);
- mod->arch.plt_num_entries = 0;
- mod->arch.plt_max_entries = plt_max_entries;
+ mod->arch.core.plt->sh_type = SHT_NOBITS;
+ mod->arch.core.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
+ mod->arch.core.plt->sh_addralign = L1_CACHE_BYTES;
+ mod->arch.core.plt->sh_size = plt_max_core * sizeof(struct plt_entry);
+ mod->arch.core.plt_num_entries = 0;
+ mod->arch.core.plt_max_entries = plt_max_core;
+
+ mod->arch.init.plt->sh_type = SHT_NOBITS;
+ mod->arch.init.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
+ mod->arch.init.plt->sh_addralign = L1_CACHE_BYTES;
+ mod->arch.init.plt->sh_size = plt_max_init * sizeof(struct plt_entry);
+ mod->arch.init.plt_num_entries = 0;
+ mod->arch.init.plt_max_entries = plt_max_init;
+
return 0;
}
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 7f316982ce00..c9a2ab446dc6 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -380,7 +380,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
ovf == -ERANGE) {
- val = module_emit_plt_entry(me, &rel[i], sym);
+ val = module_emit_plt_entry(me, loc, &rel[i], sym);
ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2,
26, AARCH64_INSN_IMM_26);
}
diff --git a/arch/arm64/kernel/module.lds b/arch/arm64/kernel/module.lds
index 8949f6c6f729..f7c9781a9d48 100644
--- a/arch/arm64/kernel/module.lds
+++ b/arch/arm64/kernel/module.lds
@@ -1,3 +1,4 @@
SECTIONS {
.plt (NOLOAD) : { BYTE(0) }
+ .init.plt (NOLOAD) : { BYTE(0) }
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFT PATCH 1/2] ARM: module: split core and init PLT sections
2017-02-20 22:00 ` [RFT PATCH 1/2] ARM: module: " Ard Biesheuvel
@ 2017-02-21 14:26 ` Angus Clark
2017-02-21 17:35 ` Ard Biesheuvel
2017-02-23 15:45 ` Ard Biesheuvel
1 sibling, 1 reply; 6+ messages in thread
From: Angus Clark @ 2017-02-21 14:26 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ard,
I have tested the patch and I think there is still a small issue. At
present, count_plts() assumes a PLT entry would only be required for
undefined symbols. However, if 'init' and 'core' are located far
apart, then we may need an entry in the init PLT if some init code
makes a call to a defined symbol in the core section.
As a quick test, I added an 'include_defined' argument to count_plts()
which gets set for init relocs. With this in place, the module loads
fine and appears to function as expected.
Happy to test any further versions :-)
Cheers,
Angus
On 20 February 2017 at 22:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Since commit 35fa91eed817 ("ARM: kernel: merge core and init PLTs"),
> the ARM module PLT code allocates all PLT entries in a single core
> section, since the overhead of having a separate init PLT section is
> not justified by the small number of PLT entries usually required for
> init code.
>
> However, the core and init module regions are allocated independently,
> and there is a corner case where the core region may be allocated from
> the VMALLOC region if the dedicated module region is exhausted, but the
> init region, being much smaller, can still be allocated from the module
> region. This puts the PLT entries out of reach of the relocated branch
> instructions, defeating the whole purpose of PLTs.
>
> So split the core and init PLT regions, and name the latter ".init.plt"
> so it gets allocated along with (and sufficiently close to) the .init
> sections that it serves. This is not a straight revert, given that the
> PLT code was heavily modified since the commit in question was merged.
>
> Fixes: 35fa91eed817 ("ARM: kernel: merge core and init PLTs")
> Reported-by: Angus Clark <angus@angusclark.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> arch/arm/include/asm/module.h | 9 ++-
> arch/arm/kernel/module-plts.c | 62 ++++++++++++++------
> arch/arm/kernel/module.lds | 1 +
> 3 files changed, 51 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
> index 464748b9fd7d..ed2319663a1e 100644
> --- a/arch/arm/include/asm/module.h
> +++ b/arch/arm/include/asm/module.h
> @@ -18,13 +18,18 @@ enum {
> };
> #endif
>
> +struct mod_plt_sec {
> + struct elf32_shdr *plt;
> + int plt_count;
> +};
> +
> struct mod_arch_specific {
> #ifdef CONFIG_ARM_UNWIND
> struct unwind_table *unwind[ARM_SEC_MAX];
> #endif
> #ifdef CONFIG_ARM_MODULE_PLTS
> - struct elf32_shdr *plt;
> - int plt_count;
> + struct mod_plt_sec core;
> + struct mod_plt_sec init;
> #endif
> };
>
> diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
> index 3a5cba90c971..1a79f2789325 100644
> --- a/arch/arm/kernel/module-plts.c
> +++ b/arch/arm/kernel/module-plts.c
> @@ -31,9 +31,17 @@ struct plt_entries {
> u32 lit[PLT_ENT_COUNT];
> };
>
> +static bool in_init(const struct module *mod, void *loc)
> +{
> + return (u32)loc - (u32)mod->init_layout.base < mod->init_layout.size;
> +}
> +
> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
> {
> - struct plt_entries *plt = (struct plt_entries *)mod->arch.plt->sh_addr;
> + struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
> + &mod->arch.init;
> +
> + struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
> int idx = 0;
>
> /*
> @@ -41,9 +49,9 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
> * relocations are sorted, this will be the last entry we allocated.
> * (if one exists).
> */
> - if (mod->arch.plt_count > 0) {
> - plt += (mod->arch.plt_count - 1) / PLT_ENT_COUNT;
> - idx = (mod->arch.plt_count - 1) % PLT_ENT_COUNT;
> + if (pltsec->plt_count > 0) {
> + plt += (pltsec->plt_count - 1) / PLT_ENT_COUNT;
> + idx = (pltsec->plt_count - 1) % PLT_ENT_COUNT;
>
> if (plt->lit[idx] == val)
> return (u32)&plt->ldr[idx];
> @@ -53,8 +61,8 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
> plt++;
> }
>
> - mod->arch.plt_count++;
> - BUG_ON(mod->arch.plt_count * PLT_ENT_SIZE > mod->arch.plt->sh_size);
> + pltsec->plt_count++;
> + BUG_ON(pltsec->plt_count * PLT_ENT_SIZE > pltsec->plt->sh_size);
>
> if (!idx)
> /* Populate a new set of entries */
> @@ -174,7 +182,8 @@ static unsigned int count_plts(const Elf32_Sym *syms, Elf32_Addr base,
> int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> char *secstrings, struct module *mod)
> {
> - unsigned long plts = 0;
> + unsigned long core_plts = 0;
> + unsigned long init_plts = 0;
> Elf32_Shdr *s, *sechdrs_end = sechdrs + ehdr->e_shnum;
> Elf32_Sym *syms = NULL;
>
> @@ -184,13 +193,15 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> */
> for (s = sechdrs; s < sechdrs_end; ++s) {
> if (strcmp(".plt", secstrings + s->sh_name) == 0)
> - mod->arch.plt = s;
> + mod->arch.core.plt = s;
> + else if (strcmp(".init.plt", secstrings + s->sh_name) == 0)
> + mod->arch.init.plt = s;
> else if (s->sh_type == SHT_SYMTAB)
> syms = (Elf32_Sym *)s->sh_addr;
> }
>
> - if (!mod->arch.plt) {
> - pr_err("%s: module PLT section missing\n", mod->name);
> + if (!mod->arch.core.plt || !mod->arch.init.plt) {
> + pr_err("%s: module PLT section(s) missing\n", mod->name);
> return -ENOEXEC;
> }
> if (!syms) {
> @@ -213,16 +224,29 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> /* sort by type and symbol index */
> sort(rels, numrels, sizeof(Elf32_Rel), cmp_rel, NULL);
>
> - plts += count_plts(syms, dstsec->sh_addr, rels, numrels);
> + if (strncmp(secstrings + dstsec->sh_name, ".init", 5) != 0)
> + core_plts += count_plts(syms, dstsec->sh_addr, rels,
> + numrels);
> + else
> + init_plts += count_plts(syms, dstsec->sh_addr, rels,
> + numrels);
> }
>
> - mod->arch.plt->sh_type = SHT_NOBITS;
> - mod->arch.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> - mod->arch.plt->sh_addralign = L1_CACHE_BYTES;
> - mod->arch.plt->sh_size = round_up(plts * PLT_ENT_SIZE,
> - sizeof(struct plt_entries));
> - mod->arch.plt_count = 0;
> -
> - pr_debug("%s: plt=%x\n", __func__, mod->arch.plt->sh_size);
> + mod->arch.core.plt->sh_type = SHT_NOBITS;
> + mod->arch.core.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> + mod->arch.core.plt->sh_addralign = L1_CACHE_BYTES;
> + mod->arch.core.plt->sh_size = round_up(core_plts * PLT_ENT_SIZE,
> + sizeof(struct plt_entries));
> + mod->arch.core.plt_count = 0;
> +
> + mod->arch.init.plt->sh_type = SHT_NOBITS;
> + mod->arch.init.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> + mod->arch.init.plt->sh_addralign = L1_CACHE_BYTES;
> + mod->arch.init.plt->sh_size = round_up(init_plts * PLT_ENT_SIZE,
> + sizeof(struct plt_entries));
> + mod->arch.init.plt_count = 0;
> +
> + pr_debug("%s: plt=%x, init.plt=%x\n", __func__,
> + mod->arch.core.plt->sh_size, mod->arch.init.plt->sh_size);
> return 0;
> }
> diff --git a/arch/arm/kernel/module.lds b/arch/arm/kernel/module.lds
> index 05881e2b414c..eacb5c67f61e 100644
> --- a/arch/arm/kernel/module.lds
> +++ b/arch/arm/kernel/module.lds
> @@ -1,3 +1,4 @@
> SECTIONS {
> .plt : { BYTE(0) }
> + .init.plt : { BYTE(0) }
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFT PATCH 1/2] ARM: module: split core and init PLT sections
2017-02-21 14:26 ` Angus Clark
@ 2017-02-21 17:35 ` Ard Biesheuvel
0 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-21 17:35 UTC (permalink / raw)
To: linux-arm-kernel
On 21 February 2017 at 14:26, Angus Clark <angus@angusclark.org> wrote:
> Hi Ard,
>
> I have tested the patch and I think there is still a small issue. At
> present, count_plts() assumes a PLT entry would only be required for
> undefined symbols. However, if 'init' and 'core' are located far
> apart, then we may need an entry in the init PLT if some init code
> makes a call to a defined symbol in the core section.
>
Argh. You're absolutely right, another complication I did not
anticipate, unfortunately.
> As a quick test, I added an 'include_defined' argument to count_plts()
> which gets set for init relocs. With this in place, the module loads
> fine and appears to function as expected.
>
> Happy to test any further versions :-)
>
Thanks, v2 coming up
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFT PATCH 1/2] ARM: module: split core and init PLT sections
2017-02-20 22:00 ` [RFT PATCH 1/2] ARM: module: " Ard Biesheuvel
2017-02-21 14:26 ` Angus Clark
@ 2017-02-23 15:45 ` Ard Biesheuvel
1 sibling, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2017-02-23 15:45 UTC (permalink / raw)
To: linux-arm-kernel
On 20 February 2017 at 22:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Since commit 35fa91eed817 ("ARM: kernel: merge core and init PLTs"),
> the ARM module PLT code allocates all PLT entries in a single core
> section, since the overhead of having a separate init PLT section is
> not justified by the small number of PLT entries usually required for
> init code.
>
> However, the core and init module regions are allocated independently,
> and there is a corner case where the core region may be allocated from
> the VMALLOC region if the dedicated module region is exhausted, but the
> init region, being much smaller, can still be allocated from the module
> region. This puts the PLT entries out of reach of the relocated branch
> instructions, defeating the whole purpose of PLTs.
>
> So split the core and init PLT regions, and name the latter ".init.plt"
> so it gets allocated along with (and sufficiently close to) the .init
> sections that it serves. This is not a straight revert, given that the
> PLT code was heavily modified since the commit in question was merged.
>
> Fixes: 35fa91eed817 ("ARM: kernel: merge core and init PLTs")
> Reported-by: Angus Clark <angus@angusclark.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
I have dropped this into the patch system as #8662
> ---
> arch/arm/include/asm/module.h | 9 ++-
> arch/arm/kernel/module-plts.c | 62 ++++++++++++++------
> arch/arm/kernel/module.lds | 1 +
> 3 files changed, 51 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
> index 464748b9fd7d..ed2319663a1e 100644
> --- a/arch/arm/include/asm/module.h
> +++ b/arch/arm/include/asm/module.h
> @@ -18,13 +18,18 @@ enum {
> };
> #endif
>
> +struct mod_plt_sec {
> + struct elf32_shdr *plt;
> + int plt_count;
> +};
> +
> struct mod_arch_specific {
> #ifdef CONFIG_ARM_UNWIND
> struct unwind_table *unwind[ARM_SEC_MAX];
> #endif
> #ifdef CONFIG_ARM_MODULE_PLTS
> - struct elf32_shdr *plt;
> - int plt_count;
> + struct mod_plt_sec core;
> + struct mod_plt_sec init;
> #endif
> };
>
> diff --git a/arch/arm/kernel/module-plts.c b/arch/arm/kernel/module-plts.c
> index 3a5cba90c971..1a79f2789325 100644
> --- a/arch/arm/kernel/module-plts.c
> +++ b/arch/arm/kernel/module-plts.c
> @@ -31,9 +31,17 @@ struct plt_entries {
> u32 lit[PLT_ENT_COUNT];
> };
>
> +static bool in_init(const struct module *mod, void *loc)
> +{
> + return (u32)loc - (u32)mod->init_layout.base < mod->init_layout.size;
> +}
> +
> u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
> {
> - struct plt_entries *plt = (struct plt_entries *)mod->arch.plt->sh_addr;
> + struct mod_plt_sec *pltsec = !in_init(mod, loc) ? &mod->arch.core :
> + &mod->arch.init;
> +
> + struct plt_entries *plt = (struct plt_entries *)pltsec->plt->sh_addr;
> int idx = 0;
>
> /*
> @@ -41,9 +49,9 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
> * relocations are sorted, this will be the last entry we allocated.
> * (if one exists).
> */
> - if (mod->arch.plt_count > 0) {
> - plt += (mod->arch.plt_count - 1) / PLT_ENT_COUNT;
> - idx = (mod->arch.plt_count - 1) % PLT_ENT_COUNT;
> + if (pltsec->plt_count > 0) {
> + plt += (pltsec->plt_count - 1) / PLT_ENT_COUNT;
> + idx = (pltsec->plt_count - 1) % PLT_ENT_COUNT;
>
> if (plt->lit[idx] == val)
> return (u32)&plt->ldr[idx];
> @@ -53,8 +61,8 @@ u32 get_module_plt(struct module *mod, unsigned long loc, Elf32_Addr val)
> plt++;
> }
>
> - mod->arch.plt_count++;
> - BUG_ON(mod->arch.plt_count * PLT_ENT_SIZE > mod->arch.plt->sh_size);
> + pltsec->plt_count++;
> + BUG_ON(pltsec->plt_count * PLT_ENT_SIZE > pltsec->plt->sh_size);
>
> if (!idx)
> /* Populate a new set of entries */
> @@ -174,7 +182,8 @@ static unsigned int count_plts(const Elf32_Sym *syms, Elf32_Addr base,
> int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> char *secstrings, struct module *mod)
> {
> - unsigned long plts = 0;
> + unsigned long core_plts = 0;
> + unsigned long init_plts = 0;
> Elf32_Shdr *s, *sechdrs_end = sechdrs + ehdr->e_shnum;
> Elf32_Sym *syms = NULL;
>
> @@ -184,13 +193,15 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> */
> for (s = sechdrs; s < sechdrs_end; ++s) {
> if (strcmp(".plt", secstrings + s->sh_name) == 0)
> - mod->arch.plt = s;
> + mod->arch.core.plt = s;
> + else if (strcmp(".init.plt", secstrings + s->sh_name) == 0)
> + mod->arch.init.plt = s;
> else if (s->sh_type == SHT_SYMTAB)
> syms = (Elf32_Sym *)s->sh_addr;
> }
>
> - if (!mod->arch.plt) {
> - pr_err("%s: module PLT section missing\n", mod->name);
> + if (!mod->arch.core.plt || !mod->arch.init.plt) {
> + pr_err("%s: module PLT section(s) missing\n", mod->name);
> return -ENOEXEC;
> }
> if (!syms) {
> @@ -213,16 +224,29 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> /* sort by type and symbol index */
> sort(rels, numrels, sizeof(Elf32_Rel), cmp_rel, NULL);
>
> - plts += count_plts(syms, dstsec->sh_addr, rels, numrels);
> + if (strncmp(secstrings + dstsec->sh_name, ".init", 5) != 0)
> + core_plts += count_plts(syms, dstsec->sh_addr, rels,
> + numrels);
> + else
> + init_plts += count_plts(syms, dstsec->sh_addr, rels,
> + numrels);
> }
>
> - mod->arch.plt->sh_type = SHT_NOBITS;
> - mod->arch.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> - mod->arch.plt->sh_addralign = L1_CACHE_BYTES;
> - mod->arch.plt->sh_size = round_up(plts * PLT_ENT_SIZE,
> - sizeof(struct plt_entries));
> - mod->arch.plt_count = 0;
> -
> - pr_debug("%s: plt=%x\n", __func__, mod->arch.plt->sh_size);
> + mod->arch.core.plt->sh_type = SHT_NOBITS;
> + mod->arch.core.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> + mod->arch.core.plt->sh_addralign = L1_CACHE_BYTES;
> + mod->arch.core.plt->sh_size = round_up(core_plts * PLT_ENT_SIZE,
> + sizeof(struct plt_entries));
> + mod->arch.core.plt_count = 0;
> +
> + mod->arch.init.plt->sh_type = SHT_NOBITS;
> + mod->arch.init.plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> + mod->arch.init.plt->sh_addralign = L1_CACHE_BYTES;
> + mod->arch.init.plt->sh_size = round_up(init_plts * PLT_ENT_SIZE,
> + sizeof(struct plt_entries));
> + mod->arch.init.plt_count = 0;
> +
> + pr_debug("%s: plt=%x, init.plt=%x\n", __func__,
> + mod->arch.core.plt->sh_size, mod->arch.init.plt->sh_size);
> return 0;
> }
> diff --git a/arch/arm/kernel/module.lds b/arch/arm/kernel/module.lds
> index 05881e2b414c..eacb5c67f61e 100644
> --- a/arch/arm/kernel/module.lds
> +++ b/arch/arm/kernel/module.lds
> @@ -1,3 +1,4 @@
> SECTIONS {
> .plt : { BYTE(0) }
> + .init.plt : { BYTE(0) }
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-23 15:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-20 22:00 [RFT PATCH 0/2] ARM/arm64: module-plt: split core and init PLT sections Ard Biesheuvel
2017-02-20 22:00 ` [RFT PATCH 1/2] ARM: module: " Ard Biesheuvel
2017-02-21 14:26 ` Angus Clark
2017-02-21 17:35 ` Ard Biesheuvel
2017-02-23 15:45 ` Ard Biesheuvel
2017-02-20 22:00 ` [RFT PATCH 2/2] arm64: " Ard Biesheuvel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.