Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French <smfrench@gmail.com>]

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

Attached updated patch which also adds check to make sure max write
size is at least 4K

On Tue, Feb 6, 2024 at 10:58 PM Steve French <smfrench@gmail.com> wrote:
>
> > his netfslib work looks like quite a big refactor. Is there any plans to land this in 6.8? Or will this be 6.9 / later?
>
> I don't object to putting them in 6.8 if there was additional review
> (it is quite large), but I expect there would be pushback, and am
> concerned that David's status update did still show some TODOs for
> that patch series.  I do plan to upload his most recent set to
> cifs-2.6.git for-next later in the week and target would be for
> merging the patch series would be 6.9-rc1 unless major issues were
> found in review or testing
>
> On Tue, Feb 6, 2024 at 9:42 PM Matthew Ruffell
> <matthew.ruffell@canonical.com> wrote:
> >
> > I have bisected the issue, and found the commit that introduces the problem:
> >
> > commit d08089f649a0cfb2099c8551ac47eef0cc23fdf2
> > Author: David Howells <dhowells@redhat.com>
> > Date:   Mon Jan 24 21:13:24 2022 +0000
> > Subject: cifs: Change the I/O paths to use an iterator rather than a page list
> > Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d08089f649a0cfb2099c8551ac47eef0cc23fdf2
> >
> > $ git describe --contains d08089f649a0cfb2099c8551ac47eef0cc23fdf2
> > v6.3-rc1~136^2~7
> >
> > David, I also tried your cifs-netfs tree available here:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=cifs-netfs
> >
> > This tree solves the issue. Specifically:
> >
> > commit 34efb2a814f1882ddb4a518c2e8a54db119fd0d8
> > Author: David Howells <dhowells@redhat.com>
> > Date:   Fri Oct 6 18:29:59 2023 +0100
> > Subject: cifs: Cut over to using netfslib
> > Link: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=cifs-netfs&id=34efb2a814f1882ddb4a518c2e8a54db119fd0d8
> >
> > This netfslib work looks like quite a big refactor. Is there any plans to land this in 6.8? Or will this be 6.9 / later?
> >
> > Do you have any suggestions on how to fix this with a smaller delta in 6.3 -> 6.8-rc3 that the stable kernels can use?
> >
> > Thanks,
> > Matthew
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb-Fix-regression-in-writes-when-non-standard-maxim.patch --]
[-- Type: text/x-patch, Size: 3278 bytes --]

From f2ca862debd8b9875b5448553be0f2178fc4231f Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 6 Feb 2024 16:34:22 -0600
Subject: [PATCH] smb: Fix regression in writes when non-standard maximum write
 size negotiated

The conversion to netfs in the 6.3 kernel caused a regression when
maximum write size is set by the server to an unexpected value which is
not a multiple of 4096 (similarly if the user overrides the maximum
write size by setting mount parm "wsize", but sets it to a value that
is not a multiple of 4096).  When negotiated write size is not a
multiple of 4096 the netfs code can skip the end of the final
page when doing large sequential writes, causing data corruption.

This section of code is being rewritten/removed due to a large
netfs change, but until that point (ie for the 6.3 kernel until now)
we can not support non-standard maximum write sizes.

Add a warning if a user specifies a wsize on mount that is not
a multiple of 4096, and also add a change where we round down the
maximum write size if the server negotiates a value that is not
a multiple of 4096 (we also have to check to make sure that
we do not round it down to zero).

Reported-by: R. Diez" <rdiez-2006@rd10.de>
Fixes: d08089f649a0 ("cifs: Change the I/O paths to use an iterator rather than a page list")
Suggested-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Cc: stable@vger.kernel.org # v6.3+
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/connect.c    | 13 +++++++++++--
 fs/smb/client/fs_context.c |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index bfd568f89710..46b3aeebfbf2 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -3438,8 +3438,17 @@ int cifs_mount_get_tcon(struct cifs_mount_ctx *mnt_ctx)
 	 * the user on mount
 	 */
 	if ((cifs_sb->ctx->wsize == 0) ||
-	    (cifs_sb->ctx->wsize > server->ops->negotiate_wsize(tcon, ctx)))
-		cifs_sb->ctx->wsize = server->ops->negotiate_wsize(tcon, ctx);
+	    (cifs_sb->ctx->wsize > server->ops->negotiate_wsize(tcon, ctx))) {
+		cifs_sb->ctx->wsize = round_down(server->ops->negotiate_wsize(tcon, ctx), 4096);
+		/*
+		 * in the very unlikely event that the server sent a max write size under 4K,
+		 * (which would get rounded down to 0) then reset wsize to absolute minimum ie 4096
+		 */
+		if (cifs_sb->ctx->wsize == 0) {
+			cifs_sb->ctx->wsize = 4096;
+			cifs_dbg(VFS, "wsize too small, reset to minimum ie 4096\n");
+		}
+	}
 	if ((cifs_sb->ctx->rsize == 0) ||
 	    (cifs_sb->ctx->rsize > server->ops->negotiate_rsize(tcon, ctx)))
 		cifs_sb->ctx->rsize = server->ops->negotiate_rsize(tcon, ctx);
diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 52cbef2eeb28..600a77052c3b 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1111,6 +1111,8 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 	case Opt_wsize:
 		ctx->wsize = result.uint_32;
 		ctx->got_wsize = true;
+		if (round_up(ctx->wsize, 4096) != ctx->wsize)
+			cifs_dbg(VFS, "wsize should be a multiple of 4096\n");
 		break;
 	case Opt_acregmax:
 		ctx->acregmax = HZ * result.uint_32;
-- 
2.40.1