Re: [PATCH][2.6-mm] i386: enable interrupts on contention in spin_lock_irq

[Keith Owens]

On Wed, 26 May 2004 03:11:07 -0400 (EDT), 
Zwane Mwaikambo <zwane@xxxxxxxxxxx> wrote:
>This little bit was missing from the previous patch. It will enable
>interrupts whilst a cpu is spinning on a lock in spin_lock_irq as well as
>spin_lock_irqsave. UP/SMP compile and runtime/stress tested on i386,
>UP/SMP compile tested on amd64.
>
>+#define _raw_spin_lock_irq(lock)	_raw_spin_lock_flags(lock, X86_EFLAGS_IF)

You are assuming that all uses of spin_lock_irq() are done when
interrupts are already enabled.  This _should_ be true, because the
matching spin_unlock_irq() will unconditionally reenable interrupts.
However I have seen buggy code where spin_lock_irq() was issued with
interrupts disabled.  By unconditionally passing X86_EFLAGS_IF, that
buggy code can now run in one of two states :-

state 1
Enter with interrupts disabled
Do some work
spin_lock_irq()
No lock contention, do not enable interrupts
Do some more work
spin_unlock_irq()

state 2
Enter with interrupts disabled
Do some work
spin_lock_irq()
Lock contention, enable interrupts, get lock, disable interrupts
Do some more work
spin_unlock_irq()

Your patch opens a window where data that was protected by the disabled
interrupt on entry becomes unprotected while waiting for the lock and
can therefore change.

It could be that I am worrying unnecessarily, after all any code that
calls spin_lock_irq() with interrupts already disabled is probably
wrong to start off with.  But it does need to be considered as a
possible failure mode.

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