Re: [pve-devel] [[PATCH kernel]] fix 5683: netfs: reset subreq iov iter before tail clean

[Fiona Ebner <f.ebner@proxmox.com>]

Am 02.10.24 um 16:36 schrieb Christian Ebner:
> Fixes rare read corruption issues using the in kernel ceph client.
> 
> On incomplete read requests, the clean tail flag should make sure to
> zero fill the remaining bytes for the subrequest.
> If the iov iterator is not at the correct position, this can however
> zero fill downloaded data, corrupting the read content.
> 
> Link to issue:
> https://bugzilla.proxmox.com/show_bug.cgi?id=5683
> 
> Link to upstream issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=219237
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> This fixes the read corruption issue with my local reproducer.
> 
> Providing a patched kernel to users affected by the issue for testing
> would be probably the best way to verify the fix.
> 
> Also, I reached out once again to the kernel developers asking if
> this fix is a valid approach, hoping this can be included in current
> stable (as the patch does fix the issue also when applied on 6.11.1).
> 
>  ...et-subreq-iov-iter-before-tail-clean.patch | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch
> 
> diff --git a/patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch b/patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch
> new file mode 100644
> index 0000000..a87e722
> --- /dev/null
> +++ b/patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch
> @@ -0,0 +1,31 @@
> +From cd27abf0c555f39b12c05f9f6a8cb59ff25dfe45 Mon Sep 17 00:00:00 2001
> +From: Christian Ebner <c.ebner@proxmox.com>
> +Date: Wed, 2 Oct 2024 15:24:31 +0200
> +Subject: [PATCH] netfs: reset subreq iov iter before tail clean
> +
> +Make sure the iter is at the correct location when cleaning up tail
> +bytes for incomplete read subrequests.
> +

Disclaimer that I'm not familiar at all with the code.

So AFAIU, after short IO, the iov_iter_count() and subreq->len -
subreq->transferred might disagree. That is why before resubmission,
netfs_reset_subreq_iter() is called. That function aligns the iterator
position, so it will match the information from 'subreq'.

In your edge case, there is no resubmission though, because the
NETFS_SREQ_CLEAR_TAIL flag is set. But it still was short IO, so the
mentioned mismatch happened.

Now netfs_clear_unread() relies on the information from
iov_iter_count(), which does not match the actual 'subreq'. To fix it,
you call netfs_reset_subreq_iter() (like is done before resubmission) to
align that information.

Before commit 92b6cc5d1e7c ("netfs: Add iov_iters to (sub)requests to
describe various buffers"), the information from the 'subreq' was used
to set up the iterator:

> diff --git a/fs/netfs/io.c b/fs/netfs/io.c
> index 7f753380e047..e9d408e211b8 100644
> --- a/fs/netfs/io.c
> +++ b/fs/netfs/io.c
> @@ -21,12 +21,7 @@
>   */
>  static void netfs_clear_unread(struct netfs_io_subrequest *subreq)
>  {
> -       struct iov_iter iter;
> -
> -       iov_iter_xarray(&iter, ITER_DEST, &subreq->rreq->mapping->i_pages,
> -                       subreq->start + subreq->transferred,
> -                       subreq->len   - subreq->transferred);
> -       iov_iter_zero(iov_iter_count(&iter), &iter);
> +       iov_iter_zero(iov_iter_count(&subreq->io_iter), &subreq->io_iter);
>  }

so that sounds good :)

So with and without your change, after the netfs_clear_unread() call,
the iterator will be in the final position, i.e. iov_iter_count() == 0?
Then the information in 'subreq' is updated manually in the same branch
and it moves on to completion.

How far off from reality am I ;)? FWIW, the change looks okay to me, but
again, I'm not familiar with the code and I haven't done any testing
(and have no reproducer).

Of course it would be much nicer to have some confirmation from upstream
and/or users about this.

> +Fixes: 92b6cc5d ("netfs: Add iov_iters to (sub)requests to describe various buffers")
> +Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219237
> +
> +Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> +---
> + fs/netfs/io.c | 1 +
> + 1 file changed, 1 insertion(+)
> +
> +diff --git a/fs/netfs/io.c b/fs/netfs/io.c
> +index d6ada4eba744..500119285346 100644
> +--- a/fs/netfs/io.c
> ++++ b/fs/netfs/io.c
> +@@ -528,6 +528,7 @@ void netfs_subreq_terminated(struct netfs_io_subrequest *subreq,
> + 
> + incomplete:
> + 	if (test_bit(NETFS_SREQ_CLEAR_TAIL, &subreq->flags)) {
> ++		netfs_reset_subreq_iter(rreq, subreq);
> + 		netfs_clear_unread(subreq);
> + 		subreq->transferred = subreq->len;
> + 		goto complete;
> +-- 
> +2.39.5
> +


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel