SMB 1.0 broken between Kernel versions 6.2 and 6.5

SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

Hi all:

I just wanted to bring to your attention that SMB 1.0 writes appear to have gone broken between Kernel versions 6.2 and 6.5. Writing about 111 kBytes of data to a file is not reliable any more, you get 5 holes with binary zeros at regular intervals.

Here is the research about this bug I have done so far:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

Do you know if this is also broken in current mainline e.g. 6.7 or
6.8-rc2 (for Ubuntu and some other distros it is fairly easy to
download from their website current kernel packages to make testing
easy).

David,
any thoughts whether this could be related to folios/netfs changes?

On Sat, Feb 3, 2024 at 4:23 PM R. Diez <rdiez-2006@rd10.de> wrote:
>
> Hi all:
>
> I just wanted to bring to your attention that SMB 1.0 writes appear to have gone broken between Kernel versions 6.2 and 6.5. Writing about 111 kBytes of data to a file is not reliable any more, you get 5 holes with binary zeros at regular intervals.
>
> Here is the research about this bug I have done so far:
>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634
>
> Regards,
>    rdiez
>
>


-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

> Do you know if this is also broken in current mainline e.g. 6.7 or
> 6.8-rc2 (for Ubuntu and some other distros it is fairly easy to
> download from their website current kernel packages to make testing
> easy).

I don't know, I didn't test so much. I am not familiar with this kind of Kernel work.

I found some newer Kernels for Ubuntu here:

https://kernel.ubuntu.com/mainline/?C=N;O=D

But it looks like many of the self-tests fail, which does not help build confidence. I fear breaking my Linux PCs, which I need daily.

I'll see if I can find some time and some spare computer to try this out, but it may take some time.

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

> Do you know if this is also broken in current mainline e.g. 6.7 or
> [...]

I just tested on an older laptop with the same Ubuntu MATE 22.04.3. I tried with the latest Linux Kernel build I found:

Linux rdiez-L2017 6.8.0-060800rc3-generic #202402041232 SMP PREEMPT_DYNAMIC Sun Feb  4 12:41:05 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

The test text file was corrupted in the same way. In fact, the resulting corrupted file compares (with 'cmp') with the other laptop running 6.5.0-15-generic #15~22.04.1-Ubuntu.

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

>> Do you know if this is also broken in current mainline e.g. 6.7 or
>> [...]

Matthew Ruffell has confirmed this bug with a different CIFS client and server, and has narrowed down further the affected Kernel versions, see the bug report I mentioned in my first post:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[David Howells]

Steve French <smfrench@gmail.com> wrote:

> any thoughts whether this could be related to folios/netfs changes?

Unlikely as you didn't take them for the last merge window, let alone 6.2.

Daivd

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> > any thoughts whether this could be related to folios/netfs changes?
> 
> Unlikely as you didn't take them for the last merge window, let alone 6.2.

That said, you did take my iteratorisation patches in 6.3 - but that shouldn't
affect 6.2 unless someone backported them.

David

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

>> Unlikely as you didn't take them for the last merge window, let alone 6.2.
> 
> That said, you did take my iteratorisation patches in 6.3 - but that shouldn't
> affect 6.2 unless someone backported them.

Please note that 6.2 is not affected, the breakage occurred afterwards. See the bug report here for more information:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[David Howells]

R. Diez <rdiez-2006@rd10.de> wrote:

> >> Unlikely as you didn't take them for the last merge window, let alone 6.2.
> > That said, you did take my iteratorisation patches in 6.3 - but that
> > shouldn't
> > affect 6.2 unless someone backported them.
> 
> Please note that 6.2 is not affected, the breakage occurred afterwards. See
> the bug report here for more information:

Ah, right - so the 6.2 in the subject is the last definitely working version.

David

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

I can reproduce this now with a simple smb1 cp - but only with the small wsize
ie mount option: wsize=16850.  As mentioned earlier the problem is
that we see a 16K write, then the next write is at the wrong offset
(leaving a hole)

(it worked for SMB1 with default wsize)

so focus is on these two functions in the call stack:

[19085.611988]  cifs_async_writev+0x90/0x380 [cifs]
[19085.612083]  cifs_writepages_region+0xadc/0xbb0 [cifs]

On Mon, Feb 5, 2024 at 3:37 AM R. Diez <rdiez-2006@rd10.de> wrote:
>
>
> >> Unlikely as you didn't take them for the last merge window, let alone 6.2.
> >
> > That said, you did take my iteratorisation patches in 6.3 - but that shouldn't
> > affect 6.2 unless someone backported them.
>
> Please note that 6.2 is not affected, the breakage occurred afterwards. See the bug report here for more information:
>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634
>
> Regards,
>    rdiez
>


-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

In my additional experiments I could reproduce this but only with
wsize < 32768 but it wasn't SMB1 specific - I could reproduce it with
current dialects (smb3.1.1 e.g.) too not just SMB1 - so it is more
about you picking  small wsize that found the bug than an SMB1
specific problem.

On Mon, Feb 5, 2024 at 7:30 PM Steve French <smfrench@gmail.com> wrote:
>
> I can reproduce this now with a simple smb1 cp - but only with the small wsize
> ie mount option: wsize=16850.  As mentioned earlier the problem is
> that we see a 16K write, then the next write is at the wrong offset
> (leaving a hole)
>
> (it worked for SMB1 with default wsize)
>
> so focus is on these two functions in the call stack:
>
> [19085.611988]  cifs_async_writev+0x90/0x380 [cifs]
> [19085.612083]  cifs_writepages_region+0xadc/0xbb0 [cifs]
>
> On Mon, Feb 5, 2024 at 3:37 AM R. Diez <rdiez-2006@rd10.de> wrote:
> >
> >
> > >> Unlikely as you didn't take them for the last merge window, let alone 6.2.
> > >
> > > That said, you did take my iteratorisation patches in 6.3 - but that shouldn't
> > > affect 6.2 unless someone backported them.
> >
> > Please note that 6.2 is not affected, the breakage occurred afterwards. See the bug report here for more information:
> >
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634
> >
> > Regards,
> >    rdiez
> >
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

Digging deeper into this it looks like the problem is not the size
being bigger than 32K but picking a write size (wsize) that is not a
multiple of page size (4096).  I was able to reproduce this e.g. with
wsize=70000 but not with 69632 (ie a multiple of page size, 17*4096)

On Mon, Feb 5, 2024 at 10:05 PM Steve French <smfrench@gmail.com> wrote:
>
> In my additional experiments I could reproduce this but only with
> wsize < 32768 but it wasn't SMB1 specific - I could reproduce it with
> current dialects (smb3.1.1 e.g.) too not just SMB1 - so it is more
> about you picking  small wsize that found the bug than an SMB1
> specific problem.
>
> On Mon, Feb 5, 2024 at 7:30 PM Steve French <smfrench@gmail.com> wrote:
> >
> > I can reproduce this now with a simple smb1 cp - but only with the small wsize
> > ie mount option: wsize=16850.  As mentioned earlier the problem is
> > that we see a 16K write, then the next write is at the wrong offset
> > (leaving a hole)
> >
> > (it worked for SMB1 with default wsize)
> >
> > so focus is on these two functions in the call stack:
> >
> > [19085.611988]  cifs_async_writev+0x90/0x380 [cifs]
> > [19085.612083]  cifs_writepages_region+0xadc/0xbb0 [cifs]
> >
> > On Mon, Feb 5, 2024 at 3:37 AM R. Diez <rdiez-2006@rd10.de> wrote:
> > >
> > >
> > > >> Unlikely as you didn't take them for the last merge window, let alone 6.2.
> > > >
> > > > That said, you did take my iteratorisation patches in 6.3 - but that shouldn't
> > > > affect 6.2 unless someone backported them.
> > >
> > > Please note that 6.2 is not affected, the breakage occurred afterwards. See the bug report here for more information:
> > >
> > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634
> > >
> > > Regards,
> > >    rdiez
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[ronnie sahlberg]

On Tue, 6 Feb 2024 at 15:52, Steve French <smfrench@gmail.com> wrote:
>
> Digging deeper into this it looks like the problem is not the size
> being bigger than 32K but picking a write size (wsize) that is not a
> multiple of page size (4096).  I was able to reproduce this e.g. with
> wsize=70000 but not with 69632 (ie a multiple of page size, 17*4096)

Probably the easiest/quickest fix is to enforce rsize/wsize MUST be a
multiple of page-size ?
Is there any reason to support other sizes?
In the mount api you could just round these sizes up to the nearest
page size multiple.

>
> On Mon, Feb 5, 2024 at 10:05 PM Steve French <smfrench@gmail.com> wrote:
> >
> > In my additional experiments I could reproduce this but only with
> > wsize < 32768 but it wasn't SMB1 specific - I could reproduce it with
> > current dialects (smb3.1.1 e.g.) too not just SMB1 - so it is more
> > about you picking  small wsize that found the bug than an SMB1
> > specific problem.
> >
> > On Mon, Feb 5, 2024 at 7:30 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > I can reproduce this now with a simple smb1 cp - but only with the small wsize
> > > ie mount option: wsize=16850.  As mentioned earlier the problem is
> > > that we see a 16K write, then the next write is at the wrong offset
> > > (leaving a hole)
> > >
> > > (it worked for SMB1 with default wsize)
> > >
> > > so focus is on these two functions in the call stack:
> > >
> > > [19085.611988]  cifs_async_writev+0x90/0x380 [cifs]
> > > [19085.612083]  cifs_writepages_region+0xadc/0xbb0 [cifs]
> > >
> > > On Mon, Feb 5, 2024 at 3:37 AM R. Diez <rdiez-2006@rd10.de> wrote:
> > > >
> > > >
> > > > >> Unlikely as you didn't take them for the last merge window, let alone 6.2.
> > > > >
> > > > > That said, you did take my iteratorisation patches in 6.3 - but that shouldn't
> > > > > affect 6.2 unless someone backported them.
> > > >
> > > > Please note that 6.2 is not affected, the breakage occurred afterwards. See the bug report here for more information:
> > > >
> > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634
> > > >
> > > > Regards,
> > > >    rdiez
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve
>

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

> Probably the easiest/quickest fix is to enforce rsize/wsize MUST be a
> multiple of page-size ?
> Is there any reason to support other sizes?
> In the mount api you could just round these sizes up to the nearest
> page size multiple.

This feels like a quick shot which may land near your foot.

The documentation does not say why you may want to choose a non-default wsize, but I could think of a scenario where the packet size is chosen based on some network packet fragmentation size. If you suddenly increase the sizes, you may trigger a network performance problem.

Besides, the page size is not always fixed. I am assuming we are talking about PAGE_SIZE on the Linux kernel here, but I don't really know anything about this code. Most architectures have 4 KiB page size, but there is the odd one with 8 KiB, and there is a "kernel-64k" variant for ARM with a 64 KiB page size.

Before rounding any sizes up, I suggest finding out where the issue is, and whether it could be fixed respecting the user-chosen sizes.

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

> I can reproduce this now with a simple smb1 cp - but only with the small wsize
> ie mount option: wsize=16850.  As mentioned earlier the problem is
> that we see a 16K write, then the next write is at the wrong offset
> (leaving a hole)
> 
> (it worked for SMB1 with default wsize)

As stated twice in the original bug report on Launchpad, I didn't set a wsize, so the wsize=16580 must be a default value coming from somewhere.

This is not the first time we have a little communication issue in this thread. Would you guys please read the bug report once again carefully, and even better, subscribe to it?

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[David Howells]

Out of interest, are you able to try an arbitrary kernel?  If so, would it be
possible to see if:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=cifs-netfs

fixes it?

Thanks,
David

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

> Out of interest, are you able to try an arbitrary kernel?

I'm afraid that is beyond my Linux Kernel abilities. I am just a user able to poke in some configuration files, and maybe fight a little with Synaptic / APT.

But maybe the Ubuntu guys can. You can reach the guy doing some extra testing here:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[David Howells]

R. Diez <rdiez-2006@rd10.de> wrote:

> > Out of interest, are you able to try an arbitrary kernel?
> 
> I'm afraid that is beyond my Linux Kernel abilities. I am just a user able to poke in some configuration files, and maybe fight a little with Synaptic / APT.
> 
> But maybe the Ubuntu guys can. You can reach the guy doing some extra testing here:
> 
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634

No worries.  Steve can probably try that.

Thanks,
David

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

My best guess so far is that this change caused the regression:

$ git show ab7362d04d7
commit ab7362d04d7c14923420c1e19e889da512a65cd7
Author: David Howells <dhowells@redhat.com>
Date:   Fri Feb 24 14:31:15 2023 +0000

    cifs: Fix cifs_writepages_region()

    Fix the cifs_writepages_region() to just jump over members of the batch
    that have been cleaned up rather than counting them as skipped.

    Unlike the other "skip_write" cases, this situation happens even for
    WB_SYNC_ALL, simply because the page has either been cleaned by somebody
    else, or was truncated.

    So in this case we're not "skipping" the write, we simply no longer need
    any write at all, so it's very different from the other skip_write cases.

    And we definitely shouldn't stop writing the rest just because of too
    many of these cases (or because we want to be rescheduled).

    Fixes: 3822a7c40997 ("Merge tag 'mm-stable-2023-02-20-13-37' of
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm")
    Signed-off-by: David Howells <dhowells@redhat.com>
    Link: https://lore.kernel.org/lkml/2213409.1677249075@warthog.procyon.org.uk/
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 5365a3299088..ebfcaae8c437 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2893,8 +2893,9 @@ static int cifs_writepages_region(struct
address_space *mapping,

                        if (folio_mapping(folio) != mapping ||
                            !folio_test_dirty(folio)) {
+                               start += folio_size(folio);
                                folio_unlock(folio);
-                               goto skip_write;
+                               continue;
                        }

                        if (folio_test_writeback(folio) ||

On Tue, Feb 6, 2024 at 2:22 AM David Howells <dhowells@redhat.com> wrote:
>
> R. Diez <rdiez-2006@rd10.de> wrote:
>
> > > Out of interest, are you able to try an arbitrary kernel?
> >
> > I'm afraid that is beyond my Linux Kernel abilities. I am just a user able to poke in some configuration files, and maybe fight a little with Synaptic / APT.
> >
> > But maybe the Ubuntu guys can. You can reach the guy doing some extra testing here:
> >
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2049634
>
> No worries.  Steve can probably try that.
>
> Thanks,
> David
>


-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Matthew Ruffell]

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

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

> 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

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

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

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

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

Updated patch - now use PAGE_SIZE instead of hard coding to 4096.

See attached

On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
>
> 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



-- 
Thanks,

Steve

[-- Attachment #2: .0001-smb-Fix-regression-in-writes-when-non-standard-maxim.patch.swp --]
[-- Type: application/octet-stream, Size: 12288 bytes --]

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

Hallo Steve:

I wonder what would happen if the SMB server said it can take a maximum of 2048 Bytes, and you insist on 4096. Would the connection still work later on? Wouldn't it be better to abort the connection with a descriptive error message?

You stated that this scenario is very unlikely, but my Linux client is negotiating 16580 bytes at the moment, so if PAGE_SIZE happens to be 64 KiB, that wouldn't be very unlikely any more.

About this other change:

if (round_up(ctx->wsize, 4096) != ctx->wsize)
   cifs_dbg(VFS, "wsize should be a multiple of 4096\n")

All my SMB connections have been automated, therefore I am very unlikely to realise of such a warning. It would also mislead people using the current Kernel versions into thinking that this limitation is there to stay.

If the SMB client cannot really honour the user request, wouldn't it be better to fail? In any case, how about mentioning in the error or warning message that this is only a temporary limitation?

The second version of your patch file looks like a VIM swap file. I gather you attached the wrong file. The best way to fix this is obviously to switch to Emacs. ;-)

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[ronnie sahlberg]

On Wed, 7 Feb 2024 at 18:56, R. Diez <rdiez-2006@rd10.de> wrote:
>
> Hallo Steve:
>
> I wonder what would happen if the SMB server said it can take a maximum of 2048 Bytes, and you insist on 4096. Would the connection still work later on? Wouldn't it be better to abort the connection with a descriptive error message?

There are no servers that we would care about that have limits that
low. And if there were we could just open a bug against the server to
fix it server-side.

If a server refuses to work with anything larger than 2048 bytes it is
completely valid to just shutdown the session and refuse to connect to
the server.

>
> You stated that this scenario is very unlikely, but my Linux client is negotiating 16580 bytes at the moment, so if PAGE_SIZE happens to be 64 KiB, that wouldn't be very unlikely any more.
>
> About this other change:
>
> if (round_up(ctx->wsize, 4096) != ctx->wsize)
>    cifs_dbg(VFS, "wsize should be a multiple of 4096\n")
>
> All my SMB connections have been automated, therefore I am very unlikely to realise of such a warning. It would also mislead people using the current Kernel versions into thinking that this limitation is there to stay.
>
> If the SMB client cannot really honour the user request, wouldn't it be better to fail? In any case, how about mentioning in the error or warning message that this is only a temporary limitation?
>
> The second version of your patch file looks like a VIM swap file. I gather you attached the wrong file. The best way to fix this is obviously to switch to Emacs. ;-)
>
> Regards,
>    rdiez
>
>

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

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

I had attached the wrong file - reattaching the correct patch (ie that
updates the previous version to use PAGE_SIZE instead of 4096)

On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
>
> Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
>
> See attached
>
> On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> >
> > 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
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb-Fix-regression-in-writes-when-non-standard-maxim.patch --]
[-- Type: text/x-patch, Size: 3390 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 01/26] 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>
Acked-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), PAGE_SIZE);
+		/*
+		 * in the very unlikely event that the server sent a max write size under PAGE_SIZE,
+		 * (which would get rounded down to 0) then reset wsize to absolute minimum eg 4096
+		 */
+		if (cifs_sb->ctx->wsize == 0) {
+			cifs_sb->ctx->wsize = PAGE_SIZE;
+			cifs_dbg(VFS, "wsize too small, reset to minimum ie PAGE_SIZE, usually 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, PAGE_SIZE) != ctx->wsize)
+			cifs_dbg(VFS, "wsize should be a multiple of 4096 (PAGE_SIZE)\n");
 		break;
 	case Opt_acregmax:
 		ctx->acregmax = HZ * result.uint_32;
-- 
2.40.1

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Matthew Ruffell]

Hi Steve,

I built your latest patch ontop of 6.8-rc3, but the problem still persists.

Looking at dmesg, I see the debug statement from the second hunk, but not from
the first hunk, so I don't believe that wsize was ever rounded down to
PAGE_SIZE.

[  541.918267] Use of the less secure dialect vers=1.0 is not
recommended unless required for access to very old servers
[  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
not recommended unless required for access to very old servers
[  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
[  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare

$ sha256sum sambashare/testdata.txt
9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
sambashare/testdata.txt
$ less sambashare/testdata.txt
...
8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
...

Would you be able compile and test your patch and see if we enter the logic from
the first hunk?

I'll be happy to test a V2 tomorrow.

Thanks,
Matthew

On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
>
> I had attached the wrong file - reattaching the correct patch (ie that
> updates the previous version to use PAGE_SIZE instead of 4096)
>
> On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> >
> > See attached
> >
> > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > 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
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

Are you specifying "wsize" on the mount in your example?  The intent
of the patch is to warn the user using a non-recommended wsize (since
the user can control and fix that) but to force round_down when the
server sends a dangerous wsize (ie one that is not a multiple of
4096).

On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
<matthew.ruffell@canonical.com> wrote:
>
> Hi Steve,
>
> I built your latest patch ontop of 6.8-rc3, but the problem still persists.
>
> Looking at dmesg, I see the debug statement from the second hunk, but not from
> the first hunk, so I don't believe that wsize was ever rounded down to
> PAGE_SIZE.
>
> [  541.918267] Use of the less secure dialect vers=1.0 is not
> recommended unless required for access to very old servers
> [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> not recommended unless required for access to very old servers
> [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
>
> $ sha256sum sambashare/testdata.txt
> 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> sambashare/testdata.txt
> $ less sambashare/testdata.txt
> ...
> 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> ...
>
> Would you be able compile and test your patch and see if we enter the logic from
> the first hunk?
>
> I'll be happy to test a V2 tomorrow.
>
> Thanks,
> Matthew
>
> On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> >
> > I had attached the wrong file - reattaching the correct patch (ie that
> > updates the previous version to use PAGE_SIZE instead of 4096)
> >
> > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > >
> > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > >
> > > See attached
> > >
> > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > 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
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Matthew Ruffell]

Hi Steve,

Yes, I am specifying "wsize" on the mount in my example, as its a little easier
to reproduce the issue that way.

If the user does set their own "wsize", any value that is not a multiple of
PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
their data (un)intentionally if they happen to specify a wrong value? Especially
since we know about it now. I know there haven't been any other reports in the
year or so between 6.3 and present day, so there probably isn't any users out
there actually setting their own "wsize", but it still feels bad to allow users
to expose themselves to data corruption in this form.

Please consider also rounding down "wsize" set on mount command line to a safe
multiple of PAGE_SIZE. The code will only be around until David's netfslib cut
over is merged anyway.

I built a distro kernel and sent it to R. Diez for testing, so hopefully we will
have some testing performed against an actual SMB server that sends a dangerous
wsize during negotiation. I'll let you know how that goes, or R. Diez, you can
tell us about how it goes here.

Thanks,
Matthew

On Fri, 9 Feb 2024 at 18:38, Steve French <smfrench@gmail.com> wrote:
>
> Are you specifying "wsize" on the mount in your example?  The intent
> of the patch is to warn the user using a non-recommended wsize (since
> the user can control and fix that) but to force round_down when the
> server sends a dangerous wsize (ie one that is not a multiple of
> 4096).
>
> On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
> <matthew.ruffell@canonical.com> wrote:
> >
> > Hi Steve,
> >
> > I built your latest patch ontop of 6.8-rc3, but the problem still persists.
> >
> > Looking at dmesg, I see the debug statement from the second hunk, but not from
> > the first hunk, so I don't believe that wsize was ever rounded down to
> > PAGE_SIZE.
> >
> > [  541.918267] Use of the less secure dialect vers=1.0 is not
> > recommended unless required for access to very old servers
> > [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > not recommended unless required for access to very old servers
> > [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> > [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
> >
> > $ sha256sum sambashare/testdata.txt
> > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> > sambashare/testdata.txt
> > $ less sambashare/testdata.txt
> > ...
> > 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> > 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> > ...
> >
> > Would you be able compile and test your patch and see if we enter the logic from
> > the first hunk?
> >
> > I'll be happy to test a V2 tomorrow.
> >
> > Thanks,
> > Matthew
> >
> > On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> > >
> > > I had attached the wrong file - reattaching the correct patch (ie that
> > > updates the previous version to use PAGE_SIZE instead of 4096)
> > >
> > > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > > >
> > > > See attached
> > > >
> > > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > > >
> > > > > 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
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[R. Diez]

Hallo Matthew:

> [...]
> If the user does set their own "wsize", any value that is not a multiple of
> PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
> their data (un)intentionally if they happen to specify a wrong value?

I already pointed that out in my e-mail dated 07.02.24 together with other potential issues:

https://www.spinics.net/lists/linux-cifs/msg30973.html

I'll recap here:

1) If the user specifies a wsize which is not multiple of PAGE_SIZE, I would abort, instead of issuing a warning. Like you said, it's too risky, you will corrupt data and you may not see the warning in an automated environment where connections are scripted.

2) Whether error or warning, I would state in the message that this is a temporary limitation. This "fix", which is more of a work-around, will probably be used for years, and people are going to think that the multiple of PAGE_SIZE is a permanent limitation in the client or the SMB protocol, which is not the case.

3) I am worried that, if the server states 60 KiB, and the CIFS client rounds it up to 64 KiB, then the connection will no longer work, because the CIFS client is exceeding the maximum that the server stated.

I wouldn't warn, I would just abort the connection in this case too.

With an old Windows Server and a page size of 64 KiB (like some ARM architecture already has), it is no longer an unlikely scenario, it will certainly occur. In my case, the connection negotiated a wsize of 16580, even though the server should actually default to 16644 bytes(?). In any case, well below 64 KiB.


Now that I mentioned misleading messages: The man page for mount.cifs, parameters rsize and wsize, talks about "maximum amount of data the kernel will request", and about the "maximum size that servers will accept". It is not clear that this is a maximum value for the negotiation phase, so 1) you do not have to worry about setting it too high on the Linux client, as the server will not reject it but negotiate it down if necessary (is that true?), and 2) the negotiation result may actually be much lower than the value you requested, but that is fine, as it wasn't really a hard request, but a soft petition.

I suggest that you guys rephrase that man page, in order to prevent other people scratching their heads again.

I would write something along this line: "Maximum amount of data that the kernel will negotiate for read [or write] requests in bytes. Maximum size that servers will negotiate is typically ...".

By the way, the current option naming is quite misleading too. I am guessing that you can specify "wsize=xxx" and then "mount -l" will show "wsize=yyy", leaving you wondering why your value was not actually taken. Or, like it happened this time, other people automatically assume that I specified a wsize, when I didn't. I would have called these parameters "maxwsize" and "negotiatedwsize", to make the distinction clear. I wonder if it is not too late to change the name of the one listed by "mount -l", that is, the "negotiatedwsize".

Regards,
   rdiez

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

On Fri, Feb 9, 2024 at 3:42 AM R. Diez <rdiez-2006@rd10.de> wrote:
<...>
> Now that I mentioned misleading messages: The man page for mount.cifs, parameters rsize and wsize, talks about "maximum amount of data the kernel will request", and about the "maximum size that servers will accept". It is not clear that this is a maximum value for the negotiation phase, so 1) you do not have to worry about setting it too high on the Linux client, as the server will not reject it but negotiate it down if necessary (is that true?), and 2) the negotiation result may actually be much lower than the value you requested, but that is fine, as it wasn't really a hard request, but a soft petition.
>
> I suggest that you guys rephrase that man page, in order to prevent other people scratching their heads again.
>
> I would write something along this line: "Maximum amount of data that the kernel will negotiate for read [or write] requests in bytes. Maximum size that servers will negotiate is typically ...".

That is a good idea - there are also other changes to the mount.cifs
man page to be done (e.g. to go through the mount parameters in
fs/smb/client/fs_context.c and compare with the mount.cifs man page to
see which are missing descriptions)


> By the way, the current option naming is quite misleading too. I am guessing that you can specify "wsize=xxx" and then "mount -l" will show "wsize=yyy", leaving you wondering why your value was not actually taken. Or, like it happened this time, other people automatically assume that I specified a wsize, when I didn't. I would have called these parameters "maxwsize" and "negotiatedwsize", to make the distinction clear. I wonder if it is not too late to change the name of the one listed by "mount -l", that is, the "negotiatedwsize".
>
> Regards,
>    rdiez

The mount parm names wsize= and rsize= were used to match other
filesystems (e.g. nfs) which have similarly named mount params for a
similar purpose.

-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

> > If the user does set their own "wsize", any value that is not a multiple of
> PAGE_SIZE is dangerous right?

Yes for kernels 6.3 through 6.8-rc such a write size (ie that is not a
multiple of page size) can
be dangerous - that is why I added the warning on mount if the user
specifies the
potentially problematic wsize, since the wsize specified on mount
unlike the server
negotiated maximum write size is under the user's control.  The server
negotiated
maximum write size can't be controlled by the user, so for this
temporary fix we are
forced to round it down.   The actually bug is due to a folios/netfs
bug that David or
one of the mm experts may be able to spot (and fix) so for this
temporary workaround
I wanted to do the smaller change here so we don't have to revert it
later. I got close to
finding the actual bug (where the offset was getting reset, rounded up
incorrectly
inside one of the folios routines mentioned earlier in the thread) but
wanted to get something

On Fri, Feb 9, 2024 at 2:51 AM Matthew Ruffell
<matthew.ruffell@canonical.com> wrote:
>
> Hi Steve,
>
> Yes, I am specifying "wsize" on the mount in my example, as its a little easier
> to reproduce the issue that way.
>
> If the user does set their own "wsize", any value that is not a multiple of
> PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
> their data (un)intentionally if they happen to specify a wrong value? Especially
> since we know about it now. I know there haven't been any other reports in the
> year or so between 6.3 and present day, so there probably isn't any users out
> there actually setting their own "wsize", but it still feels bad to allow users
> to expose themselves to data corruption in this form.
>
> Please consider also rounding down "wsize" set on mount command line to a safe
> multiple of PAGE_SIZE. The code will only be around until David's netfslib cut
> over is merged anyway.
>
> I built a distro kernel and sent it to R. Diez for testing, so hopefully we will
> have some testing performed against an actual SMB server that sends a dangerous
> wsize during negotiation. I'll let you know how that goes, or R. Diez, you can
> tell us about how it goes here.
>
> Thanks,
> Matthew
>
> On Fri, 9 Feb 2024 at 18:38, Steve French <smfrench@gmail.com> wrote:
> >
> > Are you specifying "wsize" on the mount in your example?  The intent
> > of the patch is to warn the user using a non-recommended wsize (since
> > the user can control and fix that) but to force round_down when the
> > server sends a dangerous wsize (ie one that is not a multiple of
> > 4096).
> >
> > On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
> > <matthew.ruffell@canonical.com> wrote:
> > >
> > > Hi Steve,
> > >
> > > I built your latest patch ontop of 6.8-rc3, but the problem still persists.
> > >
> > > Looking at dmesg, I see the debug statement from the second hunk, but not from
> > > the first hunk, so I don't believe that wsize was ever rounded down to
> > > PAGE_SIZE.
> > >
> > > [  541.918267] Use of the less secure dialect vers=1.0 is not
> > > recommended unless required for access to very old servers
> > > [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > > not recommended unless required for access to very old servers
> > > [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> > > [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
> > >
> > > $ sha256sum sambashare/testdata.txt
> > > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> > > sambashare/testdata.txt
> > > $ less sambashare/testdata.txt
> > > ...
> > > 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> > > 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> > > ...
> > >
> > > Would you be able compile and test your patch and see if we enter the logic from
> > > the first hunk?
> > >
> > > I'll be happy to test a V2 tomorrow.
> > >
> > > Thanks,
> > > Matthew
> > >
> > > On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > I had attached the wrong file - reattaching the correct patch (ie that
> > > > updates the previous version to use PAGE_SIZE instead of 4096)
> > > >
> > > > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > > > >
> > > > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > > > >
> > > > > See attached
> > > > >
> > > > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > 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
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > >
> > > > > Steve
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Thanks,

Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

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

Minor update to patch to work around the folios/netfs data corruption.

In addition to printing the warning if "wsize=" is specified on mount
with a size that is not a multiple of PAGE_SIZE, it also rounds the
wsize down to the nearest multiple of PAGE_SIZE (as it was already
doing if the server tried to negotiate a wsize that was not a multiple
of PAGE_SIZE).

On Fri, Feb 9, 2024 at 2:25 PM Steve French <smfrench@gmail.com> wrote:
>
> > > If the user does set their own "wsize", any value that is not a multiple of
> > PAGE_SIZE is dangerous right?
>
> Yes for kernels 6.3 through 6.8-rc such a write size (ie that is not a
> multiple of page size) can
> be dangerous - that is why I added the warning on mount if the user
> specifies the
> potentially problematic wsize, since the wsize specified on mount
> unlike the server
> negotiated maximum write size is under the user's control.  The server
> negotiated
> maximum write size can't be controlled by the user, so for this
> temporary fix we are
> forced to round it down.   The actually bug is due to a folios/netfs
> bug that David or
> one of the mm experts may be able to spot (and fix) so for this
> temporary workaround
> I wanted to do the smaller change here so we don't have to revert it
> later. I got close to
> finding the actual bug (where the offset was getting reset, rounded up
> incorrectly
> inside one of the folios routines mentioned earlier in the thread) but
> wanted to get something
>
> On Fri, Feb 9, 2024 at 2:51 AM Matthew Ruffell
> <matthew.ruffell@canonical.com> wrote:
> >
> > Hi Steve,
> >
> > Yes, I am specifying "wsize" on the mount in my example, as its a little easier
> > to reproduce the issue that way.
> >
> > If the user does set their own "wsize", any value that is not a multiple of
> > PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
> > their data (un)intentionally if they happen to specify a wrong value? Especially
> > since we know about it now. I know there haven't been any other reports in the
> > year or so between 6.3 and present day, so there probably isn't any users out
> > there actually setting their own "wsize", but it still feels bad to allow users
> > to expose themselves to data corruption in this form.
> >
> > Please consider also rounding down "wsize" set on mount command line to a safe
> > multiple of PAGE_SIZE. The code will only be around until David's netfslib cut
> > over is merged anyway.
> >
> > I built a distro kernel and sent it to R. Diez for testing, so hopefully we will
> > have some testing performed against an actual SMB server that sends a dangerous
> > wsize during negotiation. I'll let you know how that goes, or R. Diez, you can
> > tell us about how it goes here.
> >
> > Thanks,
> > Matthew
> >
> > On Fri, 9 Feb 2024 at 18:38, Steve French <smfrench@gmail.com> wrote:
> > >
> > > Are you specifying "wsize" on the mount in your example?  The intent
> > > of the patch is to warn the user using a non-recommended wsize (since
> > > the user can control and fix that) but to force round_down when the
> > > server sends a dangerous wsize (ie one that is not a multiple of
> > > 4096).
> > >
> > > On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
> > > <matthew.ruffell@canonical.com> wrote:
> > > >
> > > > Hi Steve,
> > > >
> > > > I built your latest patch ontop of 6.8-rc3, but the problem still persists.
> > > >
> > > > Looking at dmesg, I see the debug statement from the second hunk, but not from
> > > > the first hunk, so I don't believe that wsize was ever rounded down to
> > > > PAGE_SIZE.
> > > >
> > > > [  541.918267] Use of the less secure dialect vers=1.0 is not
> > > > recommended unless required for access to very old servers
> > > > [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > > > not recommended unless required for access to very old servers
> > > > [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> > > > [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
> > > >
> > > > $ sha256sum sambashare/testdata.txt
> > > > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> > > > sambashare/testdata.txt
> > > > $ less sambashare/testdata.txt
> > > > ...
> > > > 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> > > > 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> > > > ...
> > > >
> > > > Would you be able compile and test your patch and see if we enter the logic from
> > > > the first hunk?
> > > >
> > > > I'll be happy to test a V2 tomorrow.
> > > >
> > > > Thanks,
> > > > Matthew
> > > >
> > > > On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> > > > >
> > > > > I had attached the wrong file - reattaching the correct patch (ie that
> > > > > updates the previous version to use PAGE_SIZE instead of 4096)
> > > > >
> > > > > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > > > > >
> > > > > > See attached
> > > > > >
> > > > > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > >
> > > > > > > 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
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > >
> > > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

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

From 9996b98c6b7d6a48f271335def6e9046ba1a3ad6 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 round down), 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>
Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Reviewed-by: Shyam Prasad N <sprasad@microsoft.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    | 14 ++++++++++++--
 fs/smb/client/fs_context.c | 11 +++++++++++
 fs/smb/client/fs_context.h |  1 -
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index d03253f8f145..ac9595504f4b 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -3444,8 +3444,18 @@ 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), PAGE_SIZE);
+		/*
+		 * in the very unlikely event that the server sent a max write size under PAGE_SIZE,
+		 * (which would get rounded down to 0) then reset wsize to absolute minimum eg 4096
+		 */
+		if (cifs_sb->ctx->wsize == 0) {
+			cifs_sb->ctx->wsize = PAGE_SIZE;
+			cifs_dbg(VFS, "wsize too small, reset to minimum ie PAGE_SIZE, usually 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 aec8dbd1f9db..14419154cb33 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1111,6 +1111,17 @@ 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_down(ctx->wsize, PAGE_SIZE) != ctx->wsize) {
+			ctx->wsize = round_down(ctx->wsize, PAGE_SIZE);
+			if (ctx->wsize == 0) {
+				ctx->wsize = PAGE_SIZE;
+				cifs_dbg(VFS, "wsize too small, reset to minimum %ld\n", PAGE_SIZE);
+			} else {
+				cifs_dbg(VFS,
+					 "wsize rounded down to %d to multiple of PAGE_SIZE %ld\n",
+					 ctx->wsize, PAGE_SIZE);
+			}
+		}
 		break;
 	case Opt_acregmax:
 		ctx->acregmax = HZ * result.uint_32;
diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
index 182ce11cbe93..f806069fcae3 100644
--- a/fs/smb/client/fs_context.h
+++ b/fs/smb/client/fs_context.h
@@ -166,7 +166,6 @@ struct smb3_fs_context {
 	bool got_wsize;
 	bool got_bsize;
 	unsigned short port;
-
 	char *username;
 	char *password;
 	char *domainname;
-- 
2.40.1

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Shyam Prasad N]

On Thu, Feb 15, 2024 at 1:02 PM Steve French <smfrench@gmail.com> wrote:
>
> Minor update to patch to work around the folios/netfs data corruption.
>
> In addition to printing the warning if "wsize=" is specified on mount
> with a size that is not a multiple of PAGE_SIZE, it also rounds the
> wsize down to the nearest multiple of PAGE_SIZE (as it was already
> doing if the server tried to negotiate a wsize that was not a multiple
> of PAGE_SIZE).
>
> On Fri, Feb 9, 2024 at 2:25 PM Steve French <smfrench@gmail.com> wrote:
> >
> > > > If the user does set their own "wsize", any value that is not a multiple of
> > > PAGE_SIZE is dangerous right?
> >
> > Yes for kernels 6.3 through 6.8-rc such a write size (ie that is not a
> > multiple of page size) can
> > be dangerous - that is why I added the warning on mount if the user
> > specifies the
> > potentially problematic wsize, since the wsize specified on mount
> > unlike the server
> > negotiated maximum write size is under the user's control.  The server
> > negotiated
> > maximum write size can't be controlled by the user, so for this
> > temporary fix we are
> > forced to round it down.   The actually bug is due to a folios/netfs
> > bug that David or
> > one of the mm experts may be able to spot (and fix) so for this
> > temporary workaround
> > I wanted to do the smaller change here so we don't have to revert it
> > later. I got close to
> > finding the actual bug (where the offset was getting reset, rounded up
> > incorrectly
> > inside one of the folios routines mentioned earlier in the thread) but
> > wanted to get something
> >
> > On Fri, Feb 9, 2024 at 2:51 AM Matthew Ruffell
> > <matthew.ruffell@canonical.com> wrote:
> > >
> > > Hi Steve,
> > >
> > > Yes, I am specifying "wsize" on the mount in my example, as its a little easier
> > > to reproduce the issue that way.
> > >
> > > If the user does set their own "wsize", any value that is not a multiple of
> > > PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
> > > their data (un)intentionally if they happen to specify a wrong value? Especially
> > > since we know about it now. I know there haven't been any other reports in the
> > > year or so between 6.3 and present day, so there probably isn't any users out
> > > there actually setting their own "wsize", but it still feels bad to allow users
> > > to expose themselves to data corruption in this form.
> > >
> > > Please consider also rounding down "wsize" set on mount command line to a safe
> > > multiple of PAGE_SIZE. The code will only be around until David's netfslib cut
> > > over is merged anyway.
> > >
> > > I built a distro kernel and sent it to R. Diez for testing, so hopefully we will
> > > have some testing performed against an actual SMB server that sends a dangerous
> > > wsize during negotiation. I'll let you know how that goes, or R. Diez, you can
> > > tell us about how it goes here.
> > >
> > > Thanks,
> > > Matthew
> > >
> > > On Fri, 9 Feb 2024 at 18:38, Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > Are you specifying "wsize" on the mount in your example?  The intent
> > > > of the patch is to warn the user using a non-recommended wsize (since
> > > > the user can control and fix that) but to force round_down when the
> > > > server sends a dangerous wsize (ie one that is not a multiple of
> > > > 4096).
> > > >
> > > > On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
> > > > <matthew.ruffell@canonical.com> wrote:
> > > > >
> > > > > Hi Steve,
> > > > >
> > > > > I built your latest patch ontop of 6.8-rc3, but the problem still persists.
> > > > >
> > > > > Looking at dmesg, I see the debug statement from the second hunk, but not from
> > > > > the first hunk, so I don't believe that wsize was ever rounded down to
> > > > > PAGE_SIZE.
> > > > >
> > > > > [  541.918267] Use of the less secure dialect vers=1.0 is not
> > > > > recommended unless required for access to very old servers
> > > > > [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > > > > not recommended unless required for access to very old servers
> > > > > [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> > > > > [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
> > > > >
> > > > > $ sha256sum sambashare/testdata.txt
> > > > > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> > > > > sambashare/testdata.txt
> > > > > $ less sambashare/testdata.txt
> > > > > ...
> > > > > 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> > > > > 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> > > > > ...
> > > > >
> > > > > Would you be able compile and test your patch and see if we enter the logic from
> > > > > the first hunk?
> > > > >
> > > > > I'll be happy to test a V2 tomorrow.
> > > > >
> > > > > Thanks,
> > > > > Matthew
> > > > >
> > > > > On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > I had attached the wrong file - reattaching the correct patch (ie that
> > > > > > updates the previous version to use PAGE_SIZE instead of 4096)
> > > > > >
> > > > > > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > > > > > >
> > > > > > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > > > > > >
> > > > > > > See attached
> > > > > > >
> > > > > > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > > >
> > > > > > > > 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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Steve
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

Minor comments.
In smb3_fs_context_parse_param, we don't strictly need to use
round_down twice. We could use a modulo operation for the check.
Also, there's an unnecessary change in fs_context.h though.
Other than that, the patch looks good to me. RB

-- 
Regards,
Shyam

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Matthew Ruffell]

Hi Steve,

I tested the patch ontop of 6.8-rc4 and it works great.

$ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850
//192.168.122.172/sambashare ~/share
$ mount -l
//192.168.122.172/sambashare on /home/ubuntu/share type cifs
(rw,relatime,vers=1.0,cache=strict,username=ubuntu,uid=0,noforceuid,gid=0,noforcegid,
addr=192.168.122.172,soft,unix,posixpaths,serverino,mapposix,acl,rsize=1048576,wsize=16384,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)
$ sudo dmesg | tail
[   48.767560] Use of the less secure dialect vers=1.0 is not
recommended unless required for access to very old servers
[   48.768399] CIFS: VFS: Use of the less secure dialect vers=1.0 is
not recommended unless required for access to very old servers
[   48.769427] CIFS: VFS: wsize rounded down to 16384 to multiple of
PAGE_SIZE 4096
[   48.770069] CIFS: Attempting to mount //192.168.122.172/sambashare

Setting the wsize=16850 rounds it down to 16384 like clockwork.

I have built R. Diez a new distro kernel with the patch applied, and will ask
him to test it. He did test the last one, which worked, and also rounded down
the wsize that was negotiated with his old 1.0 server.

When I get some time I can help try bisect and locate the folios/netfs data
corruption, but I think this is a good solution for the time being, or until
the netfslib changeover happens.

Thanks,
Matthew

On Thu, 15 Feb 2024 at 20:32, Steve French <smfrench@gmail.com> wrote:
>
> Minor update to patch to work around the folios/netfs data corruption.
>
> In addition to printing the warning if "wsize=" is specified on mount
> with a size that is not a multiple of PAGE_SIZE, it also rounds the
> wsize down to the nearest multiple of PAGE_SIZE (as it was already
> doing if the server tried to negotiate a wsize that was not a multiple
> of PAGE_SIZE).
>
> On Fri, Feb 9, 2024 at 2:25 PM Steve French <smfrench@gmail.com> wrote:
> >
> > > > If the user does set their own "wsize", any value that is not a multiple of
> > > PAGE_SIZE is dangerous right?
> >
> > Yes for kernels 6.3 through 6.8-rc such a write size (ie that is not a
> > multiple of page size) can
> > be dangerous - that is why I added the warning on mount if the user
> > specifies the
> > potentially problematic wsize, since the wsize specified on mount
> > unlike the server
> > negotiated maximum write size is under the user's control.  The server
> > negotiated
> > maximum write size can't be controlled by the user, so for this
> > temporary fix we are
> > forced to round it down.   The actually bug is due to a folios/netfs
> > bug that David or
> > one of the mm experts may be able to spot (and fix) so for this
> > temporary workaround
> > I wanted to do the smaller change here so we don't have to revert it
> > later. I got close to
> > finding the actual bug (where the offset was getting reset, rounded up
> > incorrectly
> > inside one of the folios routines mentioned earlier in the thread) but
> > wanted to get something
> >
> > On Fri, Feb 9, 2024 at 2:51 AM Matthew Ruffell
> > <matthew.ruffell@canonical.com> wrote:
> > >
> > > Hi Steve,
> > >
> > > Yes, I am specifying "wsize" on the mount in my example, as its a little easier
> > > to reproduce the issue that way.
> > >
> > > If the user does set their own "wsize", any value that is not a multiple of
> > > PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
> > > their data (un)intentionally if they happen to specify a wrong value? Especially
> > > since we know about it now. I know there haven't been any other reports in the
> > > year or so between 6.3 and present day, so there probably isn't any users out
> > > there actually setting their own "wsize", but it still feels bad to allow users
> > > to expose themselves to data corruption in this form.
> > >
> > > Please consider also rounding down "wsize" set on mount command line to a safe
> > > multiple of PAGE_SIZE. The code will only be around until David's netfslib cut
> > > over is merged anyway.
> > >
> > > I built a distro kernel and sent it to R. Diez for testing, so hopefully we will
> > > have some testing performed against an actual SMB server that sends a dangerous
> > > wsize during negotiation. I'll let you know how that goes, or R. Diez, you can
> > > tell us about how it goes here.
> > >
> > > Thanks,
> > > Matthew
> > >
> > > On Fri, 9 Feb 2024 at 18:38, Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > Are you specifying "wsize" on the mount in your example?  The intent
> > > > of the patch is to warn the user using a non-recommended wsize (since
> > > > the user can control and fix that) but to force round_down when the
> > > > server sends a dangerous wsize (ie one that is not a multiple of
> > > > 4096).
> > > >
> > > > On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
> > > > <matthew.ruffell@canonical.com> wrote:
> > > > >
> > > > > Hi Steve,
> > > > >
> > > > > I built your latest patch ontop of 6.8-rc3, but the problem still persists.
> > > > >
> > > > > Looking at dmesg, I see the debug statement from the second hunk, but not from
> > > > > the first hunk, so I don't believe that wsize was ever rounded down to
> > > > > PAGE_SIZE.
> > > > >
> > > > > [  541.918267] Use of the less secure dialect vers=1.0 is not
> > > > > recommended unless required for access to very old servers
> > > > > [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > > > > not recommended unless required for access to very old servers
> > > > > [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> > > > > [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
> > > > >
> > > > > $ sha256sum sambashare/testdata.txt
> > > > > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> > > > > sambashare/testdata.txt
> > > > > $ less sambashare/testdata.txt
> > > > > ...
> > > > > 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> > > > > 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> > > > > ...
> > > > >
> > > > > Would you be able compile and test your patch and see if we enter the logic from
> > > > > the first hunk?
> > > > >
> > > > > I'll be happy to test a V2 tomorrow.
> > > > >
> > > > > Thanks,
> > > > > Matthew
> > > > >
> > > > > On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > I had attached the wrong file - reattaching the correct patch (ie that
> > > > > > updates the previous version to use PAGE_SIZE instead of 4096)
> > > > > >
> > > > > > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > > > > > >
> > > > > > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > > > > > >
> > > > > > > See attached
> > > > > > >
> > > > > > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > > >
> > > > > > > > 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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Steve
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

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

Lightly updated with Shyam's modulo suggestion


On Thu, Feb 15, 2024 at 9:46 PM Matthew Ruffell
<matthew.ruffell@canonical.com> wrote:
>
> Hi Steve,
>
> I tested the patch ontop of 6.8-rc4 and it works great.
>
> $ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850
> //192.168.122.172/sambashare ~/share
> $ mount -l
> //192.168.122.172/sambashare on /home/ubuntu/share type cifs
> (rw,relatime,vers=1.0,cache=strict,username=ubuntu,uid=0,noforceuid,gid=0,noforcegid,
> addr=192.168.122.172,soft,unix,posixpaths,serverino,mapposix,acl,rsize=1048576,wsize=16384,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)
> $ sudo dmesg | tail
> [   48.767560] Use of the less secure dialect vers=1.0 is not
> recommended unless required for access to very old servers
> [   48.768399] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> not recommended unless required for access to very old servers
> [   48.769427] CIFS: VFS: wsize rounded down to 16384 to multiple of
> PAGE_SIZE 4096
> [   48.770069] CIFS: Attempting to mount //192.168.122.172/sambashare
>
> Setting the wsize=16850 rounds it down to 16384 like clockwork.
>
> I have built R. Diez a new distro kernel with the patch applied, and will ask
> him to test it. He did test the last one, which worked, and also rounded down
> the wsize that was negotiated with his old 1.0 server.
>
> When I get some time I can help try bisect and locate the folios/netfs data
> corruption, but I think this is a good solution for the time being, or until
> the netfslib changeover happens.
>
> Thanks,
> Matthew
>
> On Thu, 15 Feb 2024 at 20:32, Steve French <smfrench@gmail.com> wrote:
> >
> > Minor update to patch to work around the folios/netfs data corruption.
> >
> > In addition to printing the warning if "wsize=" is specified on mount
> > with a size that is not a multiple of PAGE_SIZE, it also rounds the
> > wsize down to the nearest multiple of PAGE_SIZE (as it was already
> > doing if the server tried to negotiate a wsize that was not a multiple
> > of PAGE_SIZE).
> >
> > On Fri, Feb 9, 2024 at 2:25 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > > > If the user does set their own "wsize", any value that is not a multiple of
> > > > PAGE_SIZE is dangerous right?
> > >
> > > Yes for kernels 6.3 through 6.8-rc such a write size (ie that is not a
> > > multiple of page size) can
> > > be dangerous - that is why I added the warning on mount if the user
> > > specifies the
> > > potentially problematic wsize, since the wsize specified on mount
> > > unlike the server
> > > negotiated maximum write size is under the user's control.  The server
> > > negotiated
> > > maximum write size can't be controlled by the user, so for this
> > > temporary fix we are
> > > forced to round it down.   The actually bug is due to a folios/netfs
> > > bug that David or
> > > one of the mm experts may be able to spot (and fix) so for this
> > > temporary workaround
> > > I wanted to do the smaller change here so we don't have to revert it
> > > later. I got close to
> > > finding the actual bug (where the offset was getting reset, rounded up
> > > incorrectly
> > > inside one of the folios routines mentioned earlier in the thread) but
> > > wanted to get something
> > >
> > > On Fri, Feb 9, 2024 at 2:51 AM Matthew Ruffell
> > > <matthew.ruffell@canonical.com> wrote:
> > > >
> > > > Hi Steve,
> > > >
> > > > Yes, I am specifying "wsize" on the mount in my example, as its a little easier
> > > > to reproduce the issue that way.
> > > >
> > > > If the user does set their own "wsize", any value that is not a multiple of
> > > > PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
> > > > their data (un)intentionally if they happen to specify a wrong value? Especially
> > > > since we know about it now. I know there haven't been any other reports in the
> > > > year or so between 6.3 and present day, so there probably isn't any users out
> > > > there actually setting their own "wsize", but it still feels bad to allow users
> > > > to expose themselves to data corruption in this form.
> > > >
> > > > Please consider also rounding down "wsize" set on mount command line to a safe
> > > > multiple of PAGE_SIZE. The code will only be around until David's netfslib cut
> > > > over is merged anyway.
> > > >
> > > > I built a distro kernel and sent it to R. Diez for testing, so hopefully we will
> > > > have some testing performed against an actual SMB server that sends a dangerous
> > > > wsize during negotiation. I'll let you know how that goes, or R. Diez, you can
> > > > tell us about how it goes here.
> > > >
> > > > Thanks,
> > > > Matthew
> > > >
> > > > On Fri, 9 Feb 2024 at 18:38, Steve French <smfrench@gmail.com> wrote:
> > > > >
> > > > > Are you specifying "wsize" on the mount in your example?  The intent
> > > > > of the patch is to warn the user using a non-recommended wsize (since
> > > > > the user can control and fix that) but to force round_down when the
> > > > > server sends a dangerous wsize (ie one that is not a multiple of
> > > > > 4096).
> > > > >
> > > > > On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
> > > > > <matthew.ruffell@canonical.com> wrote:
> > > > > >
> > > > > > Hi Steve,
> > > > > >
> > > > > > I built your latest patch ontop of 6.8-rc3, but the problem still persists.
> > > > > >
> > > > > > Looking at dmesg, I see the debug statement from the second hunk, but not from
> > > > > > the first hunk, so I don't believe that wsize was ever rounded down to
> > > > > > PAGE_SIZE.
> > > > > >
> > > > > > [  541.918267] Use of the less secure dialect vers=1.0 is not
> > > > > > recommended unless required for access to very old servers
> > > > > > [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > > > > > not recommended unless required for access to very old servers
> > > > > > [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> > > > > > [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
> > > > > >
> > > > > > $ sha256sum sambashare/testdata.txt
> > > > > > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> > > > > > sambashare/testdata.txt
> > > > > > $ less sambashare/testdata.txt
> > > > > > ...
> > > > > > 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> > > > > > 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> > > > > > ...
> > > > > >
> > > > > > Would you be able compile and test your patch and see if we enter the logic from
> > > > > > the first hunk?
> > > > > >
> > > > > > I'll be happy to test a V2 tomorrow.
> > > > > >
> > > > > > Thanks,
> > > > > > Matthew
> > > > > >
> > > > > > On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> > > > > > >
> > > > > > > I had attached the wrong file - reattaching the correct patch (ie that
> > > > > > > updates the previous version to use PAGE_SIZE instead of 4096)
> > > > > > >
> > > > > > > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > > > > > > >
> > > > > > > > See attached
> > > > > > > >
> > > > > > > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > 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
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > Steve
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Steve
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > >
> > > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Thanks,

Steve

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

From 4860abb91f3d7fbaf8147d54782149bb1fc45892 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 6 Feb 2024 16:34:22 -0600
Subject: [PATCH 1/2] 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 round down), 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>
Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Tested-by: Matthew Ruffell <matthew.ruffell@canonical.com>
Reviewed-by: Shyam Prasad N <sprasad@microsoft.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    | 14 ++++++++++++--
 fs/smb/client/fs_context.c | 11 +++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index d03253f8f145..ac9595504f4b 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -3444,8 +3444,18 @@ 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), PAGE_SIZE);
+		/*
+		 * in the very unlikely event that the server sent a max write size under PAGE_SIZE,
+		 * (which would get rounded down to 0) then reset wsize to absolute minimum eg 4096
+		 */
+		if (cifs_sb->ctx->wsize == 0) {
+			cifs_sb->ctx->wsize = PAGE_SIZE;
+			cifs_dbg(VFS, "wsize too small, reset to minimum ie PAGE_SIZE, usually 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 aec8dbd1f9db..4b2f5aa2ea0e 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1111,6 +1111,17 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 	case Opt_wsize:
 		ctx->wsize = result.uint_32;
 		ctx->got_wsize = true;
+		if (ctx->wsize % PAGE_SIZE != 0) {
+			ctx->wsize = round_down(ctx->wsize, PAGE_SIZE);
+			if (ctx->wsize == 0) {
+				ctx->wsize = PAGE_SIZE;
+				cifs_dbg(VFS, "wsize too small, reset to minimum %ld\n", PAGE_SIZE);
+			} else {
+				cifs_dbg(VFS,
+					 "wsize rounded down to %d to multiple of PAGE_SIZE %ld\n",
+					 ctx->wsize, PAGE_SIZE);
+			}
+		}
 		break;
 	case Opt_acregmax:
 		ctx->acregmax = HZ * result.uint_32;
-- 
2.40.1

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Matthew Ruffell]

Hi Steve,

I have also tested this patch, and it is functionally the same as the
previous one.

I'm happy with this, please pick it up in your tree for 6.8 if possible.

Thanks,
Matthew

On Fri, 16 Feb 2024 at 17:22, Steve French <smfrench@gmail.com> wrote:
>
> Lightly updated with Shyam's modulo suggestion
>
>
> On Thu, Feb 15, 2024 at 9:46 PM Matthew Ruffell
> <matthew.ruffell@canonical.com> wrote:
> >
> > Hi Steve,
> >
> > I tested the patch ontop of 6.8-rc4 and it works great.
> >
> > $ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850
> > //192.168.122.172/sambashare ~/share
> > $ mount -l
> > //192.168.122.172/sambashare on /home/ubuntu/share type cifs
> > (rw,relatime,vers=1.0,cache=strict,username=ubuntu,uid=0,noforceuid,gid=0,noforcegid,
> > addr=192.168.122.172,soft,unix,posixpaths,serverino,mapposix,acl,rsize=1048576,wsize=16384,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)
> > $ sudo dmesg | tail
> > [   48.767560] Use of the less secure dialect vers=1.0 is not
> > recommended unless required for access to very old servers
> > [   48.768399] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > not recommended unless required for access to very old servers
> > [   48.769427] CIFS: VFS: wsize rounded down to 16384 to multiple of
> > PAGE_SIZE 4096
> > [   48.770069] CIFS: Attempting to mount //192.168.122.172/sambashare
> >
> > Setting the wsize=16850 rounds it down to 16384 like clockwork.
> >
> > I have built R. Diez a new distro kernel with the patch applied, and will ask
> > him to test it. He did test the last one, which worked, and also rounded down
> > the wsize that was negotiated with his old 1.0 server.
> >
> > When I get some time I can help try bisect and locate the folios/netfs data
> > corruption, but I think this is a good solution for the time being, or until
> > the netfslib changeover happens.
> >
> > Thanks,
> > Matthew
> >
> > On Thu, 15 Feb 2024 at 20:32, Steve French <smfrench@gmail.com> wrote:
> > >
> > > Minor update to patch to work around the folios/netfs data corruption.
> > >
> > > In addition to printing the warning if "wsize=" is specified on mount
> > > with a size that is not a multiple of PAGE_SIZE, it also rounds the
> > > wsize down to the nearest multiple of PAGE_SIZE (as it was already
> > > doing if the server tried to negotiate a wsize that was not a multiple
> > > of PAGE_SIZE).
> > >
> > > On Fri, Feb 9, 2024 at 2:25 PM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > > > If the user does set their own "wsize", any value that is not a multiple of
> > > > > PAGE_SIZE is dangerous right?
> > > >
> > > > Yes for kernels 6.3 through 6.8-rc such a write size (ie that is not a
> > > > multiple of page size) can
> > > > be dangerous - that is why I added the warning on mount if the user
> > > > specifies the
> > > > potentially problematic wsize, since the wsize specified on mount
> > > > unlike the server
> > > > negotiated maximum write size is under the user's control.  The server
> > > > negotiated
> > > > maximum write size can't be controlled by the user, so for this
> > > > temporary fix we are
> > > > forced to round it down.   The actually bug is due to a folios/netfs
> > > > bug that David or
> > > > one of the mm experts may be able to spot (and fix) so for this
> > > > temporary workaround
> > > > I wanted to do the smaller change here so we don't have to revert it
> > > > later. I got close to
> > > > finding the actual bug (where the offset was getting reset, rounded up
> > > > incorrectly
> > > > inside one of the folios routines mentioned earlier in the thread) but
> > > > wanted to get something
> > > >
> > > > On Fri, Feb 9, 2024 at 2:51 AM Matthew Ruffell
> > > > <matthew.ruffell@canonical.com> wrote:
> > > > >
> > > > > Hi Steve,
> > > > >
> > > > > Yes, I am specifying "wsize" on the mount in my example, as its a little easier
> > > > > to reproduce the issue that way.
> > > > >
> > > > > If the user does set their own "wsize", any value that is not a multiple of
> > > > > PAGE_SIZE is dangerous right? Shouldn't we prevent the user from corrupting
> > > > > their data (un)intentionally if they happen to specify a wrong value? Especially
> > > > > since we know about it now. I know there haven't been any other reports in the
> > > > > year or so between 6.3 and present day, so there probably isn't any users out
> > > > > there actually setting their own "wsize", but it still feels bad to allow users
> > > > > to expose themselves to data corruption in this form.
> > > > >
> > > > > Please consider also rounding down "wsize" set on mount command line to a safe
> > > > > multiple of PAGE_SIZE. The code will only be around until David's netfslib cut
> > > > > over is merged anyway.
> > > > >
> > > > > I built a distro kernel and sent it to R. Diez for testing, so hopefully we will
> > > > > have some testing performed against an actual SMB server that sends a dangerous
> > > > > wsize during negotiation. I'll let you know how that goes, or R. Diez, you can
> > > > > tell us about how it goes here.
> > > > >
> > > > > Thanks,
> > > > > Matthew
> > > > >
> > > > > On Fri, 9 Feb 2024 at 18:38, Steve French <smfrench@gmail.com> wrote:
> > > > > >
> > > > > > Are you specifying "wsize" on the mount in your example?  The intent
> > > > > > of the patch is to warn the user using a non-recommended wsize (since
> > > > > > the user can control and fix that) but to force round_down when the
> > > > > > server sends a dangerous wsize (ie one that is not a multiple of
> > > > > > 4096).
> > > > > >
> > > > > > On Thu, Feb 8, 2024 at 3:31 AM Matthew Ruffell
> > > > > > <matthew.ruffell@canonical.com> wrote:
> > > > > > >
> > > > > > > Hi Steve,
> > > > > > >
> > > > > > > I built your latest patch ontop of 6.8-rc3, but the problem still persists.
> > > > > > >
> > > > > > > Looking at dmesg, I see the debug statement from the second hunk, but not from
> > > > > > > the first hunk, so I don't believe that wsize was ever rounded down to
> > > > > > > PAGE_SIZE.
> > > > > > >
> > > > > > > [  541.918267] Use of the less secure dialect vers=1.0 is not
> > > > > > > recommended unless required for access to very old servers
> > > > > > > [  541.920913] CIFS: VFS: Use of the less secure dialect vers=1.0 is
> > > > > > > not recommended unless required for access to very old servers
> > > > > > > [  541.923533] CIFS: VFS: wsize should be a multiple of 4096 (PAGE_SIZE)
> > > > > > > [  541.924755] CIFS: Attempting to mount //192.168.122.172/sambashare
> > > > > > >
> > > > > > > $ sha256sum sambashare/testdata.txt
> > > > > > > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866
> > > > > > > sambashare/testdata.txt
> > > > > > > $ less sambashare/testdata.txt
> > > > > > > ...
> > > > > > > 8dc8da96f7e5de0f312a2dbcc3c5c6facbfcc2fc206e29283274582ec93daa2a1496ca8edd49e3c1
> > > > > > > 6b^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^
> > > > > > > ...
> > > > > > >
> > > > > > > Would you be able compile and test your patch and see if we enter the logic from
> > > > > > > the first hunk?
> > > > > > >
> > > > > > > I'll be happy to test a V2 tomorrow.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Matthew
> > > > > > >
> > > > > > > On Thu, 8 Feb 2024 at 03:50, Steve French <smfrench@gmail.com> wrote:
> > > > > > > >
> > > > > > > > I had attached the wrong file - reattaching the correct patch (ie that
> > > > > > > > updates the previous version to use PAGE_SIZE instead of 4096)
> > > > > > > >
> > > > > > > > On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> > > > > > > > >
> > > > > > > > > See attached
> > > > > > > > >
> > > > > > > > > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > 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
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Thanks,
> > > > > > > > >
> > > > > > > > > Steve
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > Steve
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > >
> > > > > > Steve
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve

Re: SMB 1.0 broken between Kernel versions 6.2 and 6.5

[Steve French]

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

Minor update to patch following suggestion by Shyam - more accurately
state the PAGE_SIZE in the debug message from:

   cifs_dbg(VFS, "wsize should be a multiple of 4096 (PAGE_SIZE)\n");

to

   cifs_dbg(VFS, "wsize should be a multiple of %ld (PAGE_SIZE)\n", PAGE_SIZE);

On Wed, Feb 7, 2024 at 8:50 AM Steve French <smfrench@gmail.com> wrote:
>
> I had attached the wrong file - reattaching the correct patch (ie that
> updates the previous version to use PAGE_SIZE instead of 4096)
>
> On Wed, Feb 7, 2024 at 1:12 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Updated patch - now use PAGE_SIZE instead of hard coding to 4096.
> >
> > See attached
> >
> > On Tue, Feb 6, 2024 at 11:32 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > 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
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

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

From 10b7daf1427d6bce8c01b836172e89689fe0aecf 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>
Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Reviewed-by: Shyam Prasad N <sprasad@microsoft.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..79ce4a29d1ef 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), PAGE_SIZE);
+		/*
+		 * in the very unlikely event that the server sent a max write size under PAGE_SIZE,
+		 * (which would get rounded down to 0) then reset wsize to absolute minimum eg 4096
+		 */
+		if (cifs_sb->ctx->wsize == 0) {
+			cifs_sb->ctx->wsize = PAGE_SIZE;
+			cifs_dbg(VFS, "wsize too small, reset to minimum ie PAGE_SIZE, usually 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..8b090f709194 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, PAGE_SIZE) != ctx->wsize)
+			cifs_dbg(VFS, "wsize should be a multiple of %ld (PAGE_SIZE)\n", PAGE_SIZE);
 		break;
 	case Opt_acregmax:
 		ctx->acregmax = HZ * result.uint_32;
-- 
2.40.1