block: don't revert iter for -EIOCBQUEUED

Commit Message

Jens Axboe Jan. 23, 2025, 1:18 p.m. UTC

blkdev_read_iter() has a few odd checks, like gating the position and
count adjustment on whether or not the result is bigger-than-or-equal to
zero (where bigger than makes more sense), and not checking the return
value of blkdev_direct_IO() before doing an iov_iter_revert(). The
latter can lead to attempting to revert with a negative value, which
when passed to iov_iter_revert() as an unsigned value will lead to
throwing a WARN_ON() because unroll is bigger than MAX_RW_COUNT.

Be sane and don't revert for -EIOCBQUEUED, like what is done in other
spots.

Cc: [e-mail]
Signed-off-by: Jens Axboe <[e-mail]>

---

Comments

Christoph Hellwig Jan. 28, 2025, 5:59 a.m. UTC | #1

On Thu, Jan 23, 2025 at 06:18:01AM -0700, Jens Axboe wrote:
> blkdev_read_iter() has a few odd checks, like gating the position and
> count adjustment on whether or not the result is bigger-than-or-equal to
> zero (where bigger than makes more sense), and not checking the return
> value of blkdev_direct_IO() before doing an iov_iter_revert(). The
> latter can lead to attempting to revert with a negative value, which
> when passed to iov_iter_revert() as an unsigned value will lead to
> throwing a WARN_ON() because unroll is bigger than MAX_RW_COUNT.

How did you reproduce that?  Can we add it to blktests?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

Jens Axboe Jan. 28, 2025, 3:33 p.m. UTC | #2

On 1/27/25 10:59 PM, Christoph Hellwig wrote:
> On Thu, Jan 23, 2025 at 06:18:01AM -0700, Jens Axboe wrote:
>> blkdev_read_iter() has a few odd checks, like gating the position and
>> count adjustment on whether or not the result is bigger-than-or-equal to
>> zero (where bigger than makes more sense), and not checking the return
>> value of blkdev_direct_IO() before doing an iov_iter_revert(). The
>> latter can lead to attempting to revert with a negative value, which
>> when passed to iov_iter_revert() as an unsigned value will lead to
>> throwing a WARN_ON() because unroll is bigger than MAX_RW_COUNT.
> 
> How did you reproduce that?  Can we add it to blktests?

Via one of the io_uring test cases, when used on a SCSI device. Not
easy to write a reliable reproducer for this, and honestly I'm kind
of puzzled I haven't hit it before recently.

Patch

diff --git a/block/fops.c b/block/fops.c
index 6d5c4fc5a216..be9f1dbea9ce 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -783,11 +783,12 @@  static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 		file_accessed(iocb->ki_filp);
 
 		ret = blkdev_direct_IO(iocb, to);
-		if (ret >= 0) {
+		if (ret > 0) {
 			iocb->ki_pos += ret;
 			count -= ret;
 		}
-		iov_iter_revert(to, count - iov_iter_count(to));
+		if (ret != -EIOCBQUEUED)
+			iov_iter_revert(to, count - iov_iter_count(to));
 		if (ret < 0 || !count)
 			goto reexpand;
 	}