x86: check for valid irq_cfg pointer in smp_irq_move_cleanup_interrupt

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it. It also seems that this should be done after taking the desc lock.

Quoted 3 lines · Dimitri Sivanich

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it. It also seems that this should be done after taking the desc lock.

It seems that you either missed or failed to explain why it should be done _after_ taking the lock.

Changelogs matter, really.

Quoted 9 lines · Thomas Gleixner

Quoted 3 lines · Dimitri Sivanich

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it. It also seems that this should be done after taking the desc lock.

It seems that you either missed or failed to explain why it should be done _after_ taking the lock.

Quoted 2 lines

Changelogs matter, really.

How about this?

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it.

Follow the same protocol shown in irq_set_chip_data(), by taking the desc lock before accessing this location.

Don't misunderstand me. The patch is correct, just the explanation sucks.

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

Quoted 3 lines · Dimitri Sivanich

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

BTW - is there a reason why we're calling irq_set_chip_data() in create_irq_nr() rather than in __assign_irq_vector() for the case where irq_cfg is NULL?

Quoted 3 lines · Dimitri Sivanich

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

BTW - is there a reason why we're calling irq_set_chip_data() in create_irq_nr() rather than in __assign_irq_vector() for the case where irq_cfg is NULL?

Quoted 4 lines · Dimitri Sivanich

Quoted 3 lines · Dimitri Sivanich

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

Ha. Now I understand how it can happen during boot/module load time. Thanks.

Quoted 2 lines

BTW - is there a reason why we're calling irq_set_chip_data() in create_irq_nr() rather than in __assign_irq_vector() for the case where irq_cfg is NULL?

assign_irq_vector() is also for setting up vectors during irq migration. So may be we could have done the irq_set_chip_data() in create_irq_nr() itself before calling assign_irq_vector(). Anyways, this change can't help in case of destroy irq path which can also lead to the same issue of de-referencing null pointer.

Also, it will be nice if you can refer to this destroy irq path in your changelog.

Acked-by Suresh Siddha <[email protected]>

OK. Hopefully this covers it.

A NULL pointer dereference can occur in smp_irq_move_cleanup_interrupt() if we haven't yet setup the irq_cfg pointer in the irq_desc.irq_data.chip_data.

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

Should an IRQ_MOVE_CLEANUP_VECTOR hit the cpu in question during this time, smp_irq_move_cleanup_interrupt() will attempt to process the aforementioned irq, but panic when accessing irq_cfg.

There is also a window in destroy_irq() where we've cleared the irq_cfg pointer in free_irq_cfg(), but have not yet called irq_free_desc(). Note that we have cleared vector_irq in __clear_irq_vector() prior to free_irq_cfg(), but smp_irq_move_cleanup_interrupt() might've already referenced the irq_desc.

Quoted 1 line · Dimitri Sivanich

OK. Hopefully this covers it.

Sorry No. Now you will understand why Thomas wanted detailed changelog. I found one more issue with the help of your new modification to the changelog.

Quoted 11 lines

A NULL pointer dereference can occur in smp_irq_move_cleanup_interrupt() if we haven't yet setup the irq_cfg pointer in the irq_desc.irq_data.chip_data.

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

Should an IRQ_MOVE_CLEANUP_VECTOR hit the cpu in question during this time, smp_irq_move_cleanup_interrupt() will attempt to process the aforementioned irq, but panic when accessing irq_cfg.

Quoted 4 lines

There is also a window in destroy_irq() where we've cleared the irq_cfg pointer in free_irq_cfg(), but have not yet called irq_free_desc(). Note that we have cleared vector_irq in __clear_irq_vector() prior to free_irq_cfg(), but smp_irq_move_cleanup_interrupt() might've already referenced the irq_desc.

So, what happens if the irq_desc gets freed by the destroy_irq() in the sparse irq case? smp_irq_move_cleanup_interrupt() will refer to freed irq desc memory! Right?

May we should really do something like the appended (untested patch)? Can you please review and give this a try? Let me review a bit more to see if this really fixes the issue.

Thanks.

---
 arch/x86/kernel/apic/io_apic.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

arch/x86/kernel/apic/io_apic.c +9 -10

index ffdc152..81f4cab 100644
@@ -2295,6 +2295,8 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 	exit_idle();
 
 	me = smp_processor_id();
+
+	raw_spin_lock(&vector_lock);
 	for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
 		unsigned int irq;
 		unsigned int irr;

@@ -2310,17 +2312,16 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 			continue;
 
 		cfg = irq_cfg(irq);
-		raw_spin_lock(&desc->lock);
 
 		/*
 		 * Check if the irq migration is in progress. If so, we
 		 * haven't received the cleanup request yet for this irq.
 		 */
 		if (cfg->move_in_progress)
-			goto unlock;
+			continue;
 
 		if (vector == cfg->vector && cpumask_test_cpu(me, cfg->domain))
-			goto unlock;
+			continue;
 
 		irr = apic_read(APIC_IRR + (vector / 32 * 0x10));
 		/*

@@ -2332,12 +2333,11 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 		 */
 		if (irr  & (1 << (vector % 32))) {
 			apic->send_IPI_self(IRQ_MOVE_CLEANUP_VECTOR);
-			goto unlock;
+			continue;
 		}
 		__this_cpu_write(vector_irq[vector], -1);
-unlock:
-		raw_spin_unlock(&desc->lock);
 	}
+	raw_spin_unlock(&vector_lock);
 
 	irq_exit();
 }

@@ -2986,17 +2986,16 @@ unsigned int create_irq_nr(unsigned int from, int node)
 		return 0;
 	}
 
+	irq_set_chip_data(irq, cfg);
 	raw_spin_lock_irqsave(&vector_lock, flags);
 	if (!__assign_irq_vector(irq, cfg, apic->target_cpus()))
 		ret = irq;
 	raw_spin_unlock_irqrestore(&vector_lock, flags);
 
-	if (ret) {
-		irq_set_chip_data(irq, cfg);
+	if (ret)
 		irq_clear_status_flags(irq, IRQ_NOREQUEST);
-	} else {
+	else
 		free_irq_at(irq, cfg);
-	}
 	return ret;
 }
 

Quoted 1 line · Dimitri Sivanich

OK. Hopefully this covers it.

Sorry No. Now you will understand why Thomas wanted detailed changelog. I found one more issue with the help of your new modification to the changelog.

Quoted 11 lines

A NULL pointer dereference can occur in smp_irq_move_cleanup_interrupt() if we haven't yet setup the irq_cfg pointer in the irq_desc.irq_data.chip_data.

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

Should an IRQ_MOVE_CLEANUP_VECTOR hit the cpu in question during this time, smp_irq_move_cleanup_interrupt() will attempt to process the aforementioned irq, but panic when accessing irq_cfg.

Quoted 4 lines

There is also a window in destroy_irq() where we've cleared the irq_cfg pointer in free_irq_cfg(), but have not yet called irq_free_desc(). Note that we have cleared vector_irq in __clear_irq_vector() prior to free_irq_cfg(), but smp_irq_move_cleanup_interrupt() might've already referenced the irq_desc.

So, what happens if the irq_desc gets freed by the destroy_irq() in the sparse irq case? smp_irq_move_cleanup_interrupt() will refer to freed irq desc memory! Right?

May we should really do something like the appended (untested patch)? Can you please review and give this a try? Let me review a bit more to see if this really fixes the issue.

Thanks.

---
 arch/x86/kernel/apic/io_apic.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

arch/x86/kernel/apic/io_apic.c +9 -10

index ffdc152..81f4cab 100644
@@ -2295,6 +2295,8 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 	exit_idle();
 
 	me = smp_processor_id();
+
+	raw_spin_lock(&vector_lock);
 	for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
 		unsigned int irq;
 		unsigned int irr;

@@ -2310,17 +2312,16 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 			continue;
 
 		cfg = irq_cfg(irq);
-		raw_spin_lock(&desc->lock);
 
 		/*
 		 * Check if the irq migration is in progress. If so, we
 		 * haven't received the cleanup request yet for this irq.
 		 */
 		if (cfg->move_in_progress)
-			goto unlock;
+			continue;
 
 		if (vector == cfg->vector && cpumask_test_cpu(me, cfg->domain))
-			goto unlock;
+			continue;
 
 		irr = apic_read(APIC_IRR + (vector / 32 * 0x10));
 		/*

@@ -2332,12 +2333,11 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 		 */
 		if (irr  & (1 << (vector % 32))) {
 			apic->send_IPI_self(IRQ_MOVE_CLEANUP_VECTOR);
-			goto unlock;
+			continue;
 		}
 		__this_cpu_write(vector_irq[vector], -1);
-unlock:
-		raw_spin_unlock(&desc->lock);
 	}
+	raw_spin_unlock(&vector_lock);
 
 	irq_exit();
 }

@@ -2986,17 +2986,16 @@ unsigned int create_irq_nr(unsigned int from, int node)
 		return 0;
 	}
 
+	irq_set_chip_data(irq, cfg);
 	raw_spin_lock_irqsave(&vector_lock, flags);
 	if (!__assign_irq_vector(irq, cfg, apic->target_cpus()))
 		ret = irq;
 	raw_spin_unlock_irqrestore(&vector_lock, flags);
 
-	if (ret) {
-		irq_set_chip_data(irq, cfg);
+	if (ret)
 		irq_clear_status_flags(irq, IRQ_NOREQUEST);
-	} else {
+	else
 		free_irq_at(irq, cfg);
-	}
 	return ret;
 }
 

And speaking of possible holes in destroy_irq()..

What happens if we're running __assign_irq_vector() (say we're changing irq affinity), and on another cpu we had just run through __clear_irq_vector() via destroy_irq(). Now destroy_irq() is going to call free_irq_at()->free_irq_cfg, which will clear irq_cfg. Then __assign_irq_vector goes to access irq_cfg (cfg->vector or cfg->move_in_progress, for instance), which was already freed.

I'm not sure if this can happen, but just eyeballing it, it does look that that way.

Quoted 9 lines · Dimitri Sivanich

And speaking of possible holes in destroy_irq()..

What happens if we're running __assign_irq_vector() (say we're changing irq affinity), and on another cpu we had just run through __clear_irq_vector() via destroy_irq(). Now destroy_irq() is going to call free_irq_at()->free_irq_cfg, which will clear irq_cfg. Then __assign_irq_vector goes to access irq_cfg (cfg->vector or cfg->move_in_progress, for instance), which was already freed.

Quoted 3 lines

I'm not sure if this can happen, but just eyeballing it, it does look that that way.

I wanted to say, irq desc is locked when we change the irq affinity, which calls assign_irq_vector() and friends, so this should be fine.

BUT NO. I don't see any reference counts being maintained when we do irq_to_desc(). So locking/unlocking that desc pointer is bogus when destroy_irq() can go ahead and free the desc in parallel.

So, SPARSE_IRQ looks terribly broken! Yinghai, Thomas?

Quoted 21 lines · Suresh Siddha

Quoted 9 lines · Dimitri Sivanich

And speaking of possible holes in destroy_irq()..

What happens if we're running __assign_irq_vector() (say we're changing irq affinity), and on another cpu we had just run through __clear_irq_vector() via destroy_irq(). Now destroy_irq() is going to call free_irq_at()->free_irq_cfg, which will clear irq_cfg. Then __assign_irq_vector goes to access irq_cfg (cfg->vector or cfg->move_in_progress, for instance), which was already freed.

Quoted 3 lines

I'm not sure if this can happen, but just eyeballing it, it does look that that way.

I wanted to say, irq desc is locked when we change the irq affinity, which calls assign_irq_vector() and friends, so this should be fine.

BUT NO. I don't see any reference counts being maintained when we do irq_to_desc(). So locking/unlocking that desc pointer is bogus when destroy_irq() can go ahead and free the desc in parallel.

Quoted 1 line

So, SPARSE_IRQ looks terribly broken! Yinghai, Thomas?

Yes, we need refcounts for that. We talked about that before, but then the argument was against it was that all that code is serialized already, so no need. How wrong :)

On Thu, 2012-05-24 at 21:16 +0200, Thomas Gleixner wrote:
There are other (not-so common) irq desc references, like in the show_interrupts() (cat /proc/interrupts path) etc, that does things like this in the process context:

        desc = irq_to_desc(i);
        if (!desc)
                return 0;

        raw_spin_lock_irqsave(&desc->lock, flags);

May be we should introduce something like get_irq_desc_locked()/put_irq_desc_locked() that can safely access the irq desc with pre-emption/irq's disabled and lock it etc. And the synchronize_sched() will enable the destroy_irq()/free_desc() to free it safely etc.

Quoted 11 lines · Suresh Siddha

On Thu, 2012-05-24 at 21:16 +0200, Thomas Gleixner wrote:
There are other (not-so common) irq desc references, like in the show_interrupts() (cat /proc/interrupts path) etc, that does things like this in the process context:

        desc = irq_to_desc(i);
        if (!desc)
                return 0;

        raw_spin_lock_irqsave(&desc->lock, flags);

Quoted 5 lines

May be we should introduce something like get_irq_desc_locked()/put_irq_desc_locked() that can safely access the irq desc with pre-emption/irq's disabled and lock it etc. And the synchronize_sched() will enable the destroy_irq()/free_desc() to free it safely etc.

I want to avoid that and instead use proper refcounting. The reason is that we want to move the irq descriptor when the affinity changes nodes, and for that we need refcounting anyway.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 11 lines · Suresh Siddha

On Thu, 2012-05-24 at 21:16 +0200, Thomas Gleixner wrote:
There are other (not-so common) irq desc references, like in the show_interrupts() (cat /proc/interrupts path) etc, that does things like this in the process context:

        desc = irq_to_desc(i);
        if (!desc)
                return 0;

        raw_spin_lock_irqsave(&desc->lock, flags);

Quoted 5 lines

May be we should introduce something like get_irq_desc_locked()/put_irq_desc_locked() that can safely access the irq desc with pre-emption/irq's disabled and lock it etc. And the synchronize_sched() will enable the destroy_irq()/free_desc() to free it safely etc.

I want to avoid that and instead use proper refcounting. The reason is that we want to move the irq descriptor when the affinity changes nodes, and for that we need refcounting anyway.

Quoted 1 line · Dimitri Sivanich

OK. Hopefully this covers it.

Sorry No. Now you will understand why Thomas wanted detailed changelog. I found one more issue with the help of your new modification to the changelog.

Quoted 7 lines

A NULL pointer dereference can occur in smp_irq_move_cleanup_interrupt() if we haven't yet setup the irq_cfg pointer in the irq_desc.irq_data.chip_data.

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

Quoted 8 lines

Should an IRQ_MOVE_CLEANUP_VECTOR hit the cpu in question during this time, smp_irq_move_cleanup_interrupt() will attempt to process the aforementioned irq, but panic when accessing irq_cfg.

There is also a window in destroy_irq() where we've cleared the irq_cfg pointer in free_irq_cfg(), but have not yet called irq_free_desc(). Note that we have cleared vector_irq in __clear_irq_vector() prior to free_irq_cfg(), but smp_irq_move_cleanup_interrupt() might've already referenced the irq_desc.

So, what happens if the irq_desc gets freed by the destroy_irq() in the sparse irq case? smp_irq_move_cleanup_interrupt() will refer to freed irq desc memory! Right?

May we should really do something like the appended (untested patch)? Can you please review and give this a try? Let me review a bit more to see if this really fixes the issue.

Quoted 23 lines · Suresh Siddha

Quoted 1 line · Dimitri Sivanich

OK. Hopefully this covers it.

Sorry No. Now you will understand why Thomas wanted detailed changelog. I found one more issue with the help of your new modification to the changelog.

Quoted 7 lines

A NULL pointer dereference can occur in smp_irq_move_cleanup_interrupt() if we haven't yet setup the irq_cfg pointer in the irq_desc.irq_data.chip_data.

In create_irq_nr() there is a window where we have set vector_irq in __assign_irq_vector(), but not yet called irq_set_chip_data() to set the irq_cfg pointer.

Quoted 8 lines

Should an IRQ_MOVE_CLEANUP_VECTOR hit the cpu in question during this time, smp_irq_move_cleanup_interrupt() will attempt to process the aforementioned irq, but panic when accessing irq_cfg.

There is also a window in destroy_irq() where we've cleared the irq_cfg pointer in free_irq_cfg(), but have not yet called irq_free_desc(). Note that we have cleared vector_irq in __clear_irq_vector() prior to free_irq_cfg(), but smp_irq_move_cleanup_interrupt() might've already referenced the irq_desc.

Quoted 7 lines

So, what happens if the irq_desc gets freed by the destroy_irq() in the sparse irq case? smp_irq_move_cleanup_interrupt() will refer to freed irq desc memory! Right?

May we should really do something like the appended (untested patch)? Can you please review and give this a try? Let me review a bit more to see if this really fixes the issue.

It's fixing the problem.

But this move_cleanup stuff could be made less stupid.

The check for irq_desc is superflous. irq_cfg() calls irq_get_chip_data() which will return NULL if the irq descriptor is not there.

To avoid the lookup business completely we should really store irq_desc instead of the irq number in the per cpu vector array, that would also get rid of the lookup in the irq delivery path.

Now that still needs to iterate over all vectors, but this could be optimized in a second step.

In complete_move() we send the IPI to all cpus in the old mask. We really should set the corresponding vector bit in a per cpu bitfield on those cpus in the mask. The cleanup can rely on the bits and avoid looking at 200+ vectors to find a single one.

Thoughts?

tglx

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it. It also seems that this should be done after taking the desc lock.

Signed-off-by Dimitri Sivanich <[email protected]>

---
 arch/x86/kernel/apic/io_apic.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux/arch/x86/kernel/apic/io_apic.c
===================================================================

arch/x86/kernel/apic/io_apic.c +4 -1

@@ -2478,9 +2478,12 @@ asmlinkage void smp_irq_move_cleanup_int
 		if (!desc)
 			continue;
 
-		cfg = irq_cfg(irq);
 		raw_spin_lock(&desc->lock);
 
+		cfg = irq_cfg(irq);
+		if (!cfg)
+			goto unlock;
+
 		/*
 		 * Check if the irq migration is in progress. If so, we
 		 * haven't received the cleanup request yet for this irq.

Quoted 3 lines · Dimitri Sivanich

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it. It also seems that this should be done after taking the desc lock.

I think these changes are correct. Did you see any crashes during module unload etc?

Reviewed-by Suresh Siddha <[email protected]>

Quoted 24 lines

Signed-off-by Dimitri Sivanich <[email protected]>

---
 arch/x86/kernel/apic/io_apic.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux/arch/x86/kernel/apic/io_apic.c
===================================================================

arch/x86/kernel/apic/io_apic.c +4 -1

@@ -2478,9 +2478,12 @@ asmlinkage void smp_irq_move_cleanup_int
 		if (!desc)
 			continue;
 
-		cfg = irq_cfg(irq);
 		raw_spin_lock(&desc->lock);
 
+		cfg = irq_cfg(irq);
+		if (!cfg)
+			goto unlock;
+
 		/*
 		 * Check if the irq migration is in progress. If so, we
 		 * haven't received the cleanup request yet for this irq.

Quoted 7 lines · Suresh Siddha

Quoted 3 lines · Dimitri Sivanich

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it. It also seems that this should be done after taking the desc lock.

I think these changes are correct. Did you see any crashes during module unload etc?

Yes, we have seen these on occasion during boot.

Quoted 6 lines · Dimitri Sivanich

Quoted 5 lines · Suresh Siddha

Quoted 3 lines · Dimitri Sivanich

The smp_irq_move_cleanup_interrupt routine should be checking for a valid irq_cfg pointer prior to accessing it. It also seems that this should be done after taking the desc lock.

Quoted 4 lines

Quoted 2 lines

I think these changes are correct. Did you see any crashes during module unload etc?

Yes, we have seen these on occasion during boot.

During boot or shutdown?

Review of the code shows that this can trigger during module unload which can call destroy_irq() etc and can trigger the crash if there is a parallel irq migration related cleanup.

Unsuccessful module loads can also call destroy_irq() but I doubt that is what happening here.

thanks,
suresh