Is returning while holding a spinlock automatically unsafe?

Question

The venerated book Linux Driver Development says that

The flags argument passed to spin_unlock_irqrestore must be the same variable passed to spin_lock_irqsave. You must also call spin_lock_irqsave and spin_unlock_irqrestore in the same function; otherwise your code may break on some architectures.

Yet I can't find any such restriction required by the official documentation bundled with the kernel code itself. And I find driver code that violates this guidance.

Obviously it isn't a good idea to call spin_lock_irqsave and spin_unlock_irqrestore from separate functions, because you're supposed to minimize the work done while holding a lock (with interrupts disabled, no less!). But have changes to the kernel made it possible if done with care, was it never actually against the API contract, or is it still verboten to do so?

If the restriction has been removed at some point, did it apply to version 3.10.17?

score 6 · Answer 1 · edited Dec 15 '15 at 17:43

6

This is just a guess, but the might be unclearly referring to a potential bug which could happen if you try to use a nonlocal variable or storage location for flags.

Basically, flags has to be private to the current execution context, which is why spin_lock_irqsave is a macro which takes the name of the flags. While flags is being saved, you don't have the spinlock yet.

How this is related to locking and unlocking in a different function:

Consider two functions that some driver developer might write:

void my_lock(my_object *ctx)
{
  spin_lock_irqsave(&ctx->mylock, ctx->myflags);  /* BUG */
}

void my_unlock(my_object *ctx)
{
  spin_unlock_irqrestore(&ctx->mylock, ctx->myflags);
}

This is a bug because at the time ctx->myflags is written, the lock is not yet held, and it is a shared variable visible to other contexts and processors. The local flags must be saved to a private location on the stack. Then when the lock is owned, by the caller, a copy of the flags can be saved into the exclusively owned object. In other words, it can be fixed like this:

void my_lock(my_object *ctx)
{
  unsigned long flags;
  spin_lock_irqsave(&ctx->mylock, flag);
  ctx->myflags = flags;
}

void my_unlock(my_object *ctx)
{
  unsigned long flags = ctx->myflags; /* probably unnecessary */
  spin_unlock_irqrestore(&ctx->mylock, flags); 
}

If it couldn't be fixed like that, it would be very difficult to implement higher level primitives which need to wrap IRQ spinlocks.

How it could be arch-dependent:

Suppose that spin_lock_irqsave expands into machine code which saves the current flags in some register, then acquires the lock, and then saves that register into specified flags destination. In that case, the buggy code is actually safe. If the expanded code saves the flags into the actual flags object designated by the caller and then tries to acquire the lock, then it's broken.

edited Dec 15 '15 at 17:43

Sam Protsenko

14,045
4
59
75

answered Dec 15 '15 at 00:25

Kaz

55,781
9
100
149

Good point. Actually, in [2.4.37 kernel](http://lxr.free-electrons.com/source/include/linux/spinlock.h?v=2.4.37#L12) flags are stored *before* lock is acquired, but in [2.6.32](http://lxr.free-electrons.com/source/include/linux/spinlock.h?v=2.6.32#L208) (and thereafter) things are different. It would be interesting to see into `2.6.20`, as the book describes this period of kernel's evolution. – Tsyvarev Dec 15 '15 at 07:48
This is a great answer to a different question... I can't find anything in the documentation forbidding this either, though, so perhaps the behavior prior to 2.6.32 was considered a breach of API contract? In any case, the linked code does exactly what you indicate being wrong (or at the very least, a bad idea) – Ben Voigt Dec 15 '15 at 14:36
@BenVoigt, actually this answer tend to explain cite from the book you refer: one should be very careful while working with *shared* `flags` variable. And using same function context with *local* `flags` variable easily eliminates this difficult. The code you refers as example of *shared* flags variable actually *looks suspicious*: according its [another line](https://github.com/ToshibaSemiconductor/TJET/blob/master/tjet-driver/src/os/linux/syscall/cmn_lock.c#L36), it is expected to be run on kernels before `2.6.26`. – Tsyvarev Dec 15 '15 at 15:04
@Kaz: The line marked `/* probably unnecessary */` is **actually needed**: because `spin_unlock_irqrestore` is *macro*, compiler isn't forced to evaluate its arguments before macro expansion. So, without local variable, it is very possible, that `ctx->myflags` will be evaluated *after* unlocking the lock, which is bad. – Tsyvarev Dec 15 '15 at 15:11
@Tsyvarev: gotcha. Yes; once the lock is gone, `ctx->myflags` is garbage churned by other contexts. – Kaz Dec 15 '15 at 17:16
1

Indeed this may be a historic issue that is no longer real. I actually committed the bug in a kernel driver I was working on. I realized the bug while on vacation, totally away from the code. I was strolling an ocean waterfront in a distant city, when I suddenly realized "that thing I'm doing with the flags is wrong". **That was in 1998**. – Kaz Dec 15 '15 at 17:19

score 1 · Accepted Answer · answered Dec 14 '15 at 23:25

1

I have never see that constraint aside from the book. Probably, given information in the book is just outdated, or .. simply wrong.

In the current kernel(and at least since 2.6.32, which I start to work with) actual locking is done through many level of nested calls from spin_lock_irqsave(see, e.g. __raw_spin_lock_irqsave, which is called in the middle). So different function's context for lock and unlock may hardly be a reason for misfunction.

answered Dec 14 '15 at 23:25

Tsyvarev

60,011
17
110
153

1

I think the constraint is about callers of `spin_lock_irqsave`, not internal calls. For example, if the implementation (which is documented to be a macro -- it has to be in order for the second parameter to be overwritten without using a pointer) declares additional local variables, those would be lost during return from the caller function. It's unclear whether architectures could use such an ugly macro implementation -- book suggests that they might. – Ben Voigt Dec 14 '15 at 23:35
1

But current high-level spinlock API is architecture-independent, and calls architecture-dependent functions through nested (inline) function call or through the macro in a form `do {...} while(0)`. So local variables are lost in any case. – Tsyvarev Dec 15 '15 at 08:04
Ahh yes, if architectures aren't allowed to replace the outer macro, then your analysis seems correct. – Ben Voigt Dec 15 '15 at 14:35

Is returning while holding a spinlock automatically unsafe?

2 Answers2

How this is related to locking and unlocking in a different function:

How it could be arch-dependent:

Linked