[PATCH] Resolve data corruption of TCP server info fields

[PATCH] Resolve data corruption of TCP server info fields

[Rohith Surabattula]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

Hi All,

With the "esize" mount option, I observed data corruption and cifs
reconnects during performance tests.

TCP server info field server->total_read is modified parallely by
demultiplex thread and decrypt offload worker thread. server->total_read
is used in calculation to discard the remaining data of PDU which is
not read into memory.

Because of parallel modification, “server->total_read” value got
corrupted and instead of discarding the remaining data, it discarded
some valid data from the next PDU.

server->total_read field is already updated properly during read from
socket. So, no need to update the same field again after decryption.

Regards,
Rohith

[-- Attachment #2: 0001-Resolve-data-corruption-of-TCP-server-info-fields.patch --]
[-- Type: application/octet-stream, Size: 1066 bytes --]

From 8bb9241f4afa414366f2b6c7c1f5981b9ece190e Mon Sep 17 00:00:00 2001
From: Rohith Surabattula <rohiths@microsoft.com>
Date: Thu, 8 Oct 2020 09:58:41 +0000
Subject: [PATCH] Resolve data corruption of TCP server info fields

TCP server info field server->total_read is modified parallely by
demultiplex thread and decrypt offload worker thread. server->total_read
is used in calculation to discard the remaining data of PDU which is
not read into memory.

Because of parallel modification, server->total_read can get corrupted
and can result in discarding the valid data of next PDU.

Signed-off-by: Rohith Surabattula <rohiths@microsoft.com>
---
 fs/cifs/smb2ops.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 32f90dc82c84..5a15301b80a8 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -4129,8 +4129,6 @@ decrypt_raw_data(struct TCP_Server_Info *server, char *buf,
 
 	memmove(buf, iov[1].iov_base, buf_data_size);
 
-	server->total_read = buf_data_size + page_data_size;
-
 	return rc;
 }
 
-- 
2.25.1

Re: [PATCH] Resolve data corruption of TCP server info fields

[Aurélien Aptel]

Hi Rohith,

Rohith Surabattula <rohiths.msft@gmail.com> writes:
> server->total_read field is already updated properly during read from
> socket. So, no need to update the same field again after decryption.

Good catch! From scanning the code it looks right but better run some
tests to be sure. If you have a reproducer it could be useful to add to
the buildbot.

Reviewed-by: Aurelien Aptel <aaptel@suse.com>

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH] Resolve data corruption of TCP server info fields

[Steve French]

Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next

On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
<rohiths.msft@gmail.com> wrote:
>
> Hi All,
>
> With the "esize" mount option, I observed data corruption and cifs
> reconnects during performance tests.
>
> TCP server info field server->total_read is modified parallely by
> demultiplex thread and decrypt offload worker thread. server->total_read
> is used in calculation to discard the remaining data of PDU which is
> not read into memory.
>
> Because of parallel modification, “server->total_read” value got
> corrupted and instead of discarding the remaining data, it discarded
> some valid data from the next PDU.
>
> server->total_read field is already updated properly during read from
> socket. So, no need to update the same field again after decryption.
>
> Regards,
> Rohith



-- 
Thanks,

Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Pavel Shilovsky]

Hi Rohith,

Thanks for catching the problem and proposing the patch!

I think there is a problem with just removing server->total_read
updates inside decrypt_raw_data():

The same function is used in receive_encrypted_standard() which then
calls cifs_handle_standard(). The latter uses server->total_read in at
least two places: in server->ops->check_message and cifs_dump_mem().

There may be other places in the code that assume server->total_read
to be correct. I would avoid simply removing this in all code paths
and would rather make a more specific fix for the offloaded reads.

--
Best regards,
Pavel Shilovsky

чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
>
> Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
>
> On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> <rohiths.msft@gmail.com> wrote:
> >
> > Hi All,
> >
> > With the "esize" mount option, I observed data corruption and cifs
> > reconnects during performance tests.
> >
> > TCP server info field server->total_read is modified parallely by
> > demultiplex thread and decrypt offload worker thread. server->total_read
> > is used in calculation to discard the remaining data of PDU which is
> > not read into memory.
> >
> > Because of parallel modification, “server->total_read” value got
> > corrupted and instead of discarding the remaining data, it discarded
> > some valid data from the next PDU.
> >
> > server->total_read field is already updated properly during read from
> > socket. So, no need to update the same field again after decryption.
> >
> > Regards,
> > Rohith
>
>
>
> --
> Thanks,
>
> Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Rohith Surabattula]

Hi Pavel,

In receive_encrypted_standard function also, server->total_read is
updated properly before calling decrypt_raw_data. So, no need to
update the same field again.

I have checked all instances where decrypt_raw_data is used and didn’t
find any issue.

Regards,
Rohith

On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Hi Rohith,
>
> Thanks for catching the problem and proposing the patch!
>
> I think there is a problem with just removing server->total_read
> updates inside decrypt_raw_data():
>
> The same function is used in receive_encrypted_standard() which then
> calls cifs_handle_standard(). The latter uses server->total_read in at
> least two places: in server->ops->check_message and cifs_dump_mem().
>
> There may be other places in the code that assume server->total_read
> to be correct. I would avoid simply removing this in all code paths
> and would rather make a more specific fix for the offloaded reads.
>
> --
> Best regards,
> Pavel Shilovsky
>
> чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> >
> > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> >
> > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > <rohiths.msft@gmail.com> wrote:
> > >
> > > Hi All,
> > >
> > > With the "esize" mount option, I observed data corruption and cifs
> > > reconnects during performance tests.
> > >
> > > TCP server info field server->total_read is modified parallely by
> > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > is used in calculation to discard the remaining data of PDU which is
> > > not read into memory.
> > >
> > > Because of parallel modification, “server->total_read” value got
> > > corrupted and instead of discarding the remaining data, it discarded
> > > some valid data from the next PDU.
> > >
> > > server->total_read field is already updated properly during read from
> > > socket. So, no need to update the same field again after decryption.
> > >
> > > Regards,
> > > Rohith
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Pavel Shilovsky]

In receive_encrypted_standard(), server->total_read is set to the
total packet length before calling decrypt_raw_data(). The total
packet length includes the transform header but the idea of updating
server->total_read after decryption is to set it to a decrypted packet
size without the transform header (see memmove in decrypt_raw_data).

We would probably need to backport the patch to stable trees, so, I
would try to make the smallest possible change in terms of scope -
meaning just fixing the read codepath with esize mount option turned
on.

--
Best regards,
Pavel Shilovsky

ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
>
> Hi Pavel,
>
> In receive_encrypted_standard function also, server->total_read is
> updated properly before calling decrypt_raw_data. So, no need to
> update the same field again.
>
> I have checked all instances where decrypt_raw_data is used and didn’t
> find any issue.
>
> Regards,
> Rohith
>
> On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > Hi Rohith,
> >
> > Thanks for catching the problem and proposing the patch!
> >
> > I think there is a problem with just removing server->total_read
> > updates inside decrypt_raw_data():
> >
> > The same function is used in receive_encrypted_standard() which then
> > calls cifs_handle_standard(). The latter uses server->total_read in at
> > least two places: in server->ops->check_message and cifs_dump_mem().
> >
> > There may be other places in the code that assume server->total_read
> > to be correct. I would avoid simply removing this in all code paths
> > and would rather make a more specific fix for the offloaded reads.
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > >
> > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > >
> > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > <rohiths.msft@gmail.com> wrote:
> > > >
> > > > Hi All,
> > > >
> > > > With the "esize" mount option, I observed data corruption and cifs
> > > > reconnects during performance tests.
> > > >
> > > > TCP server info field server->total_read is modified parallely by
> > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > is used in calculation to discard the remaining data of PDU which is
> > > > not read into memory.
> > > >
> > > > Because of parallel modification, “server->total_read” value got
> > > > corrupted and instead of discarding the remaining data, it discarded
> > > > some valid data from the next PDU.
> > > >
> > > > server->total_read field is already updated properly during read from
> > > > socket. So, no need to update the same field again after decryption.
> > > >
> > > > Regards,
> > > > Rohith
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Rohith Surabattula]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

Hi Pavel,

Corrected the patch with the suggested changes.
Attached the patch.

Regards,
Rohith

On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> In receive_encrypted_standard(), server->total_read is set to the
> total packet length before calling decrypt_raw_data(). The total
> packet length includes the transform header but the idea of updating
> server->total_read after decryption is to set it to a decrypted packet
> size without the transform header (see memmove in decrypt_raw_data).
>
> We would probably need to backport the patch to stable trees, so, I
> would try to make the smallest possible change in terms of scope -
> meaning just fixing the read codepath with esize mount option turned
> on.
>
> --
> Best regards,
> Pavel Shilovsky
>
> ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
> >
> > Hi Pavel,
> >
> > In receive_encrypted_standard function also, server->total_read is
> > updated properly before calling decrypt_raw_data. So, no need to
> > update the same field again.
> >
> > I have checked all instances where decrypt_raw_data is used and didn’t
> > find any issue.
> >
> > Regards,
> > Rohith
> >
> > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> > > Hi Rohith,
> > >
> > > Thanks for catching the problem and proposing the patch!
> > >
> > > I think there is a problem with just removing server->total_read
> > > updates inside decrypt_raw_data():
> > >
> > > The same function is used in receive_encrypted_standard() which then
> > > calls cifs_handle_standard(). The latter uses server->total_read in at
> > > least two places: in server->ops->check_message and cifs_dump_mem().
> > >
> > > There may be other places in the code that assume server->total_read
> > > to be correct. I would avoid simply removing this in all code paths
> > > and would rather make a more specific fix for the offloaded reads.
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> > >
> > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > > >
> > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > > >
> > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > > <rohiths.msft@gmail.com> wrote:
> > > > >
> > > > > Hi All,
> > > > >
> > > > > With the "esize" mount option, I observed data corruption and cifs
> > > > > reconnects during performance tests.
> > > > >
> > > > > TCP server info field server->total_read is modified parallely by
> > > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > > is used in calculation to discard the remaining data of PDU which is
> > > > > not read into memory.
> > > > >
> > > > > Because of parallel modification, “server->total_read” value got
> > > > > corrupted and instead of discarding the remaining data, it discarded
> > > > > some valid data from the next PDU.
> > > > >
> > > > > server->total_read field is already updated properly during read from
> > > > > socket. So, no need to update the same field again after decryption.
> > > > >
> > > > > Regards,
> > > > > Rohith
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve

[-- Attachment #2: 0001-Resolve-data-corruption-of-TCP-server-info-fields.patch --]
[-- Type: application/octet-stream, Size: 2544 bytes --]

From 05b38fe720c9ea8aa430a6a692311c79cafe233d Mon Sep 17 00:00:00 2001
From: Rohith Surabattula <rohiths@microsoft.com>
Date: Thu, 8 Oct 2020 09:58:41 +0000
Subject: [PATCH] Resolve data corruption of TCP server info fields

TCP server info field server->total_read is modified parallely by
demultiplex thread and decrypt offload worker thread. server->total_read
is used in calculation to discard the remaining data of PDU which is
not read into memory.

Because of parallel modification, server->total_read can get corrupted
and can result in discarding the valid data of next PDU.

Signed-off-by: Rohith Surabattula <rohiths@microsoft.com>
---
 fs/cifs/smb2ops.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 32f90dc82c84..3138173f63a2 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -4103,7 +4103,8 @@ smb3_is_transform_hdr(void *buf)
 static int
 decrypt_raw_data(struct TCP_Server_Info *server, char *buf,
 		 unsigned int buf_data_size, struct page **pages,
-		 unsigned int npages, unsigned int page_data_size)
+		 unsigned int npages, unsigned int page_data_size,
+		 bool is_offloaded)
 {
 	struct kvec iov[2];
 	struct smb_rqst rqst = {NULL};
@@ -4129,7 +4130,8 @@ decrypt_raw_data(struct TCP_Server_Info *server, char *buf,
 
 	memmove(buf, iov[1].iov_base, buf_data_size);
 
-	server->total_read = buf_data_size + page_data_size;
+	if (!is_offloaded)
+		server->total_read = buf_data_size + page_data_size;
 
 	return rc;
 }
@@ -4342,7 +4344,7 @@ static void smb2_decrypt_offload(struct work_struct *work)
 	struct mid_q_entry *mid;
 
 	rc = decrypt_raw_data(dw->server, dw->buf, dw->server->vals->read_rsp_size,
-			      dw->ppages, dw->npages, dw->len);
+			      dw->ppages, dw->npages, dw->len, true);
 	if (rc) {
 		cifs_dbg(VFS, "error decrypting rc=%d\n", rc);
 		goto free_pages;
@@ -4448,7 +4450,7 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid,
 
 non_offloaded_decrypt:
 	rc = decrypt_raw_data(server, buf, server->vals->read_rsp_size,
-			      pages, npages, len);
+			      pages, npages, len, false);
 	if (rc)
 		goto free_pages;
 
@@ -4504,7 +4506,7 @@ receive_encrypted_standard(struct TCP_Server_Info *server,
 	server->total_read += length;
 
 	buf_size = pdu_length - sizeof(struct smb2_transform_hdr);
-	length = decrypt_raw_data(server, buf, buf_size, NULL, 0, 0);
+	length = decrypt_raw_data(server, buf, buf_size, NULL, 0, 0, false);
 	if (length)
 		return length;
 
-- 
2.25.1

Re: [PATCH] Resolve data corruption of TCP server info fields

[Pavel Shilovsky]

Thanks for the patch!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky

пн, 19 окт. 2020 г. в 00:28, Rohith Surabattula <rohiths.msft@gmail.com>:
>
> Hi Pavel,
>
> Corrected the patch with the suggested changes.
> Attached the patch.
>
> Regards,
> Rohith
>
> On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > In receive_encrypted_standard(), server->total_read is set to the
> > total packet length before calling decrypt_raw_data(). The total
> > packet length includes the transform header but the idea of updating
> > server->total_read after decryption is to set it to a decrypted packet
> > size without the transform header (see memmove in decrypt_raw_data).
> >
> > We would probably need to backport the patch to stable trees, so, I
> > would try to make the smallest possible change in terms of scope -
> > meaning just fixing the read codepath with esize mount option turned
> > on.
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
> > >
> > > Hi Pavel,
> > >
> > > In receive_encrypted_standard function also, server->total_read is
> > > updated properly before calling decrypt_raw_data. So, no need to
> > > update the same field again.
> > >
> > > I have checked all instances where decrypt_raw_data is used and didn’t
> > > find any issue.
> > >
> > > Regards,
> > > Rohith
> > >
> > > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > >
> > > > Hi Rohith,
> > > >
> > > > Thanks for catching the problem and proposing the patch!
> > > >
> > > > I think there is a problem with just removing server->total_read
> > > > updates inside decrypt_raw_data():
> > > >
> > > > The same function is used in receive_encrypted_standard() which then
> > > > calls cifs_handle_standard(). The latter uses server->total_read in at
> > > > least two places: in server->ops->check_message and cifs_dump_mem().
> > > >
> > > > There may be other places in the code that assume server->total_read
> > > > to be correct. I would avoid simply removing this in all code paths
> > > > and would rather make a more specific fix for the offloaded reads.
> > > >
> > > > --
> > > > Best regards,
> > > > Pavel Shilovsky
> > > >
> > > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > > > >
> > > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > > > >
> > > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > > > <rohiths.msft@gmail.com> wrote:
> > > > > >
> > > > > > Hi All,
> > > > > >
> > > > > > With the "esize" mount option, I observed data corruption and cifs
> > > > > > reconnects during performance tests.
> > > > > >
> > > > > > TCP server info field server->total_read is modified parallely by
> > > > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > > > is used in calculation to discard the remaining data of PDU which is
> > > > > > not read into memory.
> > > > > >
> > > > > > Because of parallel modification, “server->total_read” value got
> > > > > > corrupted and instead of discarding the remaining data, it discarded
> > > > > > some valid data from the next PDU.
> > > > > >
> > > > > > server->total_read field is already updated properly during read from
> > > > > > socket. So, no need to update the same field again after decryption.
> > > > > >
> > > > > > Regards,
> > > > > > Rohith
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > >
> > > > > Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Pavel Shilovsky]

Any ideas about stable? esize mount option went into kernel v5.4.

--
Best regards,
Pavel Shilovsky

пн, 19 окт. 2020 г. в 09:48, Pavel Shilovsky <piastryyy@gmail.com>:
>
> Thanks for the patch!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky
>
> пн, 19 окт. 2020 г. в 00:28, Rohith Surabattula <rohiths.msft@gmail.com>:
> >
> > Hi Pavel,
> >
> > Corrected the patch with the suggested changes.
> > Attached the patch.
> >
> > Regards,
> > Rohith
> >
> > On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> > > In receive_encrypted_standard(), server->total_read is set to the
> > > total packet length before calling decrypt_raw_data(). The total
> > > packet length includes the transform header but the idea of updating
> > > server->total_read after decryption is to set it to a decrypted packet
> > > size without the transform header (see memmove in decrypt_raw_data).
> > >
> > > We would probably need to backport the patch to stable trees, so, I
> > > would try to make the smallest possible change in terms of scope -
> > > meaning just fixing the read codepath with esize mount option turned
> > > on.
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> > >
> > > ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > >
> > > > Hi Pavel,
> > > >
> > > > In receive_encrypted_standard function also, server->total_read is
> > > > updated properly before calling decrypt_raw_data. So, no need to
> > > > update the same field again.
> > > >
> > > > I have checked all instances where decrypt_raw_data is used and didn’t
> > > > find any issue.
> > > >
> > > > Regards,
> > > > Rohith
> > > >
> > > > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > >
> > > > > Hi Rohith,
> > > > >
> > > > > Thanks for catching the problem and proposing the patch!
> > > > >
> > > > > I think there is a problem with just removing server->total_read
> > > > > updates inside decrypt_raw_data():
> > > > >
> > > > > The same function is used in receive_encrypted_standard() which then
> > > > > calls cifs_handle_standard(). The latter uses server->total_read in at
> > > > > least two places: in server->ops->check_message and cifs_dump_mem().
> > > > >
> > > > > There may be other places in the code that assume server->total_read
> > > > > to be correct. I would avoid simply removing this in all code paths
> > > > > and would rather make a more specific fix for the offloaded reads.
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Pavel Shilovsky
> > > > >
> > > > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > > > > >
> > > > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > > > > >
> > > > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > > > > <rohiths.msft@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi All,
> > > > > > >
> > > > > > > With the "esize" mount option, I observed data corruption and cifs
> > > > > > > reconnects during performance tests.
> > > > > > >
> > > > > > > TCP server info field server->total_read is modified parallely by
> > > > > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > > > > is used in calculation to discard the remaining data of PDU which is
> > > > > > > not read into memory.
> > > > > > >
> > > > > > > Because of parallel modification, “server->total_read” value got
> > > > > > > corrupted and instead of discarding the remaining data, it discarded
> > > > > > > some valid data from the next PDU.
> > > > > > >
> > > > > > > server->total_read field is already updated properly during read from
> > > > > > > socket. So, no need to update the same field again after decryption.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Rohith
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Steve French]

Added cc:stable 5.4+ and the 2 Reviewed-bys and merged into
cifs-2.6.git for-next

On Tue, Oct 20, 2020 at 4:43 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Any ideas about stable? esize mount option went into kernel v5.4.
>
> --
> Best regards,
> Pavel Shilovsky
>
> пн, 19 окт. 2020 г. в 09:48, Pavel Shilovsky <piastryyy@gmail.com>:
> >
> > Thanks for the patch!
> >
> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > пн, 19 окт. 2020 г. в 00:28, Rohith Surabattula <rohiths.msft@gmail.com>:
> > >
> > > Hi Pavel,
> > >
> > > Corrected the patch with the suggested changes.
> > > Attached the patch.
> > >
> > > Regards,
> > > Rohith
> > >
> > > On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > >
> > > > In receive_encrypted_standard(), server->total_read is set to the
> > > > total packet length before calling decrypt_raw_data(). The total
> > > > packet length includes the transform header but the idea of updating
> > > > server->total_read after decryption is to set it to a decrypted packet
> > > > size without the transform header (see memmove in decrypt_raw_data).
> > > >
> > > > We would probably need to backport the patch to stable trees, so, I
> > > > would try to make the smallest possible change in terms of scope -
> > > > meaning just fixing the read codepath with esize mount option turned
> > > > on.
> > > >
> > > > --
> > > > Best regards,
> > > > Pavel Shilovsky
> > > >
> > > > ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > > >
> > > > > Hi Pavel,
> > > > >
> > > > > In receive_encrypted_standard function also, server->total_read is
> > > > > updated properly before calling decrypt_raw_data. So, no need to
> > > > > update the same field again.
> > > > >
> > > > > I have checked all instances where decrypt_raw_data is used and didn’t
> > > > > find any issue.
> > > > >
> > > > > Regards,
> > > > > Rohith
> > > > >
> > > > > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > > >
> > > > > > Hi Rohith,
> > > > > >
> > > > > > Thanks for catching the problem and proposing the patch!
> > > > > >
> > > > > > I think there is a problem with just removing server->total_read
> > > > > > updates inside decrypt_raw_data():
> > > > > >
> > > > > > The same function is used in receive_encrypted_standard() which then
> > > > > > calls cifs_handle_standard(). The latter uses server->total_read in at
> > > > > > least two places: in server->ops->check_message and cifs_dump_mem().
> > > > > >
> > > > > > There may be other places in the code that assume server->total_read
> > > > > > to be correct. I would avoid simply removing this in all code paths
> > > > > > and would rather make a more specific fix for the offloaded reads.
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > > Pavel Shilovsky
> > > > > >
> > > > > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > > > > > >
> > > > > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > > > > > >
> > > > > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > > > > > <rohiths.msft@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi All,
> > > > > > > >
> > > > > > > > With the "esize" mount option, I observed data corruption and cifs
> > > > > > > > reconnects during performance tests.
> > > > > > > >
> > > > > > > > TCP server info field server->total_read is modified parallely by
> > > > > > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > > > > > is used in calculation to discard the remaining data of PDU which is
> > > > > > > > not read into memory.
> > > > > > > >
> > > > > > > > Because of parallel modification, “server->total_read” value got
> > > > > > > > corrupted and instead of discarding the remaining data, it discarded
> > > > > > > > some valid data from the next PDU.
> > > > > > > >
> > > > > > > > server->total_read field is already updated properly during read from
> > > > > > > > socket. So, no need to update the same field again after decryption.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Rohith
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Steve



-- 
Thanks,

Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Pavel Shilovsky]

Thanks. Should we add "cifs: " prefix to the beginning of the patch
title? Given that the patch goes to stable this may worth doing
another rebase.
--
Best regards,
Pavel Shilovsky

вт, 20 окт. 2020 г. в 15:08, Steve French <smfrench@gmail.com>:
>
> Added cc:stable 5.4+ and the 2 Reviewed-bys and merged into
> cifs-2.6.git for-next
>
> On Tue, Oct 20, 2020 at 4:43 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > Any ideas about stable? esize mount option went into kernel v5.4.
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > пн, 19 окт. 2020 г. в 09:48, Pavel Shilovsky <piastryyy@gmail.com>:
> > >
> > > Thanks for the patch!
> > >
> > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> > >
> > > пн, 19 окт. 2020 г. в 00:28, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > >
> > > > Hi Pavel,
> > > >
> > > > Corrected the patch with the suggested changes.
> > > > Attached the patch.
> > > >
> > > > Regards,
> > > > Rohith
> > > >
> > > > On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > >
> > > > > In receive_encrypted_standard(), server->total_read is set to the
> > > > > total packet length before calling decrypt_raw_data(). The total
> > > > > packet length includes the transform header but the idea of updating
> > > > > server->total_read after decryption is to set it to a decrypted packet
> > > > > size without the transform header (see memmove in decrypt_raw_data).
> > > > >
> > > > > We would probably need to backport the patch to stable trees, so, I
> > > > > would try to make the smallest possible change in terms of scope -
> > > > > meaning just fixing the read codepath with esize mount option turned
> > > > > on.
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Pavel Shilovsky
> > > > >
> > > > > ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > > > >
> > > > > > Hi Pavel,
> > > > > >
> > > > > > In receive_encrypted_standard function also, server->total_read is
> > > > > > updated properly before calling decrypt_raw_data. So, no need to
> > > > > > update the same field again.
> > > > > >
> > > > > > I have checked all instances where decrypt_raw_data is used and didn’t
> > > > > > find any issue.
> > > > > >
> > > > > > Regards,
> > > > > > Rohith
> > > > > >
> > > > > > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Rohith,
> > > > > > >
> > > > > > > Thanks for catching the problem and proposing the patch!
> > > > > > >
> > > > > > > I think there is a problem with just removing server->total_read
> > > > > > > updates inside decrypt_raw_data():
> > > > > > >
> > > > > > > The same function is used in receive_encrypted_standard() which then
> > > > > > > calls cifs_handle_standard(). The latter uses server->total_read in at
> > > > > > > least two places: in server->ops->check_message and cifs_dump_mem().
> > > > > > >
> > > > > > > There may be other places in the code that assume server->total_read
> > > > > > > to be correct. I would avoid simply removing this in all code paths
> > > > > > > and would rather make a more specific fix for the offloaded reads.
> > > > > > >
> > > > > > > --
> > > > > > > Best regards,
> > > > > > > Pavel Shilovsky
> > > > > > >
> > > > > > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > > > > > > >
> > > > > > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > > > > > > >
> > > > > > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > > > > > > <rohiths.msft@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi All,
> > > > > > > > >
> > > > > > > > > With the "esize" mount option, I observed data corruption and cifs
> > > > > > > > > reconnects during performance tests.
> > > > > > > > >
> > > > > > > > > TCP server info field server->total_read is modified parallely by
> > > > > > > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > > > > > > is used in calculation to discard the remaining data of PDU which is
> > > > > > > > > not read into memory.
> > > > > > > > >
> > > > > > > > > Because of parallel modification, “server->total_read” value got
> > > > > > > > > corrupted and instead of discarding the remaining data, it discarded
> > > > > > > > > some valid data from the next PDU.
> > > > > > > > >
> > > > > > > > > server->total_read field is already updated properly during read from
> > > > > > > > > socket. So, no need to update the same field again after decryption.
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Rohith
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > Steve
>
>
>
> --
> Thanks,
>
> Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Steve French]

updated ... I added SMB3: to the title

62593011247c SMB3: Resolve data corruption of TCP server info fields

(since it was SMB3 only fix doesn't affect cifs)

On Wed, Oct 21, 2020 at 4:03 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Thanks. Should we add "cifs: " prefix to the beginning of the patch
> title? Given that the patch goes to stable this may worth doing
> another rebase.
> --
> Best regards,
> Pavel Shilovsky
>
> вт, 20 окт. 2020 г. в 15:08, Steve French <smfrench@gmail.com>:
> >
> > Added cc:stable 5.4+ and the 2 Reviewed-bys and merged into
> > cifs-2.6.git for-next
> >
> > On Tue, Oct 20, 2020 at 4:43 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> > > Any ideas about stable? esize mount option went into kernel v5.4.
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> > >
> > > пн, 19 окт. 2020 г. в 09:48, Pavel Shilovsky <piastryyy@gmail.com>:
> > > >
> > > > Thanks for the patch!
> > > >
> > > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> > > >
> > > > --
> > > > Best regards,
> > > > Pavel Shilovsky
> > > >
> > > > пн, 19 окт. 2020 г. в 00:28, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > > >
> > > > > Hi Pavel,
> > > > >
> > > > > Corrected the patch with the suggested changes.
> > > > > Attached the patch.
> > > > >
> > > > > Regards,
> > > > > Rohith
> > > > >
> > > > > On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > > >
> > > > > > In receive_encrypted_standard(), server->total_read is set to the
> > > > > > total packet length before calling decrypt_raw_data(). The total
> > > > > > packet length includes the transform header but the idea of updating
> > > > > > server->total_read after decryption is to set it to a decrypted packet
> > > > > > size without the transform header (see memmove in decrypt_raw_data).
> > > > > >
> > > > > > We would probably need to backport the patch to stable trees, so, I
> > > > > > would try to make the smallest possible change in terms of scope -
> > > > > > meaning just fixing the read codepath with esize mount option turned
> > > > > > on.
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > > Pavel Shilovsky
> > > > > >
> > > > > > ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > > > > >
> > > > > > > Hi Pavel,
> > > > > > >
> > > > > > > In receive_encrypted_standard function also, server->total_read is
> > > > > > > updated properly before calling decrypt_raw_data. So, no need to
> > > > > > > update the same field again.
> > > > > > >
> > > > > > > I have checked all instances where decrypt_raw_data is used and didn’t
> > > > > > > find any issue.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Rohith
> > > > > > >
> > > > > > > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Rohith,
> > > > > > > >
> > > > > > > > Thanks for catching the problem and proposing the patch!
> > > > > > > >
> > > > > > > > I think there is a problem with just removing server->total_read
> > > > > > > > updates inside decrypt_raw_data():
> > > > > > > >
> > > > > > > > The same function is used in receive_encrypted_standard() which then
> > > > > > > > calls cifs_handle_standard(). The latter uses server->total_read in at
> > > > > > > > least two places: in server->ops->check_message and cifs_dump_mem().
> > > > > > > >
> > > > > > > > There may be other places in the code that assume server->total_read
> > > > > > > > to be correct. I would avoid simply removing this in all code paths
> > > > > > > > and would rather make a more specific fix for the offloaded reads.
> > > > > > > >
> > > > > > > > --
> > > > > > > > Best regards,
> > > > > > > > Pavel Shilovsky
> > > > > > > >
> > > > > > > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > > > > > > > >
> > > > > > > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > > > > > > > >
> > > > > > > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > > > > > > > <rohiths.msft@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi All,
> > > > > > > > > >
> > > > > > > > > > With the "esize" mount option, I observed data corruption and cifs
> > > > > > > > > > reconnects during performance tests.
> > > > > > > > > >
> > > > > > > > > > TCP server info field server->total_read is modified parallely by
> > > > > > > > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > > > > > > > is used in calculation to discard the remaining data of PDU which is
> > > > > > > > > > not read into memory.
> > > > > > > > > >
> > > > > > > > > > Because of parallel modification, “server->total_read” value got
> > > > > > > > > > corrupted and instead of discarding the remaining data, it discarded
> > > > > > > > > > some valid data from the next PDU.
> > > > > > > > > >
> > > > > > > > > > server->total_read field is already updated properly during read from
> > > > > > > > > > socket. So, no need to update the same field again after decryption.
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Rohith
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Thanks,
> > > > > > > > >
> > > > > > > > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Thanks,

Steve

Re: [PATCH] Resolve data corruption of TCP server info fields

[Pavel Shilovsky]

Sounds good. Thanks!
--
Best regards,
Pavel Shilovsky

ср, 21 окт. 2020 г. в 15:59, Steve French <smfrench@gmail.com>:
>
> updated ... I added SMB3: to the title
>
> 62593011247c SMB3: Resolve data corruption of TCP server info fields
>
> (since it was SMB3 only fix doesn't affect cifs)
>
> On Wed, Oct 21, 2020 at 4:03 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > Thanks. Should we add "cifs: " prefix to the beginning of the patch
> > title? Given that the patch goes to stable this may worth doing
> > another rebase.
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > вт, 20 окт. 2020 г. в 15:08, Steve French <smfrench@gmail.com>:
> > >
> > > Added cc:stable 5.4+ and the 2 Reviewed-bys and merged into
> > > cifs-2.6.git for-next
> > >
> > > On Tue, Oct 20, 2020 at 4:43 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > >
> > > > Any ideas about stable? esize mount option went into kernel v5.4.
> > > >
> > > > --
> > > > Best regards,
> > > > Pavel Shilovsky
> > > >
> > > > пн, 19 окт. 2020 г. в 09:48, Pavel Shilovsky <piastryyy@gmail.com>:
> > > > >
> > > > > Thanks for the patch!
> > > > >
> > > > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Pavel Shilovsky
> > > > >
> > > > > пн, 19 окт. 2020 г. в 00:28, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > > > >
> > > > > > Hi Pavel,
> > > > > >
> > > > > > Corrected the patch with the suggested changes.
> > > > > > Attached the patch.
> > > > > >
> > > > > > Regards,
> > > > > > Rohith
> > > > > >
> > > > > > On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > > > >
> > > > > > > In receive_encrypted_standard(), server->total_read is set to the
> > > > > > > total packet length before calling decrypt_raw_data(). The total
> > > > > > > packet length includes the transform header but the idea of updating
> > > > > > > server->total_read after decryption is to set it to a decrypted packet
> > > > > > > size without the transform header (see memmove in decrypt_raw_data).
> > > > > > >
> > > > > > > We would probably need to backport the patch to stable trees, so, I
> > > > > > > would try to make the smallest possible change in terms of scope -
> > > > > > > meaning just fixing the read codepath with esize mount option turned
> > > > > > > on.
> > > > > > >
> > > > > > > --
> > > > > > > Best regards,
> > > > > > > Pavel Shilovsky
> > > > > > >
> > > > > > > ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@gmail.com>:
> > > > > > > >
> > > > > > > > Hi Pavel,
> > > > > > > >
> > > > > > > > In receive_encrypted_standard function also, server->total_read is
> > > > > > > > updated properly before calling decrypt_raw_data. So, no need to
> > > > > > > > update the same field again.
> > > > > > > >
> > > > > > > > I have checked all instances where decrypt_raw_data is used and didn’t
> > > > > > > > find any issue.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Rohith
> > > > > > > >
> > > > > > > > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Rohith,
> > > > > > > > >
> > > > > > > > > Thanks for catching the problem and proposing the patch!
> > > > > > > > >
> > > > > > > > > I think there is a problem with just removing server->total_read
> > > > > > > > > updates inside decrypt_raw_data():
> > > > > > > > >
> > > > > > > > > The same function is used in receive_encrypted_standard() which then
> > > > > > > > > calls cifs_handle_standard(). The latter uses server->total_read in at
> > > > > > > > > least two places: in server->ops->check_message and cifs_dump_mem().
> > > > > > > > >
> > > > > > > > > There may be other places in the code that assume server->total_read
> > > > > > > > > to be correct. I would avoid simply removing this in all code paths
> > > > > > > > > and would rather make a more specific fix for the offloaded reads.
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Best regards,
> > > > > > > > > Pavel Shilovsky
> > > > > > > > >
> > > > > > > > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@gmail.com>:
> > > > > > > > > >
> > > > > > > > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next
> > > > > > > > > >
> > > > > > > > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula
> > > > > > > > > > <rohiths.msft@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi All,
> > > > > > > > > > >
> > > > > > > > > > > With the "esize" mount option, I observed data corruption and cifs
> > > > > > > > > > > reconnects during performance tests.
> > > > > > > > > > >
> > > > > > > > > > > TCP server info field server->total_read is modified parallely by
> > > > > > > > > > > demultiplex thread and decrypt offload worker thread. server->total_read
> > > > > > > > > > > is used in calculation to discard the remaining data of PDU which is
> > > > > > > > > > > not read into memory.
> > > > > > > > > > >
> > > > > > > > > > > Because of parallel modification, “server->total_read” value got
> > > > > > > > > > > corrupted and instead of discarding the remaining data, it discarded
> > > > > > > > > > > some valid data from the next PDU.
> > > > > > > > > > >
> > > > > > > > > > > server->total_read field is already updated properly during read from
> > > > > > > > > > > socket. So, no need to update the same field again after decryption.
> > > > > > > > > > >
> > > > > > > > > > > Regards,
> > > > > > > > > > > Rohith
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Thanks,
> > > > > > > > > >
> > > > > > > > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve