Lukas Czerner <lczerner@xxxxxxxxxx> writes: > 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@xxxxxxxxxx> > Reported-by: Eric Whitney <eric.whitney@xxxxxx> > Tested-by: Eric Whitney <eric.whitney@xxxxxx> > CC: Jens Axboe <axboe@xxxxxxxxx> > CC: Dmitry Monakhov <dmonakhov@xxxxxxxxxx> > CC: Jeff Moyer <jmoyer@xxxxxxxxxx> > --- > block/blk-lib.c | 19 +++++++------------ > 1 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/block/blk-lib.c b/block/blk-lib.c > index eec78be..bd3e8df 100644 > --- a/block/blk-lib.c > +++ b/block/blk-lib.c > @@ -109,7 +109,6 @@ struct bio_batch > atomic_t done; > unsigned long flags; > struct completion *wait; > - bio_end_io_t *end_io; > }; > > static void bio_batch_end_io(struct bio *bio, int err) > @@ -122,12 +121,9 @@ static void bio_batch_end_io(struct bio *bio, int err) > else > clear_bit(BIO_UPTODATE, &bb->flags); > } > - if (bb) { > - if (bb->end_io) > - bb->end_io(bio, err); > - atomic_inc(&bb->done); > - complete(bb->wait); > - } > + if (bb) > + if (atomic_dec_and_test(&bb->done)) > + complete(bb->wait); I think bb will always be set here, no real need to check. Anyway, I though I already added my: Reviewed-by: Jeff Moyer <jmoyer@xxxxxxxxxx> to this. No? Cheers, Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
