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

Carlos Llamas
2025-10-30T04:54:28Z

On Tue, Oct 28, 2025 at 11:03:50PM +0000, Eric Biggers wrote:
> 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.

I realize this is a bit of a band-aid but here is the patch to fail the
bad alignment with EINVAL:
https://lore.kernel.org/all/20251030043919.2787231-1-cmllamas@google.com/

As for the more achitectural fix suggested by Christoph, I'm absolutely
out of my depth so I can't comment on that.

Cheers,
--
Carlos Llamas