Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570460 > unrolled thread

Re: [PATCH] x86/microcode: Do not access the initrd after it has been freed

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Ingo Molnar <mingo@kernel.org> - 2017-01-31 08:50 +0100
    Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Borislav Petkov <bp@alien8.de> - 2017-01-31 11:10 +0100
      Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Mike Galbraith <efault@gmx.de> - 2017-01-31 12:40 +0100
        Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Borislav Petkov <bp@alien8.de> - 2017-01-31 13:50 +0100
          Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Borislav Petkov <bp@alien8.de> - 2017-01-31 18:50 +0100
            Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Mike Galbraith <efault@gmx.de> - 2017-01-31 19:10 +0100
        Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Thomas Gleixner <tglx@linutronix.de> - 2017-01-31 19:10 +0100
          [tip:irq/urgent] x86/irq: Make irq activate operations symmetric tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-01-31 20:30 +0100
      Re: [PATCH] x86/microcode: Do not access the initrd after it has  been freed Ingo Molnar <mingo@kernel.org> - 2017-01-31 17:50 +0100

#1570460 — Re: [PATCH] x86/microcode: Do not access the initrd after it has been freed

(Cc:-ed Mike as this could explain his early boot crash/hang?
        Mike: please try -tip f18a8a0143b1 that I just pushed out. )

* Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Jan 30, 2017 at 09:46:32AM +0100, Ingo Molnar wrote:
> > Ok, I have applied this to tip:x86/urgent.
> > 
> > Note that there are new conflicts with your pending work in tip:x86/microcode, and 
> > I fixed them up in:
> > 
> >   7c5b4112040e Merge branch 'x86/urgent' into x86/microcode, to resolve conflicts
> > 
> > Could you please double-check my conflict resolution?
> 
> Almost, this part is wrong:
> 
> --------------------- arch/x86/kernel/cpu/microcode/amd.c ---------------------
> index 7889ae492af0,079e81733a58..73082365ed1c
> @@@ -268,20 -316,43 +268,20 @@@ void __load_ucode_amd(unsigned int cpui
>   		use_pa	= false;
>   	}
>   
> - 	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
>  -	if (!get_builtin_microcode(&cp, family))
> ++	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)) && !initrd_gone)
>   		cp = find_microcode_in_initrd(path, use_pa);
> 
> --
> 
> Btw, I did experiment with the merging because I knew it'll cause
> trouble due to the urgent fix and here's what I did:
> 
> You're merging tip/x86/urgent into tip/x86/microcode so I checked out
> the microcode branch and did:
> 
> $ git checkout -b tip-microcode tip/x86/microcode
> $ git merge -s recursive -X ours tip/x86/urgent
> 
> This way I'm favouring our changes in the conflicting files. It merges
> cleanly and the resulting diff is below.

Nice - I've updated the branch with your resolution. Could you please 
double-check the double checked resolution?

> The logic behind it is is that tip/x86/microcode does away with a bunch
> of code and the urgent change touches some of that code but that's only
> for 4.10.
> 
> It goes away in 4.11 and that's why we should prefer "ours" as the merge
> option.
> 
> 	 [ Btw, I'll send a patch for 4.11 later to make initrd_gone static as
> 	   it is going to be used only in microcode/core.c after the cleanup. ]
> 
> However, I still haven't figured out how to say "prefer ours but only
> for specific files or subtree" because the diff has that hunk in
> arch/x86/kernel/fpu/core.c too which should definitely not be "ours" as
> it is a fix and there the urgent version should be the one going in.
> 
> Hmmm.

So the diff between your resolution and mine is attached below - now fpu/core.c 
changes, so I'm not sure why fpu/core.c is in your diff?

Thanks,

	Ingo

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 73082365ed1c..7889ae492af0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -268,7 +268,7 @@ void __load_ucode_amd(unsigned int cpuid_1_eax, struct cpio_data *ret)
 		use_pa	= false;
 	}
 
-	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)) && !initrd_gone)
+	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
 		cp = find_microcode_in_initrd(path, use_pa);
 
 	/* Needed in load_microcode_amd() */
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index e51eeaed8016..b4a4cd39b358 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -230,7 +230,7 @@ static int __init save_microcode_in_initrd(void)
 		break;
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0x10)
-			ret = save_microcode_in_initrd_amd(cpuid_eax(1));
+			return save_microcode_in_initrd_amd(cpuid_eax(1));
 		break;
 	default:
 		break;

[toc] | [next] | [standalone]

#1570549

On Tue, Jan 31, 2017 at 08:43:55AM +0100, Ingo Molnar wrote:
> (Cc:-ed Mike as this could explain his early boot crash/hang?
>         Mike: please try -tip f18a8a0143b1 that I just pushed out. )

One other thing to try, Mike, is boot with "dis_ucode_ldr". See whether
that makes it go away.

> Nice - I've updated the branch with your resolution. Could you please 
> double-check the double checked resolution?

That's like quadruple-checked :-)

> So the diff between your resolution and mine is attached below - now fpu/core.c 
> changes, so I'm not sure why fpu/core.c is in your diff?

Hmm, maybe I did the diff wrong?

So this is how I generated it:

$ git diff tip/x86/microcode~1..tip/x86/microcode

meaning that I want to see what the merge commit changed. That's why
this pulled in the fpu/core.c hunk which is new in urgent.

In any case, the merge looks ok to me, I'll be testing it anyway in the
coming days so I'll let you know if anything goes nuts.

Thanks!

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1570621

On Tue, 2017-01-31 at 11:01 +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2017 at 08:43:55AM +0100, Ingo Molnar wrote:
> > (Cc:-ed Mike as this could explain his early boot crash/hang?
> >         Mike: please try -tip f18a8a0143b1 that I just pushed out. )
> 
> One other thing to try, Mike, is boot with "dis_ucode_ldr". See whether
> that makes it go away.

(bisect fingered irqdomain: Avoid activating interrupts more than once)

[toc] | [prev] | [next] | [standalone]

#1570695

On Tue, Jan 31, 2017 at 12:31:17PM +0100, Mike Galbraith wrote:
> (bisect fingered irqdomain: Avoid activating interrupts more than once)

Yeah, that one is not kosher on x86. It broke IO-APIC timer on a box
here.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1570940

On Tue, Jan 31, 2017 at 01:31:00PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2017 at 12:31:17PM +0100, Mike Galbraith wrote:
> > (bisect fingered irqdomain: Avoid activating interrupts more than once)
> 
> Yeah, that one is not kosher on x86. It broke IO-APIC timer on a box
> here.

Mike,

does the below hunk fix the issue for ya? (Ontop of tip/master, without
the revert).

It does fix my APIC timer detection failure.

---
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 1e35dd06b090..52f352b063fd 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2117,6 +2117,7 @@ static inline void __init check_timer(void)
 			if (idx != -1 && irq_trigger(idx))
 				unmask_ioapic_irq(irq_get_chip_data(0));
 		}
+		irq_domain_deactivate_irq(irq_data);
 		irq_domain_activate_irq(irq_data);
 		if (timer_irq_works()) {
 			if (disable_timer_pin_1 > 0)
@@ -2138,6 +2139,7 @@ static inline void __init check_timer(void)
 		 * legacy devices should be connected to IO APIC #0
 		 */
 		replace_pin_at_irq_node(data, node, apic1, pin1, apic2, pin2);
+		irq_domain_deactivate_irq(irq_data);
 		irq_domain_activate_irq(irq_data);
 		legacy_pic->unmask(0);
 		if (timer_irq_works()) {
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 85e87b46c318..dc6ba5bda9fc 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -352,6 +352,7 @@ static int hpet_resume(struct clock_event_device *evt, int timer)
 	} else {
 		struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt);
 
+		irq_domain_deactivate_irq(irq_get_irq_data(hdev->irq));
 		irq_domain_activate_irq(irq_get_irq_data(hdev->irq));
 		disable_irq(hdev->irq);
 		irq_set_affinity(hdev->irq, cpumask_of(hdev->cpu));

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1570960

On Tue, 2017-01-31 at 18:49 +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2017 at 01:31:00PM +0100, Borislav Petkov wrote:
> > On Tue, Jan 31, 2017 at 12:31:17PM +0100, Mike Galbraith wrote:
> > > (bisect fingered irqdomain: Avoid activating interrupts more than once)
> > 
> > Yeah, that one is not kosher on x86. It broke IO-APIC timer on a box
> > here.
> 
> Mike,
> 
> does the below hunk fix the issue for ya? (Ontop of tip/master, without
> the revert).
> 
> It does fix my APIC timer detection failure.

Yup, need a new doorstop.

	-Mike

[toc] | [prev] | [next] | [standalone]

#1570947

On Tue, 31 Jan 2017, Mike Galbraith wrote:

> On Tue, 2017-01-31 at 11:01 +0100, Borislav Petkov wrote:
> > On Tue, Jan 31, 2017 at 08:43:55AM +0100, Ingo Molnar wrote:
> > > (Cc:-ed Mike as this could explain his early boot crash/hang?
> > >         Mike: please try -tip f18a8a0143b1 that I just pushed out. )
> > 
> > One other thing to try, Mike, is boot with "dis_ucode_ldr". See whether
> > that makes it go away.
> 
> (bisect fingered irqdomain: Avoid activating interrupts more than once)

That commit exposed interesting code in x86 land. Can you please try the
patch below?

Thanks,

	tglx
8<--------------

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 1e35dd06b090..52f352b063fd 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2117,6 +2117,7 @@ static inline void __init check_timer(void)
 			if (idx != -1 && irq_trigger(idx))
 				unmask_ioapic_irq(irq_get_chip_data(0));
 		}
+		irq_domain_deactivate_irq(irq_data);
 		irq_domain_activate_irq(irq_data);
 		if (timer_irq_works()) {
 			if (disable_timer_pin_1 > 0)
@@ -2138,6 +2139,7 @@ static inline void __init check_timer(void)
 		 * legacy devices should be connected to IO APIC #0
 		 */
 		replace_pin_at_irq_node(data, node, apic1, pin1, apic2, pin2);
+		irq_domain_deactivate_irq(irq_data);
 		irq_domain_activate_irq(irq_data);
 		legacy_pic->unmask(0);
 		if (timer_irq_works()) {
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 85e87b46c318..dc6ba5bda9fc 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -352,6 +352,7 @@ static int hpet_resume(struct clock_event_device *evt, int timer)
 	} else {
 		struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt);
 
+		irq_domain_deactivate_irq(irq_get_irq_data(hdev->irq));
 		irq_domain_activate_irq(irq_get_irq_data(hdev->irq));
 		disable_irq(hdev->irq);
 		irq_set_affinity(hdev->irq, cpumask_of(hdev->cpu));

[toc] | [prev] | [next] | [standalone]

#1571028 — [tip:irq/urgent] x86/irq: Make irq activate operations symmetric

Commit-ID:  aaaec6fc755447a1d056765b11b24d8ff2b81366
Gitweb:     http://git.kernel.org/tip/aaaec6fc755447a1d056765b11b24d8ff2b81366
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 31 Jan 2017 19:03:21 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 31 Jan 2017 20:22:18 +0100

x86/irq: Make irq activate operations symmetric

The recent commit which prevents double activation of interrupts unearthed
interesting code in x86. The code (ab)uses irq_domain_activate_irq() to
reconfigure an already activated interrupt. That trips over the prevention
code now.

Fix it by deactivating the interrupt before activating the new configuration.

Fixes: 08d85f3ea99f1 "irqdomain: Avoid activating interrupts more than once"
Reported-and-tested-by: Mike Galbraith <efault@gmx.de>
Reported-and-tested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1701311901580.3457@nanos

---
 arch/x86/kernel/apic/io_apic.c | 2 ++
 arch/x86/kernel/hpet.c         | 1 +
 2 files changed, 3 insertions(+)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 1e35dd0..52f352b 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2117,6 +2117,7 @@ static inline void __init check_timer(void)
 			if (idx != -1 && irq_trigger(idx))
 				unmask_ioapic_irq(irq_get_chip_data(0));
 		}
+		irq_domain_deactivate_irq(irq_data);
 		irq_domain_activate_irq(irq_data);
 		if (timer_irq_works()) {
 			if (disable_timer_pin_1 > 0)
@@ -2138,6 +2139,7 @@ static inline void __init check_timer(void)
 		 * legacy devices should be connected to IO APIC #0
 		 */
 		replace_pin_at_irq_node(data, node, apic1, pin1, apic2, pin2);
+		irq_domain_deactivate_irq(irq_data);
 		irq_domain_activate_irq(irq_data);
 		legacy_pic->unmask(0);
 		if (timer_irq_works()) {
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 85e87b4..dc6ba5b 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -352,6 +352,7 @@ static int hpet_resume(struct clock_event_device *evt, int timer)
 	} else {
 		struct hpet_dev *hdev = EVT_TO_HPET_DEV(evt);
 
+		irq_domain_deactivate_irq(irq_get_irq_data(hdev->irq));
 		irq_domain_activate_irq(irq_get_irq_data(hdev->irq));
 		disable_irq(hdev->irq);
 		irq_set_affinity(hdev->irq, cpumask_of(hdev->cpu));

[toc] | [prev] | [next] | [standalone]

#1570889

* Borislav Petkov <bp@alien8.de> wrote:

> On Tue, Jan 31, 2017 at 08:43:55AM +0100, Ingo Molnar wrote:
> > (Cc:-ed Mike as this could explain his early boot crash/hang?
> >         Mike: please try -tip f18a8a0143b1 that I just pushed out. )
> 
> One other thing to try, Mike, is boot with "dis_ucode_ldr". See whether
> that makes it go away.
> 
> > Nice - I've updated the branch with your resolution. Could you please 
> > double-check the double checked resolution?
> 
> That's like quadruple-checked :-)

Yeah, so when I wrote that I was pondering whether that counts as triple or 
quadruple checked. I couldn't make up my mind ;-)

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web