Re: Race conditions galore (2.0.33 and possibly 2.1.x)

Stephen R. van den Berg (
Mon, 22 Dec 1997 18:35:28 +0100

Linus Torvalds wrote:
>In article <>,
>Stephen R. van den Berg <> wrote:
>>Could someone who created these wait-queue loops (there are several of them
>>in the kernel, potentially all of them are race conditions waiting to
>>happen) confirm if I'm reading this correctly? Maybe I still overlooked
>>possible problems, but it seems like this is the minimally-correct change.

>No, your patch should be unnecessary as far as I can see.

Well, it *does* fix some hung processes on a newsserver of mine.
First, several processes hung in buffer.c (very reproducable).
I patched it (using the described patch).
Now, a process hung in filemap.c (same construct). So I patched that
as well.

This happened within 15 minutes uptime, almost every time.
The machine has been running for 17 hours now, no hangs anymore.
I'm running with ECC, and the kernel did and does *not* generate any warnings
or errors. So hardware failures are very unlikely.

This is a K5-166, 512KB 2nd level cache, 128MB RAM (two 64MB parity SIMMS),
ECC, NCR875- BSD-driver, 3 IBM 4GB drives, de4x5 driver ethernet,
ASUS T2P4 board (or something), 2.0.33, BTW.

So, there almost *has* to be a race, one just has to find out what exactly.

>Anyway, the basic race-free wait loop looks like this (there are

> add_wait_queue(..);
> repeat:

In buffer.c it runs some queues at this point. Can these affect the
state of the should_wait_condition?
E.g. it unlocks the buffer, sets the current state to TASK_RUNNING
(redundant), then locks the buffer again. What happens to the wait-queue?
Will it still wake up the process even though it has been unlocked, then
locked again?

> current->state = TASK_UNINTERRUPTIBLE;
> if (should_wait_condition) {

What if the should_wait_condition changes here?
I.e. the buffer is unlocked. Will the current->state be affected?
Or will wake_up process avoid touching the current->state because the
current process is "still running"?

> schedule();
> goto repeat;
> }
> remove_wait_queue(..);
> current->state = TASK_RUNNING;
> }

>There are only two important rules:
> - you have to add yourself to the wait queue _before_ testing for the
> condition.
> - you have to mark yourself asleep _before_ testing for the condition.

Maybe we need to change the last rule?

- You have to mark yourself asleep _before_ adding yourself to the waitqueue.

>Anyway, the reason the two simple rules are needed is that _if_ you mark
>yourself asleep _and_ have added yourself to the wait queue before
>testing for any wake-up condition, then you know that if something
>happens to change that condition (and does a "wake_up()", then it will
>also have marked you as being awake (and thus the "schedule()" will not
>sleep, and we'll catch the condition the second time around).

>Now, admittedly the above does depend on a few ordering constraints, and
>those ordering constraints are not necessarily true in all SMP systems.
>Maybe this was what you alluded to? The rules in question are really

No, actually. Unless my system has a cache-coherency problem somewhere;
but it's not an SMP system by any stretch of the imagination.

           Stephen R. van den Berg (AKA BuGless).

"Father's Day: Nine months before Mother's Day."