[PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[PJ Waskiewicz]

This bug was introduced in the 2.6.24 i386/x86_64 tree merge, where
MSI-X vector allocation will eventually fail.  The cause is the new
bit array tracking used vectors is not getting cleared properly on
IRQ destruction on the 32-bit APIC code.

This can be seen easily using the ixgbe 10 GbE driver on multi-core
systems by simply loading and unloading the driver a few times.
Depending on the number of available vectors on the host system, the
MSI-X allocation will eventually fail, and the driver will only be
able to use legacy interrupts.

I am generating the same patch for both stable trees for 2.6.24 and
2.6.25.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
---

 arch/x86/kernel/io_apic_32.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)


diff --git a/arch/x86/kernel/io_apic_32.c b/arch/x86/kernel/io_apic_32.c
index 2e2f420..77798b1 100644
--- a/arch/x86/kernel/io_apic_32.c
+++ b/arch/x86/kernel/io_apic_32.c
@@ -2444,6 +2444,7 @@ void destroy_irq(unsigned int irq)
 	dynamic_irq_cleanup(irq);
 
 	spin_lock_irqsave(&vector_lock, flags);
+	clear_bit(irq_vector[irq], used_vectors);
 	irq_vector[irq] = 0;
 	spin_unlock_irqrestore(&vector_lock, flags);
 }

Re: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Eric W. Biederman]

"Brandeburg, Jesse" <jesse.brandeburg@intel.com>, <linux-pci@atrey.karlin.mff.cuni.cz>,  "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>,  <michael@ellerman.id.au>, Rusty Russell <rusty@rustcorp.com.au>, Andi Kleen <ak@suse.de>, Ingo Molnar <mingo@elte.hu>, Thomas Gleixner <tglx@linutronix.de>
Date: Mon, 28 Apr 2008 12:09:57 -0700
In-Reply-To: <20080426005850.7098.77479.stgit@scrappy.jf.intel.com> (PJ
	Waskiewicz's message of "Fri, 25 Apr 2008 17:58:52 -0700")
Message-ID: <m1skx53iey.fsf@frodo.ebiederm.org>
User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii


Thanks Jesse for forwarding this to me.

PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com> writes:

> This bug was introduced in the 2.6.24 i386/x86_64 tree merge, where
> MSI-X vector allocation will eventually fail.  The cause is the new
> bit array tracking used vectors is not getting cleared properly on
> IRQ destruction on the 32-bit APIC code.

In particular commit dbeb2be21d678c49a8d8bbf774903df15dd55474
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Fri Oct 19 20:35:03 2007 +0200

    i386: introduce "used_vectors" bitmap which can be used to reserve vectors.
    
    This simplifies the io_apic.c __assign_irq_vector() logic and removes
    the explicit SYSCALL_VECTOR check, and also allows for vectors to be
    reserved by other mechanisms (ie. lguest).
    
    [ tglx: arch/x86 adaptation ]
    
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
    Signed-off-by: Andi Kleen <ak@suse.de>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

> This can be seen easily using the ixgbe 10 GbE driver on multi-core
> systems by simply loading and unloading the driver a few times.
> Depending on the number of available vectors on the host system, the
> MSI-X allocation will eventually fail, and the driver will only be
> able to use legacy interrupts.
>
> I am generating the same patch for both stable trees for 2.6.24 and
> 2.6.25.

The patch below does looks ok in terms of fixing the issue with
respect to MSIs.

Unfortunately the entire used_vectors concept appears broken.  Rusty
is not taking vector_lock.  used_vectors as designed can not
work on x86_64 as vectors are allocated to interrupts in a per cpu
manner, resulting in unnecessary divergences between  
the two pieces of code.  used_vectors is an an extra data structure
that we have to keep in sync, and failing to keep that data
structure in sync is why we have a problem.

For the concept of allocating a non 0x80 vector for interrupts Rusty
was on the right track.  His implementation just appears to be 
broken in the corner cases, and has a design we can not use long
term.

To fix this my inclination is to revert:
dbeb2be21d678c49a8d8bbf774903df15dd55474 (the infracture) and
c18acd73ffc209def08003a1927473096f66c5ad (the lguest user)
as we need to rework those pieces of code anyway, and then work with
Rusty to come up with something that we can share on x86 and x86_64.

Rusty?

Eric


> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> ---
>
>  arch/x86/kernel/io_apic_32.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
>
> diff --git a/arch/x86/kernel/io_apic_32.c b/arch/x86/kernel/io_apic_32.c
> index 2e2f420..77798b1 100644
> --- a/arch/x86/kernel/io_apic_32.c
> +++ b/arch/x86/kernel/io_apic_32.c
> @@ -2444,6 +2444,7 @@ void destroy_irq(unsigned int irq)
>  	dynamic_irq_cleanup(irq);
>  
>  	spin_lock_irqsave(&vector_lock, flags);
> +	clear_bit(irq_vector[irq], used_vectors);
>  	irq_vector[irq] = 0;
>  	spin_unlock_irqrestore(&vector_lock, flags);
>  }
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Thomas Gleixner]

On Fri, 25 Apr 2008, PJ Waskiewicz wrote:
> This bug was introduced in the 2.6.24 i386/x86_64 tree merge, where

Can you please explain what exactly caused the bug. Definitely not the
move from arch/i386 to arch/x86 as the code there was not changed at
all and has not be changed since then.

CC'ed Eric as well.

Thanks
	tglx


> MSI-X vector allocation will eventually fail.  The cause is the new
> bit array tracking used vectors is not getting cleared properly on
> IRQ destruction on the 32-bit APIC code.
> 
> This can be seen easily using the ixgbe 10 GbE driver on multi-core
> systems by simply loading and unloading the driver a few times.
> Depending on the number of available vectors on the host system, the
> MSI-X allocation will eventually fail, and the driver will only be
> able to use legacy interrupts.
> 
> I am generating the same patch for both stable trees for 2.6.24 and
> 2.6.25.
> 
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> ---
> 
>  arch/x86/kernel/io_apic_32.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/arch/x86/kernel/io_apic_32.c b/arch/x86/kernel/io_apic_32.c
> index 2e2f420..77798b1 100644
> --- a/arch/x86/kernel/io_apic_32.c
> +++ b/arch/x86/kernel/io_apic_32.c
> @@ -2444,6 +2444,7 @@ void destroy_irq(unsigned int irq)
>  	dynamic_irq_cleanup(irq);
>  
>  	spin_lock_irqsave(&vector_lock, flags);
> +	clear_bit(irq_vector[irq], used_vectors);
>  	irq_vector[irq] = 0;
>  	spin_unlock_irqrestore(&vector_lock, flags);
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

RE: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Waskiewicz Jr, Peter P]

> On Fri, 25 Apr 2008, PJ Waskiewicz wrote:
> > This bug was introduced in the 2.6.24 i386/x86_64 tree merge, where
> 
> Can you please explain what exactly caused the bug. 
> Definitely not the move from arch/i386 to arch/x86 as the 
> code there was not changed at all and has not be changed since then.
> 
> CC'ed Eric as well.

Eric replied with the actual commit during the 2.6.24 merge window that
introduced this bug.  The io_apic.c code from the i386 tree did not stay
completely static when it was merged into the x86 io_apic_32.c code.
Here is the commit that Eric identified that introduced the defect:

In particular commit dbeb2be21d678c49a8d8bbf774903df15dd55474
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Fri Oct 19 20:35:03 2007 +0200

    i386: introduce "used_vectors" bitmap which can be used to reserve
vectors.
    
    This simplifies the io_apic.c __assign_irq_vector() logic and
removes
    the explicit SYSCALL_VECTOR check, and also allows for vectors to be
    reserved by other mechanisms (ie. lguest).
    
    [ tglx: arch/x86 adaptation ]
    
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
    Signed-off-by: Andi Kleen <ak@suse.de>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Basically the code introduced the test_and_set_bit() on the used_vectors
bitmap, but it didn't have a corresponding clear_bit() on IRQ
destruction.

Cheers,
-PJ Waskiewicz

RE: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Thomas Gleixner]

On Mon, 28 Apr 2008, Waskiewicz Jr, Peter P wrote:
> > On Fri, 25 Apr 2008, PJ Waskiewicz wrote:
> > > This bug was introduced in the 2.6.24 i386/x86_64 tree merge, where
> > 
> > Can you please explain what exactly caused the bug. 
> > Definitely not the move from arch/i386 to arch/x86 as the 
> > code there was not changed at all and has not be changed since then.
> > 
> > CC'ed Eric as well.
> 
> Eric replied with the actual commit during the 2.6.24 merge window that
> introduced this bug.  The io_apic.c code from the i386 tree did not stay
> completely static when it was merged into the x86 io_apic_32.c code.
> Here is the commit that Eric identified that introduced the defect:
> 
> In particular commit dbeb2be21d678c49a8d8bbf774903df15dd55474
> Author: Rusty Russell <rusty@rustcorp.com.au>
> Date:   Fri Oct 19 20:35:03 2007 +0200
> 
>     i386: introduce "used_vectors" bitmap which can be used to reserve
> vectors.
>     
>     This simplifies the io_apic.c __assign_irq_vector() logic and
> removes
>     the explicit SYSCALL_VECTOR check, and also allows for vectors to be
>     reserved by other mechanisms (ie. lguest).
>     
>     [ tglx: arch/x86 adaptation ]
>     
>     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>     Signed-off-by: Andi Kleen <ak@suse.de>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Basically the code introduced the test_and_set_bit() on the used_vectors
> bitmap, but it didn't have a corresponding clear_bit() on IRQ
> destruction.

Ok, that makes sense. I just did not get the context out of your
commit log. Adding the commit which caused the trouble to the commit
log makes it easier to understand. So the problem is unrelated to the
merger, it's caused by a patch which was pending against the i386 tree
around the time of the merger and which was converted to the merge
tree.

Thanks,
	tglx

RE: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Waskiewicz Jr, Peter P]

> Ok, that makes sense. I just did not get the context out of 
> your commit log. Adding the commit which caused the trouble 
> to the commit log makes it easier to understand. So the 
> problem is unrelated to the merger, it's caused by a patch 
> which was pending against the i386 tree around the time of 
> the merger and which was converted to the merge tree.

That's my miss for not including the specific context.  I will try to be
better about that in the future.  I do appreciate the interest though to
make sure it's specifically understood when this bug was introduced.

Cheers,

-PJ Waskiewicz

RE: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Waskiewicz Jr, Peter P]

> Thanks Jesse for forwarding this to me.

Yes, Jesse took care of that oversight for me.  I owe him a tasty
beverage.  :-)

> The patch below does looks ok in terms of fixing the issue 
> with respect to MSIs.
> 
> Unfortunately the entire used_vectors concept appears broken. 
>  Rusty is not taking vector_lock.  used_vectors as designed 
> can not work on x86_64 as vectors are allocated to interrupts 
> in a per cpu manner, resulting in unnecessary divergences 
> between the two pieces of code.  used_vectors is an an extra 
> data structure that we have to keep in sync, and failing to 
> keep that data structure in sync is why we have a problem.
> 
> For the concept of allocating a non 0x80 vector for 
> interrupts Rusty was on the right track.  His implementation 
> just appears to be broken in the corner cases, and has a 
> design we can not use long term.
> 
> To fix this my inclination is to revert:
> dbeb2be21d678c49a8d8bbf774903df15dd55474 (the infracture) and 
> c18acd73ffc209def08003a1927473096f66c5ad (the lguest user) as 
> we need to rework those pieces of code anyway, and then work 
> with Rusty to come up with something that we can share on x86 
> and x86_64.

Either way, I'm fine if that gets reverted or not, just as long as we
don't revert back to leaking vectors on device unload.  But if things
are somewhat functional today, I'd say leave the code in there until the
proper solution can be worked out.  Then go ahead and pull it out and
put the new bits in, so anyone currently using the functionality
provided isn't left out in the cold.  That is assuming that these bits
are being used...

Cheers,
-PJ Waskiewicz

Re: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Ingo Molnar]

* PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com> wrote:

> This bug was introduced in the 2.6.24 i386/x86_64 tree merge, [...]

that's wrong, that code was there in v2.6.23 too - and it wasnt touched 
in the unification. Why do you think it was introduced in the 
i386/x86_64 tree merge?

... the fix is right though, thanks!
 
	Ingo

RE: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Waskiewicz Jr, Peter P]

> * PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com> wrote:
> 
> > This bug was introduced in the 2.6.24 i386/x86_64 tree merge, [...]
> 
> that's wrong, that code was there in v2.6.23 too - and it 
> wasnt touched in the unification. Why do you think it was 
> introduced in the
> i386/x86_64 tree merge?

2.6.23.17 didn't have the used_vectors bitmap.  The first time I could
find it used was in 2.6.24.  Where was the code existing in 2.6.23?  The
addition may have not been part of the merge, but it was definitely
introduced in the same timeframe, meaning the 2.6.24 release timeframe.

Cheers,
-PJ Waskiewicz

Re: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Ingo Molnar]

* Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@intel.com> wrote:

> > * PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com> wrote:
> > 
> > > This bug was introduced in the 2.6.24 i386/x86_64 tree merge, [...]
> > 
> > that's wrong, that code was there in v2.6.23 too - and it wasnt 
> > touched in the unification. Why do you think it was introduced in 
> > the i386/x86_64 tree merge?
> 
> 2.6.23.17 didn't have the used_vectors bitmap.  The first time I could 
> find it used was in 2.6.24.  Where was the code existing in 2.6.23?  
> The addition may have not been part of the merge, but it was 
> definitely introduced in the same timeframe, meaning the 2.6.24 
> release timeframe.

that change came in via lguest (commit dbeb2be2), not the 32-bit/64-bit 
x86 unification. Lguest is a new feature in 2.6.24 and this side-effect 
of it was not known until you fixed it - thanks for that!

So i was just splitting hairs over your statement that it was due to the 
unification :-)

	Ingo

RE: [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage

[Waskiewicz Jr, Peter P]

> that change came in via lguest (commit dbeb2be2), not the 
> 32-bit/64-bit
> x86 unification. Lguest is a new feature in 2.6.24 and this 
> side-effect of it was not known until you fixed it - thanks for that!
> 
> So i was just splitting hairs over your statement that it was 
> due to the unification :-)

100% agree.  Thanks for making sure about the distinction.

> 
> 	Ingo
>