linux-ext4 - Re: [PATCHv4 5/8] iomap: simplify direct io validity check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251028230350.GB1639650@google.com>
Date: Tue, 28 Oct 2025 23:03:50 +0000
From: Eric Biggers <ebiggers@...nel.org>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: Keith Busch <kbusch@...nel.org>, Keith Busch <kbusch@...a.com>,
	linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-xfs@...r.kernel.org, linux-ext4@...r.kernel.org, hch@....de,
	axboe@...nel.dk, Hannes Reinecke <hare@...e.de>,
	"Martin K. Petersen" <martin.petersen@...cle.com>
Subject: Re: [PATCHv4 5/8] iomap: simplify direct io validity check

On Tue, Oct 28, 2025 at 10:56:48PM +0000, Eric Biggers wrote:
> On Tue, Oct 28, 2025 at 10:47:53PM +0000, Carlos Llamas wrote:
> > Ok, I did a bit more digging. I'm using f2fs but the problem in this
> > case is the blk_crypto layer. The OP_READ request goes through
> > submit_bio() which then calls blk_crypto_bio_prep() and if the bio has
> > crypto context then it checks for bio_crypt_check_alignment().
> > 
> > This is where the LTP tests fails the alignment. However, the propagated
> > error goes through "bio->bi_status = BLK_STS_IOERR" which in bio_endio()
> > get translates to EIO due to blk_status_to_errno().
> > 
> > I've verified this restores the original behavior matching the LTP test,
> > so I'll write up a patch and send it a bit later.
> > 
> > diff --git a/block/blk-crypto.c b/block/blk-crypto.c
> > index 1336cbf5e3bd..a417843e7e4a 100644
> > --- a/block/blk-crypto.c
> > +++ b/block/blk-crypto.c
> > @@ -293,7 +293,7 @@ bool __blk_crypto_bio_prep(struct bio **bio_ptr)
> >  	}
> >  
> >  	if (!bio_crypt_check_alignment(bio)) {
> > -		bio->bi_status = BLK_STS_IOERR;
> > +		bio->bi_status = BLK_STS_INVAL;
> >  		goto fail;
> >  	}
> 
> That change looks fine, but I'm wondering how this case was reached in
> the first place.  Upper layers aren't supposed to be submitting
> misaligned bios like this.  For example, ext4 and f2fs require
> filesystem logical block size alignment for direct I/O on encrypted
> files.  They check for this early, before getting to the point of
> submitting a bio, and fall back to buffered I/O if needed.

I suppose it's this code in f2fs_should_use_dio():

	/*
	 * Direct I/O not aligned to the disk's logical_block_size will be
	 * attempted, but will fail with -EINVAL.
	 *
	 * f2fs additionally requires that direct I/O be aligned to the
	 * filesystem block size, which is often a stricter requirement.
	 * However, f2fs traditionally falls back to buffered I/O on requests
	 * that are logical_block_size-aligned but not fs-block aligned.
	 *
	 * The below logic implements this behavior.
	 */
	align = iocb->ki_pos | iov_iter_alignment(iter);
	if (!IS_ALIGNED(align, i_blocksize(inode)) &&
	    IS_ALIGNED(align, bdev_logical_block_size(inode->i_sb->s_bdev)))
		return false;

So it relies on the alignment check in iomap in the case where the
request is neither logical_block_size nor filesystem_block_size aligned.

f2fs_should_use_dio() probably should just handle that case explicitly.

But making __blk_crypto_bio_prep() use a better error code sounds good
too.

- Eric