On Thu, 3 Jul 2014 06:45:49 -0400 Jeff Layton <jlayton@xxxxxxxxx> wrote: > On Fri, 27 Jun 2014 20:06:48 +0400 > Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote: > > > 2014-06-27 14:52 GMT+04:00 Jeff Layton <jlayton@xxxxxxxxxxxxxxx>: > > > On Fri, 27 Jun 2014 13:57:38 +0400 > > > Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote: > > > > > >> If we get into read_into_pages() from cifs_readv_receive() and then > > >> loose a network, we issue cifs_reconnect that moves all mids to > > >> a private list and issue their callbacks. The callback of the async > > >> read request sets a mid to retry, frees it and wakes up a process > > >> that waits on the rdata completion. > > >> > > >> After the connection is established we return from read_into_pages() > > >> with a short read, use the mid that was freed before and try to read > > >> the remaining data from the a newly created socket. Both actions are > > >> not what we want to do. In reconnect cases (-EAGAIN) we should not > > >> mask off the error with a short read but should return the error > > >> code instead. > > >> > > > > > > I'm not sure I understand what problem this solves. Why is returning a > > > short read wrong here? > > > > > > > This patch solves several problems in cifs_readv_receive() when > > read_into_pages() call ended up with cifs_reconnect inside it: > > > > 1) prevents use-after-free and a possible double free for "mid" > > variable; this can cause kernel oopses. > > > > 2) prevents updating rdata->bytes with a short read length that causes > > the cifs_async_readv() caller to retry write request with a shorten > > wrong length; this can cause data coherency problems. > > > > 3) prevents reading an actual data from a newly created socket through > > cifs_readv_discard() -- this data can be a part of a negotiate > > response received from the server; this can cause the client to retry > > negotiate or hang. > > > > Oopses from the 1st scenario was not observed since > > cifs_readv_discard() returns negative value while trying to get a > > remaining data. The 2nd and 3rd one were caught during testing: issue > > reading with "dd", then switch off network on the server, wait 120 sec > > and switch on network. The bug is caught when we are lucky enough to > > stop the link while the client is in read_into_pages(). > > > > The patch fixes all these problems for me. It stays on the following > > logic if a reconnect has happened: > > 1) all mids are already marked to retry and their callbacks are issued > > - we shouldn't do anything with our mid/rdata since the requests will > > be retried anyway. > > 2) the socket is newly created - we shouldn't try to get any remaining > > data of the previous connection. > > > > While -EAGAIN error code will be able to indicate something else in > > possible future patches I prefer to leave it that way since the patch > > is small and easy to apply that is critical for stable series. More > > accurate way is to add a flag that is passed through > > cifs_readv_from_socket() and switched on if cifs_reconnect() was > > issued (like Steve suggested). > > > > Hmm, ok. I think the best fix would be to use a different error code > than -EAGAIN for the case where we have or are going to reconnect the > socket. > > My main concern here is that you may end up doing 99% of a valid read > into the set of pages, only to get a disconnect right at the end. If > you eventually end up returning a non-retryable error back to userland, > it may not expect that the pages it passed in were written to. If you > don't return a short read in that situation then you're potentially not > communicating what actually happened to userland. > After chatting with Pavel, I'll go ahead and Ack this patch. I think it's probably the lesser evil... It does seem to me though it seems to me that if you got part of a read request and then hit an error, then it would be: a) more efficient to not reissue the reads that were successful ...and... b) more correct to go ahead and return a short read. There's no guarantee that you'll be able to reach the server in order to do the reconnect, so it seems like it would be better to go ahead and return what we have than keep retrying in the kernel. So...with that in mind... Acked-by: Jeff Layton <jlayton@xxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
