> > 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@xxxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > I have bisected the issue, and found the commit that introduces the problem: > > > > > > > > > > > > > > > > commit d08089f649a0cfb2099c8551ac47eef0cc23fdf2 > > > > > > > > Author: David Howells <dhowells@xxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxx> > > > > > > > > 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
