Re: [PATCH] io_uring: fix short read slow path corruptions

[Kevin Wolf]

Am 29.06.2022 um 07:23 hat Dominique Martinet geschrieben:
> sqeq.off here is the offset to read within the disk image, so obviously
> not 'nread' (the amount we just read), but as the author meant to write
> its current value incremented by the amount we just read.
> 
> Normally recent versions of linux will not issue short reads,
> but apparently btrfs with O_DIRECT (cache=none) does.
> 
> This lead to weird image corruptions when short read happened
> 
> Fixes: 6663a0a33764 ("block/io_uring: implements interfaces for io_uring")
> Link: https://lkml.kernel.org/r/YrrFGO4A1jS0GI0G@atmark-techno.com
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> ---
> Forgive the double mail if it gets to you twice: I missed Ccs on the first
> try, I should have known better...
> 
> I just spent a couple of days on this bug, will follow up with kernel to
> see if we can also not get rid of the short read but perhaps a warning
> should be added the first time we get a short read, as it's not supposed
> to happen?
> Well, slow path now seems to work (at least my VM now boots fine), but
> if the code clearly states it should never be used I assume there might
> be other bugs laying there as it's not tested... That this one was easy
> enough to spot once I noticed the short reads was its only grace...
> 
> Thanks!
> 
>  block/io_uring.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/io_uring.c b/block/io_uring.c
> index d48e472e74cb..d58aff9615ce 100644
> --- a/block/io_uring.c
> +++ b/block/io_uring.c
> @@ -103,7 +103,7 @@ static void luring_resubmit_short_read(LuringState *s, 
> LuringAIOCB *luringcb,
>                        remaining);
>  
>      /* Update sqe */
> -    luringcb->sqeq.off = nread;
> +    luringcb->sqeq.off += nread;
>      luringcb->sqeq.addr = (__u64)(uintptr_t)luringcb->resubmit_qiov.iov;
>      luringcb->sqeq.len = luringcb->resubmit_qiov.niov;

I see this a few lines above:

    /* Update read position */
    luringcb->total_read = nread;

Doesn't it have the same problem? Though maybe getting two short reads
is more of a theoretical case.

Kevin