CIFS: Reconnect expired SMB sessions
diff mbox

Message ID 1499549520-113585-1-git-send-email-pshilov@microsoft.com
State New
Headers show

Commit Message

Pavel Shilovskiy July 8, 2017, 9:32 p.m. UTC
According to the MS-SMB2 spec (3.2.5.1.6) once the client receives
STATUS_NETWORK_SESSION_EXPIRED error code from a server it should
reconnect the current SMB session. Currently the client doesn't do
that. This can result in subsequent client requests failing by
the server. The patch adds an additional logic to the demultiplex
thread to identify expired sessions and reconnect them.

Cc: <stable@vger.kernel.org>
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
---
 fs/cifs/cifsglob.h |  2 ++
 fs/cifs/cifssmb.c  |  7 +++++++
 fs/cifs/connect.c  |  7 +++++++
 fs/cifs/smb2ops.c  | 23 +++++++++++++++++++++++
 4 files changed, 39 insertions(+)

Comments

Germano Percossi July 10, 2017, 2:04 p.m. UTC | #1
Hi Pavel,

I have one comment/question.

On 07/08/2017 10:32 PM, Pavel Shilovsky wrote:
> According to the MS-SMB2 spec (3.2.5.1.6) once the client receives
> STATUS_NETWORK_SESSION_EXPIRED error code from a server it should
> reconnect the current SMB session. Currently the client doesn't do
> that. This can result in subsequent client requests failing by
> the server. The patch adds an additional logic to the demultiplex
> thread to identify expired sessions and reconnect them.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> ---
>  fs/cifs/cifsglob.h |  2 ++
>  fs/cifs/cifssmb.c  |  7 +++++++
>  fs/cifs/connect.c  |  7 +++++++
>  fs/cifs/smb2ops.c  | 23 +++++++++++++++++++++++
>  4 files changed, 39 insertions(+)
> 
[cut]
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index fbb0d4c..72a53bd 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -1460,6 +1460,13 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
>  		return length;
>  	server->total_read += length;
>  
> +	if (server->ops->is_session_expired &&
> +	    server->ops->is_session_expired(buf)) {
> +		cifs_reconnect(server);
> +		wake_up(&server->response_q);
> +		return -1;
> +	}
> +

Here (but the same applied to the other similar code blocks), as soon as
the session is found expired, there is an attempt to reconnect and -1 is
returned, regardless of the reconnect outcome.

However, according to the specs, if the re-authentication attempt fails
the client must fail the operation and terminate the session.
Is the latter done implicitly, because I cannot see it?

Cheers,
Germano

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Shilovskiy July 11, 2017, 12:07 a.m. UTC | #2
2017-07-10 7:04 GMT-07:00 Germano Percossi <germano.percossi@citrix.com>:
> Hi Pavel,

>

> I have one comment/question.

>

> On 07/08/2017 10:32 PM, Pavel Shilovsky wrote:

>> According to the MS-SMB2 spec (3.2.5.1.6) once the client receives

>> STATUS_NETWORK_SESSION_EXPIRED error code from a server it should

>> reconnect the current SMB session. Currently the client doesn't do

>> that. This can result in subsequent client requests failing by

>> the server. The patch adds an additional logic to the demultiplex

>> thread to identify expired sessions and reconnect them.

>>

>> Cc: <stable@vger.kernel.org>

>> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>

>> ---

>>  fs/cifs/cifsglob.h |  2 ++

>>  fs/cifs/cifssmb.c  |  7 +++++++

>>  fs/cifs/connect.c  |  7 +++++++

>>  fs/cifs/smb2ops.c  | 23 +++++++++++++++++++++++

>>  4 files changed, 39 insertions(+)

>>

> [cut]

>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c

>> index fbb0d4c..72a53bd 100644

>> --- a/fs/cifs/cifssmb.c

>> +++ b/fs/cifs/cifssmb.c

>> @@ -1460,6 +1460,13 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)

>>               return length;

>>       server->total_read += length;

>>

>> +     if (server->ops->is_session_expired &&

>> +         server->ops->is_session_expired(buf)) {

>> +             cifs_reconnect(server);

>> +             wake_up(&server->response_q);

>> +             return -1;

>> +     }

>> +

>

> Here (but the same applied to the other similar code blocks), as soon as

> the session is found expired, there is an attempt to reconnect and -1 is

> returned, regardless of the reconnect outcome.

>

> However, according to the specs, if the re-authentication attempt fails

> the client must fail the operation and terminate the session.

> Is the latter done implicitly, because I cannot see it?

>

> Cheers,

> Germano

>


Hi Germano,

cifs_reconnect does shutdown and then connect again the socket. Shutting down the socket implicitly terminates the existing SMB session which will be scheduled for re-negotiation. The negotiation is done in a separate thread: see cifs_reconnect() calling mod_delayed_work() that schedules server->echo work to be executed immediately; the latter then starts negotiating SMB session.

cifs_reconnect() itself should be a void function because it always returns 0 (patches are welcome to clean it up). Demultiplex thread treats -1 error code as an indicator that the request should not be processed further and we need to continue polling the socket for data.

--
Best regards,
Pavel Shilovsky

Patch
diff mbox

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index bcc7d9a..fb48251 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -367,6 +367,8 @@  struct smb_version_operations {
 	unsigned int (*calc_smb_size)(void *);
 	/* check for STATUS_PENDING and process it in a positive case */
 	bool (*is_status_pending)(char *, struct TCP_Server_Info *, int);
+	/* check for STATUS_NETWORK_SESSION_EXPIRED */
+	bool (*is_session_expired)(char *);
 	/* send oplock break response */
 	int (*oplock_response)(struct cifs_tcon *, struct cifs_fid *,
 			       struct cifsInodeInfo *);
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index fbb0d4c..72a53bd 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -1460,6 +1460,13 @@  cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		return length;
 	server->total_read += length;
 
+	if (server->ops->is_session_expired &&
+	    server->ops->is_session_expired(buf)) {
+		cifs_reconnect(server);
+		wake_up(&server->response_q);
+		return -1;
+	}
+
 	if (server->ops->is_status_pending &&
 	    server->ops->is_status_pending(buf, server, 0)) {
 		cifs_discard_remaining_data(server);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9365c0c..c59d77f 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -812,6 +812,13 @@  cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 		cifs_dump_mem("Bad SMB: ", buf,
 			min_t(unsigned int, server->total_read, 48));
 
+	if (server->ops->is_session_expired &&
+	    server->ops->is_session_expired(buf)) {
+		cifs_reconnect(server);
+		wake_up(&server->response_q);
+		return -1;
+	}
+
 	if (server->ops->is_status_pending &&
 	    server->ops->is_status_pending(buf, server, length))
 		return -1;
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index ccbb397..0866dda 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1036,6 +1036,18 @@  smb2_is_status_pending(char *buf, struct TCP_Server_Info *server, int length)
 	return true;
 }
 
+static bool
+smb2_is_session_expired(char *buf)
+{
+	struct smb2_sync_hdr *shdr = get_sync_hdr(buf);
+
+	if (shdr->Status != STATUS_NETWORK_SESSION_EXPIRED)
+		return false;
+
+	cifs_dbg(FYI, "Session expired\n");
+	return true;
+}
+
 static int
 smb2_oplock_response(struct cifs_tcon *tcon, struct cifs_fid *fid,
 		     struct cifsInodeInfo *cinode)
@@ -2160,6 +2172,13 @@  handle_read_data(struct TCP_Server_Info *server, struct mid_q_entry *mid,
 		return -ENOTSUPP;
 	}
 
+	if (server->ops->is_session_expired &&
+	    server->ops->is_session_expired(buf)) {
+		cifs_reconnect(server);
+		wake_up(&server->response_q);
+		return -1;
+	}
+
 	if (server->ops->is_status_pending &&
 			server->ops->is_status_pending(buf, server, 0))
 		return -1;
@@ -2477,6 +2496,7 @@  struct smb_version_operations smb20_operations = {
 	.close_dir = smb2_close_dir,
 	.calc_smb_size = smb2_calc_size,
 	.is_status_pending = smb2_is_status_pending,
+	.is_session_expired = smb2_is_session_expired,
 	.oplock_response = smb2_oplock_response,
 	.queryfs = smb2_queryfs,
 	.mand_lock = smb2_mand_lock,
@@ -2565,6 +2585,7 @@  struct smb_version_operations smb21_operations = {
 	.close_dir = smb2_close_dir,
 	.calc_smb_size = smb2_calc_size,
 	.is_status_pending = smb2_is_status_pending,
+	.is_session_expired = smb2_is_session_expired,
 	.oplock_response = smb2_oplock_response,
 	.queryfs = smb2_queryfs,
 	.mand_lock = smb2_mand_lock,
@@ -2655,6 +2676,7 @@  struct smb_version_operations smb30_operations = {
 	.close_dir = smb2_close_dir,
 	.calc_smb_size = smb2_calc_size,
 	.is_status_pending = smb2_is_status_pending,
+	.is_session_expired = smb2_is_session_expired,
 	.oplock_response = smb2_oplock_response,
 	.queryfs = smb2_queryfs,
 	.mand_lock = smb2_mand_lock,
@@ -2755,6 +2777,7 @@  struct smb_version_operations smb311_operations = {
 	.close_dir = smb2_close_dir,
 	.calc_smb_size = smb2_calc_size,
 	.is_status_pending = smb2_is_status_pending,
+	.is_session_expired = smb2_is_session_expired,
 	.oplock_response = smb2_oplock_response,
 	.queryfs = smb2_queryfs,
 	.mand_lock = smb2_mand_lock,