Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix)

[Ingo Molnar <mingo@kernel.org>]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 12/23, Oleg Nesterov wrote:
> >
> > Perhaps we should ask the maintainers upstream? Even if this works, I am
> > not sure this is _supposed_ to work. I mean, in theory spin_lock_irqave()
> > can be changed as, say
> >
> > 	#define spin_lock_irqsave(lock, flags)		\
> > 		do {    				\
> > 			local_irq_save(flags);		\
> > 			spin_lock(lock);		\
> > 		} while (0)
> >
> > (and iirc it was defined this way a long ago). In this case "flags" is
> > obviously not protected.
> 
> Yes, lets ask the maintainers.
> 
> In short, is this code
> 
> 	spinlock_t LOCK;
> 	unsigned long FLAGS;
> 
> 	void my_lock(void)
> 	{
> 		spin_lock_irqsave(&LOCK, FLAGS);
> 	}
> 
> 	void my_unlock(void)
> 	{
> 		spin_unlock_irqrestore(&LOCK, FLAGS);
> 	}
> 
> correct or not?
> 
> Initially I thought that this is obviously wrong, irqsave/irqrestore 
> assume that "flags" is owned by the caller, not by the lock. And 
> iirc this was certainly wrong in the past.
> 
> But when I look at spinlock.c it seems that this code can actually 
> work. _irqsave() writes to FLAGS after it takes the lock, and 
> _irqrestore() has a copy of FLAGS before it drops this lock.

I don't think that's true: if it was then the lock would not be 
irqsave, a hardware-irq could come in after the lock has been taken 
and before flags are saved+disabled.

So AFAICS this is an unsafe pattern, beyond being ugly as hell.

Thanks,

	Ingo