| Message ID | 20240222005039.15585-1-matthew.ruffell@canonical.com |
|---|---|
| Headers | show |
| Series | smb: wsize blocks of bytes followed with binary zeros on copy, destroying data | expand |
On Thu, Feb 22, 2024 at 01:50:38PM +1300, Matthew Ruffell wrote: > BugLink: https://bugs.launchpad.net/bugs/2049634 > > [Impact] > > Upon installing the 6.5 HWE kernel on Jammy, users with a custom wsize set will > see data destruction when copying files from their systems onto a cifs smb mount. > > wsize defaults to 65535 bytes, but when set to smaller values, like 16850, users > will see blocks of 16850 bytes copied over, followed by 3900 binary zeros, > followed by the next block of data followed by more binary zeros. This destroys > what you are copying, and the data corruption is completely silent. > > A workaround is to set wsize to a multiple of PAGE_SIZE. > > [Fix] > > This was fixed upstream in 6.8-rc5 by the following: > > commit 4860abb91f3d7fbaf8147d54782149bb1fc45892 > Author: Steve French <stfrench@microsoft.com> > Date: Tue Feb 6 16:34:22 2024 -0600 > Subject: smb: Fix regression in writes when non-standard maximum write size negotiated > Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4860abb91f3d7fbaf8147d54782149bb1fc45892 > > This patch has been selected for upstream stable. > > The patch looks at wsize negotiation from the server, and also what is set on > the mount command line, and if not a multiple of PAGE_SIZE, rounds it down, > taking care to not be 0. > The real corruption bug still exists in netfs/folios subsystems and we are > looking for it still, but this solves the immediate data corruption issues on > smb. > > [Testcase] > > Start two VMs, one for the server, and the other, the client. > > Server > ------ > > $ sudo apt update > $ sudo apt upgrade > $ sudo apt install samba > $ sudo vim /etc/samba/smb.conf > server min protocol = NT1 > [sambashare] > comment = Samba on Ubuntu > path = /home/ubuntu/sambashare > read only = no > browsable = yes > $ mkdir ~/sambashare > $ sudo smbpasswd -a ubuntu > > Client > ------ > > $ sudo apt update > $ sudo apt install cifs-utils > $ mkdir ~/share > $ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850 //192.168.122.172/sambashare ~/share > $ ( set -o pipefail && head --bytes=$(( 55 * 1000 )) /dev/zero | openssl enc -aes-128-ctr -nosalt -pass "pass:my-seed" -iter 1 | hexdump --no-squeezing --format '40/1 "%02x"' --format '"\n"' >"testdata.txt" ) > $ sha256sum testdata.txt > 9ec09af020dce3270ea76531141940106f173c7243b7193a553480fb8500b3f2 testdata.txt > > Now copy the file to the share. > > Client > ------ > $ cp testdata.txt share/ > > Server > ------ > $ sha256sum sambashare/testdata.txt > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866 sambashare/testdata.txt > > The SHA256 hash is different. If you view the file with less, you will find a > block of wsize=16850 bytes, then 3900 bytes of binary zeros, followed by another > wsize=16850 bytes, then 3900 bytes of binary zeros, etc. > > An example of a broken file is: > https://launchpadlibrarian.net/712573213/testdata-back-from-server.txt > > [Where problems could occur] > > We are changing the wsize that the SMB server negotiates or the user explicitly > sets on the mount command line to a safe value, if the asked for value was not > a multiple of PAGE_SIZE. > > It is unlikely that any users will notice any difference. If the wsize happens > to be rounded down to a significantly smaller number, there may be a small > performance impact, e.g. you set wsize=8094 for some reason, it would round down > to wsize=4096, and lead to double the writes. At least you have data integrity > though. > > Most users default to wsize=65535, and will have no impact at all. Only those > with very old smb 1.0 servers that negotiate down to 16850 will see any > difference. > > If a regression were to occur, it could result in user's wsize being incorrectly > set, leading to the underlying netfs/folios data corruption being triggered and > causing data corruption. > > [Other info] > > The issue was introduced in 6.3-rc1 by: > > 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 > > Upstream mailing list discussion: > > https://lore.kernel.org/linux-cifs/6a65b2d1-7596-438a-8ade-2f7526b15596@rd10.de/T/#m22cd9b7289f87cd945978bd7995bcaf1beebfe67 > > Steve French (1): > smb: Fix regression in writes when non-standard maximum write size > negotiated > > fs/smb/client/connect.c | 14 ++++++++++++-- > fs/smb/client/fs_context.c | 11 +++++++++++ > 2 files changed, 23 insertions(+), 2 deletions(-) > > -- > 2.40.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Acked-by: Manuel Diewald <manuel.diewald@canonical.com>
On 2/21/24 5:50 PM, Matthew Ruffell wrote: > BugLink: https://bugs.launchpad.net/bugs/2049634 > > [Impact] > > Upon installing the 6.5 HWE kernel on Jammy, users with a custom wsize set will > see data destruction when copying files from their systems onto a cifs smb mount. > > wsize defaults to 65535 bytes, but when set to smaller values, like 16850, users > will see blocks of 16850 bytes copied over, followed by 3900 binary zeros, > followed by the next block of data followed by more binary zeros. This destroys > what you are copying, and the data corruption is completely silent. > > A workaround is to set wsize to a multiple of PAGE_SIZE. > > [Fix] > > This was fixed upstream in 6.8-rc5 by the following: > > commit 4860abb91f3d7fbaf8147d54782149bb1fc45892 > Author: Steve French <stfrench@microsoft.com> > Date: Tue Feb 6 16:34:22 2024 -0600 > Subject: smb: Fix regression in writes when non-standard maximum write size negotiated > Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4860abb91f3d7fbaf8147d54782149bb1fc45892 > > This patch has been selected for upstream stable. > > The patch looks at wsize negotiation from the server, and also what is set on > the mount command line, and if not a multiple of PAGE_SIZE, rounds it down, > taking care to not be 0. > The real corruption bug still exists in netfs/folios subsystems and we are > looking for it still, but this solves the immediate data corruption issues on > smb. > > [Testcase] > > Start two VMs, one for the server, and the other, the client. > > Server > ------ > > $ sudo apt update > $ sudo apt upgrade > $ sudo apt install samba > $ sudo vim /etc/samba/smb.conf > server min protocol = NT1 > [sambashare] > comment = Samba on Ubuntu > path = /home/ubuntu/sambashare > read only = no > browsable = yes > $ mkdir ~/sambashare > $ sudo smbpasswd -a ubuntu > > Client > ------ > > $ sudo apt update > $ sudo apt install cifs-utils > $ mkdir ~/share > $ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850 //192.168.122.172/sambashare ~/share > $ ( set -o pipefail && head --bytes=$(( 55 * 1000 )) /dev/zero | openssl enc -aes-128-ctr -nosalt -pass "pass:my-seed" -iter 1 | hexdump --no-squeezing --format '40/1 "%02x"' --format '"\n"' >"testdata.txt" ) > $ sha256sum testdata.txt > 9ec09af020dce3270ea76531141940106f173c7243b7193a553480fb8500b3f2 testdata.txt > > Now copy the file to the share. > > Client > ------ > $ cp testdata.txt share/ > > Server > ------ > $ sha256sum sambashare/testdata.txt > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866 sambashare/testdata.txt > > The SHA256 hash is different. If you view the file with less, you will find a > block of wsize=16850 bytes, then 3900 bytes of binary zeros, followed by another > wsize=16850 bytes, then 3900 bytes of binary zeros, etc. > > An example of a broken file is: > https://launchpadlibrarian.net/712573213/testdata-back-from-server.txt > > [Where problems could occur] > > We are changing the wsize that the SMB server negotiates or the user explicitly > sets on the mount command line to a safe value, if the asked for value was not > a multiple of PAGE_SIZE. > > It is unlikely that any users will notice any difference. If the wsize happens > to be rounded down to a significantly smaller number, there may be a small > performance impact, e.g. you set wsize=8094 for some reason, it would round down > to wsize=4096, and lead to double the writes. At least you have data integrity > though. > > Most users default to wsize=65535, and will have no impact at all. Only those > with very old smb 1.0 servers that negotiate down to 16850 will see any > difference. > > If a regression were to occur, it could result in user's wsize being incorrectly > set, leading to the underlying netfs/folios data corruption being triggered and > causing data corruption. > > [Other info] > > The issue was introduced in 6.3-rc1 by: > > 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 > > Upstream mailing list discussion: > > https://lore.kernel.org/linux-cifs/6a65b2d1-7596-438a-8ade-2f7526b15596@rd10.de/T/#m22cd9b7289f87cd945978bd7995bcaf1beebfe67 > > Steve French (1): > smb: Fix regression in writes when non-standard maximum write size > negotiated > > fs/smb/client/connect.c | 14 ++++++++++++-- > fs/smb/client/fs_context.c | 11 +++++++++++ > 2 files changed, 23 insertions(+), 2 deletions(-) > Acked-by: Tim Gardner <tim.gardner@canonical.com>
On 22/02/2024 01:50, Matthew Ruffell wrote: > BugLink: https://bugs.launchpad.net/bugs/2049634 > > [Impact] > > Upon installing the 6.5 HWE kernel on Jammy, users with a custom wsize set will > see data destruction when copying files from their systems onto a cifs smb mount. > > wsize defaults to 65535 bytes, but when set to smaller values, like 16850, users > will see blocks of 16850 bytes copied over, followed by 3900 binary zeros, > followed by the next block of data followed by more binary zeros. This destroys > what you are copying, and the data corruption is completely silent. > > A workaround is to set wsize to a multiple of PAGE_SIZE. > > [Fix] > > This was fixed upstream in 6.8-rc5 by the following: > > commit 4860abb91f3d7fbaf8147d54782149bb1fc45892 > Author: Steve French <stfrench@microsoft.com> > Date: Tue Feb 6 16:34:22 2024 -0600 > Subject: smb: Fix regression in writes when non-standard maximum write size negotiated > Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4860abb91f3d7fbaf8147d54782149bb1fc45892 > > This patch has been selected for upstream stable. > > The patch looks at wsize negotiation from the server, and also what is set on > the mount command line, and if not a multiple of PAGE_SIZE, rounds it down, > taking care to not be 0. > The real corruption bug still exists in netfs/folios subsystems and we are > looking for it still, but this solves the immediate data corruption issues on > smb. > > [Testcase] > > Start two VMs, one for the server, and the other, the client. > > Server > ------ > > $ sudo apt update > $ sudo apt upgrade > $ sudo apt install samba > $ sudo vim /etc/samba/smb.conf > server min protocol = NT1 > [sambashare] > comment = Samba on Ubuntu > path = /home/ubuntu/sambashare > read only = no > browsable = yes > $ mkdir ~/sambashare > $ sudo smbpasswd -a ubuntu > > Client > ------ > > $ sudo apt update > $ sudo apt install cifs-utils > $ mkdir ~/share > $ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850 //192.168.122.172/sambashare ~/share > $ ( set -o pipefail && head --bytes=$(( 55 * 1000 )) /dev/zero | openssl enc -aes-128-ctr -nosalt -pass "pass:my-seed" -iter 1 | hexdump --no-squeezing --format '40/1 "%02x"' --format '"\n"' >"testdata.txt" ) > $ sha256sum testdata.txt > 9ec09af020dce3270ea76531141940106f173c7243b7193a553480fb8500b3f2 testdata.txt > > Now copy the file to the share. > > Client > ------ > $ cp testdata.txt share/ > > Server > ------ > $ sha256sum sambashare/testdata.txt > 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866 sambashare/testdata.txt > > The SHA256 hash is different. If you view the file with less, you will find a > block of wsize=16850 bytes, then 3900 bytes of binary zeros, followed by another > wsize=16850 bytes, then 3900 bytes of binary zeros, etc. > > An example of a broken file is: > https://launchpadlibrarian.net/712573213/testdata-back-from-server.txt > > [Where problems could occur] > > We are changing the wsize that the SMB server negotiates or the user explicitly > sets on the mount command line to a safe value, if the asked for value was not > a multiple of PAGE_SIZE. > > It is unlikely that any users will notice any difference. If the wsize happens > to be rounded down to a significantly smaller number, there may be a small > performance impact, e.g. you set wsize=8094 for some reason, it would round down > to wsize=4096, and lead to double the writes. At least you have data integrity > though. > > Most users default to wsize=65535, and will have no impact at all. Only those > with very old smb 1.0 servers that negotiate down to 16850 will see any > difference. > > If a regression were to occur, it could result in user's wsize being incorrectly > set, leading to the underlying netfs/folios data corruption being triggered and > causing data corruption. > > [Other info] > > The issue was introduced in 6.3-rc1 by: > > 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 > > Upstream mailing list discussion: > > https://lore.kernel.org/linux-cifs/6a65b2d1-7596-438a-8ade-2f7526b15596@rd10.de/T/#m22cd9b7289f87cd945978bd7995bcaf1beebfe67 > > Steve French (1): > smb: Fix regression in writes when non-standard maximum write size > negotiated > > fs/smb/client/connect.c | 14 ++++++++++++-- > fs/smb/client/fs_context.c | 11 +++++++++++ > 2 files changed, 23 insertions(+), 2 deletions(-) > Applied to mantic master-next branch. Thanks!
BugLink: https://bugs.launchpad.net/bugs/2049634 [Impact] Upon installing the 6.5 HWE kernel on Jammy, users with a custom wsize set will see data destruction when copying files from their systems onto a cifs smb mount. wsize defaults to 65535 bytes, but when set to smaller values, like 16850, users will see blocks of 16850 bytes copied over, followed by 3900 binary zeros, followed by the next block of data followed by more binary zeros. This destroys what you are copying, and the data corruption is completely silent. A workaround is to set wsize to a multiple of PAGE_SIZE. [Fix] This was fixed upstream in 6.8-rc5 by the following: commit 4860abb91f3d7fbaf8147d54782149bb1fc45892 Author: Steve French <stfrench@microsoft.com> Date: Tue Feb 6 16:34:22 2024 -0600 Subject: smb: Fix regression in writes when non-standard maximum write size negotiated Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4860abb91f3d7fbaf8147d54782149bb1fc45892 This patch has been selected for upstream stable. The patch looks at wsize negotiation from the server, and also what is set on the mount command line, and if not a multiple of PAGE_SIZE, rounds it down, taking care to not be 0. The real corruption bug still exists in netfs/folios subsystems and we are looking for it still, but this solves the immediate data corruption issues on smb. [Testcase] Start two VMs, one for the server, and the other, the client. Server ------ $ sudo apt update $ sudo apt upgrade $ sudo apt install samba $ sudo vim /etc/samba/smb.conf server min protocol = NT1 [sambashare] comment = Samba on Ubuntu path = /home/ubuntu/sambashare read only = no browsable = yes $ mkdir ~/sambashare $ sudo smbpasswd -a ubuntu Client ------ $ sudo apt update $ sudo apt install cifs-utils $ mkdir ~/share $ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850 //192.168.122.172/sambashare ~/share $ ( set -o pipefail && head --bytes=$(( 55 * 1000 )) /dev/zero | openssl enc -aes-128-ctr -nosalt -pass "pass:my-seed" -iter 1 | hexdump --no-squeezing --format '40/1 "%02x"' --format '"\n"' >"testdata.txt" ) $ sha256sum testdata.txt 9ec09af020dce3270ea76531141940106f173c7243b7193a553480fb8500b3f2 testdata.txt Now copy the file to the share. Client ------ $ cp testdata.txt share/ Server ------ $ sha256sum sambashare/testdata.txt 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866 sambashare/testdata.txt The SHA256 hash is different. If you view the file with less, you will find a block of wsize=16850 bytes, then 3900 bytes of binary zeros, followed by another wsize=16850 bytes, then 3900 bytes of binary zeros, etc. An example of a broken file is: https://launchpadlibrarian.net/712573213/testdata-back-from-server.txt [Where problems could occur] We are changing the wsize that the SMB server negotiates or the user explicitly sets on the mount command line to a safe value, if the asked for value was not a multiple of PAGE_SIZE. It is unlikely that any users will notice any difference. If the wsize happens to be rounded down to a significantly smaller number, there may be a small performance impact, e.g. you set wsize=8094 for some reason, it would round down to wsize=4096, and lead to double the writes. At least you have data integrity though. Most users default to wsize=65535, and will have no impact at all. Only those with very old smb 1.0 servers that negotiate down to 16850 will see any difference. If a regression were to occur, it could result in user's wsize being incorrectly set, leading to the underlying netfs/folios data corruption being triggered and causing data corruption. [Other info] The issue was introduced in 6.3-rc1 by: 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 Upstream mailing list discussion: https://lore.kernel.org/linux-cifs/6a65b2d1-7596-438a-8ade-2f7526b15596@rd10.de/T/#m22cd9b7289f87cd945978bd7995bcaf1beebfe67 Steve French (1): smb: Fix regression in writes when non-standard maximum write size negotiated fs/smb/client/connect.c | 14 ++++++++++++-- fs/smb/client/fs_context.c | 11 +++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-)