Commit 0aeea18964173715a1037034ef6838198f319319
Committed by
Jens Axboe
1 parent
9179746652
Exists in
master
and in
39 other branches
block: fix mis-synchronisation in blkdev_issue_zeroout()
BZ29402 https://bugzilla.kernel.org/show_bug.cgi?id=29402 We can hit serious mis-synchronization in bio completion path of blkdev_issue_zeroout() leading to a panic. The problem is that when we are going to wait_for_completion() in blkdev_issue_zeroout() we check if the bb.done equals issued (number of submitted bios). If it does, we can skip the wait_for_completition() and just out of the function since there is nothing to wait for. However, there is a ordering problem because bio_batch_end_io() is calling atomic_inc(&bb->done) before complete(), hence it might seem to blkdev_issue_zeroout() that all bios has been completed and exit. At this point when bio_batch_end_io() is going to call complete(bb->wait), bb and wait does not longer exist since it was allocated on stack in blkdev_issue_zeroout() ==> panic! (thread 1) (thread 2) bio_batch_end_io() blkdev_issue_zeroout() if(bb) { ... if (bb->end_io) ... bb->end_io(bio, err); ... atomic_inc(&bb->done); ... ... while (issued != atomic_read(&bb.done)) ... (let issued == bb.done) ... (do the rest of the function) ... return ret; complete(bb->wait); ^^^^^^^^ panic We can fix this easily by simplifying bio_batch and completion counting. Also remove bio_end_io_t *end_io since it is not used. Signed-off-by: Lukas Czerner <lczerner@redhat.com> Reported-by: Eric Whitney <eric.whitney@hp.com> Tested-by: Eric Whitney <eric.whitney@hp.com> Reviewed-by: Jeff Moyer <jmoyer@redhat.com> CC: Dmitry Monakhov <dmonakhov@openvz.org> Signed-off-by: Jens Axboe <jaxboe@fusionio.com>
Showing 1 changed file with 7 additions and 12 deletions Side-by-side Diff
block/blk-lib.c
... | ... | @@ -109,7 +109,6 @@ |
109 | 109 | atomic_t done; |
110 | 110 | unsigned long flags; |
111 | 111 | struct completion *wait; |
112 | - bio_end_io_t *end_io; | |
113 | 112 | }; |
114 | 113 | |
115 | 114 | static void bio_batch_end_io(struct bio *bio, int err) |
... | ... | @@ -122,12 +121,9 @@ |
122 | 121 | else |
123 | 122 | clear_bit(BIO_UPTODATE, &bb->flags); |
124 | 123 | } |
125 | - if (bb) { | |
126 | - if (bb->end_io) | |
127 | - bb->end_io(bio, err); | |
128 | - atomic_inc(&bb->done); | |
129 | - complete(bb->wait); | |
130 | - } | |
124 | + if (bb) | |
125 | + if (atomic_dec_and_test(&bb->done)) | |
126 | + complete(bb->wait); | |
131 | 127 | bio_put(bio); |
132 | 128 | } |
133 | 129 | |
134 | 130 | |
135 | 131 | |
... | ... | @@ -150,13 +146,12 @@ |
150 | 146 | int ret; |
151 | 147 | struct bio *bio; |
152 | 148 | struct bio_batch bb; |
153 | - unsigned int sz, issued = 0; | |
149 | + unsigned int sz; | |
154 | 150 | DECLARE_COMPLETION_ONSTACK(wait); |
155 | 151 | |
156 | - atomic_set(&bb.done, 0); | |
152 | + atomic_set(&bb.done, 1); | |
157 | 153 | bb.flags = 1 << BIO_UPTODATE; |
158 | 154 | bb.wait = &wait; |
159 | - bb.end_io = NULL; | |
160 | 155 | |
161 | 156 | submit: |
162 | 157 | ret = 0; |
163 | 158 | |
... | ... | @@ -185,12 +180,12 @@ |
185 | 180 | break; |
186 | 181 | } |
187 | 182 | ret = 0; |
188 | - issued++; | |
183 | + atomic_inc(&bb.done); | |
189 | 184 | submit_bio(WRITE, bio); |
190 | 185 | } |
191 | 186 | |
192 | 187 | /* Wait for bios in-flight */ |
193 | - while (issued != atomic_read(&bb.done)) | |
188 | + if (!atomic_dec_and_test(&bb.done)) | |
194 | 189 | wait_for_completion(&wait); |
195 | 190 | |
196 | 191 | if (!test_bit(BIO_UPTODATE, &bb.flags)) |