From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id B24661FF161
for <inbox@lore.proxmox.com>; Tue, 22 Oct 2024 14:49:47 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 05E07D626;
Tue, 22 Oct 2024 14:50:25 +0200 (CEST)
Message-ID: <7d487881-6851-4c32-b2a2-dbb7ccdfe4e5@proxmox.com>
Date: Tue, 22 Oct 2024 14:50:21 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Christian Ebner <c.ebner@proxmox.com>
References: <20241002143624.1260363-1-c.ebner@proxmox.com>
Content-Language: en-US
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <20241002143624.1260363-1-c.ebner@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.058 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [proxmox.com]
Subject: Re: [pve-devel] [[PATCH kernel]] fix 5683: netfs: reset subreq iov
iter before tail clean
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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