| Message ID | 20170124095209.GA22003@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/24/2017 10:52 AM, Christoph Hellwig wrote: > On Tue, Jan 24, 2017 at 10:32:11AM +0100, Matias Bjørling wrote: >> *(gdb) list *blkdev_direct_IO+0x50c >> 0xffffffff8142ab8c is in blkdev_direct_IO (fs/block_dev.c:401). >> 396 submit_bio(bio); >> 397 bio = bio_alloc(GFP_KERNEL, nr_pages); >> 398 } >> 399 blk_finish_plug(&plug); >> 400 >> 401 if (!dio->is_sync) >> 402 return -EIOCBQUEUED; >> >> It looks like the qc = submit_bio() completes the I/O before the >> blkdev_direct_IO completes, which then leads to the use after free case, >> when the private dio struct is accessed. > > Ok, the is_sync check here is clearly a bug, we need a local variable > for is_sync as well for the aio case: > > diff --git a/fs/block_dev.c b/fs/block_dev.c > index 5db5d13..3c47614 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -331,7 +331,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) > struct blk_plug plug; > struct blkdev_dio *dio; > struct bio *bio; > - bool is_read = (iov_iter_rw(iter) == READ); > + bool is_read = (iov_iter_rw(iter) == READ), is_sync; > loff_t pos = iocb->ki_pos; > blk_qc_t qc = BLK_QC_T_NONE; > int ret; > @@ -344,7 +344,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) > bio_get(bio); /* extra ref for the completion handler */ > > dio = container_of(bio, struct blkdev_dio, bio); > - dio->is_sync = is_sync_kiocb(iocb); > + dio->is_sync = is_sync = is_sync_kiocb(iocb); > if (dio->is_sync) > dio->waiter = current; > else > @@ -398,7 +398,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) > } > blk_finish_plug(&plug); > > - if (!dio->is_sync) > + if (!is_sync) > return -EIOCBQUEUED; > > for (;;) { > Yup. That fixes it. Should I put together the patch, or will you take care of it? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 24, 2017 at 11:01:42AM +0100, Matias Bjørling wrote: > Yup. That fixes it. Should I put together the patch, or will you take > care of it? I'll send it out. Of course with proper reporting credits for you. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/24/2017 02:34 PM, Christoph Hellwig wrote: > On Tue, Jan 24, 2017 at 11:01:42AM +0100, Matias Bjørling wrote: >> Yup. That fixes it. Should I put together the patch, or will you take >> care of it? > > I'll send it out. Of course with proper reporting credits for you. > Awesome. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/block_dev.c b/fs/block_dev.c index 5db5d13..3c47614 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -331,7 +331,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) struct blk_plug plug; struct blkdev_dio *dio; struct bio *bio; - bool is_read = (iov_iter_rw(iter) == READ); + bool is_read = (iov_iter_rw(iter) == READ), is_sync; loff_t pos = iocb->ki_pos; blk_qc_t qc = BLK_QC_T_NONE; int ret; @@ -344,7 +344,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) bio_get(bio); /* extra ref for the completion handler */ dio = container_of(bio, struct blkdev_dio, bio); - dio->is_sync = is_sync_kiocb(iocb); + dio->is_sync = is_sync = is_sync_kiocb(iocb); if (dio->is_sync) dio->waiter = current; else @@ -398,7 +398,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) } blk_finish_plug(&plug); - if (!dio->is_sync) + if (!is_sync) return -EIOCBQUEUED; for (;;) {