[PATCH -tip/master] x86: io-apic - interrupt remapping fix

[PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Cyrill Gorcunov]

Interrupt remapping could lead to NULL dereference in case of
kzalloc failed and memory leak in other way. So fix the
both cases.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Ingo, the patch is on top of applied one.

If I remember correctly Suresh was involved in
this - so I think he could take a look and review
the patch (please).

LKML list restored in CC.

Index: linux-2.6.git/arch/x86/kernel/apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic.c	2008-09-19 12:12:54.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/apic.c	2008-09-19 13:57:04.000000000 +0400
@@ -1360,7 +1360,12 @@ void enable_IR_x2apic(void)
 
 	local_irq_save(flags);
 	mask_8259A();
-	save_mask_IO_APIC_setup();
+
+	ret = save_mask_IO_APIC_setup();
+	if (ret) {
+		printk(KERN_INFO "Saving IO-APIC state failed: %d\n", ret);
+		goto end;
+	}
 
 	ret = enable_intr_remapping(1);
 
@@ -1370,14 +1375,15 @@ void enable_IR_x2apic(void)
 	}
 
 	if (ret)
-		goto end;
+		goto end_restore;
 
 	if (!x2apic) {
 		x2apic = 1;
 		apic_ops = &x2apic_ops;
 		enable_x2apic();
 	}
-end:
+
+end_restore:
 	if (ret)
 		/*
 		 * IR enabling failed
@@ -1386,6 +1392,7 @@ end:
 	else
 		reinit_intr_remapped_IO_APIC(x2apic_preenabled);
 
+end:
 	unmask_8259A();
 	local_irq_restore(flags);
 
Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-19 12:12:54.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-19 13:33:01.000000000 +0400
@@ -831,7 +831,7 @@ int save_mask_IO_APIC_setup(void)
 			kzalloc(sizeof(struct IO_APIC_route_entry) *
 				nr_ioapic_registers[apic], GFP_KERNEL);
 		if (!early_ioapic_entries[apic])
-			return -ENOMEM;
+			goto nomem;
 	}
 
 	for (apic = 0; apic < nr_ioapics; apic++)
@@ -845,7 +845,17 @@ int save_mask_IO_APIC_setup(void)
 				ioapic_write_entry(apic, pin, entry);
 			}
 		}
+
 	return 0;
+
+nomem:
+	for (; apic > 0; apic--)
+		kfree(early_ioapic_entries[apic]);
+	kfree(early_ioapic_entries[apic]);
+	memset(early_ioapic_entries, 0,
+		ARRAY_SIZE(early_ioapic_entries));
+
+	return -ENOMEM;
 }
 
 void restore_IO_APIC_setup(void)
@@ -853,12 +863,13 @@ void restore_IO_APIC_setup(void)
 	int apic, pin;
 
 	for (apic = 0; apic < nr_ioapics; apic++) {
-		/* FIXME: it should be handled at allocation time --cvg */
 		if (!early_ioapic_entries[apic])
 			break;
 		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++)
 			ioapic_write_entry(apic, pin,
 					   early_ioapic_entries[apic][pin]);
+		kfree(early_ioapic_entries[apic]);
+		early_ioapic_entries[apic] = NULL;
 	}
 }
 

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Interrupt remapping could lead to NULL dereference in case of kzalloc 
> failed and memory leak in other way. So fix the both cases.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> 
> Ingo, the patch is on top of applied one.
> 
> If I remember correctly Suresh was involved in
> this - so I think he could take a look and review
> the patch (please).

applied the combined patch below to tip/irq/sparseirq, thanks Cyrill.

	Ingo

----------------->
>From 77322deb4bc676a5ee645444e7ed1a89f854473d Mon Sep 17 00:00:00 2001
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Thu, 18 Sep 2008 23:37:57 +0400
Subject: [PATCH] x86: io-apic - interrupt remapping fix

Interrupt remapping could lead to NULL dereference in case of
kzalloc failed and memory leak in other way. So fix the
both cases.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: "Maciej W. Rozycki" <macro@linux-mips.org>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/apic.c    |   13 ++++++++++---
 arch/x86/kernel/io_apic.c |   19 +++++++++++++++++--
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic.c b/arch/x86/kernel/apic.c
index 7841169..1f48bd1 100644
--- a/arch/x86/kernel/apic.c
+++ b/arch/x86/kernel/apic.c
@@ -1360,7 +1360,12 @@ void enable_IR_x2apic(void)
 
 	local_irq_save(flags);
 	mask_8259A();
-	save_mask_IO_APIC_setup();
+
+	ret = save_mask_IO_APIC_setup();
+	if (ret) {
+		printk(KERN_INFO "Saving IO-APIC state failed: %d\n", ret);
+		goto end;
+	}
 
 	ret = enable_intr_remapping(1);
 
@@ -1370,14 +1375,15 @@ void enable_IR_x2apic(void)
 	}
 
 	if (ret)
-		goto end;
+		goto end_restore;
 
 	if (!x2apic) {
 		x2apic = 1;
 		apic_ops = &x2apic_ops;
 		enable_x2apic();
 	}
-end:
+
+end_restore:
 	if (ret)
 		/*
 		 * IR enabling failed
@@ -1386,6 +1392,7 @@ end:
 	else
 		reinit_intr_remapped_IO_APIC(x2apic_preenabled);
 
+end:
 	unmask_8259A();
 	local_irq_restore(flags);
 
diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
index 5279b50..cb53a49 100644
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -810,7 +810,7 @@ int save_mask_IO_APIC_setup(void)
 			kzalloc(sizeof(struct IO_APIC_route_entry) *
 				nr_ioapic_registers[apic], GFP_KERNEL);
 		if (!early_ioapic_entries[apic])
-			return -ENOMEM;
+			goto nomem;
 	}
 
 	for (apic = 0; apic < nr_ioapics; apic++)
@@ -824,17 +824,32 @@ int save_mask_IO_APIC_setup(void)
 				ioapic_write_entry(apic, pin, entry);
 			}
 		}
+
 	return 0;
+
+nomem:
+	for (; apic > 0; apic--)
+		kfree(early_ioapic_entries[apic]);
+	kfree(early_ioapic_entries[apic]);
+	memset(early_ioapic_entries, 0,
+		ARRAY_SIZE(early_ioapic_entries));
+
+	return -ENOMEM;
 }
 
 void restore_IO_APIC_setup(void)
 {
 	int apic, pin;
 
-	for (apic = 0; apic < nr_ioapics; apic++)
+	for (apic = 0; apic < nr_ioapics; apic++) {
+		if (!early_ioapic_entries[apic])
+			break;
 		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++)
 			ioapic_write_entry(apic, pin,
 					   early_ioapic_entries[apic][pin]);
+		kfree(early_ioapic_entries[apic]);
+		early_ioapic_entries[apic] = NULL;
+	}
 }
 
 void reinit_intr_remapped_IO_APIC(int intr_remapping)

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Yinghai Lu]

On Mon, Sep 22, 2008 at 2:04 AM, Ingo Molnar <mingo@elte.hu> wrote:

BTW

x2apic and IRmapping make io_apic.c and apic.c ugly.

YH

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> On Mon, Sep 22, 2008 at 2:04 AM, Ingo Molnar <mingo@elte.hu> wrote:
> 
> BTW
> 
> x2apic and IRmapping make io_apic.c and apic.c ugly.

yes. Any ideas how to clean it up some more?

the kmalloc looks quite ugly, especially with this kfree() teardown 
sequence we have now.

	Ingo

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Cyrill Gorcunov]

[Ingo Molnar - Mon, Sep 22, 2008 at 12:05:32PM +0200]
| 
| * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
| 
| > On Mon, Sep 22, 2008 at 2:04 AM, Ingo Molnar <mingo@elte.hu> wrote:
| > 
| > BTW
| > 
| > x2apic and IRmapping make io_apic.c and apic.c ugly.
| 
| yes. Any ideas how to clean it up some more?
| 
| the kmalloc looks quite ugly, especially with this kfree() teardown 
| sequence we have now.
| 
| 	Ingo
| 

yes Ingo - it's ugly but I had not that many choises:

1) allocate/deallocate if failed inside same routine
2) deallocate in restore_IO_APIC_setup (which would be much more
   ugly and obscure)
3) allocate and deallocate in completely separated routines - didn't
   even tried :)

So I've stopped on (1) 'case it was less_changing_the_sources patch.   

		- Cyrill -

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Suresh Siddha]

On Fri, Sep 19, 2008 at 05:33:20AM -0700, Cyrill Gorcunov wrote:
> Interrupt remapping could lead to NULL dereference in case of
> kzalloc failed and memory leak in other way. So fix the
> both cases.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> 
> Ingo, the patch is on top of applied one.
> 
> If I remember correctly Suresh was involved in
> this - so I think he could take a look and review
> the patch (please).
> 

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Suresh Siddha]

On Mon, Sep 22, 2008 at 05:57:38PM -0700, Siddha, Suresh B wrote:
> On Fri, Sep 19, 2008 at 05:33:20AM -0700, Cyrill Gorcunov wrote:
> > Interrupt remapping could lead to NULL dereference in case of
> > kzalloc failed and memory leak in other way. So fix the
> > both cases.
> >
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> >
> > Ingo, the patch is on top of applied one.
> >
> > If I remember correctly Suresh was involved in
> > this - so I think he could take a look and review
> > the patch (please).
> >
> 
> Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

oops. Cyrill some typo here:

+       for (; apic > 0; apic--)
+               kfree(early_ioapic_entries[apic]);
+       kfree(early_ioapic_entries[apic]);

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Suresh Siddha]

On Mon, Sep 22, 2008 at 03:37:36AM -0700, Cyrill Gorcunov wrote:
> [Ingo Molnar - Mon, Sep 22, 2008 at 12:05:32PM +0200]
> |
> | * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> |
> | > On Mon, Sep 22, 2008 at 2:04 AM, Ingo Molnar <mingo@elte.hu> wrote:
> | >
> | > BTW
> | >
> | > x2apic and IRmapping make io_apic.c and apic.c ugly.
> |
> | yes. Any ideas how to clean it up some more?
> |
> | the kmalloc looks quite ugly, especially with this kfree() teardown
> | sequence we have now.
> |
> |       Ingo
> |
> 
> yes Ingo - it's ugly but I had not that many choises:
> 
> 1) allocate/deallocate if failed inside same routine
> 2) deallocate in restore_IO_APIC_setup (which would be much more
>    ugly and obscure)
> 3) allocate and deallocate in completely separated routines - didn't
>    even tried :)
> 
> So I've stopped on (1) 'case it was less_changing_the_sources patch.

I think Yinghai is complaing about other code aswell (HAVE_X2APIC etc).
Probably I should move some of the x2apic/interrupt-remapping code
setup out of io_apic.c and apic.c to a common file.

thanks,
suresh

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Cyrill Gorcunov]

[Suresh Siddha - Mon, Sep 22, 2008 at 06:16:21PM -0700]
| On Mon, Sep 22, 2008 at 05:57:38PM -0700, Siddha, Suresh B wrote:
| > On Fri, Sep 19, 2008 at 05:33:20AM -0700, Cyrill Gorcunov wrote:
| > > Interrupt remapping could lead to NULL dereference in case of
| > > kzalloc failed and memory leak in other way. So fix the
| > > both cases.
| > >
| > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| > > ---
| > >
| > > Ingo, the patch is on top of applied one.
| > >
| > > If I remember correctly Suresh was involved in
| > > this - so I think he could take a look and review
| > > the patch (please).
| > >
| > 
| > Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
| 
| oops. Cyrill some typo here:
| 
| +       for (; apic > 0; apic--)
| +               kfree(early_ioapic_entries[apic]);
| +       kfree(early_ioapic_entries[apic]);
|

Hi Suresh, thanks for review!

Well it's not typo actually :) Of course it could
be like

	for (--apic; apic > 0; apic--)
or
	for (apic--; apic > 0; apic--)

but it will be a rpoblem in case if apic = 0 and
if someday apic would be unsigned int. So I prefered
to have _one_ kfree(NULL) call instead :)

I hope i didn't miss anything.

		- Cyrill -

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Suresh Siddha]

On Mon, Sep 22, 2008 at 09:56:37PM -0700, Cyrill Gorcunov wrote:
> [Suresh Siddha - Mon, Sep 22, 2008 at 06:16:21PM -0700]
> | On Mon, Sep 22, 2008 at 05:57:38PM -0700, Siddha, Suresh B wrote:
> | > On Fri, Sep 19, 2008 at 05:33:20AM -0700, Cyrill Gorcunov wrote:
> | > > Interrupt remapping could lead to NULL dereference in case of
> | > > kzalloc failed and memory leak in other way. So fix the
> | > > both cases.
> | > >
> | > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> | > > ---
> | > >
> | > > Ingo, the patch is on top of applied one.
> | > >
> | > > If I remember correctly Suresh was involved in
> | > > this - so I think he could take a look and review
> | > > the patch (please).
> | > >
> | >
> | > Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
> |
> | oops. Cyrill some typo here:
> |
> | +       for (; apic > 0; apic--)
> | +               kfree(early_ioapic_entries[apic]);
> | +       kfree(early_ioapic_entries[apic]);
> |
> 
> Hi Suresh, thanks for review!
> 
> Well it's not typo actually :) Of course it could
> be like
> 
>         for (--apic; apic > 0; apic--)
> or
>         for (apic--; apic > 0; apic--)
> 
> but it will be a rpoblem in case if apic = 0 and
> if someday apic would be unsigned int. So I prefered
> to have _one_ kfree(NULL) call instead :)
> 
> I hope i didn't miss anything.

This is too confusing. Please change it to something simple, like:

	for (i = 0; i < apic; i++)
		kfree(early_ioapic_entries[i]);

or

	for (apic = 0; apic < nr_ioapics; apic++)
		kfree(early_ioapic_entries[apic]);

thanks,
suresh

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Cyrill Gorcunov]

[Suresh Siddha - Tue, Sep 23, 2008 at 11:34:12AM -0700]
| On Mon, Sep 22, 2008 at 09:56:37PM -0700, Cyrill Gorcunov wrote:
| > [Suresh Siddha - Mon, Sep 22, 2008 at 06:16:21PM -0700]
| > | On Mon, Sep 22, 2008 at 05:57:38PM -0700, Siddha, Suresh B wrote:
| > | > On Fri, Sep 19, 2008 at 05:33:20AM -0700, Cyrill Gorcunov wrote:
| > | > > Interrupt remapping could lead to NULL dereference in case of
| > | > > kzalloc failed and memory leak in other way. So fix the
| > | > > both cases.
| > | > >
| > | > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| > | > > ---
| > | > >
| > | > > Ingo, the patch is on top of applied one.
| > | > >
| > | > > If I remember correctly Suresh was involved in
| > | > > this - so I think he could take a look and review
| > | > > the patch (please).
| > | > >
| > | >
| > | > Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
| > |
| > | oops. Cyrill some typo here:
| > |
| > | +       for (; apic > 0; apic--)
| > | +               kfree(early_ioapic_entries[apic]);
| > | +       kfree(early_ioapic_entries[apic]);
| > |
| > 
| > Hi Suresh, thanks for review!
| > 
| > Well it's not typo actually :) Of course it could
| > be like
| > 
| >         for (--apic; apic > 0; apic--)
| > or
| >         for (apic--; apic > 0; apic--)
| > 
| > but it will be a rpoblem in case if apic = 0 and
| > if someday apic would be unsigned int. So I prefered
| > to have _one_ kfree(NULL) call instead :)
| > 
| > I hope i didn't miss anything.
| 
| This is too confusing. Please change it to something simple, like:
| 
| 	for (i = 0; i < apic; i++)
| 		kfree(early_ioapic_entries[i]);
| 
| or
| 
| 	for (apic = 0; apic < nr_ioapics; apic++)
| 		kfree(early_ioapic_entries[apic]);
| 
| thanks,
| suresh
| 

ok Suresh - you choose :)

Since Ingo already has updated his tree - will
send delta patch.

		- Cyrill -

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Cyrill Gorcunov]

[Suresh Siddha - Tue, Sep 23, 2008 at 11:34:12AM -0700]
...
| 
| This is too confusing. Please change it to something simple, like:
| 
| 	for (i = 0; i < apic; i++)
| 		kfree(early_ioapic_entries[i]);
| 
| or
| 
| 	for (apic = 0; apic < nr_ioapics; apic++)
| 		kfree(early_ioapic_entries[apic]);
| 
| thanks,
| suresh
| 

Suresh,

what about this one?

		- Cyrill -
---
From: Cyrill Gorcunov <gorcunov@gmail.com>

Clean up obscure for() cycle with straight while() form

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
CC: Suresh Siddha <suresh.b.siddha@intel.com>
---

Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-22 17:42:33.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-23 22:45:58.000000000 +0400
@@ -830,9 +830,8 @@ int save_mask_IO_APIC_setup(void)
 	return 0;
 
 nomem:
-	for (; apic > 0; apic--)
-		kfree(early_ioapic_entries[apic]);
-	kfree(early_ioapic_entries[apic]);
+	while (apic >= 0)
+		kfree(early_ioapic_entries[apic--]);
 	memset(early_ioapic_entries, 0,
 		ARRAY_SIZE(early_ioapic_entries));
 

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Suresh Siddha]

On Tue, Sep 23, 2008 at 12:00:02PM -0700, Cyrill Gorcunov wrote:
> ---
> From: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> Clean up obscure for() cycle with straight while() form
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> CC: Suresh Siddha <suresh.b.siddha@intel.com>
> ---
> 
> Index: linux-2.6.git/arch/x86/kernel/io_apic.c
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/io_apic.c        2008-09-22 17:42:33.000000000 +0400
> +++ linux-2.6.git/arch/x86/kernel/io_apic.c     2008-09-23 22:45:58.000000000 +0400
> @@ -830,9 +830,8 @@ int save_mask_IO_APIC_setup(void)
>         return 0;
> 
>  nomem:
> -       for (; apic > 0; apic--)
> -               kfree(early_ioapic_entries[apic]);
> -       kfree(early_ioapic_entries[apic]);
> +       while (apic >= 0)
> +               kfree(early_ioapic_entries[apic--]);
>         memset(early_ioapic_entries, 0,
>                 ARRAY_SIZE(early_ioapic_entries));
> 

looks better. thanks.

Re: [PATCH -tip/master] x86: io-apic - interrupt remapping fix

[Ingo Molnar]

* Suresh Siddha <suresh.b.siddha@intel.com> wrote:

> > +       while (apic >= 0)
> > +               kfree(early_ioapic_entries[apic--]);
> >         memset(early_ioapic_entries, 0,
> >                 ARRAY_SIZE(early_ioapic_entries));
> > 
> 
> looks better. thanks.

applied to tip/irq/sparseirq - and added your Acked-by. Thanks guys!

	Ingo