[01/15] CIFS: Do not reconnect TCP session in add_credits()
diff mbox series

Message ID 1548277845-6746-2-git-send-email-pshilov@microsoft.com
State New
Headers show
Series
  • Improve credits handling on reconnects
Related show

Commit Message

Pavel Shilovsky Jan. 23, 2019, 9:10 p.m. UTC
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(-)

Comments

Pavel Shilovsky Jan. 25, 2019, 6:56 p.m. UTC | #1
--
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
>

Patch
diff mbox series

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