Re: [PATCH v4 5/8] block: fail a short atomic pin in bio_iov_iter_get_pages()

[Hannes Reinecke <hare@suse.de>]

On 9/22/26 4:54 AM, Tal Zussman wrote:
> On a partial page pin, __blkdev_direct_IO_simple() and
> __blkdev_direct_IO_async() submit what was pinned with REQ_ATOMIC set
> and leave the rest to the buffered fallback, tearing an IOCB_ATOMIC
> write.
> 
> This can be triggered deterministically. A 16K pwritev2(RWF_ATOMIC)
> whose last page is PROT_NONE, on a scsi_debug device with atomic_wr=1,
> completes short with only three of the four pages written, violating
> RWF_ATOMIC semantics.
> 
> Make bio_iov_iter_get_pages() release the pins and return -EINVAL when
> a REQ_ATOMIC bio doesn't cover the whole iterator, since an atomic
> write is submitted as a single bio and a short one would be torn. That
> covers iomap as well, where a partially unmapped buffer could trip the
> WARN_ON_ONCE() in iomap_dio_bio_iter_one(). The async block device path
> currently sets REQ_ATOMIC after pinning, so set it before, and move
> REQ_NOWAIT along with it.
> 
> Fixes: caf336f81b3a ("block: Add fops atomic write support")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20260802-blkdev-fixes-v1-0-a82fc549fd74%40columbia.edu?part=2
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> ---
>   block/bio.c  | 29 ++++++++++++++++++++++-------
>   block/fops.c | 12 ++++++------
>   2 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index f95b63c0604a..14429a5d4e68 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1285,6 +1285,7 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
>   			   unsigned mem_align_mask, unsigned len_align_mask)
>   {
>   	iov_iter_extraction_t flags = 0;
> +	int ret;
>   
>   	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>   		return -EIO;
> @@ -1304,34 +1305,48 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
>   		flags |= ITER_ALLOW_P2PDMA;
>   
>   	do {
> -		ssize_t ret;
> +		ssize_t size;
>   
> -		ret = iov_iter_extract_bvecs(iter, bio->bi_io_vec,
> +		size = iov_iter_extract_bvecs(iter, bio->bi_io_vec,
>   				BIO_MAX_SIZE - bio->bi_iter.bi_size,
>   				&bio->bi_vcnt, bio->bi_max_vecs,
>   				mem_align_mask, flags);
> -		if (ret <= 0) {
> +		if (size <= 0) {
>   			/*
>   			 * A misaligned vector fails the whole I/O.  Release any
>   			 * pages pinned by earlier iterations before returning
>   			 * since this bio won't be submitted to release them.
>   			 */
> -			if (ret == -EINVAL) {
> +			if (size == -EINVAL) {
>   				bio_release_pages(bio, false);
>   				bio_clear_flag(bio, BIO_PAGE_PINNED);
>   				bio->bi_vcnt = 0;
>   			}
>   			if (!bio->bi_vcnt)
> -				return ret;
> +				return size;
>   			break;
>   		}
> -		bio->bi_iter.bi_size += ret;
> +		bio->bi_iter.bi_size += size;
>   	} while (iov_iter_count(iter) && !bio_full(bio, 0));
>   
I would swizzle the use of 'ret' and 'size' here; leaving 'ret'
untouched and use 'size' as the new variable. That will reduce
code churn and makes the patch easier to read.

>   	if (is_pci_p2pdma_page(bio->bi_io_vec->bv_page))
>   		bio->bi_opf |= REQ_NOMERGE;
> -	return bio_iov_iter_align_down(bio, iter,
> +	ret = bio_iov_iter_align_down(bio, iter,
>   			&bio->bi_io_vec[bio->bi_vcnt - 1], len_align_mask);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * An atomic write is submitted as a single bio, so it has to cover
> +	 * the whole iterator or it would be torn.
> +	 */
> +	if ((bio->bi_opf & REQ_ATOMIC) && iov_iter_count(iter)) {
> +		bio_release_pages(bio, false);
> +		bio_clear_flag(bio, BIO_PAGE_PINNED);
> +		bio->bi_vcnt = 0;
> +		return -EINVAL;
> +	}
> +	return 0;
>   }
>   
>   static struct folio *folio_alloc_greedy(gfp_t gfp, size_t *size,
> diff --git a/block/fops.c b/block/fops.c
> index a3a709697b40..90777e8a9a6c 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -342,6 +342,12 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
>   	bio->bi_end_io = blkdev_bio_end_io_async;
>   	bio->bi_ioprio = iocb->ki_ioprio;
>   
> +	if (iocb->ki_flags & IOCB_ATOMIC)
> +		bio->bi_opf |= REQ_ATOMIC;
> +
> +	if (iocb->ki_flags & IOCB_NOWAIT)
> +		bio->bi_opf |= REQ_NOWAIT;
> +
>   	/*
>   	 * Users don't rely on the iterator being in any particular
>   	 * state for async I/O returning -EIOCBQUEUED, hence we can
> @@ -371,12 +377,6 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
>   			goto out_bio_put;
>   	}
>   
> -	if (iocb->ki_flags & IOCB_ATOMIC)
> -		bio->bi_opf |= REQ_ATOMIC;
> -
> -	if (iocb->ki_flags & IOCB_NOWAIT)
> -		bio->bi_opf |= REQ_NOWAIT;
> -
>   	if (iocb->ki_flags & IOCB_HIPRI) {
>   		bio->bi_opf |= REQ_POLLED;
>   		submit_bio(bio);
> 

Otherwise looks good.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich