[PATCH] Add remote arch caching

Ralph Boehme slow at samba.org
Tue Apr 26 08:18:07 UTC 2016


Hi!

On Mon, Apr 25, 2016 at 09:38:49PM +0200, Volker Lendecke wrote:
> >  const char *get_remote_arch_str(void)
> >  {
> > -	if (!remote_arch_str) {
> > -		return "UNKNOWN";
> > -	}
> > -	return remote_arch_str;
> > +	return remote_arch_strings[ra_type];
> >  }
> 
> Question -- do we want range checking here? I don't think compilers
> enforce the enum ranges.

the range checking is already done at set time, so I had skipped it
here. It doesn't hurt and adds a safety net, so I added it.

> 
> > +enum remote_arch_types get_remote_arch_from_str(const char *remote_arch_string)
> > +{
> > +	for (int i = 0; i < sizeof(remote_arch_strings); i++) {
> 
> Shouldn't that be ARRAY_SIZE(remote_arch_strings)?

yes. Emberrassing, hiding away in shame.

> > +	guid_string = GUID_string(mem_ctx, client_guid);
> > +	if (guid_string == NULL) {
> > +		return NULL;
> > +	}
> 
> Probably it does not matter significantly performance-wise, but we
> have GUID_buf_string(). Can we use that here?

yes, good idea.

> > +	ra = get_remote_arch_from_str(ra_str);
> > +	if (ra != RA_UNKNOWN) {
> > +		DBG_INFO("Settting remote arch [%s] from cache\n", ra_str);
> > +		set_remote_arch(ra);
> > +	}
> > +
> > +	return true;
> > +}
> 
> Doesn't this leak "ra_str"? gencache_parse() would avoid that....

Using gencache_parse now as well as fstrings which got this comletely
rid of talloc.

Thanks!
-Ralph
-------------- next part --------------
From e2960c57211081b308766f11dc16a54e91e69707 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 13 Apr 2016 17:39:26 +0200
Subject: [PATCH 1/4] s3/lib: rework get_remote_arch_str() to use an array

By using C99 designated array initializers we can simplify the code and
remove the dependency on initializers appearing in a particular order.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/include/smb.h |  2 +-
 source3/lib/util.c    | 80 ++++++++++++++++++++++-----------------------------
 2 files changed, 35 insertions(+), 47 deletions(-)

diff --git a/source3/include/smb.h b/source3/include/smb.h
index 7eeef88..dec3189 100644
--- a/source3/include/smb.h
+++ b/source3/include/smb.h
@@ -529,7 +529,7 @@ http://msdn.microsoft.com/en-us/library/cc246334(PROT.13).aspx
 #define NO_SUBSTREAMS		0x2
 #define NO_REPARSETAG		0x4
 
-/* Remote architectures we know about. */
+/* Remote architectures we know about, keep in sync with remote_arch_strings */
 enum remote_arch_types {RA_UNKNOWN, RA_WFWG, RA_OS2, RA_WIN95, RA_WINNT,
 			RA_WIN2K, RA_WINXP, RA_WIN2K3, RA_VISTA,
 			RA_SAMBA, RA_CIFSFS, RA_WINXP64, RA_OSX};
diff --git a/source3/lib/util.c b/source3/lib/util.c
index 2895c14..a02e0cf 100644
--- a/source3/lib/util.c
+++ b/source3/lib/util.c
@@ -1230,14 +1230,34 @@ void ra_lanman_string( const char *native_lanman )
 		set_remote_arch( RA_WIN2K3 );
 }
 
-static const char *remote_arch_str;
+static const char *remote_arch_strings[] = {
+	[RA_UNKNOWN] =	"UNKNOWN",
+	[RA_WFWG] =	"WfWg",
+	[RA_OS2] =	"OS2",
+	[RA_WIN95] =	"Win95",
+	[RA_WINNT] =	"WinNT",
+	[RA_WIN2K] =	"Win2K",
+	[RA_WINXP] =	"WinXP",
+	[RA_WIN2K3] =	"Win2K3",
+	[RA_VISTA] =	"Vista",
+	[RA_SAMBA] =	"Samba",
+	[RA_CIFSFS] =	"CIFSFS",
+	[RA_WINXP64] =	"WinXP64",
+	[RA_OSX] =	"OSX",
+};
 
 const char *get_remote_arch_str(void)
 {
-	if (!remote_arch_str) {
-		return "UNKNOWN";
+	if (ra_type >= ARRAY_SIZE(remote_arch_strings)) {
+		/*
+		 * set_remote_arch() already checks this so ra_type
+		 * should be in the allowed range, but anyway, let's
+		 * do another bound check here.
+		 */
+		DBG_ERR("Remote arch info out of sync [%d] missing\n", ra_type);
+		ra_type = RA_UNKNOWN;
 	}
-	return remote_arch_str;
+	return remote_arch_strings[ra_type];
 }
 
 /*******************************************************************
@@ -1246,52 +1266,20 @@ const char *get_remote_arch_str(void)
 
 void set_remote_arch(enum remote_arch_types type)
 {
-	ra_type = type;
-	switch( type ) {
-	case RA_WFWG:
-		remote_arch_str = "WfWg";
-		break;
-	case RA_OS2:
-		remote_arch_str = "OS2";
-		break;
-	case RA_WIN95:
-		remote_arch_str = "Win95";
-		break;
-	case RA_WINNT:
-		remote_arch_str = "WinNT";
-		break;
-	case RA_WIN2K:
-		remote_arch_str = "Win2K";
-		break;
-	case RA_WINXP:
-		remote_arch_str = "WinXP";
-		break;
-	case RA_WINXP64:
-		remote_arch_str = "WinXP64";
-		break;
-	case RA_WIN2K3:
-		remote_arch_str = "Win2K3";
-		break;
-	case RA_VISTA:
-		remote_arch_str = "Vista";
-		break;
-	case RA_SAMBA:
-		remote_arch_str = "Samba";
-		break;
-	case RA_CIFSFS:
-		remote_arch_str = "CIFSFS";
-		break;
-	case RA_OSX:
-		remote_arch_str = "OSX";
-		break;
-	default:
+	if (ra_type >= ARRAY_SIZE(remote_arch_strings)) {
+		/*
+		 * This protects against someone adding values to enum
+		 * remote_arch_types without updating
+		 * remote_arch_strings array.
+		 */
+		DBG_ERR("Remote arch info out of sync [%d] missing\n", ra_type);
 		ra_type = RA_UNKNOWN;
-		remote_arch_str = "UNKNOWN";
-		break;
+		return;
 	}
 
+	ra_type = type;
 	DEBUG(10,("set_remote_arch: Client arch is \'%s\'\n",
-				remote_arch_str));
+		  get_remote_arch_str()));
 }
 
 /*******************************************************************
-- 
2.5.0


From fbf33b8dcf8863aee3c10cde84dd38c79bbe3f6d Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 13 Apr 2016 17:55:11 +0200
Subject: [PATCH 2/4] s3/lib: add get_remote_arch_from_str()

This will be used when fetching remote arch from gencache.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/include/proto.h |  1 +
 source3/lib/util.c      | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/source3/include/proto.h b/source3/include/proto.h
index afbbff0..8baf9d9 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -389,6 +389,7 @@ bool fcntl_getlock(int fd, off_t *poffset, off_t *pcount, int *ptype, pid_t *ppi
 bool is_myname(const char *s);
 void ra_lanman_string( const char *native_lanman );
 const char *get_remote_arch_str(void);
+enum remote_arch_types get_remote_arch_from_str(const char *remote_arch_string);
 void set_remote_arch(enum remote_arch_types type);
 enum remote_arch_types get_remote_arch(void);
 const char *tab_depth(int level, int depth);
diff --git a/source3/lib/util.c b/source3/lib/util.c
index a02e0cf..290224f 100644
--- a/source3/lib/util.c
+++ b/source3/lib/util.c
@@ -1260,6 +1260,16 @@ const char *get_remote_arch_str(void)
 	return remote_arch_strings[ra_type];
 }
 
+enum remote_arch_types get_remote_arch_from_str(const char *remote_arch_string)
+{
+	for (int i = 0; i < ARRAY_SIZE(remote_arch_strings); i++) {
+		if (strcmp(remote_arch_string, remote_arch_strings[i]) == 0) {
+			return i;
+		}
+	}
+	return RA_UNKNOWN;
+}
+
 /*******************************************************************
  Set the horrid remote_arch string based on an enum.
 ********************************************************************/
-- 
2.5.0


From b26289a15bb6c648e4f1a004feb8a12efaf04cd5 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 13 Apr 2016 17:42:55 +0200
Subject: [PATCH 3/4] s3/lib: add remote arch caching

This allows caching the remote arch string in gencache. A subsequent
commit will use this in SMB2 negprot.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/include/proto.h |   2 +
 source3/lib/util.c      | 134 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

diff --git a/source3/include/proto.h b/source3/include/proto.h
index 8baf9d9..c032b9e 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -392,6 +392,8 @@ const char *get_remote_arch_str(void);
 enum remote_arch_types get_remote_arch_from_str(const char *remote_arch_string);
 void set_remote_arch(enum remote_arch_types type);
 enum remote_arch_types get_remote_arch(void);
+bool remote_arch_cache_update(const struct GUID *client_guid);
+bool remote_arch_cache_delete(const struct GUID *client_guid);
 const char *tab_depth(int level, int depth);
 int str_checksum(const char *s);
 void zero_free(void *p, size_t size);
diff --git a/source3/lib/util.c b/source3/lib/util.c
index 290224f..1b7fc37 100644
--- a/source3/lib/util.c
+++ b/source3/lib/util.c
@@ -1301,6 +1301,140 @@ enum remote_arch_types get_remote_arch(void)
 	return ra_type;
 }
 
+#define RA_CACHE_TTL 7*24*3600
+
+static bool remote_arch_cache_key(const struct GUID *client_guid,
+				  fstring key)
+{
+	struct GUID_txt_buf guid_buf;
+	char *guid_string = NULL;
+
+	guid_string = GUID_buf_string(client_guid, &guid_buf);
+	if (guid_string == NULL) {
+		return false;
+	}
+
+	fstr_sprintf(key, "RA/%s", guid_string);
+	return true;
+}
+
+struct ra_parser_state {
+	bool ok;
+	bool expired;
+	enum remote_arch_types ra;
+};
+
+static void ra_parser(time_t timeout, DATA_BLOB blob, void *priv_data)
+{
+	struct ra_parser_state *state = (struct ra_parser_state *)priv_data;
+
+	if ((blob.data[0] == '\0') || (blob.data[blob.length-1] != '\0')) {
+		DBG_ERR("Remote arch cache key not a string\n");
+		return;
+	}
+
+	if (time(NULL) > timeout) {
+		state->expired = true;
+		state->ok = true;
+		return;
+	}
+
+	DBG_INFO("Got remote arch [%s] from cache\n", (char *)blob.data);
+
+	state->ra = get_remote_arch_from_str((char *)blob.data);
+	state->ok = true;
+	return;
+}
+
+static bool remote_arch_cache_get(const struct GUID *client_guid)
+{
+	bool ok;
+	fstring ra_key;
+	struct ra_parser_state state;
+
+	state = (struct ra_parser_state) {
+		.ok = false,
+		.expired = false,
+		.ra = RA_UNKNOWN,
+	};
+
+	ok = remote_arch_cache_key(client_guid, ra_key);
+	if (!ok) {
+		return false;
+	}
+
+	ok = gencache_parse(ra_key, ra_parser, &state);
+	if (!ok || !state.ok) {
+		return false;
+	}
+
+	if (state.expired) {
+		return gencache_del(ra_key);
+	}
+
+	if (state.ra == RA_UNKNOWN) {
+		return true;
+	}
+
+	set_remote_arch(state.ra);
+	return true;
+}
+
+static bool remote_arch_cache_set(const struct GUID *client_guid)
+{
+	bool ok;
+	fstring ra_key;
+	const char *ra_str = NULL;
+
+	if (get_remote_arch() == RA_UNKNOWN) {
+		return true;
+	}
+
+	ok = remote_arch_cache_key(client_guid, ra_key);
+	if (!ok) {
+		return false;
+	}
+
+	ra_str = get_remote_arch_str();
+	if (ra_str == NULL) {
+		return false;
+	}
+
+	ok = gencache_set(ra_key, ra_str, time(NULL) + RA_CACHE_TTL);
+	if (!ok) {
+		return false;
+	}
+
+	return true;
+}
+
+bool remote_arch_cache_update(const struct GUID *client_guid)
+{
+	if (get_remote_arch() == RA_UNKNOWN) {
+		return remote_arch_cache_get(client_guid);
+	}
+
+	return remote_arch_cache_set(client_guid);
+}
+
+bool remote_arch_cache_delete(const struct GUID *client_guid)
+{
+	bool ok;
+	fstring ra_key;
+
+	ok = remote_arch_cache_key(client_guid, ra_key);
+	if (!ok) {
+		return false;
+	}
+
+	ok = gencache_del(ra_key);
+	if (!ok) {
+		return false;
+	}
+
+	return true;
+}
+
 const char *tab_depth(int level, int depth)
 {
 	if( CHECK_DEBUGLVL(level) ) {
-- 
2.5.0


From 3ebe1186300be56df73fbd746fd91efbceff30ab Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 13 Apr 2016 17:44:26 +0200
Subject: [PATCH 4/4] smbd: use remote arch caching

We're using the client guid as gencache db key, so this can only be used
with SMB 2_10 or higher.

The idea is that whenever we get a direct SMB2 negprot, we can then try
to see if a value is cached for the client's guid.

When a user logs off the cache entry is deleted.

Signed-off-by: Ralph Boehme <slow at samba.org>
---
 source3/smbd/smb2_negprot.c   | 13 ++++++++++++-
 source3/smbd/smb2_sesssetup.c | 11 +++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/source3/smbd/smb2_negprot.c b/source3/smbd/smb2_negprot.c
index 9c03b2c..6cfa64f 100644
--- a/source3/smbd/smb2_negprot.c
+++ b/source3/smbd/smb2_negprot.c
@@ -162,6 +162,7 @@ NTSTATUS smbd_smb2_request_process_negprot(struct smbd_smb2_request *req)
 	uint32_t max_write = lp_smb2_max_write();
 	NTTIME now = timeval_to_nttime(&req->request_time);
 	bool signing_required = true;
+	bool ok;
 
 	status = smbd_smb2_request_verify_sizes(req, 0x24);
 	if (!NT_STATUS_IS_OK(status)) {
@@ -260,6 +261,17 @@ NTSTATUS smbd_smb2_request_process_negprot(struct smbd_smb2_request *req)
 		}
 	}
 
+	if ((dialect != SMB2_DIALECT_REVISION_2FF) &&
+	    (protocol >= PROTOCOL_SMB2_10) &&
+	    !GUID_all_zero(&in_guid))
+	{
+		ok = remote_arch_cache_update(&in_guid);
+		if (!ok) {
+			return smbd_smb2_request_error(
+				req, NT_STATUS_UNSUCCESSFUL);
+		}
+	}
+
 	switch (get_remote_arch()) {
 	case RA_VISTA:
 	case RA_SAMBA:
@@ -532,7 +544,6 @@ NTSTATUS smbd_smb2_request_process_negprot(struct smbd_smb2_request *req)
 		static const uint8_t zeros[8];
 		size_t pad = 0;
 		size_t ofs;
-		bool ok;
 
 		outdyn = data_blob_dup_talloc(req, security_buffer);
 		if (outdyn.length != security_buffer.length) {
diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c
index 78bda7b..873caed 100644
--- a/source3/smbd/smb2_sesssetup.c
+++ b/source3/smbd/smb2_sesssetup.c
@@ -1305,6 +1305,9 @@ static void smbd_smb2_logoff_shutdown_done(struct tevent_req *subreq)
 	struct smbd_smb2_logoff_state *state = tevent_req_data(
 		req, struct smbd_smb2_logoff_state);
 	NTSTATUS status;
+	bool ok;
+	const struct GUID *client_guid =
+		&state->smb2req->session->client->connections->smb2.client.guid;
 
 	status = smb2srv_session_shutdown_recv(subreq);
 	if (tevent_req_nterror(req, status)) {
@@ -1312,6 +1315,14 @@ static void smbd_smb2_logoff_shutdown_done(struct tevent_req *subreq)
 	}
 	TALLOC_FREE(subreq);
 
+	if (!GUID_all_zero(client_guid)) {
+		ok = remote_arch_cache_delete(client_guid);
+		if (!ok) {
+			/* Most likely not an error, but not in cache */
+			DBG_DEBUG("Deletion from remote arch cache failed\n");
+		}
+	}
+
 	/*
 	 * As we've been awoken, we may have changed
 	 * uid in the meantime. Ensure we're still
-- 
2.5.0



More information about the samba-technical mailing list