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

Ingo Molnar
2013-12-23T18:23:30Z

* 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