5 ms·
It was really interesting how this was found. A user started describing file corruption when copying to/from Windows with the io_uring VFS module loaded. Test
by jra_samba 6y ago
It was really interesting how this was found.
A user started describing file corruption when copying to/from Windows with the io_uring VFS module loaded.
Tests using the Linux kernel cifsfs client and the Samba libsmbclient libraries/smbclient user-space transfer utility couldn't reproduce the problem, neither could running Windows against Samba on Ubuntu 19.04.
What turned out to be happening was a combination of things. Firstly, the kernel changed so an SMB2_READ request against Samba with io_uring loaded was sometimes hitting a short read, where some of file data was already in the buffer cache, so io_uring now returned a short read to smbd.
We returned this to the client, as in the SMB2 protocol it isn't an error to return a short read, the client is supposed to check read returns and then re-issue another read request for any missing bytes. The Linux kernel cifsfs client and Samba libsmbclient/smbclient did this correctly.
But it turned out that Windows10 clients and MacOSX Catalina (maybe earlier versions of clients too, I don't have access to those) clients have a horrible bug, where they're not checking read returns when doing pipeline reads.
When trying to read a 10GB file for example, they'll issue a series of 1MB reads at 1MB boundaries, up to their SMB2 credit limit, without waiting for replies. This is an excellent way to improve network file copy performance as you fill the read pipe without waiting for reply latency - indeed both Linux cifsfs and smbclient do exactly the same.
But if one of those reads returns a short value, Windows10 and MacOSX Catalina DON'T GO BACK AND RE-READ THE MISSING BYTES FROM THE SHORT READ REPLY !!!! This is catastrophic, and will corrupt any file read from the server (the local client buffer cache fills the file contents I'm assuming with zeros - I haven't checked, but the files are corrupt as checked by SHA256 hashing anyway).
That's how we discovered the behavior and ended up leading back to the io_uring behavior change. And that's why I hate it when kernel interfaces expose changes to user-space :-).
- andoma 6y agoSo it's a Windows and MacOS bug then? Ie, no shadow should fall on io_uring really? That said, nice dig figuring this out. These type of bugs can be really frustrating to round up.
- jra_samba 6y agoIt is a Windows and MacOS bug, but only visible with the io_uring change in behavior. Problem is we have to fix the server, as even when buggy, one billion clients can never be wrong.
- jstarks 6y ago> in the SMB2 protocol it isn't an error to return a short read, the client is supposed to check read returns and then re-issue another read request for any missing bytes This is interesting and somewhat surprising, since Windows IO is internally asynchronous and completion based, and AFAIK file system drivers are not allowed to return a short read except for EOF. And actually, even on Linux file systems are not supposed to return short reads, right? Even on signal? Since user apps don't expect it? (And thus it's not surprising that io_uring's change broke user apps.) So it wouldn't be surprising to learn that the Windows SMB server never returns short reads, and thus it's interesting that the protocol would allow it. Do you know what the purpose of this is?
- jra_samba 6y agoObviously the Windows SMB server never returns short reads, otherwise this bug would never have made it out of Redmond or Cupertino. On Linux, pread also never returns short reads against disk files if the bytes are available, which is why no one noticed this client bug as our default io backend is a pthread-pool that does pread/pwrite calls. It only happens when someone tried our (flagged as experimental thank god) vfs_io_uring backend. Yeah the protocol even has a field in the SMB2_READ request called MinimumBytes, for which the server should fail the read if less than these bytes are available on return. The Windows 10 clients set this to zero :-). The MacOSX Catalina client sets it to 1. So yes, the clients are supposed to be able to handle short reads.
- jstarks 6y agoOut of curiosity, I took a look at how the MinimumBytes (actually MinimumCount) field is used by the Windows SMB server. Interestingly, it fails with STATUS_END_OF_FILE if the actual bytes read is less than MinimumCount, which suggests to me that this is supposed to be a minimum on the (remaining) file length, not on the number of bytes that the server is able to return at the moment. I can't find any history of MinimumCount being used in the RTM version of any Windows SMB client, so without deeper archeology the reason this field was introduced remains a mystery to me. Regardless, I agree that the client should validate the returned byte count. But (only having thought about this briefly), I do not think a client should retry in this case--it seems to me if the client sees a short read, it can assume that the read was short because the read reached EOF (which may have changed since the file's length was queried).
- dirtydroog 6y agoWell, Linus did have an infamous rant about never breaking userspace; it's surprising this happened.
- loeg 6y agoHe's not especially consistent about it. Linus was totally prepared to break userspace re: getrandom() in recent history.