On Wed, May 06, 2020 at 10:03:44AM -0700, Jeremy Allison via samba-technical wrote: > > Well we pay attention to the amount of data returned > and only increment the next read request by the amount > actually returned. > > I'm amazed that the Windows client doesn't seem to > check this ! > > > The attached test against liburing (git://git.kernel.dk/liburing) should > > be able to demonstrate the problem. It can also be found in > > https://github.com/metze-samba/liburing/tree/implicit-rwf-nowaithttps://github.com/metze-samba/liburing/commit/eb06dcfde747e46bd08bedf9def2e6cb536c39e3 > > > > > > I added the sqe->rw_flags = RWF_NOWAIT; line in order to demonstrate it > > against the Ubuntu 5.3 and 5.4 kernels. They both seem to have the bug. > > > > Can someone run the unmodified test/implicit-rwf_nowait against > > a newer kernel? > > Aha. I wondered about the short read issue when this > was first reported but I could never catch it in the > act. > > If the Windows client doesn't check and the kernel > returns short reads I guess we'll have to add logic > similar to tstream_readv_send()/tstream_writev_send() > that ensure all bytes requested/send actually go through > the interface and from/into the kernel unless a read > returns 0 (EOF) or a write returns an error. > > What a pain though :-(. SMB2+ server implementors > really need to take note that Windows clients will corrupt > files if they get a short read/write return. > > The fact that early kernels don't return short > reads on io_uring but later kernels do makes it > even worse :-(. > > There's even an SMB2 protocol field in SMB2_READ: > > "MinimumCount (4 bytes): The minimum number of bytes to be read for this operation to be > successful. If fewer than the minimum number of bytes are read by the server, the server > MUST return an error rather than the bytes read." > > We correctly return EOF if the amount read from > the kernel is less than SMB2_READ.MinimumCount > so I'm guessing they're not using it or looking > at it (or setting it to zero). > > MinimumCount is supposed to allow the client to cope with > this. Anoop, do you have wireshark traces so we can > see what the Windows clients are setting here ? Just did a quick check myself and Windows10 clients are setting Minimumcount==0 on read, so any amount should be good here.
