[PATCH] block: fix dio leak on integrity metadata mapping failure

[PATCH] block: fix dio leak on integrity metadata mapping failure

[Aaron Esau]

In __blkdev_direct_IO(), when bio_integrity_map_iter() fails on a bio
after earlier bios have already been submitted, the goto fail path frees
only the current bio and returns the error directly to the caller.

This is incorrect because the fail label does not decrement dio->ref for
the current bio, and does not return -EIOCBQUEUED. The in-flight bios
each hold a reference via dio->ref, which was incremented before their
submission. When they eventually complete, blkdev_bio_end_io() decrements
dio->ref but it will never reach zero since the failing bio did not
participate in the completion mechanism. This permanently leaks the
embedded dio/bio structure from blkdev_dio_pool.

The trigger is deterministic: a multi-segment direct IO with integrity
metadata where the user-provided metadata buffer is too small for all
segments causes bio_integrity_map_iter() to return -EINVAL on a later
bio after earlier bios are already in flight.

Fix this by handling the integrity mapping failure the same way
blkdev_iov_iter_get_pages() failure is handled: set bi_status and call
bio_endio() to enter the normal completion path, then break out of the
loop so the function returns -EIOCBQUEUED for async IO.

Fixes: 3d8b5a22d404 ("block: add support to pass user meta buffer")
Cc: stable@vger.kernel.org
Signed-off-by: Aaron Esau <aaron1esau@gmail.com>
---
 block/fops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/fops.c b/block/fops.c
index bb6642b459..39280d761c 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -239,8 +239,11 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 		}
 		if (iocb->ki_flags & IOCB_HAS_METADATA) {
 			ret = bio_integrity_map_iter(bio, iocb->private);
-			if (unlikely(ret))
-				goto fail;
+			if (unlikely(ret)) {
+				bio->bi_status = BLK_STS_IOERR;
+				bio_endio(bio);
+				break;
+			}
 		}
 
 		if (is_read) {
-- 
2.54.0

Re: [PATCH] block: fix dio leak on integrity metadata mapping failure

[Christoph Hellwig]

> +			if (unlikely(ret)) {
> +				bio->bi_status = BLK_STS_IOERR;
> +				bio_endio(bio);
> +				break;
> +			}

AFAICS the same issue also exists for the other goto fail case,
so we should convert the code at that label to a bio_endio().

I think both this and the existing -EAGAIN case and even the
-EIOCBQUEUED case leak the reference on the original bio.  Or am
I missing something?

It might makes sense to stop playing games with that bio refcount
and just have a status field in struct blkdev_dio.