| Message ID | 20210125105019.2946057-19-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Early CPU feature override, and applications to VHE, BTI and PAuth | expand |
On Mon, 25 Jan 2021 at 11:53, Marc Zyngier <maz@kernel.org> wrote: > > Given that the early cpufeature infrastructure has borrowed quite > a lot of code from the kaslr implementation, let's reimplement > the matching of the "nokaslr" option with it. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > Acked-by: David Brazdil <dbrazdil@google.com> > --- > arch/arm64/kernel/idreg-override.c | 15 +++++++++++++ > arch/arm64/kernel/kaslr.c | 36 ++---------------------------- > 2 files changed, 17 insertions(+), 34 deletions(-) > > diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c > index cbb8eaa48742..3ccf51b84ba4 100644 > --- a/arch/arm64/kernel/idreg-override.c > +++ b/arch/arm64/kernel/idreg-override.c > @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = { > }, > }; > > +extern struct arm64_ftr_override kaslr_feature_override; > + > +static const struct ftr_set_desc kaslr __initdata = { This should be __initconst not __initdata (below too) > + .name = "kaslr", > +#ifdef CONFIG_RANDOMIZE_BASE > + .override = &kaslr_feature_override, > +#endif > + .fields = { > + { "disabled", 0 }, > + {} > + }, > +}; > + > static const struct ftr_set_desc * const regs[] __initdata = { > &mmfr1, > + &kaslr, > }; > > static const struct { > @@ -41,6 +55,7 @@ static const struct { > } aliases[] __initdata = { > { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, > { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, > + { "nokaslr", "kaslr.disabled=1" }, > }; > This struct now takes up - ~100 bytes for the characters themselves (which btw are not emitted into __initdata or __initconst) - 6x8 bytes for the char pointers - 6x24 bytes for the RELA relocations that annotate these pointers as quantities that need to be relocated at boot (on a kernel built with KASLR) I know it's only a drop in the ocean, but in this case, where the struct is statically declared and defined only once, and in the same place, we could easily turn this into static const struct { char alias[24]; char param[20]; }; and get rid of all the overhead. The only slightly annoying thing is that the array sizes need to be kept in sync with the largest instance appearing in the array, but this is easy when the struct type is declared in the same place where its only instance is defined. > static char *cmdline_contains_option(const char *cmdline, const char *option) > diff --git a/arch/arm64/kernel/kaslr.c b/arch/arm64/kernel/kaslr.c > index 5fc86e7d01a1..27f8939deb1b 100644 > --- a/arch/arm64/kernel/kaslr.c > +++ b/arch/arm64/kernel/kaslr.c > @@ -51,39 +51,7 @@ static __init u64 get_kaslr_seed(void *fdt) > return ret; > } > > -static __init bool cmdline_contains_nokaslr(const u8 *cmdline) > -{ > - const u8 *str; > - > - str = strstr(cmdline, "nokaslr"); > - return str == cmdline || (str > cmdline && *(str - 1) == ' '); > -} > - > -static __init bool is_kaslr_disabled_cmdline(void *fdt) > -{ > - if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) { > - int node; > - const u8 *prop; > - > - node = fdt_path_offset(fdt, "/chosen"); > - if (node < 0) > - goto out; > - > - prop = fdt_getprop(fdt, node, "bootargs", NULL); > - if (!prop) > - goto out; > - > - if (cmdline_contains_nokaslr(prop)) > - return true; > - > - if (IS_ENABLED(CONFIG_CMDLINE_EXTEND)) > - goto out; > - > - return false; > - } > -out: > - return cmdline_contains_nokaslr(CONFIG_CMDLINE); > -} > +struct arm64_ftr_override kaslr_feature_override __initdata; > > /* > * This routine will be executed with the kernel mapped at its default virtual > @@ -126,7 +94,7 @@ u64 __init kaslr_early_init(void) > * Check if 'nokaslr' appears on the command line, and > * return 0 if that is the case. > */ > - if (is_kaslr_disabled_cmdline(fdt)) { > + if (kaslr_feature_override.val & kaslr_feature_override.mask & 0xf) { > kaslr_status = KASLR_DISABLED_CMDLINE; > return 0; > } > -- > 2.29.2 >
On 2021-01-25 12:54, Ard Biesheuvel wrote: > On Mon, 25 Jan 2021 at 11:53, Marc Zyngier <maz@kernel.org> wrote: >> >> Given that the early cpufeature infrastructure has borrowed quite >> a lot of code from the kaslr implementation, let's reimplement >> the matching of the "nokaslr" option with it. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com> >> Acked-by: David Brazdil <dbrazdil@google.com> >> --- >> arch/arm64/kernel/idreg-override.c | 15 +++++++++++++ >> arch/arm64/kernel/kaslr.c | 36 >> ++---------------------------- >> 2 files changed, 17 insertions(+), 34 deletions(-) >> >> diff --git a/arch/arm64/kernel/idreg-override.c >> b/arch/arm64/kernel/idreg-override.c >> index cbb8eaa48742..3ccf51b84ba4 100644 >> --- a/arch/arm64/kernel/idreg-override.c >> +++ b/arch/arm64/kernel/idreg-override.c >> @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = >> { >> }, >> }; >> >> +extern struct arm64_ftr_override kaslr_feature_override; >> + >> +static const struct ftr_set_desc kaslr __initdata = { > > This should be __initconst not __initdata (below too) > >> + .name = "kaslr", >> +#ifdef CONFIG_RANDOMIZE_BASE >> + .override = &kaslr_feature_override, >> +#endif >> + .fields = { >> + { "disabled", 0 }, >> + {} >> + }, >> +}; >> + >> static const struct ftr_set_desc * const regs[] __initdata = { >> &mmfr1, >> + &kaslr, >> }; >> >> static const struct { >> @@ -41,6 +55,7 @@ static const struct { >> } aliases[] __initdata = { >> { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, >> { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, >> + { "nokaslr", "kaslr.disabled=1" }, >> }; >> > > This struct now takes up > - ~100 bytes for the characters themselves (which btw are not emitted > into __initdata or __initconst) > - 6x8 bytes for the char pointers > - 6x24 bytes for the RELA relocations that annotate these pointers as > quantities that need to be relocated at boot (on a kernel built with > KASLR) > > I know it's only a drop in the ocean, but in this case, where the > struct is statically declared and defined only once, and in the same > place, we could easily turn this into > > static const struct { > char alias[24]; > char param[20]; > }; > > and get rid of all the overhead. The only slightly annoying thing is > that the array sizes need to be kept in sync with the largest instance > appearing in the array, but this is easy when the struct type is > declared in the same place where its only instance is defined. Fair enough. I personally find the result butt-ugly, but I agree that it certainly saves some memory. Does the following work for you? I can even give symbolic names to the various constants (how generous of me! ;-). Thanks, M. diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index d1310438d95c..9e7043bdc808 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -14,15 +14,15 @@ #include <asm/setup.h> struct ftr_set_desc { - const char *name; + char name[20]; struct arm64_ftr_override *override; struct { - const char *name; + char name[20]; u8 shift; } fields[]; }; -static const struct ftr_set_desc mmfr1 __initdata = { +static const struct ftr_set_desc mmfr1 __initconst = { .name = "id_aa64mmfr1", .override = &id_aa64mmfr1_override, .fields = { @@ -31,7 +31,7 @@ static const struct ftr_set_desc mmfr1 __initdata = { }, }; -static const struct ftr_set_desc pfr1 __initdata = { +static const struct ftr_set_desc pfr1 __initconst = { .name = "id_aa64pfr1", .override = &id_aa64pfr1_override, .fields = { @@ -40,7 +40,7 @@ static const struct ftr_set_desc pfr1 __initdata = { }, }; -static const struct ftr_set_desc isar1 __initdata = { +static const struct ftr_set_desc isar1 __initconst = { .name = "id_aa64isar1", .override = &id_aa64isar1_override, .fields = { @@ -54,7 +54,7 @@ static const struct ftr_set_desc isar1 __initdata = { extern struct arm64_ftr_override kaslr_feature_override; -static const struct ftr_set_desc kaslr __initdata = { +static const struct ftr_set_desc kaslr __initconst = { .name = "kaslr", #ifdef CONFIG_RANDOMIZE_BASE .override = &kaslr_feature_override, @@ -65,7 +65,7 @@ static const struct ftr_set_desc kaslr __initdata = { }, }; -static const struct ftr_set_desc * const regs[] __initdata = { +static const struct ftr_set_desc * const regs[] __initconst = { &mmfr1, &pfr1, &isar1, @@ -73,9 +73,9 @@ static const struct ftr_set_desc * const regs[] __initdata = { }; static const struct { - const char *alias; - const char *feature; -} aliases[] __initdata = { + char alias[30]; + char feature[80]; +} aliases[] __initconst = { { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, { "arm64.nobti", "id_aa64pfr1.bt=0" },
On Mon, 25 Jan 2021 at 14:54, Marc Zyngier <maz@kernel.org> wrote: > > On 2021-01-25 12:54, Ard Biesheuvel wrote: > > On Mon, 25 Jan 2021 at 11:53, Marc Zyngier <maz@kernel.org> wrote: > >> > >> Given that the early cpufeature infrastructure has borrowed quite > >> a lot of code from the kaslr implementation, let's reimplement > >> the matching of the "nokaslr" option with it. > >> > >> Signed-off-by: Marc Zyngier <maz@kernel.org> > >> Acked-by: Catalin Marinas <catalin.marinas@arm.com> > >> Acked-by: David Brazdil <dbrazdil@google.com> > >> --- > >> arch/arm64/kernel/idreg-override.c | 15 +++++++++++++ > >> arch/arm64/kernel/kaslr.c | 36 > >> ++---------------------------- > >> 2 files changed, 17 insertions(+), 34 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/idreg-override.c > >> b/arch/arm64/kernel/idreg-override.c > >> index cbb8eaa48742..3ccf51b84ba4 100644 > >> --- a/arch/arm64/kernel/idreg-override.c > >> +++ b/arch/arm64/kernel/idreg-override.c > >> @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = > >> { > >> }, > >> }; > >> > >> +extern struct arm64_ftr_override kaslr_feature_override; > >> + > >> +static const struct ftr_set_desc kaslr __initdata = { > > > > This should be __initconst not __initdata (below too) > > > >> + .name = "kaslr", > >> +#ifdef CONFIG_RANDOMIZE_BASE > >> + .override = &kaslr_feature_override, > >> +#endif > >> + .fields = { > >> + { "disabled", 0 }, > >> + {} > >> + }, > >> +}; > >> + > >> static const struct ftr_set_desc * const regs[] __initdata = { > >> &mmfr1, > >> + &kaslr, > >> }; > >> > >> static const struct { > >> @@ -41,6 +55,7 @@ static const struct { > >> } aliases[] __initdata = { > >> { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, > >> { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, > >> + { "nokaslr", "kaslr.disabled=1" }, > >> }; > >> > > > > This struct now takes up > > - ~100 bytes for the characters themselves (which btw are not emitted > > into __initdata or __initconst) > > - 6x8 bytes for the char pointers > > - 6x24 bytes for the RELA relocations that annotate these pointers as > > quantities that need to be relocated at boot (on a kernel built with > > KASLR) > > > > I know it's only a drop in the ocean, but in this case, where the > > struct is statically declared and defined only once, and in the same > > place, we could easily turn this into > > > > static const struct { > > char alias[24]; > > char param[20]; > > }; > > > > and get rid of all the overhead. The only slightly annoying thing is > > that the array sizes need to be kept in sync with the largest instance > > appearing in the array, but this is easy when the struct type is > > declared in the same place where its only instance is defined. > > Fair enough. I personally find the result butt-ugly, but I agree > that it certainly saves some memory. Does the following work for > you? I can even give symbolic names to the various constants (how > generous of me! ;-). > To be honest, I was anticipating more of a discussion, but this looks reasonable to me. Does 'char feature[80];' really need 80 bytes though? > diff --git a/arch/arm64/kernel/idreg-override.c > b/arch/arm64/kernel/idreg-override.c > index d1310438d95c..9e7043bdc808 100644 > --- a/arch/arm64/kernel/idreg-override.c > +++ b/arch/arm64/kernel/idreg-override.c > @@ -14,15 +14,15 @@ > #include <asm/setup.h> > > struct ftr_set_desc { > - const char *name; > + char name[20]; > struct arm64_ftr_override *override; > struct { > - const char *name; > + char name[20]; > u8 shift; > } fields[]; > }; > > -static const struct ftr_set_desc mmfr1 __initdata = { > +static const struct ftr_set_desc mmfr1 __initconst = { > .name = "id_aa64mmfr1", > .override = &id_aa64mmfr1_override, > .fields = { > @@ -31,7 +31,7 @@ static const struct ftr_set_desc mmfr1 __initdata = { > }, > }; > > -static const struct ftr_set_desc pfr1 __initdata = { > +static const struct ftr_set_desc pfr1 __initconst = { > .name = "id_aa64pfr1", > .override = &id_aa64pfr1_override, > .fields = { > @@ -40,7 +40,7 @@ static const struct ftr_set_desc pfr1 __initdata = { > }, > }; > > -static const struct ftr_set_desc isar1 __initdata = { > +static const struct ftr_set_desc isar1 __initconst = { > .name = "id_aa64isar1", > .override = &id_aa64isar1_override, > .fields = { > @@ -54,7 +54,7 @@ static const struct ftr_set_desc isar1 __initdata = { > > extern struct arm64_ftr_override kaslr_feature_override; > > -static const struct ftr_set_desc kaslr __initdata = { > +static const struct ftr_set_desc kaslr __initconst = { > .name = "kaslr", > #ifdef CONFIG_RANDOMIZE_BASE > .override = &kaslr_feature_override, > @@ -65,7 +65,7 @@ static const struct ftr_set_desc kaslr __initdata = { > }, > }; > > -static const struct ftr_set_desc * const regs[] __initdata = { > +static const struct ftr_set_desc * const regs[] __initconst = { > &mmfr1, > &pfr1, > &isar1, > @@ -73,9 +73,9 @@ static const struct ftr_set_desc * const regs[] > __initdata = { > }; > > static const struct { > - const char *alias; > - const char *feature; > -} aliases[] __initdata = { > + char alias[30]; > + char feature[80]; > +} aliases[] __initconst = { > { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, > { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, > { "arm64.nobti", "id_aa64pfr1.bt=0" }, > > -- > Jazz is not dead. It just smells funny...
On 2021-01-25 14:19, Ard Biesheuvel wrote: > On Mon, 25 Jan 2021 at 14:54, Marc Zyngier <maz@kernel.org> wrote: >> >> On 2021-01-25 12:54, Ard Biesheuvel wrote: [...] >> > This struct now takes up >> > - ~100 bytes for the characters themselves (which btw are not emitted >> > into __initdata or __initconst) >> > - 6x8 bytes for the char pointers >> > - 6x24 bytes for the RELA relocations that annotate these pointers as >> > quantities that need to be relocated at boot (on a kernel built with >> > KASLR) >> > >> > I know it's only a drop in the ocean, but in this case, where the >> > struct is statically declared and defined only once, and in the same >> > place, we could easily turn this into >> > >> > static const struct { >> > char alias[24]; >> > char param[20]; >> > }; >> > >> > and get rid of all the overhead. The only slightly annoying thing is >> > that the array sizes need to be kept in sync with the largest instance >> > appearing in the array, but this is easy when the struct type is >> > declared in the same place where its only instance is defined. >> >> Fair enough. I personally find the result butt-ugly, but I agree >> that it certainly saves some memory. Does the following work for >> you? I can even give symbolic names to the various constants (how >> generous of me! ;-). >> > > To be honest, I was anticipating more of a discussion, but this looks > reasonable to me. It looked like a reasonable ask: all the strings are completely useless once the kernel has booted, and I'm the first to moan that I can't boot an arm64 kernel with less than 60MB of RAM (OK, it's a pretty bloated kernel...). > Does 'char feature[80];' really need 80 bytes though? It really needs 75 bytes, because of this: { "arm64.nopauth", "id_aa64isar1.gpi=0 id_aa64isar1.gpa=0 " "id_aa64isar1.api=0 id_aa64isar1.apa=0" }, 80 is a round enough number. Thanks, M.
On Mon, 25 Jan 2021 at 15:28, Marc Zyngier <maz@kernel.org> wrote: > > On 2021-01-25 14:19, Ard Biesheuvel wrote: > > On Mon, 25 Jan 2021 at 14:54, Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2021-01-25 12:54, Ard Biesheuvel wrote: > > [...] > > >> > This struct now takes up > >> > - ~100 bytes for the characters themselves (which btw are not emitted > >> > into __initdata or __initconst) > >> > - 6x8 bytes for the char pointers > >> > - 6x24 bytes for the RELA relocations that annotate these pointers as > >> > quantities that need to be relocated at boot (on a kernel built with > >> > KASLR) > >> > > >> > I know it's only a drop in the ocean, but in this case, where the > >> > struct is statically declared and defined only once, and in the same > >> > place, we could easily turn this into > >> > > >> > static const struct { > >> > char alias[24]; > >> > char param[20]; > >> > }; > >> > > >> > and get rid of all the overhead. The only slightly annoying thing is > >> > that the array sizes need to be kept in sync with the largest instance > >> > appearing in the array, but this is easy when the struct type is > >> > declared in the same place where its only instance is defined. > >> > >> Fair enough. I personally find the result butt-ugly, but I agree > >> that it certainly saves some memory. Does the following work for > >> you? I can even give symbolic names to the various constants (how > >> generous of me! ;-). > >> > > > > To be honest, I was anticipating more of a discussion, but this looks > > reasonable to me. > > It looked like a reasonable ask: all the strings are completely useless > once the kernel has booted, and I'm the first to moan that I can't boot > an arm64 kernel with less than 60MB of RAM (OK, it's a pretty bloated > kernel...). > > > Does 'char feature[80];' really need 80 bytes though? > > It really needs 75 bytes, because of this: > > { "arm64.nopauth", > "id_aa64isar1.gpi=0 id_aa64isar1.gpa=0 " > "id_aa64isar1.api=0 id_aa64isar1.apa=0" }, > > 80 is a round enough number. > Fair enough. This will inflate the struct substantially, but at least it's all __initconst data now, and it's all NUL bytes so it compresses much better than the pointers and RELA entries.
diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c index cbb8eaa48742..3ccf51b84ba4 100644 --- a/arch/arm64/kernel/idreg-override.c +++ b/arch/arm64/kernel/idreg-override.c @@ -31,8 +31,22 @@ static const struct ftr_set_desc mmfr1 __initdata = { }, }; +extern struct arm64_ftr_override kaslr_feature_override; + +static const struct ftr_set_desc kaslr __initdata = { + .name = "kaslr", +#ifdef CONFIG_RANDOMIZE_BASE + .override = &kaslr_feature_override, +#endif + .fields = { + { "disabled", 0 }, + {} + }, +}; + static const struct ftr_set_desc * const regs[] __initdata = { &mmfr1, + &kaslr, }; static const struct { @@ -41,6 +55,7 @@ static const struct { } aliases[] __initdata = { { "kvm-arm.mode=nvhe", "id_aa64mmfr1.vh=0" }, { "kvm-arm.mode=protected", "id_aa64mmfr1.vh=0" }, + { "nokaslr", "kaslr.disabled=1" }, }; static char *cmdline_contains_option(const char *cmdline, const char *option) diff --git a/arch/arm64/kernel/kaslr.c b/arch/arm64/kernel/kaslr.c index 5fc86e7d01a1..27f8939deb1b 100644 --- a/arch/arm64/kernel/kaslr.c +++ b/arch/arm64/kernel/kaslr.c @@ -51,39 +51,7 @@ static __init u64 get_kaslr_seed(void *fdt) return ret; } -static __init bool cmdline_contains_nokaslr(const u8 *cmdline) -{ - const u8 *str; - - str = strstr(cmdline, "nokaslr"); - return str == cmdline || (str > cmdline && *(str - 1) == ' '); -} - -static __init bool is_kaslr_disabled_cmdline(void *fdt) -{ - if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) { - int node; - const u8 *prop; - - node = fdt_path_offset(fdt, "/chosen"); - if (node < 0) - goto out; - - prop = fdt_getprop(fdt, node, "bootargs", NULL); - if (!prop) - goto out; - - if (cmdline_contains_nokaslr(prop)) - return true; - - if (IS_ENABLED(CONFIG_CMDLINE_EXTEND)) - goto out; - - return false; - } -out: - return cmdline_contains_nokaslr(CONFIG_CMDLINE); -} +struct arm64_ftr_override kaslr_feature_override __initdata; /* * This routine will be executed with the kernel mapped at its default virtual @@ -126,7 +94,7 @@ u64 __init kaslr_early_init(void) * Check if 'nokaslr' appears on the command line, and * return 0 if that is the case. */ - if (is_kaslr_disabled_cmdline(fdt)) { + if (kaslr_feature_override.val & kaslr_feature_override.mask & 0xf) { kaslr_status = KASLR_DISABLED_CMDLINE; return 0; }