Fix race in get_request()
PATCH 1/1 Fix race in get_request()
Hello Jens!
I came across the below while investigating some other problem. Something here doesn't seem right. This looks like an obvious bug and something roughly along the lines of my patch would fix it. But I must be in the wrong decade to find such a bug in the block layer.
Is this for real? Or if not, what am I missing?
Jörn
--
If __get_request() returns NULL, get_request will call prepare_to_wait_exclusive() followed by io_schedule(). Not rechecking the sleep condition after prepare_to_wait_exclusive() leaves a race where the condition changes before prepare_to_wait_exclusive(), but not after and accordingly this thread never gets woken up.
The race must be exceedingly hard to hit, otherwise I cannot explain how such a classic race could outlive the last millenium.
--- block/blk-core.c | 5 +++++ 1 file changed, 5 insertions(+)
block/blk-core.c +5 -0
@@ -1068,6 +1068,11 @@ retry:trace_block_sleeprq (q, bio, rw_flags &1 ); + rq =__get_request (rl, rw_flags, bio, gfp_mask); +if (rq) { +finish_wait (&rl->wait[is_sync], &wait); +return rq; + }spin_unlock_irq (q->queue_lock);io_schedule ();
Signature · 1 line
2.0.0.rc0.1.g7b2ba98
Jens Axboe
maintainerQuoted 13 lines · Jörn Engel
Hello Jens!
I came across the below while investigating some other problem. Something here doesn't seem right. This looks like an obvious bug and something roughly along the lines of my patch would fix it. But I must be in the wrong decade to find such a bug in the block layer.
Is this for real? Or if not, what am I missing?
Jörn
--
Quoted 8 lines
If
__get_request()returns NULL,get_requestwill callprepare_to_wait_exclusive()followed byio_schedule(). Not rechecking the sleep condition afterprepare_to_wait_exclusive()leaves a race where the condition changes beforeprepare_to_wait_exclusive(), but not after and accordingly this thread never gets woken up.The race must be exceedingly hard to hit, otherwise I cannot explain how such a classic race could outlive the last millenium.
I think that is a genuine bug, it's just extremely hard to hit in real life. It has probably only potentially ever triggered in the cases where we are so out of memory that a blocking ~300b alloc fails, and Linux generally shits itself pretty hard when it gets to that stage anyway... And for the bug to be critical, you'd need this to happen for a device that otherwise has no IO pending, since you'd get woken up by the next completed request anyway.
block/blk-core.c retry: · line 1075
Quoted 13 lines
block/blk-core.c +5
-0@@ -1068,6 +1068,11 @@ retry:trace_block_sleeprq (q, bio, rw_flags &1 ); + rq =__get_request (rl, rw_flags, bio, gfp_mask); +if (rq) { +finish_wait (&rl->wait[is_sync], &wait); +return rq; + }
The extra __get_request() call should go before the trace_block_sleeprq(), however. I'll rejuggle that when applying.
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerQuoted 23 lines · Jens Axboe
Quoted 13 lines · Jörn Engel
Hello Jens!
I came across the below while investigating some other problem. Something here doesn't seem right. This looks like an obvious bug and something roughly along the lines of my patch would fix it. But I must be in the wrong decade to find such a bug in the block layer.
Is this for real? Or if not, what am I missing?
Jörn
--
Quoted 8 lines
If
__get_request()returns NULL,get_requestwill callprepare_to_wait_exclusive()followed byio_schedule(). Not rechecking the sleep condition afterprepare_to_wait_exclusive()leaves a race where the condition changes beforeprepare_to_wait_exclusive(), but not after and accordingly this thread never gets woken up.The race must be exceedingly hard to hit, otherwise I cannot explain how such a classic race could outlive the last millenium.
Quoted 7 lines
I think that is a genuine bug, it's just extremely hard to hit in real life. It has probably only potentially ever triggered in the cases where we are so out of memory that a blocking ~300b alloc fails, and Linux generally shits itself pretty hard when it gets to that stage anyway... And for the bug to be critical, you'd need this to happen for a device that otherwise has no IO pending, since you'd get woken up by the next completed request anyway.
Actually, this can't trigger for an empty queue, since the mempool holds a few requests. So it should never result in a softlock, we will make progress. Given that we also still hold the queue spinlock (that will be held for a free as well), we should not be able to get a free of a request until the prepare_to_wait() has been done. So not sure there is an actual bug there, but I agree the code looks confusing that way.
Signature · 1 line
Jens Axboe
Quoted 20 lines · Jens Axboe
Quoted 11 lines · Jens Axboe
Quoted 9 lines · Jörn Engel
If
__get_request()returns NULL,get_requestwill callprepare_to_wait_exclusive()followed byio_schedule(). Not rechecking the sleep condition afterprepare_to_wait_exclusive()leaves a race where the condition changes beforeprepare_to_wait_exclusive(), but not after and accordingly this thread never gets woken up.The race must be exceedingly hard to hit, otherwise I cannot explain how such a classic race could outlive the last millenium.
Quoted 7 lines
I think that is a genuine bug, it's just extremely hard to hit in real life. It has probably only potentially ever triggered in the cases where we are so out of memory that a blocking ~300b alloc fails, and Linux generally shits itself pretty hard when it gets to that stage anyway... And for the bug to be critical, you'd need this to happen for a device that otherwise has no IO pending, since you'd get woken up by the next completed request anyway.
Quoted 6 lines
Actually, this can't trigger for an empty queue, since the mempool holds a few requests. So it should never result in a softlock, we will make progress. Given that we also still hold the queue spinlock (that will be held for a free as well), we should not be able to get a free of a request until the
prepare_to_wait()has been done. So not sure there is an actual bug there, but I agree the code looks confusing that way.
The race I was afraid of is not on an empty queue, but where all IO gets completed between the __get_request() and prepare_to_wait_exclusive(). That may well be hundreds of requests, so under normal circumstances they should take far longer than the race window to complete. So something has to enlarge the window.
With the spin_lock_irq held, that removes interrupts and a preemptible kernel from the candidate list. Now we would need hardware support, either some kind of unfair SMT cpus or a hypervisor that doesn't schedule the victim cpu's thread for a long time and at just the right time.
Maybe the hypervisor case is at least theoretically possible. But I would also be happy with leaving the code as-is and at most adding a comment for the next person.
Jörn
--
The grand essentials of happiness are: something to do, something to love, and something to hope for.
-- Allan K. Chalmers