Re: [PATCH 1/2] file-posix: fix Linux alignment probing when EIO is returned

[Kevin Wolf <kwolf@redhat.com>]

Am 02.11.2022 um 03:49 hat Eric Biggers geschrieben:
> On Tue, Nov 01, 2022 at 07:27:16PM -0700, Eric Biggers wrote:
> > On Tue, Nov 01, 2022 at 03:00:30PM -0400, Stefan Hajnoczi wrote:
> > > Linux dm-crypt returns errno EIO from unaligned O_DIRECT pread(2) calls.
> > 
> > Citation needed.  For direct I/O to block devices, the kernel's block layer
> > checks the alignment before the I/O is actually submitted to the underlying
> > block device.  See
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/fops.c?h=v6.1-rc3#n306
> > 
> > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/1290
> > 
> > That "bug" seems to be based on a misunderstanding of the kernel source code,
> > and not any actual testing.
> > 
> > I just tested it, and the error code is EINVAL.
> > 
> 
> I think I see what's happening.  The kernel code was broken just a few months
> ago, in v6.0 by the commit "block: relax direct io memory alignment"
> (https://git.kernel.org/linus/b1a000d3b8ec582d).  Now the block layer lets DIO
> through when the user buffer is only aligned to the device's dma_alignment.  But
> a dm-crypt device has a dma_alignment of 512 even when the crypto sector size
> (and thus also the logical block size) is 4096.  So there is now a case where
> misaligned DIO can reach dm-crypt, when that shouldn't be possible.
> 
> It also means that STATX_DIOALIGN will give the wrong value for
> stx_dio_mem_align in the above case, 512 instead of 4096.  This is because
> STATX_DIOALIGN for block devices relies on the dma_alignment.

In other words, STATX_DIOALIGN is unusable from the start because we
don't know whether the information it returns is actually correct? :-/

I guess we could still use the value returned by STATX_DIOALIGN as a
preferred value that we'll use if it survives probing, and otherwise
fall back to the same probing we've always been doing because there was
no (or no sane) way to get the information from the kernel.

> I'll raise this on the linux-block and dm-devel mailing lists.  It
> would be nice if people reported kernel bugs instead of silently
> working around them...

I wasn't involved in this specific one, but in case you're wondering why
I wouldn't have reported it either...

On one hand, I agree with you because I want bugs in my code reported,
too, but on the other hand, it has also happened to me before that
you're treated as the stupid userland developer who doesn't know how
the kernel works and who should better have kept his ideas of how it
should work to himself - which is not exactly encouraging to report
things when you can just deal with the way they are. I wouldn't have
been able to tell whether in the mind of the respective maintainers,
-EINVAL is required behaviour or whether that was just a totally
unreasonable assumption on our side. Erring on the safe side, I'll give
up an assumption that obviously doesn't match reality.

And even a kernel fix now doesn't change that there are broken kernels
out there and we need to work with them. So reporting it doesn't even
solve our problem, it's just additional effort with limited expectations
of success.

Kevin