diff mbox series

[3/5] cifs: avoid races in parallel reconnects in smb1

Message ID 20230329201423.32134-3-pc@manguebit.com (mailing list archive)
State New, archived
Headers show
Series [1/5] cifs: get rid of cifs_mount_ctx::{origin,leaf}_fullpath | expand

Commit Message

Paulo Alcantara March 29, 2023, 8:14 p.m. UTC
Prevent multiple threads of doing negotiate, session setup and tree
connect by holding @ses->session_mutex in cifs_reconnect_tcon() while
reconnecting session and tcon.

Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/cifs/cifssmb.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

ronnie sahlberg March 29, 2023, 8:27 p.m. UTC | #1
reviewed-by me

On Thu, 30 Mar 2023 at 06:20, Paulo Alcantara <pc@manguebit.com> wrote:
>
> Prevent multiple threads of doing negotiate, session setup and tree
> connect by holding @ses->session_mutex in cifs_reconnect_tcon() while
> reconnecting session and tcon.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
>  fs/cifs/cifssmb.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 38a697eca305..c9d57ba84be4 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -71,7 +71,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         int rc;
>         struct cifs_ses *ses;
>         struct TCP_Server_Info *server;
> -       struct nls_table *nls_codepage;
> +       struct nls_table *nls_codepage = NULL;
>
>         /*
>          * SMBs NegProt, SessSetup, uLogoff do not have tcon yet so check for
> @@ -99,6 +99,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         }
>         spin_unlock(&tcon->tc_lock);
>
> +again:
>         rc = cifs_wait_for_server_reconnect(server, tcon->retry);
>         if (rc)
>                 return rc;
> @@ -110,8 +111,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         }
>         spin_unlock(&ses->chan_lock);
>
> -       nls_codepage = load_nls_default();
> -
> +       mutex_lock(&ses->session_mutex);
>         /*
>          * Recheck after acquire mutex. If another thread is negotiating
>          * and the server never sends an answer the socket will be closed
> @@ -120,29 +120,38 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         spin_lock(&server->srv_lock);
>         if (server->tcpStatus == CifsNeedReconnect) {
>                 spin_unlock(&server->srv_lock);
> +               mutex_lock(&ses->session_mutex);
> +
> +               if (tcon->retry)
> +                       goto again;
>                 rc = -EHOSTDOWN;
>                 goto out;
>         }
>         spin_unlock(&server->srv_lock);
>
> +       nls_codepage = load_nls_default();
> +
>         /*
>          * need to prevent multiple threads trying to simultaneously
>          * reconnect the same SMB session
>          */
> +       spin_lock(&ses->ses_lock);
>         spin_lock(&ses->chan_lock);
> -       if (!cifs_chan_needs_reconnect(ses, server)) {
> +       if (!cifs_chan_needs_reconnect(ses, server) &&
> +           ses->ses_status == SES_GOOD) {
>                 spin_unlock(&ses->chan_lock);
> +               spin_unlock(&ses->ses_lock);
>
>                 /* this means that we only need to tree connect */
>                 if (tcon->need_reconnect)
>                         goto skip_sess_setup;
>
> -               rc = -EHOSTDOWN;
> +               mutex_unlock(&ses->session_mutex);
>                 goto out;
>         }
>         spin_unlock(&ses->chan_lock);
> +       spin_unlock(&ses->ses_lock);
>
> -       mutex_lock(&ses->session_mutex);
>         rc = cifs_negotiate_protocol(0, ses, server);
>         if (!rc)
>                 rc = cifs_setup_session(0, ses, server, nls_codepage);
> --
> 2.40.0
>
Steve French March 30, 2023, 10:59 p.m. UTC | #2
added ronnie's RB and merged into cifs-2.6.git for-next (I will hold
off on patches 1 and 2 since they are cleanup until 6.4-rc)

On Wed, Mar 29, 2023 at 3:14 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Prevent multiple threads of doing negotiate, session setup and tree
> connect by holding @ses->session_mutex in cifs_reconnect_tcon() while
> reconnecting session and tcon.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
>  fs/cifs/cifssmb.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 38a697eca305..c9d57ba84be4 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -71,7 +71,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         int rc;
>         struct cifs_ses *ses;
>         struct TCP_Server_Info *server;
> -       struct nls_table *nls_codepage;
> +       struct nls_table *nls_codepage = NULL;
>
>         /*
>          * SMBs NegProt, SessSetup, uLogoff do not have tcon yet so check for
> @@ -99,6 +99,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         }
>         spin_unlock(&tcon->tc_lock);
>
> +again:
>         rc = cifs_wait_for_server_reconnect(server, tcon->retry);
>         if (rc)
>                 return rc;
> @@ -110,8 +111,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         }
>         spin_unlock(&ses->chan_lock);
>
> -       nls_codepage = load_nls_default();
> -
> +       mutex_lock(&ses->session_mutex);
>         /*
>          * Recheck after acquire mutex. If another thread is negotiating
>          * and the server never sends an answer the socket will be closed
> @@ -120,29 +120,38 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>         spin_lock(&server->srv_lock);
>         if (server->tcpStatus == CifsNeedReconnect) {
>                 spin_unlock(&server->srv_lock);
> +               mutex_lock(&ses->session_mutex);
> +
> +               if (tcon->retry)
> +                       goto again;
>                 rc = -EHOSTDOWN;
>                 goto out;
>         }
>         spin_unlock(&server->srv_lock);
>
> +       nls_codepage = load_nls_default();
> +
>         /*
>          * need to prevent multiple threads trying to simultaneously
>          * reconnect the same SMB session
>          */
> +       spin_lock(&ses->ses_lock);
>         spin_lock(&ses->chan_lock);
> -       if (!cifs_chan_needs_reconnect(ses, server)) {
> +       if (!cifs_chan_needs_reconnect(ses, server) &&
> +           ses->ses_status == SES_GOOD) {
>                 spin_unlock(&ses->chan_lock);
> +               spin_unlock(&ses->ses_lock);
>
>                 /* this means that we only need to tree connect */
>                 if (tcon->need_reconnect)
>                         goto skip_sess_setup;
>
> -               rc = -EHOSTDOWN;
> +               mutex_unlock(&ses->session_mutex);
>                 goto out;
>         }
>         spin_unlock(&ses->chan_lock);
> +       spin_unlock(&ses->ses_lock);
>
> -       mutex_lock(&ses->session_mutex);
>         rc = cifs_negotiate_protocol(0, ses, server);
>         if (!rc)
>                 rc = cifs_setup_session(0, ses, server, nls_codepage);
> --
> 2.40.0
>
diff mbox series

Patch

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 38a697eca305..c9d57ba84be4 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -71,7 +71,7 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	int rc;
 	struct cifs_ses *ses;
 	struct TCP_Server_Info *server;
-	struct nls_table *nls_codepage;
+	struct nls_table *nls_codepage = NULL;
 
 	/*
 	 * SMBs NegProt, SessSetup, uLogoff do not have tcon yet so check for
@@ -99,6 +99,7 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	}
 	spin_unlock(&tcon->tc_lock);
 
+again:
 	rc = cifs_wait_for_server_reconnect(server, tcon->retry);
 	if (rc)
 		return rc;
@@ -110,8 +111,7 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	}
 	spin_unlock(&ses->chan_lock);
 
-	nls_codepage = load_nls_default();
-
+	mutex_lock(&ses->session_mutex);
 	/*
 	 * Recheck after acquire mutex. If another thread is negotiating
 	 * and the server never sends an answer the socket will be closed
@@ -120,29 +120,38 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	spin_lock(&server->srv_lock);
 	if (server->tcpStatus == CifsNeedReconnect) {
 		spin_unlock(&server->srv_lock);
+		mutex_lock(&ses->session_mutex);
+
+		if (tcon->retry)
+			goto again;
 		rc = -EHOSTDOWN;
 		goto out;
 	}
 	spin_unlock(&server->srv_lock);
 
+	nls_codepage = load_nls_default();
+
 	/*
 	 * need to prevent multiple threads trying to simultaneously
 	 * reconnect the same SMB session
 	 */
+	spin_lock(&ses->ses_lock);
 	spin_lock(&ses->chan_lock);
-	if (!cifs_chan_needs_reconnect(ses, server)) {
+	if (!cifs_chan_needs_reconnect(ses, server) &&
+	    ses->ses_status == SES_GOOD) {
 		spin_unlock(&ses->chan_lock);
+		spin_unlock(&ses->ses_lock);
 
 		/* this means that we only need to tree connect */
 		if (tcon->need_reconnect)
 			goto skip_sess_setup;
 
-		rc = -EHOSTDOWN;
+		mutex_unlock(&ses->session_mutex);
 		goto out;
 	}
 	spin_unlock(&ses->chan_lock);
+	spin_unlock(&ses->ses_lock);
 
-	mutex_lock(&ses->session_mutex);
 	rc = cifs_negotiate_protocol(0, ses, server);
 	if (!rc)
 		rc = cifs_setup_session(0, ses, server, nls_codepage);