On 2019/11/08 20:54, Tetsuo Handa wrote: > syzbot found that a thread can stall for minutes inside fallocate() > after that thread was killed by SIGKILL [1]. While trying to allocate > 64TB of disk space using fallocate() is legal, delaying termination of > killed thread for minutes is bad. Thus, allow iteration functions in > block/blk-lib.c to be killable. > > [1] https://syzkaller.appspot.com/bug?id=9386d051e11e09973d5a4cf79af5e8cedf79386d > > Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > Reported-by: syzbot <syzbot+b48daca8639150bc5e73@xxxxxxxxxxxxxxxxxxxxxxxxx> > --- > block/blk-lib.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 40 insertions(+), 4 deletions(-) > > diff --git a/block/blk-lib.c b/block/blk-lib.c > index 5f2c429..6ca7cae 100644 > --- a/block/blk-lib.c > +++ b/block/blk-lib.c > @@ -7,9 +7,22 @@ > #include <linux/bio.h> > #include <linux/blkdev.h> > #include <linux/scatterlist.h> > +#include <linux/sched/signal.h> > > #include "blk.h" > > +static int blk_should_abort(struct bio *bio) > +{ > + int ret; > + > + cond_resched(); > + if (!fatal_signal_pending(current)) > + return 0; > + ret = submit_bio_wait(bio); This will change the behavior of __blkdev_issue_discard() to a sync IO execution instead of the current async execution since submit_bio_wait() call is the responsibility of the caller (e.g. blkdev_issue_discard()). Have you checked if users of __blkdev_issue_discard() are OK with that ? f2fs, ext4, xfs, dm and nvme use this function. Looking at f2fs, this does not look like it is going to work as expected since the bio setup, including end_io callback, is done after this function is called and a regular submit_bio() execution is being used. > + bio_put(bio); > + return ret ? ret : -EINTR; > +} > + > struct bio *blk_next_bio(struct bio *bio, unsigned int nr_pages, gfp_t gfp) > { > struct bio *new = bio_alloc(gfp, nr_pages); > @@ -55,6 +68,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > return -EINVAL; > > while (nr_sects) { > + int ret; Please add a white line after the declaration similarly to your change in __blkdev_issue_write_same() and __blkdev_issue_zero_pages(). > sector_t req_sects = min_t(sector_t, nr_sects, > bio_allowed_max_sectors(q)); > > @@ -75,7 +89,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, > * us to schedule out to avoid softlocking if preempt > * is disabled. > */ > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > @@ -154,6 +172,8 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, > max_write_same_sectors = bio_allowed_max_sectors(q); > > while (nr_sects) { > + int ret; > + > bio = blk_next_bio(bio, 1, gfp_mask); > bio->bi_iter.bi_sector = sector; > bio_set_dev(bio, bdev); > @@ -171,7 +191,11 @@ static int __blkdev_issue_write_same(struct block_device *bdev, sector_t sector, > bio->bi_iter.bi_size = nr_sects << 9; > nr_sects = 0; > } > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > @@ -230,6 +254,8 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, > return -EOPNOTSUPP; > > while (nr_sects) { > + int ret; > + > bio = blk_next_bio(bio, 0, gfp_mask); > bio->bi_iter.bi_sector = sector; > bio_set_dev(bio, bdev); > @@ -245,7 +271,11 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev, > bio->bi_iter.bi_size = nr_sects << 9; > nr_sects = 0; > } > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > @@ -281,6 +311,8 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, > return -EPERM; > > while (nr_sects != 0) { > + int ret; > + > bio = blk_next_bio(bio, __blkdev_sectors_to_bio_pages(nr_sects), > gfp_mask); > bio->bi_iter.bi_sector = sector; > @@ -295,7 +327,11 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev, > if (bi_size < sz) > break; > } > - cond_resched(); > + ret = blk_should_abort(bio); > + if (ret) { > + *biop = NULL; > + return ret; > + } > } > > *biop = bio; > -- Damien Le Moal Western Digital Research
