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

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

[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.
+
+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
+
-- 
2.39.5



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

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

[Fiona Ebner]

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

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

[Thomas Lamprecht]

Am 02/10/2024 um 16:36 schrieb Christian Ebner:
> 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).

Maybe send the patch also to the mailing list with respective CC list
gathered through get_maintainer.pl and possibly also the stable mailing
list in CC, as that might speed things up, not a lot of kernel devs check
the kernel.org BZ frequently.


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

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

[Christian Ebner]

On 10/22/24 15:57, Thomas Lamprecht wrote:
> Am 02/10/2024 um 16:36 schrieb Christian Ebner:
>> 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).
> 
> Maybe send the patch also to the mailing list with respective CC list
> gathered through get_maintainer.pl and possibly also the stable mailing
> list in CC, as that might speed things up, not a lot of kernel devs check
> the kernel.org BZ frequently.

I did also already reach out to the corresponding mailing lists (see 
[0]). But I will try to send the patch to the lists directly.

I refrained from doing so as I saw that typically fixes to mainline are 
back-ported to stable, but since this is already fixed in the latest 
version, I was hoping for upstream guidance on how to proceed 
(unfortunately without reply).

[0] 
https://lore.kernel.org/stable/8b24d0d8-7e94-42eb-9fdd-c3c2e8a44c3d@proxmox.com/


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

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

[Christian Ebner]

On 10/22/24 14:50, Fiona Ebner wrote:
> 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.

I don't recall the exact code paths anymore from the top of my head, sorry.
Will have to look at it once again, but the essential is that the 
iov_iter_zero() incorrectly clears out the data, which leads to the read 
corruption, yes.
As I too do not have a in depth knowledge of this code base, I was 
hoping for upstream to confirm the validity of the patch.

> 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.

Agreed, unfortunately no feedback so far.

>> +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

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

[Thomas Lamprecht]

Am 22/10/2024 um 17:46 schrieb Christian Ebner:
> I refrained from doing so as I saw that typically fixes to mainline are 
> back-ported to stable, but since this is already fixed in the latest 
> version, I was hoping for upstream guidance on how to proceed 
> (unfortunately without reply).

Hmm, true, this makes it a bit more complicated.
If you submit it then to stable and the maintainers should definitively should
add the context for why a 1:1 cherry-pick is not an option, like it being rather
huge and the fixing the misalignment as side effect of correcting the tracking
of subreq->transferred where sensible before calling into the new
netfs_read_subreq_terminated helper that replaced the netfs_subreq_terminated
one you're adapting here, at least FWICT.

So FWIW, while I also would prefer some comment from upstream, I'm also fine
with applying this now, as it seems reasonable workaround and letting the kernel
through to the test repo to allow those affected to test it would also to get us
some more QA out of this patch before moving it to more stable repos.


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

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

[Christian Ebner]

On 10/23/24 10:28, Thomas Lamprecht wrote:
> Am 22/10/2024 um 17:46 schrieb Christian Ebner:
>> I refrained from doing so as I saw that typically fixes to mainline are
>> back-ported to stable, but since this is already fixed in the latest
>> version, I was hoping for upstream guidance on how to proceed
>> (unfortunately without reply).
> 
> Hmm, true, this makes it a bit more complicated.
> If you submit it then to stable and the maintainers should definitively should
> add the context for why a 1:1 cherry-pick is not an option, like it being rather
> huge and the fixing the misalignment as side effect of correcting the tracking
> of subreq->transferred where sensible before calling into the new
> netfs_read_subreq_terminated helper that replaced the netfs_subreq_terminated
> one you're adapting here, at least FWICT.

Yeah, I will definitely include more detailed reasoning and the context 
on why this applies to stable only.
Unfortunately I do not have my reproducer cluster with me this week, so 
I would do this first thing on Monday next week. I do wanted to double 
check and test with current stable, also hopefully gaining some more 
insides with your and Fiona's feedback.

> So FWIW, while I also would prefer some comment from upstream, I'm also fine
> with applying this now, as it seems reasonable workaround and letting the kernel
> through to the test repo to allow those affected to test it would also to get us
> some more QA out of this patch before moving it to more stable repos.

That would be great, at least to see if this really also fixes the 
issues customers see with the fixed index files being corrupted on their 
CephFS located datastores, e.g. [0]. Since I never reproduced that exact 
issue (I only reproduced the corrupt checksum after file upload [1]), 
confirmation that this is fixed would be a big plus.

[0] https://forum.proxmox.com/threads/149249/
[1] https://forum.proxmox.com/threads/151291/


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

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

[Thomas Lamprecht]

Am 02/10/2024 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
> 
>

applied, thanks!


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

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

[Christian Ebner]

Send the patch requesting to be included in upstream linux-stable:
https://lore.kernel.org/stable/20241027114315.730407-1-c.ebner@proxmox.com/T/


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