Linux CIFS client module: login rate limiting

Commit Message

Sachin Prabhu Jan. 24, 2017, 9:57 a.m. UTC

On Mon, 2017-01-23 at 16:27 +0530, Sachin Prabhu wrote:
> On Fri, 2017-01-20 at 15:30 -0600, Steve French wrote:
> > A couple quick questions:
> > 1) I would not expect "hard" vs "soft" mount option makes no
> > difference here, but just doublechecking
> > 2) How does smb2 reconnect behave in the same scenario (because we
> > prefer smb3 to be used if the server is non-Samba)?
> > 
> > Looks like a fix is doable - see line 1464-1465 of fs/cifs/sess.c
> > 
> >     while (sess_data->func)
> >         sess_data->func(sess_data);
> > 
> > looking at cifs_reconnect in the case where the ip address is not
> > available we wait 3 seconds (if needed to retry), and when that
> > succeeds we schedule delayed work to issue an "echo" (see
> > cifs_reconnect) and then as we do cifs_reconnect_tcon we could wait
> > up
> > to 10 seconds at a time for the socket to come back. If socket is
> > ok
> > we do a negotiate protocol which is not necessarily retried on
> > failure
> > (depending on the request it can return EAGAIN - e.g.
> > read/write/lock/close).  If the negprot succeeds we get to your
> > case
> > where we call cifs_setup_session in fs/cifs/connect.c which calls
> > CIFS_SessSetup (in fs/cifs/sess.c) which looks like it will loop on
> > the sessionsetup retry for the cifs case - which should as you note
> > rate limit (especially on bad password case).
> > 
> > I also would like Sachin's feedback as he made some significant
> > cleanup of session establishment for cifs and rewrote this - wanted
> > to
> > see if he wanted to move the throttling of retries differently
> 
> I think the suggestion is perfectly valid and would be a nice
> addition
> to the cifs module. Maybe a better place to add this change would be
> at
> 
> cifs_reconnect_tcon()
> {
> ..
>         mutex_lock(&ses->session_mutex);
>         rc = cifs_negotiate_protocol(0, ses);
>         if (rc == 0 && ses->need_reconnect)
>                 rc = cifs_setup_session(0, ses, nls_codepage);
> ..
> }
> Where in case of EACCES, we can setup a delayed work to unlock ses-
> > session_mutex set to run after the required interval.
> 

Having given it another look, since it is unlikely to recover
automatically, I think it is better to cache the lookup and return the
cached lookup as long as the cache is still valid. I am also in favour
of a longer cache interval.

Attached is a patch which can work in this case. I use a cache interval
of 10 seconds which can be extended further.

Comments

Valentin Hilbig April 28, 2017, 4:34 p.m. UTC | #1

On 2017-01-24 at 10:57, Sachin Prabhu wrote:
> On Mon, 2017-01-23 at 16:27 +0530, Sachin Prabhu wrote:
>> On Fri, 2017-01-20 at 15:30 -0600, Steve French wrote:

>> cifs_reconnect_tcon()
>> {
>> ..
>>          mutex_lock(&ses->session_mutex);
>>          rc = cifs_negotiate_protocol(0, ses);
>>          if (rc == 0 && ses->need_reconnect)
>>                  rc = cifs_setup_session(0, ses, nls_codepage);
>> ..
>> }
>> Where in case of EACCES, we can setup a delayed work to unlock ses-
>>> session_mutex set to run after the required interval.

> Attached is a patch which can work in this case. I use a cache interval
> of 10 seconds which can be extended further.

It took a while until I found time to test your suggested patch.
Sorry for the delay.  Please note that your patch needs a small patch:

In cifsglob.h (around line 835) the

+	bool cached_rc;

should read

+	int  cached_rc;

as it holds the rc, not a flag.

Then it works as expected!  As soon as the reconnect fails due to 
password trouble, CIFS pauses 10s until the next retry.  Wonderful. ;)

So thank you very much for pointing me into the right direction!

As soon as everything is finished here I will report back to this list 
to give you the link to GitHub where I pushed my patches against Ubuntu 
3.13 and 4.4 kernels.

CU
-Tino
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

From 7ca9125be5522679c777a8e9a27a0af22a3d273d Mon Sep 17 00:00:00 2001
From: Sachin Prabhu <[e-mail]>
Date: Tue, 24 Jan 2017 12:43:03 +0530
Subject: [PATCH] cifs: Cache Access denied errors when reconnecting

If he account credentials on a mounted share is changed while still
mounted, remounts will fail with -EACCES. Since a new session setup call
is made every time an attempt is made to access this share, a large
number of failed session setup calls are made. This causes problems with
certain server setups which consider it as an attack on the account and
block further access from the account. To avoid this, cache all -EACCES
errors and avoid this problem.

Signed-off-by: Sachin Prabhu <[e-mail]>
---
 fs/cifs/cifsglob.h |  4 ++++
 fs/cifs/cifssmb.c  | 20 ++++++++++++++++++--
 fs/cifs/connect.c  | 14 ++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 7ea8a33..3c7c0c6 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -75,6 +75,8 @@ 
 #define SMB_ECHO_INTERVAL_MAX 600
 #define SMB_ECHO_INTERVAL_DEFAULT 60
 
+#define SMB_NEGATIVE_CACHE_INTERVAL 10
+
 /*
  * Default number of credits to keep available for SMB3.
  * This value is chosen somewhat arbitrarily. The Windows client
@@ -832,6 +834,8 @@  struct cifs_ses {
 	bool sign;		/* is signing required? */
 	bool need_reconnect:1; /* connection reset, uid now invalid */
 	bool domainAuto:1;
+	bool cached_rc;
+	struct delayed_work clear_cached_rc;
 #ifdef CONFIG_CIFS_SMB2
 	__u16 session_flags;
 	__u8 smb3signingkey[SMB3_SIGN_KEY_SIZE];
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index b472618..2196d16 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -179,8 +179,24 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	 */
 	mutex_lock(&ses->session_mutex);
 	rc = cifs_negotiate_protocol(0, ses);
-	if (rc == 0 && ses->need_reconnect)
-		rc = cifs_setup_session(0, ses, nls_codepage);
+	if (rc) {
+		mutex_unlock(&ses->session_mutex);
+		goto out;
+	}
+
+	if (ses->need_reconnect) {
+		if (ses->cached_rc) {
+			rc = ses->cached_rc;
+		} else {
+			rc = cifs_setup_session(0, ses, nls_codepage);
+			if (rc == -EACCES) {
+				queue_delayed_work(cifsiod_wq,
+					&ses->clear_cached_rc,
+					SMB_NEGATIVE_CACHE_INTERVAL * HZ);
+				ses->cached_rc = rc;
+			}
+		}
+	}
 
 	/* do we need to reconnect tcon? */
 	if (rc || !tcon->need_reconnect) {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 35ae49e..f82b280 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2375,6 +2375,7 @@  cifs_put_smb_ses(struct cifs_ses *ses)
 	list_del_init(&ses->smb_ses_list);
 	spin_unlock(&cifs_tcp_ses_lock);
 
+	cancel_delayed_work_sync(&ses->clear_cached_rc);
 	sesInfoFree(ses);
 	cifs_put_tcp_session(server, 0);
 }
@@ -2510,6 +2511,16 @@  cifs_set_cifscreds(struct smb_vol *vol __attribute__((unused)),
 }
 #endif /* CONFIG_KEYS */
 
+static void clear_cached_rc(struct work_struct *work)
+{
+	struct cifs_ses *ses = container_of(work, struct cifs_ses,
+						clear_cached_rc.work);
+
+	mutex_lock(&ses->session_mutex);
+	ses->cached_rc = 0;
+	mutex_unlock(&ses->session_mutex);
+}
+
 static struct cifs_ses *
 cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 {
@@ -2592,6 +2603,9 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info)
 	ses->sectype = volume_info->sectype;
 	ses->sign = volume_info->sign;
 
+	ses->cached_rc = 0;
+	INIT_DELAYED_WORK(&ses->clear_cached_rc, clear_cached_rc);
+
 	mutex_lock(&ses->session_mutex);
 	rc = cifs_negotiate_protocol(xid, ses);
 	if (!rc)
-- 
2.9.3