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

This is a discussion on [PATCH] ARCH: Fix 32-bit x86 MSI-X allocation leakage - Kernel ; 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. ...

+ Reply to Thread
Results 1 to 11 of 11

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

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

    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
    ---

    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/

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

    "Brandeburg, Jesse" , , "Waskiewicz Jr, Peter P" , , Rusty Russell , Andi Kleen , Ingo Molnar , Thomas Gleixner
    Date: Mon, 28 Apr 2008 12:09:57 -0700
    In-Reply-To: <20080426005850.7098.77479.stgit@scrappy.jf.intel.c om> (PJ
    Waskiewicz's message of "Fri, 25 Apr 2008 17:58:52 -0700")
    Message-ID:
    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 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
    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
    Signed-off-by: Andi Kleen
    Signed-off-by: Ingo Molnar
    Signed-off-by: Thomas Gleixner

    > 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
    > ---
    >
    > 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

    --
    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/

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

    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
    > ---
    >
    > 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/
    >

    --
    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/

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

    > 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
    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
    Signed-off-by: Andi Kleen
    Signed-off-by: Ingo Molnar
    Signed-off-by: Thomas Gleixner

    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
    --
    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/

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

    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
    > 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
    > Signed-off-by: Andi Kleen
    > Signed-off-by: Ingo Molnar
    > Signed-off-by: Thomas Gleixner
    >
    > 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

    --
    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/

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

    > 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
    --
    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/

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

    > 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
    --
    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/

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


    * PJ Waskiewicz 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
    --
    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/

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

    > * PJ Waskiewicz 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
    --
    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/

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


    * Waskiewicz Jr, Peter P wrote:

    > > * PJ Waskiewicz 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
    --
    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/

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

    > 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
    >

    --
    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/

+ Reply to Thread