[PATCH 1/2] cifs: fix potential race with cifsd thread

[PATCH 1/2] cifs: fix potential race with cifsd thread

[Paulo Alcantara]

To avoid racing with demultiplex thread while it is handling data on
socket, use cifs_signal_cifsd_for_reconnect() helper for marking
current server to reconnect and let the demultiplex thread handle the
rest.

Fixes: dca65818c80c ("cifs: use a different reconnect helper for non-cifsd threads")
Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
---
 fs/cifs/connect.c | 2 +-
 fs/cifs/netmisc.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index ee3b7c15e884..3ca06bd88b6e 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -4465,7 +4465,7 @@ static int tree_connect_dfs_target(const unsigned int xid, struct cifs_tcon *tco
 	 */
 	if (rc && server->current_fullpath != server->origin_fullpath) {
 		server->current_fullpath = server->origin_fullpath;
-		cifs_reconnect(tcon->ses->server, true);
+		cifs_signal_cifsd_for_reconnect(server, true);
 	}
 
 	dfs_cache_free_tgts(tl);
diff --git a/fs/cifs/netmisc.c b/fs/cifs/netmisc.c
index ebe236b9d9f5..235aa1b395eb 100644
--- a/fs/cifs/netmisc.c
+++ b/fs/cifs/netmisc.c
@@ -896,7 +896,7 @@ map_and_check_smb_error(struct mid_q_entry *mid, bool logErr)
 		if (class == ERRSRV && code == ERRbaduid) {
 			cifs_dbg(FYI, "Server returned 0x%x, reconnecting session...\n",
 				code);
-			cifs_reconnect(mid->server, false);
+			cifs_signal_cifsd_for_reconnect(mid->server, false);
 		}
 	}
 
-- 
2.35.1

[PATCH 2/2] cifs: force new session setup and tcon for dfs

[Paulo Alcantara]

Do not reuse existing sessions and tcons in DFS failover as it might
connect to different servers and shares.

Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
---
 fs/cifs/connect.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 3ca06bd88b6e..3956672a11ae 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -536,8 +536,11 @@ int cifs_reconnect(struct TCP_Server_Info *server, bool mark_smb_session)
 		return __cifs_reconnect(server, mark_smb_session);
 	}
 	spin_unlock(&cifs_tcp_ses_lock);
-
-	return reconnect_dfs_server(server, mark_smb_session);
+	/*
+	 * Ignore @mark_smb_session and invalidate all sessions & tcons as we might be connecting to
+	 * a different server or share during failover.
+	 */
+	return reconnect_dfs_server(server, true);
 }
 #else
 int cifs_reconnect(struct TCP_Server_Info *server, bool mark_smb_session)
-- 
2.35.1

Re: [PATCH 2/2] cifs: force new session setup and tcon for dfs

[Enzo Matsumiya]

On 03/31, Paulo Alcantara wrote:
>Do not reuse existing sessions and tcons in DFS failover as it might
>connect to different servers and shares.
>
>Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
>---
> fs/cifs/connect.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>index 3ca06bd88b6e..3956672a11ae 100644
>--- a/fs/cifs/connect.c
>+++ b/fs/cifs/connect.c
>@@ -536,8 +536,11 @@ int cifs_reconnect(struct TCP_Server_Info *server, bool mark_smb_session)
> 		return __cifs_reconnect(server, mark_smb_session);
> 	}
> 	spin_unlock(&cifs_tcp_ses_lock);
>-
>-	return reconnect_dfs_server(server, mark_smb_session);
>+	/*
>+	 * Ignore @mark_smb_session and invalidate all sessions & tcons as we might be connecting to
>+	 * a different server or share during failover.
>+	 */
>+	return reconnect_dfs_server(server, true);

If you're ignoring @mark_smb_session, why not just leave @server for
reconnect_dfs_server()?


Cheers,

Enzo

Re: [PATCH 2/2] cifs: force new session setup and tcon for dfs

[Shyam Prasad N]

On Fri, Apr 1, 2022 at 12:42 AM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Do not reuse existing sessions and tcons in DFS failover as it might
> connect to different servers and shares.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> ---
>  fs/cifs/connect.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 3ca06bd88b6e..3956672a11ae 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -536,8 +536,11 @@ int cifs_reconnect(struct TCP_Server_Info *server, bool mark_smb_session)
>                 return __cifs_reconnect(server, mark_smb_session);
>         }
>         spin_unlock(&cifs_tcp_ses_lock);
> -
> -       return reconnect_dfs_server(server, mark_smb_session);
> +       /*
> +        * Ignore @mark_smb_session and invalidate all sessions & tcons as we might be connecting to
> +        * a different server or share during failover.
> +        */
> +       return reconnect_dfs_server(server, true);
>  }
>  #else
>  int cifs_reconnect(struct TCP_Server_Info *server, bool mark_smb_session)
> --
> 2.35.1
>
This makes sense.
Wondering if you could check if the reconnect happens to a new
server/share and then change mark_smb_session? Maybe that complicates
the logic.

Looks good to me.
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>

-- 
Regards,
Shyam

Re: [PATCH 1/2] cifs: fix potential race with cifsd thread

[Shyam Prasad N]

On Fri, Apr 1, 2022 at 4:38 AM Paulo Alcantara <pc@cjr.nz> wrote:
>
> To avoid racing with demultiplex thread while it is handling data on
> socket, use cifs_signal_cifsd_for_reconnect() helper for marking
> current server to reconnect and let the demultiplex thread handle the
> rest.
>
> Fixes: dca65818c80c ("cifs: use a different reconnect helper for non-cifsd threads")
> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> ---
>  fs/cifs/connect.c | 2 +-
>  fs/cifs/netmisc.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index ee3b7c15e884..3ca06bd88b6e 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -4465,7 +4465,7 @@ static int tree_connect_dfs_target(const unsigned int xid, struct cifs_tcon *tco
>          */
>         if (rc && server->current_fullpath != server->origin_fullpath) {
>                 server->current_fullpath = server->origin_fullpath;
> -               cifs_reconnect(tcon->ses->server, true);
> +               cifs_signal_cifsd_for_reconnect(server, true);
>         }
>
>         dfs_cache_free_tgts(tl);
> diff --git a/fs/cifs/netmisc.c b/fs/cifs/netmisc.c
> index ebe236b9d9f5..235aa1b395eb 100644
> --- a/fs/cifs/netmisc.c
> +++ b/fs/cifs/netmisc.c
> @@ -896,7 +896,7 @@ map_and_check_smb_error(struct mid_q_entry *mid, bool logErr)
>                 if (class == ERRSRV && code == ERRbaduid) {
>                         cifs_dbg(FYI, "Server returned 0x%x, reconnecting session...\n",
>                                 code);
> -                       cifs_reconnect(mid->server, false);
> +                       cifs_signal_cifsd_for_reconnect(mid->server, false);
>                 }
>         }
>
> --
> 2.35.1
>

Oh. Did I miss these?
Looks good to me.

Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>

-- 
Regards,
Shyam

Re: [PATCH 2/2] cifs: force new session setup and tcon for dfs

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> If you're ignoring @mark_smb_session, why not just leave @server for
> reconnect_dfs_server()?

Good point, yes.  I'll send v2 with it removed.

Re: [PATCH 2/2] cifs: force new session setup and tcon for dfs

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> This makes sense.
> Wondering if you could check if the reconnect happens to a new
> server/share and then change mark_smb_session? Maybe that complicates
> the logic.

Yes, it would complicate alot.  We expect failover to not occur quite
often, so leaving it without such logic would be OK for now, IMO.  I'm
trying to get all reconnect issues fixed and then we can talk about
possible improvements, obviously.

Re: [PATCH 1/2] cifs: fix potential race with cifsd thread

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> Oh. Did I miss these?

Yep :-)

[PATCH v2] cifs: force new session setup and tcon for dfs

[Paulo Alcantara]

Do not reuse existing sessions and tcons in DFS failover as it might
connect to different servers and shares.

Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
---
 fs/cifs/connect.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 3ca06bd88b6e..54155eb4faac 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -453,9 +453,7 @@ static int reconnect_target_unlocked(struct TCP_Server_Info *server, struct dfs_
 	return rc;
 }
 
-static int
-reconnect_dfs_server(struct TCP_Server_Info *server,
-		     bool mark_smb_session)
+static int reconnect_dfs_server(struct TCP_Server_Info *server)
 {
 	int rc = 0;
 	const char *refpath = server->current_fullpath + 1;
@@ -479,7 +477,12 @@ reconnect_dfs_server(struct TCP_Server_Info *server,
 	if (!cifs_tcp_ses_needs_reconnect(server, num_targets))
 		return 0;
 
-	cifs_mark_tcp_ses_conns_for_reconnect(server, mark_smb_session);
+	/*
+	 * Unconditionally mark all sessions & tcons for reconnect as we might be connecting to a
+	 * different server or share during failover.  It could be improved by adding some logic to
+	 * only do that in case it connects to a different server or share, though.
+	 */
+	cifs_mark_tcp_ses_conns_for_reconnect(server, true);
 
 	cifs_abort_connection(server);
 
@@ -537,7 +540,7 @@ int cifs_reconnect(struct TCP_Server_Info *server, bool mark_smb_session)
 	}
 	spin_unlock(&cifs_tcp_ses_lock);
 
-	return reconnect_dfs_server(server, mark_smb_session);
+	return reconnect_dfs_server(server);
 }
 #else
 int cifs_reconnect(struct TCP_Server_Info *server, bool mark_smb_session)
-- 
2.35.1