| Message ID | 1548277845-6746-2-git-send-email-pshilov@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve credits handling on reconnects | expand |
-- Best regards, Pavel Shilovsky ср, 23 янв. 2019 г. в 13:10, Pavel Shilovsky <piastryyy@gmail.com>: > > When executing add_credits() we currently call cifs_reconnect() > if the number of credits is zero and there are no requests in > flight. In this case we may call cifs_reconnect() recursively > twice and cause memory corruption given the following sequence > of functions: > > mid1.callback() -> add_credits() -> cifs_reconnect() -> > -> mid2.callback() -> add_credits() -> cifs_reconnect(). > > Fix this by avoiding to call cifs_reconnect() in add_credits() > and checking for zero credits in the demultiplex thread. > > Cc: <stable@vger.kernel.org> > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > --- > fs/cifs/connect.c | 21 +++++++++++++++++++++ > fs/cifs/smb2ops.c | 29 ++++++++++++++++++++++------- > 2 files changed, 43 insertions(+), 7 deletions(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 3d5e308..dc02b6d 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -699,6 +699,21 @@ server_unresponsive(struct TCP_Server_Info *server) > return false; > } > > +static inline bool > +zero_credits(struct TCP_Server_Info *server) > +{ > + int val; > + > + spin_lock(&server->req_lock); > + val = server->credits + server->echo_credits + server->oplock_credits; > + if (server->in_flight == 0 && val == 0) { > + spin_unlock(&server->req_lock); > + return true; > + } > + spin_unlock(&server->req_lock); > + return false; > +} > + > static int > cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg) > { > @@ -711,6 +726,12 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg) > for (total_read = 0; msg_data_left(smb_msg); total_read += length) { > try_to_freeze(); > > + /* reconnect if no credits and no requests in flight */ > + if (zero_credits(server)) { > + cifs_reconnect(server); > + return -ECONNABORTED; > + } > + > if (server_unresponsive(server)) > return -ECONNABORTED; > if (cifs_rdma_enabled(server) && server->smbd_conn) > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 82967f6..3b6c95d 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -34,6 +34,7 @@ > #include "cifs_ioctl.h" > #include "smbdirect.h" > > +/* Change credits for different ops and return the total number of credits */ > static int > change_conf(struct TCP_Server_Info *server) > { > @@ -41,17 +42,15 @@ change_conf(struct TCP_Server_Info *server) > server->oplock_credits = server->echo_credits = 0; > switch (server->credits) { > case 0: > - return -1; > + return 0; > case 1: > server->echoes = false; > server->oplocks = false; > - cifs_dbg(VFS, "disabling echoes and oplocks\n"); > break; > case 2: > server->echoes = true; > server->oplocks = false; > server->echo_credits = 1; > - cifs_dbg(FYI, "disabling oplocks\n"); > break; > default: > server->echoes = true; > @@ -64,14 +63,15 @@ change_conf(struct TCP_Server_Info *server) > server->echo_credits = 1; > } > server->credits -= server->echo_credits + server->oplock_credits; > - return 0; > + return server->credits + server->echo_credits + server->oplock_credits; > } > > static void > smb2_add_credits(struct TCP_Server_Info *server, const unsigned int add, > const int optype) > { > - int *val, rc = 0; > + int *val, rc = -1; > + > spin_lock(&server->req_lock); > val = server->ops->get_credits_field(server, optype); > > @@ -101,8 +101,23 @@ smb2_add_credits(struct TCP_Server_Info *server, const unsigned int add, > } > spin_unlock(&server->req_lock); > wake_up(&server->request_q); > - if (rc) > - cifs_reconnect(server); We should prevent logging if we are reconnecting to not confuse users: + + if (server->tcpStatus == CifsNeedReconnect) + return; > + > + switch (rc) { > + case -1: > + /* change_conf hasn't been executed */ > + break; > + case 0: > + cifs_dbg(VFS, "Possible client or server bug - zero credits\n"); > + break; > + case 1: > + cifs_dbg(VFS, "disabling echoes and oplocks\n"); > + break; > + case 2: > + cifs_dbg(FYI, "disabling oplocks\n"); > + break; > + default: > + cifs_dbg(FYI, "add %u credits total=%d\n", add, rc); > + } > } > > static void > -- > 2.7.4 >
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 3d5e308..dc02b6d 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -699,6 +699,21 @@ server_unresponsive(struct TCP_Server_Info *server) return false; } +static inline bool +zero_credits(struct TCP_Server_Info *server) +{ + int val; + + spin_lock(&server->req_lock); + val = server->credits + server->echo_credits + server->oplock_credits; + if (server->in_flight == 0 && val == 0) { + spin_unlock(&server->req_lock); + return true; + } + spin_unlock(&server->req_lock); + return false; +} + static int cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg) { @@ -711,6 +726,12 @@ cifs_readv_from_socket(struct TCP_Server_Info *server, struct msghdr *smb_msg) for (total_read = 0; msg_data_left(smb_msg); total_read += length) { try_to_freeze(); + /* reconnect if no credits and no requests in flight */ + if (zero_credits(server)) { + cifs_reconnect(server); + return -ECONNABORTED; + } + if (server_unresponsive(server)) return -ECONNABORTED; if (cifs_rdma_enabled(server) && server->smbd_conn) diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 82967f6..3b6c95d 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -34,6 +34,7 @@ #include "cifs_ioctl.h" #include "smbdirect.h" +/* Change credits for different ops and return the total number of credits */ static int change_conf(struct TCP_Server_Info *server) { @@ -41,17 +42,15 @@ change_conf(struct TCP_Server_Info *server) server->oplock_credits = server->echo_credits = 0; switch (server->credits) { case 0: - return -1; + return 0; case 1: server->echoes = false; server->oplocks = false; - cifs_dbg(VFS, "disabling echoes and oplocks\n"); break; case 2: server->echoes = true; server->oplocks = false; server->echo_credits = 1; - cifs_dbg(FYI, "disabling oplocks\n"); break; default: server->echoes = true; @@ -64,14 +63,15 @@ change_conf(struct TCP_Server_Info *server) server->echo_credits = 1; } server->credits -= server->echo_credits + server->oplock_credits; - return 0; + return server->credits + server->echo_credits + server->oplock_credits; } static void smb2_add_credits(struct TCP_Server_Info *server, const unsigned int add, const int optype) { - int *val, rc = 0; + int *val, rc = -1; + spin_lock(&server->req_lock); val = server->ops->get_credits_field(server, optype); @@ -101,8 +101,23 @@ smb2_add_credits(struct TCP_Server_Info *server, const unsigned int add, } spin_unlock(&server->req_lock); wake_up(&server->request_q); - if (rc) - cifs_reconnect(server); + + switch (rc) { + case -1: + /* change_conf hasn't been executed */ + break; + case 0: + cifs_dbg(VFS, "Possible client or server bug - zero credits\n"); + break; + case 1: + cifs_dbg(VFS, "disabling echoes and oplocks\n"); + break; + case 2: + cifs_dbg(FYI, "disabling oplocks\n"); + break; + default: + cifs_dbg(FYI, "add %u credits total=%d\n", add, rc); + } } static void
When executing add_credits() we currently call cifs_reconnect() if the number of credits is zero and there are no requests in flight. In this case we may call cifs_reconnect() recursively twice and cause memory corruption given the following sequence of functions: mid1.callback() -> add_credits() -> cifs_reconnect() -> -> mid2.callback() -> add_credits() -> cifs_reconnect(). Fix this by avoiding to call cifs_reconnect() in add_credits() and checking for zero credits in the demultiplex thread. Cc: <stable@vger.kernel.org> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> --- fs/cifs/connect.c | 21 +++++++++++++++++++++ fs/cifs/smb2ops.c | 29 ++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 7 deletions(-)