diff mbox series

[v1,13/13] cifs: introduce the CifsInvalidCredentials session state

Message ID 20200224131510.20608-14-metze@samba.org (mailing list archive)
State New, archived
Headers show
Series Avoid reconnects of failed session setups on soft mounts | expand

Commit Message

Stefan Metzmacher Feb. 24, 2020, 1:15 p.m. UTC
RHBZ: 1579050

If a session setup gets NT_STATUS_LOGON_FAILURE of
NT_STATUS_ACCOUNT_LOCKED_OUT, we're not able to make any further
progress with the credentials the kernel knowns about.

Instead of retrying for each new request, we should mark
the session with CifsInvalidCredentials (for know we only do that
for soft mounts). We consistently return -EKEYREVOKED for such sessions,
I guess that's better than -EHOSTDOWN.

A future addition would be an upcall to get new credentials from
userspace, or a way to use a magic file per session under /proc/fs/cifs/
to provide new credentials.

But for now we want to avoid that we lock out the users account,
by retrying every 2 seconds.

Note backports also need the other 12 patches before.

Fixes: b0dd940e582b6a6 ("cifs: fail i/o on soft mounts if sessionsetup errors out")
Signed-off-by: Stefan Metzmacher <metze@samba.org>
Cc: Stable <stable@vger.kernel.org>
Cc: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/cifs_debug.c |   2 +
 fs/cifs/cifsfs.c     |   1 +
 fs/cifs/cifsglob.h   |   4 +-
 fs/cifs/connect.c    | 145 ++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 148 insertions(+), 4 deletions(-)

Comments

Aurélien Aptel Feb. 24, 2020, 6:09 p.m. UTC | #1
Stefan Metzmacher <metze@samba.org> writes:
> A future addition would be an upcall to get new credentials from
> userspace, or a way to use a magic file per session under /proc/fs/cifs/
> to provide new credentials.

I've looked into this recently. cifscreds from cifs-utils can already
store/update credentials in keyrings.

I believe this is only used in multiuser mode (-o multiuser).  In that
mode, when a process does a syscall, cifs.ko will try to use a cifs_ses
matching the uid of that process, potentially opening a new one.

To open a new session for that user, cifs.ko looks at the current
process session keyring for that uid credentials. Take a look at
cifs_set_cifscreds(), it's the function that sets the credentials in the
volume about to be connected to.

* the key is of type "logon",
* description is "cifs:<mode>:<host>" where mode determines what host is
  ('a' for an ip address, 'd' for a domain).
* value is "<user>:<password>"

[ side-note on that keyring: it is the process session keyring. So you
need to make sure the keyring is created when the user first logs in the
system (i.e. via pam), otherwise cifscreds will create it, and since it
is the only user, will destroy it when cifscreds exits (refcount reaches
zero).  I don't know why it was decided to use the session keyring, I
feel like we should make this keyring "global" instead of per session,
it would be easier to setup and update but I don't know the security
implications. (If anyone knows please share) ]

In any case, I think we should try to update
cifs_ses->{user_name,password} before re-opening a session by looking at
this keyring.

Cheers,
diff mbox series

Patch

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 276e4b5ea8e0..c03a445e95bc 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -392,6 +392,8 @@  static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 
 			/* dump session id helpful for use with network trace */
 			seq_printf(m, " SessionId: 0x%llx", ses->Suid);
+			if (ses->status == CifsInvalidCredentials)
+				seq_printf(m, " InvalidCredentials!");
 			if (ses->session_flags & SMB2_SESSION_FLAG_ENCRYPT_DATA)
 				seq_puts(m, " encrypted");
 			if (ses->sign)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 46ebaf3f0824..9e2e5fa2cdff 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -650,6 +650,7 @@  static void cifs_umount_begin(struct super_block *sb)
 	/* cancel_notify_requests(tcon); */
 	if (tcon->ses && tcon->ses->server) {
 		cifs_dbg(FYI, "wake up tasks now - umount begin not complete\n");
+		wake_up_all(&tcon->ses->server->demultiplex_q);
 		wake_up_all(&tcon->ses->server->request_q);
 		wake_up_all(&tcon->ses->server->response_q);
 		msleep(1); /* yield */
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 8393ed7ebf96..4494b35fa5c5 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -114,7 +114,8 @@  enum statusEnum {
 	CifsGood,
 	CifsExiting,
 	CifsNeedReconnect,
-	CifsNeedNegotiate
+	CifsNeedNegotiate,
+	CifsInvalidCredentials,
 };
 
 enum securityEnum {
@@ -687,6 +688,7 @@  struct TCP_Server_Info {
 #endif
 	wait_queue_head_t response_q;
 	wait_queue_head_t request_q; /* if more than maxmpx to srvr must block*/
+	wait_queue_head_t demultiplex_q;
 	struct list_head pending_mid_q;
 	bool noblocksnd;		/* use blocking sendmsg */
 	bool noautotune;		/* do not autotune send buf sizes */
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7f4be85b7cc9..39c0a5154eb9 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -480,6 +480,65 @@  static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
 }
 #endif
 
+static bool
+server_should_poll_for_responses(struct TCP_Server_Info *server)
+{
+	switch (server->tcpStatus) {
+	case CifsNew:
+		return false;
+	case CifsExiting:
+		return false;
+	case CifsGood:
+		return true;
+	case CifsNeedReconnect:
+		return false;
+	case CifsNeedNegotiate:
+		return true;
+	case CifsInvalidCredentials:
+		/*
+		 * Only a session should have this state
+		 */
+		WARN(true, "BUG! server->tcpStatus=CifsInvalidCredentials\n");
+		return false;
+	}
+
+	WARN(true, "Unhandled server->tcpStatus=%u\n", server->tcpStatus);
+	return false;
+}
+
+static bool
+server_should_reconnect(struct TCP_Server_Info *server)
+{
+	struct cifs_ses *ses;
+	size_t num_valid = 0;
+	size_t num_invalid = 0;
+
+	/*
+	 * If we are in disconnected state, waiting for a reconnect,
+	 * we should only try to reconnect if there's a chance
+	 * of success, that's not the case of all sessions
+	 * are have invalid credentials.
+	 */
+	if (server->tcpStatus != CifsNeedReconnect)
+		return false;
+
+	spin_lock(&cifs_tcp_ses_lock);
+	list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
+		if (ses->status != CifsInvalidCredentials) {
+			num_valid++;
+		} else {
+			num_invalid++;
+		}
+	}
+	spin_unlock(&cifs_tcp_ses_lock);
+
+	if (num_invalid == 0 || num_valid != 0) {
+		return true;
+	}
+
+	return false;
+}
+
 /*
  * cifs tcp session reconnection
  *
@@ -617,6 +676,17 @@  cifs_reconnect(struct TCP_Server_Info *server)
 		try_to_freeze();
 
 		mutex_lock(&server->srv_mutex);
+
+		if (!server_should_reconnect(server)) {
+			cifs_dbg(VFS,
+				 "Server %s was disconnected. "
+				 "We should NOT reconnect...\n",
+				 server->hostname);
+			rc = -ECONNABORTED;
+			mutex_unlock(&server->srv_mutex);
+			break;
+		}
+
 		/*
 		 * Set up next DFS target server (if any) for reconnect. If DFS
 		 * feature is disabled, then we will retry last server we
@@ -669,8 +739,17 @@  cifs_reconnect(struct TCP_Server_Info *server)
 
 	put_tcp_super(sb);
 #endif
-	if (server->tcpStatus == CifsNeedNegotiate)
-		mod_delayed_work(cifsiod_wq, &server->echo, 0);
+	if (server->tcpStatus == CifsNeedNegotiate) {
+		/* unfreeze the demultiplex thread */
+		wake_up_all(&server->demultiplex_q);
+		/*
+		 * schedule a immediate reconnect of sessions
+		 * and tcons.
+		 */
+		mod_delayed_work(cifsiod_wq, &server->reconnect, 0);
+		/* reset the echo timer into the future. */
+		mod_delayed_work(cifsiod_wq, &server->echo, server->echo_interval);
+	}
 
 	wake_up(&server->response_q);
 	return rc;
@@ -872,6 +951,19 @@  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();
 
+		if (server_should_reconnect(server)) {
+			cifs_dbg(VFS, "Server %s was disconnected. Reconnecting...\n",
+				 server->hostname);
+			cifs_reconnect(server);
+			wake_up(&server->response_q);
+			return -ECONNABORTED;
+		}
+
+		if (!server_should_poll_for_responses(server)) {
+			cifs_dbg(FYI, "Only invalid credential sessions\n");
+			return -ECONNABORTED;
+		}
+
 		/* reconnect if no credits and no requests in flight */
 		if (zero_credits(server)) {
 			cifs_reconnect(server);
@@ -1222,6 +1314,22 @@  smb2_add_credits_from_hdr(char *buffer, struct TCP_Server_Info *server)
 	}
 }
 
+static bool cifs_demultiplex_thread_should_block(struct TCP_Server_Info *server)
+{
+	if (server->tcpStatus == CifsExiting) {
+		return false;
+	}
+
+	if (server_should_reconnect(server)) {
+		return false;
+	}
+
+	if (server_should_poll_for_responses(server)) {
+		return false;
+	}
+
+	return true;
+}
 
 static int
 cifs_demultiplex_thread(void *p)
@@ -1248,6 +1356,11 @@  cifs_demultiplex_thread(void *p)
 		if (try_to_freeze())
 			continue;
 
+		wait_event_freezable(server->demultiplex_q,
+				!cifs_demultiplex_thread_should_block(server));
+		if (server->tcpStatus == CifsExiting)
+			break;
+
 		if (!allocate_buffers(server))
 			continue;
 
@@ -2880,6 +2993,7 @@  cifs_get_tcp_session(struct smb_vol *volume_info)
 	tcp_ses->credits = 1;
 	init_waitqueue_head(&tcp_ses->response_q);
 	init_waitqueue_head(&tcp_ses->request_q);
+	init_waitqueue_head(&tcp_ses->demultiplex_q);
 	INIT_LIST_HEAD(&tcp_ses->pending_mid_q);
 	mutex_init(&tcp_ses->srv_mutex);
 	memcpy(tcp_ses->workstation_RFC1001_name,
@@ -2967,6 +3081,13 @@  cifs_get_tcp_session(struct smb_vol *volume_info)
 
 	cifs_fscache_get_client_cookie(tcp_ses);
 
+	/* unfreeze the demultiplex thread */
+	wake_up_all(&tcp_ses->demultiplex_q);
+	/*
+	 * we let the caller use cifs_connect_session_locked()
+	 * (directly or via cifs_get_smb_ses() and don't
+	 * schedule the reconnect timer.
+	 */
 	/* queue echo request delayed work */
 	queue_delayed_work(cifsiod_wq, &tcp_ses->echo, tcp_ses->echo_interval);
 
@@ -5454,6 +5575,10 @@  cifs_connect_session_locked(const unsigned int xid,
 {
 	int rc;
 
+	if (ses->status == CifsInvalidCredentials) {
+		return -EKEYREVOKED;
+	}
+
 	if (ses->server->tcpStatus == CifsNeedReconnect) {
 		return -EHOSTDOWN;
 	}
@@ -5463,7 +5588,12 @@  cifs_connect_session_locked(const unsigned int xid,
 		cifs_dbg(FYI, "Session needs reconnect\n");
 		rc = cifs_setup_session(xid, ses, nls_info);
 		if ((rc == -EACCES) && !retry) {
-			return -EHOSTDOWN;
+			cifs_dbg(VFS,
+				 "SessionSetup to Server %s failed, "
+				 "marking as CifsInvalidCredentials/EKEYREVOKED\n",
+				 ses->server->hostname);
+			ses->status = CifsInvalidCredentials;
+			return -EKEYREVOKED;
 		}
 	}
 
@@ -5505,6 +5635,10 @@  cifs_tcon_reconnect(struct cifs_tcon *tcon,
 
 	retries = server->nr_targets;
 
+	if (ses->status == CifsInvalidCredentials) {
+		return -EKEYREVOKED;
+	}
+
 	/*
 	 * Give demultiplex thread up to 10 seconds to each target available for
 	 * reconnect -- should be greater than cifs socket timeout which is 7
@@ -5515,6 +5649,7 @@  cifs_tcon_reconnect(struct cifs_tcon *tcon,
 			return -EAGAIN;
 		}
 
+		wake_up_all(&server->demultiplex_q);
 		rc = wait_event_interruptible_timeout(server->response_q,
 						      (server->tcpStatus != CifsNeedReconnect),
 						      10 * HZ);
@@ -5543,6 +5678,10 @@  cifs_tcon_reconnect(struct cifs_tcon *tcon,
 		retries = server->nr_targets;
 	}
 
+	if (ses->status == CifsInvalidCredentials) {
+		return -EKEYREVOKED;
+	}
+
 	if (!ses->need_reconnect && !tcon->need_reconnect)
 		return 0;