Check the insert, and if it fails cleanup and free all partial work.
Sparse irq was not checking the return code from radix_tree_insert,
but it may need to allocate memory and can fail. If it failed,
it still claimed success to the caller but the affected irq(s) are
unavailable and the reference to the affected descriptors is leaked.
Signed-off-by: Milton Miller <[email protected]>
---
I started by tring to change free_desc to take the descriptor pointer
and pushing that down, but that soon ran into conflicts between the
array and sparse implementations, and/or the old dynamic irq cleanup
function that is still used by some architectures. This version is
targeted, and also protects against scribbles to irq_data.irq.
Index: work.git/kernel/irq/irqdesc.c
===================================================================
--- work.git.orig/kernel/irq/irqdesc.c 2011-05-23 13:46:09.197635762 -0500
+++ work.git/kernel/irq/irqdesc.c 2011-05-23 14:29:22.960588100 -0500
@@ -164,10 +164,8 @@ err_desc:
return NULL;
}
-static void free_desc(unsigned int irq)
+static void free_a_desc(unsigned int irq, struct irq_desc *desc)
{
- struct irq_desc *desc = irq_to_desc(irq);
-
unregister_irq_proc(irq, desc);
mutex_lock(&sparse_irq_lock);
@@ -179,21 +177,30 @@ static void free_desc(unsigned int irq)
kfree(desc);
}
+static void free_desc(unsigned int irq)
+{
+ free_a_desc(irq, irq_to_desc(irq));
+}
+
static int alloc_descs(unsigned int start, unsigned int cnt, int node)
{
struct irq_desc *desc;
- int i;
+ int i, res;
for (i = 0; i < cnt; i++) {
desc = alloc_desc(start + i, node);
if (!desc)
goto err;
mutex_lock(&sparse_irq_lock);
- irq_insert_desc(start + i, desc);
+ res = irq_insert_desc(start + i, desc);
mutex_unlock(&sparse_irq_lock);
+ if (res)
+ goto err_insert;
}
return start;
+err_insert:
+ free_a_desc(start + i, desc);
err:
for (i--; i >= 0; i--)
free_desc(start + i);
On Wed, 25 May 2011, Milton Miller wrote:
> Check the insert, and if it fails cleanup and free all partial work.
>
> Sparse irq was not checking the return code from radix_tree_insert,
> but it may need to allocate memory and can fail. If it failed,
> it still claimed success to the caller but the affected irq(s) are
> unavailable and the reference to the affected descriptors is leaked.
>
> Signed-off-by: Milton Miller <[email protected]>
> ---
> I started by tring to change free_desc to take the descriptor pointer
> and pushing that down, but that soon ran into conflicts between the
> array and sparse implementations, and/or the old dynamic irq cleanup
> function that is still used by some architectures. This version is
> targeted, and also protects against scribbles to irq_data.irq.
The simpler solution is to move irq_insert_desc() into alloc_desc()
and deal with the error case there.
Thanks,
tglx
On Wed, 25 May 2011 about 10:18:40 +0200 (CEST), Thomas Gleixner wrote:
> On Wed, 25 May 2011, Milton Miller wrote:
>
> > Check the insert, and if it fails cleanup and free all partial work.
> >
> > Sparse irq was not checking the return code from radix_tree_insert,
> > but it may need to allocate memory and can fail. If it failed,
> > it still claimed success to the caller but the affected irq(s) are
> > unavailable and the reference to the affected descriptors is leaked.
> >
> > Signed-off-by: Milton Miller <[email protected]>
> > ---
> > I started by tring to change free_desc to take the descriptor pointer
> > and pushing that down, but that soon ran into conflicts between the
> > array and sparse implementations, and/or the old dynamic irq cleanup
> > function that is still used by some architectures. This version is
> > targeted, and also protects against scribbles to irq_data.irq.
>
> The simpler solution is to move irq_insert_desc() into alloc_desc()
> and deal with the error case there.
And I see that this one too as written does not compile, due to
my faulty config testing. I'll explore your idea tomorrow after
some sleep, but it makes sense.
milton