[for-stable-4.14,42/48] arm64: Always enable spectre-v2 vulnerability detection

Message ID 20191024124833.4158-43-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • arm64 spec mitigation backports
Related show

Commit Message

Ard Biesheuvel Oct. 24, 2019, 12:48 p.m.
From: Jeremy Linton <jeremy.linton@arm.com>


[ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

Ensure we are always able to detect whether or not the CPU is affected
by Spectre-v2, so that we can later advertise this to userspace.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

Signed-off-by: Will Deacon <will.deacon@arm.com>

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

-- 
2.20.1

Comments

Alexandru Elisei Oct. 24, 2019, 2:34 p.m. | #1
Hi,

On 10/24/19 1:48 PM, Ard Biesheuvel wrote:
> From: Jeremy Linton <jeremy.linton@arm.com>

>

> [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

>

> Ensure we are always able to detect whether or not the CPU is affected

> by Spectre-v2, so that we can later advertise this to userspace.

>

> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> Signed-off-by: Will Deacon <will.deacon@arm.com>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

>  1 file changed, 8 insertions(+), 7 deletions(-)

>

> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

> index bf6d8aa9b45a..647c533cfd90 100644

> --- a/arch/arm64/kernel/cpu_errata.c

> +++ b/arch/arm64/kernel/cpu_errata.c

> @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

>  	config_sctlr_el1(SCTLR_EL1_UCT, 0);

>  }

>  

> -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

>  #include <asm/mmu_context.h>

>  #include <asm/cacheflush.h>

>  

> @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

>  	    ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

>  		cb = qcom_link_stack_sanitization;

>  

> -	install_bp_hardening_cb(cb, smccc_start, smccc_end);

> +	if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

> +		install_bp_hardening_cb(cb, smccc_start, smccc_end);

>  

>  	return 1;

>  }

> -#endif	/* CONFIG_HARDEN_BRANCH_PREDICTOR */

>  

>  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

>  

> @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

>  	.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,			\

>  	CAP_MIDR_RANGE_LIST(midr_list)

>  

> -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

>  /*

>   * List of CPUs that do not need any Spectre-v2 mitigation at all.

>   */

> @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

>  	if (!need_wa)

>  		return false;

>  

> +	if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

> +		pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

> +		__hardenbp_enab = false;


This breaks when building, because __hardenbp_enab is declared in the next patch:

$ make -j32 defconfig && make -j32

[..]
arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:
arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first
use in this function)
   __hardenbp_enab = false;
   ^~~~~~~~~~~~~~~
arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported
only once for each function it appears in
make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1
make[1]: *** Waiting for unfinished jobs....

Thanks,
Alex

> +		return false;

> +	}

> +

>  	/* forced off */

>  	if (__nospectre_v2) {

>  		pr_info_once("spectrev2 mitigation disabled by command line option\n");

> @@ -500,7 +504,6 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

>  

>  	return (need_wa > 0);

>  }

> -#endif

>  

>  const struct arm64_cpu_capabilities arm64_errata[] = {

>  #if	defined(CONFIG_ARM64_ERRATUM_826319) || \

> @@ -640,13 +643,11 @@ const struct arm64_cpu_capabilities arm64_errata[] = {

>  		ERRATA_MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),

>  	},

>  #endif

> -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

>  	{

>  		.capability = ARM64_HARDEN_BRANCH_PREDICTOR,

>  		.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,

>  		.matches = check_branch_predictor,

>  	},

> -#endif

>  	{

>  		.desc = "Speculative Store Bypass Disable",

>  		.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,
Ard Biesheuvel Oct. 24, 2019, 2:37 p.m. | #2
On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>

> Hi,

>

> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

> > From: Jeremy Linton <jeremy.linton@arm.com>

> >

> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

> >

> > Ensure we are always able to detect whether or not the CPU is affected

> > by Spectre-v2, so that we can later advertise this to userspace.

> >

> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

> >  1 file changed, 8 insertions(+), 7 deletions(-)

> >

> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

> > index bf6d8aa9b45a..647c533cfd90 100644

> > --- a/arch/arm64/kernel/cpu_errata.c

> > +++ b/arch/arm64/kernel/cpu_errata.c

> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

> >  }

> >

> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> >  #include <asm/mmu_context.h>

> >  #include <asm/cacheflush.h>

> >

> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

> >               cb = qcom_link_stack_sanitization;

> >

> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

> >

> >       return 1;

> >  }

> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

> >

> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

> >

> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

> >       CAP_MIDR_RANGE_LIST(midr_list)

> >

> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> >  /*

> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

> >   */

> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

> >       if (!need_wa)

> >               return false;

> >

> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

> > +             __hardenbp_enab = false;

>

> This breaks when building, because __hardenbp_enab is declared in the next patch:

>

> $ make -j32 defconfig && make -j32

>

> [..]

> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

> use in this function)

>    __hardenbp_enab = false;

>    ^~~~~~~~~~~~~~~

> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

> only once for each function it appears in

> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

> make[1]: *** Waiting for unfinished jobs....

>


Indeed, but as discussed, this matches the state of both mainline and
v4.19, which carry these patches in the same [wrong] order as well.

Greg should confirm, but as I understand it, it is preferred to be
bug-compatible with mainline rather than fixing problems when spotting
them while doing the backport.



> > +             return false;

> > +     }

> > +

> >       /* forced off */

> >       if (__nospectre_v2) {

> >               pr_info_once("spectrev2 mitigation disabled by command line option\n");

> > @@ -500,7 +504,6 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

> >

> >       return (need_wa > 0);

> >  }

> > -#endif

> >

> >  const struct arm64_cpu_capabilities arm64_errata[] = {

> >  #if  defined(CONFIG_ARM64_ERRATUM_826319) || \

> > @@ -640,13 +643,11 @@ const struct arm64_cpu_capabilities arm64_errata[] = {

> >               ERRATA_MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),

> >       },

> >  #endif

> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> >       {

> >               .capability = ARM64_HARDEN_BRANCH_PREDICTOR,

> >               .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,

> >               .matches = check_branch_predictor,

> >       },

> > -#endif

> >       {

> >               .desc = "Speculative Store Bypass Disable",

> >               .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,
Sasha Levin Oct. 25, 2019, 3:25 p.m. | #3
On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:
>On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

>>

>> Hi,

>>

>> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

>> > From: Jeremy Linton <jeremy.linton@arm.com>

>> >

>> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

>> >

>> > Ensure we are always able to detect whether or not the CPU is affected

>> > by Spectre-v2, so that we can later advertise this to userspace.

>> >

>> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

>> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

>> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

>> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

>> > Signed-off-by: Will Deacon <will.deacon@arm.com>

>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> > ---

>> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

>> >  1 file changed, 8 insertions(+), 7 deletions(-)

>> >

>> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

>> > index bf6d8aa9b45a..647c533cfd90 100644

>> > --- a/arch/arm64/kernel/cpu_errata.c

>> > +++ b/arch/arm64/kernel/cpu_errata.c

>> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

>> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

>> >  }

>> >

>> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

>> >  #include <asm/mmu_context.h>

>> >  #include <asm/cacheflush.h>

>> >

>> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

>> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

>> >               cb = qcom_link_stack_sanitization;

>> >

>> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

>> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

>> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

>> >

>> >       return 1;

>> >  }

>> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

>> >

>> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

>> >

>> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

>> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

>> >       CAP_MIDR_RANGE_LIST(midr_list)

>> >

>> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

>> >  /*

>> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

>> >   */

>> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

>> >       if (!need_wa)

>> >               return false;

>> >

>> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

>> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

>> > +             __hardenbp_enab = false;

>>

>> This breaks when building, because __hardenbp_enab is declared in the next patch:

>>

>> $ make -j32 defconfig && make -j32

>>

>> [..]

>> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

>> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

>> use in this function)

>>    __hardenbp_enab = false;

>>    ^~~~~~~~~~~~~~~

>> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

>> only once for each function it appears in

>> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

>> make[1]: *** Waiting for unfinished jobs....

>>

>

>Indeed, but as discussed, this matches the state of both mainline and

>v4.19, which carry these patches in the same [wrong] order as well.

>

>Greg should confirm, but as I understand it, it is preferred to be

>bug-compatible with mainline rather than fixing problems when spotting

>them while doing the backport.


Is it just patch ordering? If so I'd rather fix it, there's no reason to
carry this issue into the stable trees.

We reserve "bug compatibility" for functional issues that are not yet
fixed upstream, it doesn't seem to be the case here.

-- 
Thanks,
Sasha
Ard Biesheuvel Oct. 25, 2019, 3:28 p.m. | #4
On Fri, 25 Oct 2019 at 17:25, Sasha Levin <sashal@kernel.org> wrote:
>

> On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:

> >On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> >>

> >> Hi,

> >>

> >> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

> >> > From: Jeremy Linton <jeremy.linton@arm.com>

> >> >

> >> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

> >> >

> >> > Ensure we are always able to detect whether or not the CPU is affected

> >> > by Spectre-v2, so that we can later advertise this to userspace.

> >> >

> >> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> >> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> >> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> >> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> > ---

> >> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

> >> >  1 file changed, 8 insertions(+), 7 deletions(-)

> >> >

> >> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

> >> > index bf6d8aa9b45a..647c533cfd90 100644

> >> > --- a/arch/arm64/kernel/cpu_errata.c

> >> > +++ b/arch/arm64/kernel/cpu_errata.c

> >> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

> >> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

> >> >  }

> >> >

> >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> >> >  #include <asm/mmu_context.h>

> >> >  #include <asm/cacheflush.h>

> >> >

> >> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

> >> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

> >> >               cb = qcom_link_stack_sanitization;

> >> >

> >> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

> >> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

> >> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

> >> >

> >> >       return 1;

> >> >  }

> >> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

> >> >

> >> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

> >> >

> >> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

> >> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

> >> >       CAP_MIDR_RANGE_LIST(midr_list)

> >> >

> >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> >> >  /*

> >> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

> >> >   */

> >> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

> >> >       if (!need_wa)

> >> >               return false;

> >> >

> >> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

> >> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

> >> > +             __hardenbp_enab = false;

> >>

> >> This breaks when building, because __hardenbp_enab is declared in the next patch:

> >>

> >> $ make -j32 defconfig && make -j32

> >>

> >> [..]

> >> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

> >> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

> >> use in this function)

> >>    __hardenbp_enab = false;

> >>    ^~~~~~~~~~~~~~~

> >> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

> >> only once for each function it appears in

> >> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

> >> make[1]: *** Waiting for unfinished jobs....

> >>

> >

> >Indeed, but as discussed, this matches the state of both mainline and

> >v4.19, which carry these patches in the same [wrong] order as well.

> >

> >Greg should confirm, but as I understand it, it is preferred to be

> >bug-compatible with mainline rather than fixing problems when spotting

> >them while doing the backport.

>

> Is it just patch ordering? If so I'd rather fix it, there's no reason to

> carry this issue into the stable trees.

>

> We reserve "bug compatibility" for functional issues that are not yet

> fixed upstream, it doesn't seem to be the case here.

>


The patches don't apply cleanly in the opposite order.
Ard Biesheuvel Oct. 25, 2019, 3:39 p.m. | #5
On Fri, 25 Oct 2019 at 17:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> On Fri, 25 Oct 2019 at 17:25, Sasha Levin <sashal@kernel.org> wrote:

> >

> > On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:

> > >On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> > >>

> > >> Hi,

> > >>

> > >> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

> > >> > From: Jeremy Linton <jeremy.linton@arm.com>

> > >> >

> > >> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

> > >> >

> > >> > Ensure we are always able to detect whether or not the CPU is affected

> > >> > by Spectre-v2, so that we can later advertise this to userspace.

> > >> >

> > >> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > >> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> > >> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> > >> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > >> > ---

> > >> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

> > >> >  1 file changed, 8 insertions(+), 7 deletions(-)

> > >> >

> > >> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

> > >> > index bf6d8aa9b45a..647c533cfd90 100644

> > >> > --- a/arch/arm64/kernel/cpu_errata.c

> > >> > +++ b/arch/arm64/kernel/cpu_errata.c

> > >> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

> > >> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

> > >> >  }

> > >> >

> > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> > >> >  #include <asm/mmu_context.h>

> > >> >  #include <asm/cacheflush.h>

> > >> >

> > >> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

> > >> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

> > >> >               cb = qcom_link_stack_sanitization;

> > >> >

> > >> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

> > >> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

> > >> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

> > >> >

> > >> >       return 1;

> > >> >  }

> > >> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

> > >> >

> > >> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

> > >> >

> > >> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

> > >> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

> > >> >       CAP_MIDR_RANGE_LIST(midr_list)

> > >> >

> > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> > >> >  /*

> > >> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

> > >> >   */

> > >> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

> > >> >       if (!need_wa)

> > >> >               return false;

> > >> >

> > >> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

> > >> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

> > >> > +             __hardenbp_enab = false;

> > >>

> > >> This breaks when building, because __hardenbp_enab is declared in the next patch:

> > >>

> > >> $ make -j32 defconfig && make -j32

> > >>

> > >> [..]

> > >> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

> > >> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

> > >> use in this function)

> > >>    __hardenbp_enab = false;

> > >>    ^~~~~~~~~~~~~~~

> > >> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

> > >> only once for each function it appears in

> > >> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

> > >> make[1]: *** Waiting for unfinished jobs....

> > >>

> > >

> > >Indeed, but as discussed, this matches the state of both mainline and

> > >v4.19, which carry these patches in the same [wrong] order as well.

> > >

> > >Greg should confirm, but as I understand it, it is preferred to be

> > >bug-compatible with mainline rather than fixing problems when spotting

> > >them while doing the backport.

> >

> > Is it just patch ordering? If so I'd rather fix it, there's no reason to

> > carry this issue into the stable trees.

> >

> > We reserve "bug compatibility" for functional issues that are not yet

> > fixed upstream, it doesn't seem to be the case here.

> >

>

> The patches don't apply cleanly in the opposite order.


What we could do is squash the two patches together. That way, we
avoid the breakage without having to modify the patches in order to be
able to apply them.
Greg KH Oct. 26, 2019, 8:01 a.m. | #6
On Fri, Oct 25, 2019 at 05:39:44PM +0200, Ard Biesheuvel wrote:
> On Fri, 25 Oct 2019 at 17:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >

> > On Fri, 25 Oct 2019 at 17:25, Sasha Levin <sashal@kernel.org> wrote:

> > >

> > > On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:

> > > >On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> > > >>

> > > >> Hi,

> > > >>

> > > >> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

> > > >> > From: Jeremy Linton <jeremy.linton@arm.com>

> > > >> >

> > > >> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

> > > >> >

> > > >> > Ensure we are always able to detect whether or not the CPU is affected

> > > >> > by Spectre-v2, so that we can later advertise this to userspace.

> > > >> >

> > > >> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > > >> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> > > >> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> > > >> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> > > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > > >> > ---

> > > >> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

> > > >> >  1 file changed, 8 insertions(+), 7 deletions(-)

> > > >> >

> > > >> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

> > > >> > index bf6d8aa9b45a..647c533cfd90 100644

> > > >> > --- a/arch/arm64/kernel/cpu_errata.c

> > > >> > +++ b/arch/arm64/kernel/cpu_errata.c

> > > >> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

> > > >> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

> > > >> >  }

> > > >> >

> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> > > >> >  #include <asm/mmu_context.h>

> > > >> >  #include <asm/cacheflush.h>

> > > >> >

> > > >> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

> > > >> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

> > > >> >               cb = qcom_link_stack_sanitization;

> > > >> >

> > > >> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

> > > >> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

> > > >> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

> > > >> >

> > > >> >       return 1;

> > > >> >  }

> > > >> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

> > > >> >

> > > >> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

> > > >> >

> > > >> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

> > > >> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

> > > >> >       CAP_MIDR_RANGE_LIST(midr_list)

> > > >> >

> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> > > >> >  /*

> > > >> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

> > > >> >   */

> > > >> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

> > > >> >       if (!need_wa)

> > > >> >               return false;

> > > >> >

> > > >> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

> > > >> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

> > > >> > +             __hardenbp_enab = false;

> > > >>

> > > >> This breaks when building, because __hardenbp_enab is declared in the next patch:

> > > >>

> > > >> $ make -j32 defconfig && make -j32

> > > >>

> > > >> [..]

> > > >> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

> > > >> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

> > > >> use in this function)

> > > >>    __hardenbp_enab = false;

> > > >>    ^~~~~~~~~~~~~~~

> > > >> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

> > > >> only once for each function it appears in

> > > >> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

> > > >> make[1]: *** Waiting for unfinished jobs....

> > > >>

> > > >

> > > >Indeed, but as discussed, this matches the state of both mainline and

> > > >v4.19, which carry these patches in the same [wrong] order as well.

> > > >

> > > >Greg should confirm, but as I understand it, it is preferred to be

> > > >bug-compatible with mainline rather than fixing problems when spotting

> > > >them while doing the backport.

> > >

> > > Is it just patch ordering? If so I'd rather fix it, there's no reason to

> > > carry this issue into the stable trees.

> > >

> > > We reserve "bug compatibility" for functional issues that are not yet

> > > fixed upstream, it doesn't seem to be the case here.

> > >

> >

> > The patches don't apply cleanly in the opposite order.

> 

> What we could do is squash the two patches together. That way, we

> avoid the breakage without having to modify the patches in order to be

> able to apply them.


No, don't do that.  Just take all of the needed commits.
Sasha Levin Oct. 26, 2019, 3:40 p.m. | #7
On Sat, Oct 26, 2019 at 10:01:21AM +0200, Greg KH wrote:
>On Fri, Oct 25, 2019 at 05:39:44PM +0200, Ard Biesheuvel wrote:

>> On Fri, 25 Oct 2019 at 17:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> >

>> > On Fri, 25 Oct 2019 at 17:25, Sasha Levin <sashal@kernel.org> wrote:

>> > >

>> > > On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:

>> > > >On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

>> > > >>

>> > > >> Hi,

>> > > >>

>> > > >> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

>> > > >> > From: Jeremy Linton <jeremy.linton@arm.com>

>> > > >> >

>> > > >> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

>> > > >> >

>> > > >> > Ensure we are always able to detect whether or not the CPU is affected

>> > > >> > by Spectre-v2, so that we can later advertise this to userspace.

>> > > >> >

>> > > >> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

>> > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

>> > > >> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

>> > > >> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

>> > > >> > Signed-off-by: Will Deacon <will.deacon@arm.com>

>> > > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> > > >> > ---

>> > > >> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

>> > > >> >  1 file changed, 8 insertions(+), 7 deletions(-)

>> > > >> >

>> > > >> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

>> > > >> > index bf6d8aa9b45a..647c533cfd90 100644

>> > > >> > --- a/arch/arm64/kernel/cpu_errata.c

>> > > >> > +++ b/arch/arm64/kernel/cpu_errata.c

>> > > >> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

>> > > >> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

>> > > >> >  }

>> > > >> >

>> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

>> > > >> >  #include <asm/mmu_context.h>

>> > > >> >  #include <asm/cacheflush.h>

>> > > >> >

>> > > >> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

>> > > >> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

>> > > >> >               cb = qcom_link_stack_sanitization;

>> > > >> >

>> > > >> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

>> > > >> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

>> > > >> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

>> > > >> >

>> > > >> >       return 1;

>> > > >> >  }

>> > > >> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

>> > > >> >

>> > > >> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

>> > > >> >

>> > > >> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

>> > > >> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

>> > > >> >       CAP_MIDR_RANGE_LIST(midr_list)

>> > > >> >

>> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

>> > > >> >  /*

>> > > >> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

>> > > >> >   */

>> > > >> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

>> > > >> >       if (!need_wa)

>> > > >> >               return false;

>> > > >> >

>> > > >> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

>> > > >> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

>> > > >> > +             __hardenbp_enab = false;

>> > > >>

>> > > >> This breaks when building, because __hardenbp_enab is declared in the next patch:

>> > > >>

>> > > >> $ make -j32 defconfig && make -j32

>> > > >>

>> > > >> [..]

>> > > >> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

>> > > >> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

>> > > >> use in this function)

>> > > >>    __hardenbp_enab = false;

>> > > >>    ^~~~~~~~~~~~~~~

>> > > >> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

>> > > >> only once for each function it appears in

>> > > >> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

>> > > >> make[1]: *** Waiting for unfinished jobs....

>> > > >>

>> > > >

>> > > >Indeed, but as discussed, this matches the state of both mainline and

>> > > >v4.19, which carry these patches in the same [wrong] order as well.

>> > > >

>> > > >Greg should confirm, but as I understand it, it is preferred to be

>> > > >bug-compatible with mainline rather than fixing problems when spotting

>> > > >them while doing the backport.

>> > >

>> > > Is it just patch ordering? If so I'd rather fix it, there's no reason to

>> > > carry this issue into the stable trees.

>> > >

>> > > We reserve "bug compatibility" for functional issues that are not yet

>> > > fixed upstream, it doesn't seem to be the case here.

>> > >

>> >

>> > The patches don't apply cleanly in the opposite order.

>>

>> What we could do is squash the two patches together. That way, we

>> avoid the breakage without having to modify the patches in order to be

>> able to apply them.

>

>No, don't do that.  Just take all of the needed commits.


Right, just make the patches apply in reverse, this shouldn't be more
than moving some code from the 2nd patch back to the 1st one, right?

We usually don't do this in stable backports, but there are three good
reasons to do it here:

1. It'll be nice to maintain bisectability.
2. The end result should be exactly the same, so there's no room for
error here.
3. It's a backport for an older kernel to begin with, so there are
changes from upstream already.

-- 
Thanks,
Sasha
Ard Biesheuvel Oct. 26, 2019, 3:46 p.m. | #8
On Sat, 26 Oct 2019 at 17:40, Sasha Levin <sashal@kernel.org> wrote:
>

> On Sat, Oct 26, 2019 at 10:01:21AM +0200, Greg KH wrote:

> >On Fri, Oct 25, 2019 at 05:39:44PM +0200, Ard Biesheuvel wrote:

> >> On Fri, 25 Oct 2019 at 17:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >> >

> >> > On Fri, 25 Oct 2019 at 17:25, Sasha Levin <sashal@kernel.org> wrote:

> >> > >

> >> > > On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:

> >> > > >On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> >> > > >>

> >> > > >> Hi,

> >> > > >>

> >> > > >> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

> >> > > >> > From: Jeremy Linton <jeremy.linton@arm.com>

> >> > > >> >

> >> > > >> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

> >> > > >> >

> >> > > >> > Ensure we are always able to detect whether or not the CPU is affected

> >> > > >> > by Spectre-v2, so that we can later advertise this to userspace.

> >> > > >> >

> >> > > >> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> >> > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> >> > > >> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> >> > > >> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> >> > > >> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> >> > > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> > > >> > ---

> >> > > >> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

> >> > > >> >  1 file changed, 8 insertions(+), 7 deletions(-)

> >> > > >> >

> >> > > >> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

> >> > > >> > index bf6d8aa9b45a..647c533cfd90 100644

> >> > > >> > --- a/arch/arm64/kernel/cpu_errata.c

> >> > > >> > +++ b/arch/arm64/kernel/cpu_errata.c

> >> > > >> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

> >> > > >> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

> >> > > >> >  }

> >> > > >> >

> >> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> >> > > >> >  #include <asm/mmu_context.h>

> >> > > >> >  #include <asm/cacheflush.h>

> >> > > >> >

> >> > > >> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

> >> > > >> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

> >> > > >> >               cb = qcom_link_stack_sanitization;

> >> > > >> >

> >> > > >> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

> >> > > >> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

> >> > > >> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

> >> > > >> >

> >> > > >> >       return 1;

> >> > > >> >  }

> >> > > >> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

> >> > > >> >

> >> > > >> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

> >> > > >> >

> >> > > >> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

> >> > > >> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

> >> > > >> >       CAP_MIDR_RANGE_LIST(midr_list)

> >> > > >> >

> >> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> >> > > >> >  /*

> >> > > >> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

> >> > > >> >   */

> >> > > >> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

> >> > > >> >       if (!need_wa)

> >> > > >> >               return false;

> >> > > >> >

> >> > > >> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

> >> > > >> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

> >> > > >> > +             __hardenbp_enab = false;

> >> > > >>

> >> > > >> This breaks when building, because __hardenbp_enab is declared in the next patch:

> >> > > >>

> >> > > >> $ make -j32 defconfig && make -j32

> >> > > >>

> >> > > >> [..]

> >> > > >> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

> >> > > >> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

> >> > > >> use in this function)

> >> > > >>    __hardenbp_enab = false;

> >> > > >>    ^~~~~~~~~~~~~~~

> >> > > >> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

> >> > > >> only once for each function it appears in

> >> > > >> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

> >> > > >> make[1]: *** Waiting for unfinished jobs....

> >> > > >>

> >> > > >

> >> > > >Indeed, but as discussed, this matches the state of both mainline and

> >> > > >v4.19, which carry these patches in the same [wrong] order as well.

> >> > > >

> >> > > >Greg should confirm, but as I understand it, it is preferred to be

> >> > > >bug-compatible with mainline rather than fixing problems when spotting

> >> > > >them while doing the backport.

> >> > >

> >> > > Is it just patch ordering? If so I'd rather fix it, there's no reason to

> >> > > carry this issue into the stable trees.

> >> > >

> >> > > We reserve "bug compatibility" for functional issues that are not yet

> >> > > fixed upstream, it doesn't seem to be the case here.

> >> > >

> >> >

> >> > The patches don't apply cleanly in the opposite order.

> >>

> >> What we could do is squash the two patches together. That way, we

> >> avoid the breakage without having to modify the patches in order to be

> >> able to apply them.

> >

> >No, don't do that.  Just take all of the needed commits.

>

> Right, just make the patches apply in reverse, this shouldn't be more

> than moving some code from the 2nd patch back to the 1st one, right?

>

> We usually don't do this in stable backports, but there are three good

> reasons to do it here:

>

> 1. It'll be nice to maintain bisectability.

> 2. The end result should be exactly the same, so there's no room for

> error here.

> 3. It's a backport for an older kernel to begin with, so there are

> changes from upstream already.

>


I really don't see the point of doing this for v4.14 while v4.19 and
mainline have the two patches in the opposite order.

Would you like me to resend the entire 48 piece series for this?
Greg KH Oct. 27, 2019, 1:39 p.m. | #9
On Sat, Oct 26, 2019 at 05:46:03PM +0200, Ard Biesheuvel wrote:
> On Sat, 26 Oct 2019 at 17:40, Sasha Levin <sashal@kernel.org> wrote:

> >

> > On Sat, Oct 26, 2019 at 10:01:21AM +0200, Greg KH wrote:

> > >On Fri, Oct 25, 2019 at 05:39:44PM +0200, Ard Biesheuvel wrote:

> > >> On Fri, 25 Oct 2019 at 17:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > >> >

> > >> > On Fri, 25 Oct 2019 at 17:25, Sasha Levin <sashal@kernel.org> wrote:

> > >> > >

> > >> > > On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:

> > >> > > >On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> > >> > > >>

> > >> > > >> Hi,

> > >> > > >>

> > >> > > >> On 10/24/19 1:48 PM, Ard Biesheuvel wrote:

> > >> > > >> > From: Jeremy Linton <jeremy.linton@arm.com>

> > >> > > >> >

> > >> > > >> > [ Upstream commit 8c1e3d2bb44cbb998cb28ff9a18f105fee7f1eb3 ]

> > >> > > >> >

> > >> > > >> > Ensure we are always able to detect whether or not the CPU is affected

> > >> > > >> > by Spectre-v2, so that we can later advertise this to userspace.

> > >> > > >> >

> > >> > > >> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > >> > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > >> > > >> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

> > >> > > >> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> > >> > > >> > Signed-off-by: Will Deacon <will.deacon@arm.com>

> > >> > > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > >> > > >> > ---

> > >> > > >> >  arch/arm64/kernel/cpu_errata.c | 15 ++++++++-------

> > >> > > >> >  1 file changed, 8 insertions(+), 7 deletions(-)

> > >> > > >> >

> > >> > > >> > diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c

> > >> > > >> > index bf6d8aa9b45a..647c533cfd90 100644

> > >> > > >> > --- a/arch/arm64/kernel/cpu_errata.c

> > >> > > >> > +++ b/arch/arm64/kernel/cpu_errata.c

> > >> > > >> > @@ -76,7 +76,6 @@ cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)

> > >> > > >> >       config_sctlr_el1(SCTLR_EL1_UCT, 0);

> > >> > > >> >  }

> > >> > > >> >

> > >> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> > >> > > >> >  #include <asm/mmu_context.h>

> > >> > > >> >  #include <asm/cacheflush.h>

> > >> > > >> >

> > >> > > >> > @@ -217,11 +216,11 @@ static int detect_harden_bp_fw(void)

> > >> > > >> >           ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))

> > >> > > >> >               cb = qcom_link_stack_sanitization;

> > >> > > >> >

> > >> > > >> > -     install_bp_hardening_cb(cb, smccc_start, smccc_end);

> > >> > > >> > +     if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))

> > >> > > >> > +             install_bp_hardening_cb(cb, smccc_start, smccc_end);

> > >> > > >> >

> > >> > > >> >       return 1;

> > >> > > >> >  }

> > >> > > >> > -#endif       /* CONFIG_HARDEN_BRANCH_PREDICTOR */

> > >> > > >> >

> > >> > > >> >  DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);

> > >> > > >> >

> > >> > > >> > @@ -457,7 +456,6 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,

> > >> > > >> >       .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,                 \

> > >> > > >> >       CAP_MIDR_RANGE_LIST(midr_list)

> > >> > > >> >

> > >> > > >> > -#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR

> > >> > > >> >  /*

> > >> > > >> >   * List of CPUs that do not need any Spectre-v2 mitigation at all.

> > >> > > >> >   */

> > >> > > >> > @@ -489,6 +487,12 @@ check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)

> > >> > > >> >       if (!need_wa)

> > >> > > >> >               return false;

> > >> > > >> >

> > >> > > >> > +     if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {

> > >> > > >> > +             pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");

> > >> > > >> > +             __hardenbp_enab = false;

> > >> > > >>

> > >> > > >> This breaks when building, because __hardenbp_enab is declared in the next patch:

> > >> > > >>

> > >> > > >> $ make -j32 defconfig && make -j32

> > >> > > >>

> > >> > > >> [..]

> > >> > > >> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

> > >> > > >> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

> > >> > > >> use in this function)

> > >> > > >>    __hardenbp_enab = false;

> > >> > > >>    ^~~~~~~~~~~~~~~

> > >> > > >> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

> > >> > > >> only once for each function it appears in

> > >> > > >> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

> > >> > > >> make[1]: *** Waiting for unfinished jobs....

> > >> > > >>

> > >> > > >

> > >> > > >Indeed, but as discussed, this matches the state of both mainline and

> > >> > > >v4.19, which carry these patches in the same [wrong] order as well.

> > >> > > >

> > >> > > >Greg should confirm, but as I understand it, it is preferred to be

> > >> > > >bug-compatible with mainline rather than fixing problems when spotting

> > >> > > >them while doing the backport.

> > >> > >

> > >> > > Is it just patch ordering? If so I'd rather fix it, there's no reason to

> > >> > > carry this issue into the stable trees.

> > >> > >

> > >> > > We reserve "bug compatibility" for functional issues that are not yet

> > >> > > fixed upstream, it doesn't seem to be the case here.

> > >> > >

> > >> >

> > >> > The patches don't apply cleanly in the opposite order.

> > >>

> > >> What we could do is squash the two patches together. That way, we

> > >> avoid the breakage without having to modify the patches in order to be

> > >> able to apply them.

> > >

> > >No, don't do that.  Just take all of the needed commits.

> >

> > Right, just make the patches apply in reverse, this shouldn't be more

> > than moving some code from the 2nd patch back to the 1st one, right?

> >

> > We usually don't do this in stable backports, but there are three good

> > reasons to do it here:

> >

> > 1. It'll be nice to maintain bisectability.

> > 2. The end result should be exactly the same, so there's no room for

> > error here.

> > 3. It's a backport for an older kernel to begin with, so there are

> > changes from upstream already.

> >

> 

> I really don't see the point of doing this for v4.14 while v4.19 and

> mainline have the two patches in the opposite order.

> 

> Would you like me to resend the entire 48 piece series for this?


No need, I've queued the whole thing up now, thanks.

greg k-h
Ard Biesheuvel Oct. 27, 2019, 5:39 p.m. | #10
On Sun, 27 Oct 2019 at 14:39, Greg KH <greg@kroah.com> wrote:
>

> On Sat, Oct 26, 2019 at 05:46:03PM +0200, Ard Biesheuvel wrote:

> > On Sat, 26 Oct 2019 at 17:40, Sasha Levin <sashal@kernel.org> wrote:

> > >

> > > On Sat, Oct 26, 2019 at 10:01:21AM +0200, Greg KH wrote:

> > > >On Fri, Oct 25, 2019 at 05:39:44PM +0200, Ard Biesheuvel wrote:

> > > >> On Fri, 25 Oct 2019 at 17:28, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > > >> >

> > > >> > On Fri, 25 Oct 2019 at 17:25, Sasha Levin <sashal@kernel.org> wrote:

> > > >> > >

> > > >> > > On Thu, Oct 24, 2019 at 04:37:12PM +0200, Ard Biesheuvel wrote:

> > > >> > > >On Thu, 24 Oct 2019 at 16:34, Alexandru Elisei <alexandru.elisei@arm.com> wrote:

...
> > > >> > > >>

> > > >> > > >> This breaks when building, because __hardenbp_enab is declared in the next patch:

> > > >> > > >>

> > > >> > > >> $ make -j32 defconfig && make -j32

> > > >> > > >>

> > > >> > > >> [..]

> > > >> > > >> arch/arm64/kernel/cpu_errata.c: In function ‘check_branch_predictor’:

> > > >> > > >> arch/arm64/kernel/cpu_errata.c:492:3: error: ‘__hardenbp_enab’ undeclared (first

> > > >> > > >> use in this function)

> > > >> > > >>    __hardenbp_enab = false;

> > > >> > > >>    ^~~~~~~~~~~~~~~

> > > >> > > >> arch/arm64/kernel/cpu_errata.c:492:3: note: each undeclared identifier is reported

> > > >> > > >> only once for each function it appears in

> > > >> > > >> make[1]: *** [scripts/Makefile.build:326: arch/arm64/kernel/cpu_errata.o] Error 1

> > > >> > > >> make[1]: *** Waiting for unfinished jobs....

> > > >> > > >>

> > > >> > > >

> > > >> > > >Indeed, but as discussed, this matches the state of both mainline and

> > > >> > > >v4.19, which carry these patches in the same [wrong] order as well.

> > > >> > > >

> > > >> > > >Greg should confirm, but as I understand it, it is preferred to be

> > > >> > > >bug-compatible with mainline rather than fixing problems when spotting

> > > >> > > >them while doing the backport.

> > > >> > >

> > > >> > > Is it just patch ordering? If so I'd rather fix it, there's no reason to

> > > >> > > carry this issue into the stable trees.

> > > >> > >

> > > >> > > We reserve "bug compatibility" for functional issues that are not yet

> > > >> > > fixed upstream, it doesn't seem to be the case here.

> > > >> > >

> > > >> >

> > > >> > The patches don't apply cleanly in the opposite order.

> > > >>

> > > >> What we could do is squash the two patches together. That way, we

> > > >> avoid the breakage without having to modify the patches in order to be

> > > >> able to apply them.

> > > >

> > > >No, don't do that.  Just take all of the needed commits.

> > >

> > > Right, just make the patches apply in reverse, this shouldn't be more

> > > than moving some code from the 2nd patch back to the 1st one, right?

> > >

> > > We usually don't do this in stable backports, but there are three good

> > > reasons to do it here:

> > >

> > > 1. It'll be nice to maintain bisectability.

> > > 2. The end result should be exactly the same, so there's no room for

> > > error here.

> > > 3. It's a backport for an older kernel to begin with, so there are

> > > changes from upstream already.

> > >

> >

> > I really don't see the point of doing this for v4.14 while v4.19 and

> > mainline have the two patches in the opposite order.

> >

> > Would you like me to resend the entire 48 piece series for this?

>

> No need, I've queued the whole thing up now, thanks.

>


Thanks Greg

Patch

diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index bf6d8aa9b45a..647c533cfd90 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -76,7 +76,6 @@  cpu_enable_trap_ctr_access(const struct arm64_cpu_capabilities *__unused)
 	config_sctlr_el1(SCTLR_EL1_UCT, 0);
 }
 
-#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
 #include <asm/mmu_context.h>
 #include <asm/cacheflush.h>
 
@@ -217,11 +216,11 @@  static int detect_harden_bp_fw(void)
 	    ((midr & MIDR_CPU_MODEL_MASK) == MIDR_QCOM_FALKOR_V1))
 		cb = qcom_link_stack_sanitization;
 
-	install_bp_hardening_cb(cb, smccc_start, smccc_end);
+	if (IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR))
+		install_bp_hardening_cb(cb, smccc_start, smccc_end);
 
 	return 1;
 }
-#endif	/* CONFIG_HARDEN_BRANCH_PREDICTOR */
 
 DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);
 
@@ -457,7 +456,6 @@  static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,
 	.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,			\
 	CAP_MIDR_RANGE_LIST(midr_list)
 
-#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
 /*
  * List of CPUs that do not need any Spectre-v2 mitigation at all.
  */
@@ -489,6 +487,12 @@  check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)
 	if (!need_wa)
 		return false;
 
+	if (!IS_ENABLED(CONFIG_HARDEN_BRANCH_PREDICTOR)) {
+		pr_warn_once("spectrev2 mitigation disabled by kernel configuration\n");
+		__hardenbp_enab = false;
+		return false;
+	}
+
 	/* forced off */
 	if (__nospectre_v2) {
 		pr_info_once("spectrev2 mitigation disabled by command line option\n");
@@ -500,7 +504,6 @@  check_branch_predictor(const struct arm64_cpu_capabilities *entry, int scope)
 
 	return (need_wa > 0);
 }
-#endif
 
 const struct arm64_cpu_capabilities arm64_errata[] = {
 #if	defined(CONFIG_ARM64_ERRATUM_826319) || \
@@ -640,13 +643,11 @@  const struct arm64_cpu_capabilities arm64_errata[] = {
 		ERRATA_MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),
 	},
 #endif
-#ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
 	{
 		.capability = ARM64_HARDEN_BRANCH_PREDICTOR,
 		.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,
 		.matches = check_branch_predictor,
 	},
-#endif
 	{
 		.desc = "Speculative Store Bypass Disable",
 		.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,