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

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(-)

--
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
>