[PATCHES] report zero on-disk file ID to Macs

Uri Simchoni uri at samba.org
Sat Mar 25 17:15:03 UTC 2017


On 03/24/2017 03:02 PM, Ralph Böhme wrote:
> On Fri, Mar 24, 2017 at 11:00:42AM +0100, Christian Ambach via samba-technical wrote:
>> Hi Uri,
>>
>>> +static bool forced_zero_file_id;
>>> +
>>
>> I thought one of the long-term goals was to eliminate all global
>> variables. So do we really need to add another one? Can't this be stored
>> in struct smbd_server_connection?
> 
> yes, that was my initial thought as well. The rest looks good, but the globals
> must die, die, die. :)
> 
> Thanks a lot for taking a stab on this one Uri, I should have added this fix
> ages ago!
> 
> Cheerio!
> -slow
> 
Updated patch attached. I moved global var into smbd_server_connection
as Christian suggested. The test and docs are the same.

Thanks,
Uri.
-------------- next part --------------
From b436fbab38d694961150f785935afaf5113f5e50 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 23 Mar 2017 14:08:26 +0200
Subject: [PATCH v2 1/5] smbd: add zero_file_id flag

This flag instructs the SMB layer to report a zero on-disk
file identifier.

According to [MS-SMB2] 3.3.5.9.9, the reported on-disk file ID
SHOULD be unique. However, macOS clients seem to expect it to be
unique over time as well, like the HFS+ CNID. Reporting a file ID
of 0 seems to instruct the Mac client not to trust the server-reported
file ID.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12715

Signed-off-by: Uri Simchoni <uri at samba.org>
---
 source3/smbd/globals.h |  1 +
 source3/smbd/proto.h   |  1 +
 source3/smbd/trans2.c  | 14 ++++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index 76ce0bb..67d3a89 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -864,6 +864,7 @@ struct smbd_server_connection {
 	struct messaging_context *msg_ctx;
 	struct notify_context *notify_ctx;
 	bool using_smb2;
+	bool aapl_zero_file_id; /* Apple-specific */
 	int trans_num;
 
 	size_t num_users;
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 5671a96..2e8df29 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -1131,6 +1131,7 @@ NTSTATUS check_access(connection_struct *conn,
 				uint32_t access_mask);
 uint64_t smb_roundup(connection_struct *conn, uint64_t val);
 uint64_t get_FileIndex(connection_struct *conn, const SMB_STRUCT_STAT *psbuf);
+void aapl_force_zero_file_id(struct smbd_server_connection *sconn);
 bool samba_private_attr_name(const char *unix_ea_name);
 NTSTATUS get_ea_value(TALLOC_CTX *mem_ctx, connection_struct *conn,
 		      files_struct *fsp, const char *fname,
diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
index b6bf93f..923fed4 100644
--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -146,6 +146,9 @@ uint64_t smb_roundup(connection_struct *conn, uint64_t val)
 uint64_t get_FileIndex(connection_struct *conn, const SMB_STRUCT_STAT *psbuf)
 {
 	uint64_t file_index;
+	if (conn->sconn->aapl_zero_file_id) {
+		return 0;
+	}
 	if (conn->base_share_dev == psbuf->st_ex_dev) {
 		return (uint64_t)psbuf->st_ex_ino;
 	}
@@ -154,6 +157,17 @@ uint64_t get_FileIndex(connection_struct *conn, const SMB_STRUCT_STAT *psbuf)
 	return file_index;
 }
 
+
+/********************************************************************
+ Globally (for this connection / multi-channel) disable file-ID
+ calculation. This is required to be global because it serves
+ Macs in AAPL mode, which is globally set.
+********************************************************************/
+void aapl_force_zero_file_id(struct smbd_server_connection *sconn)
+{
+	sconn->aapl_zero_file_id = true;
+}
+
 /****************************************************************************
  Utility functions for dealing with extended attributes.
 ****************************************************************************/
-- 
2.9.3


From 49f1809c408b33e92b6ef07dd466c2e4fe08dafd Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 23 Mar 2017 14:08:45 +0200
Subject: [PATCH v2 2/5] vfs_fruit: enable zero file id

Enable zero_file_id if both conditions are met:
- AAPL negotiated
- fruit:zero_file_id is set

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12715

Signed-off-by: Uri Simchoni <uri at samba.org>
---
 source3/modules/vfs_fruit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
index fc80629..0b5558a 100644
--- a/source3/modules/vfs_fruit.c
+++ b/source3/modules/vfs_fruit.c
@@ -135,6 +135,7 @@ struct fruit_config_data {
 	bool copyfile_enabled;
 	bool veto_appledouble;
 	bool posix_rename;
+	bool aapl_zero_file_id;
 
 	/*
 	 * Additional options, all enabled by default,
@@ -1591,6 +1592,9 @@ static int init_fruit_config(vfs_handle_struct *handle)
 	config->posix_rename = lp_parm_bool(
 		SNUM(handle->conn), FRUIT_PARAM_TYPE_NAME, "posix_rename", true);
 
+	config->aapl_zero_file_id =
+	    lp_parm_bool(-1, FRUIT_PARAM_TYPE_NAME, "zero_file_id", true);
+
 	config->readdir_attr_rsize = lp_parm_bool(
 		SNUM(handle->conn), "readdir_attr", "aapl_rsize", true);
 
@@ -2236,6 +2240,9 @@ static NTSTATUS check_aapl(vfs_handle_struct *handle,
 				      blob);
 	if (NT_STATUS_IS_OK(status)) {
 		global_fruit_config.nego_aapl = true;
+		if (config->aapl_zero_file_id) {
+			aapl_force_zero_file_id(handle->conn->sconn);
+		}
 	}
 
 	return status;
-- 
2.9.3


From 612239090d9f8fdc92df3b4072ac976b24c361d6 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 23 Mar 2017 14:51:32 +0200
Subject: [PATCH v2 3/5] vfs_fruit: document added zero_file_id parameter

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12715

Signed-off-by: Uri Simchoni <uri at samba.org>
---
 docs-xml/manpages/vfs_fruit.8.xml | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/docs-xml/manpages/vfs_fruit.8.xml b/docs-xml/manpages/vfs_fruit.8.xml
index a00f6a9..0bddd4a 100644
--- a/docs-xml/manpages/vfs_fruit.8.xml
+++ b/docs-xml/manpages/vfs_fruit.8.xml
@@ -143,6 +143,23 @@
 	    </listitem>
 	  </varlistentry>
 
+	  <varlistentry>
+	    <term>fruit:zero_file_id = yes | no</term>
+	    <listitem>
+	      <para>A <emphasis>global</emphasis> option whether to return
+	      zero to queries of on-disk file identifier, if the client
+	      has negotiated AAPL.</para>
+	      <para>Mac applications and / or the Mac SMB
+	      client code expect the on-disk file identifier to have the
+	      semantics of HFS+ Catalog Node Identifier (CNID). Samba
+	      doesn't provide those semantics, and that occasionally cause
+	      usability issues or even data loss. Returning a file identifier
+	      of zero causes the Mac client to stop using and trusting the
+	      file id returned from the server.</para>
+	      <para>The default is <emphasis>yes</emphasis>.</para>
+	    </listitem>
+	  </varlistentry>
+
 	</variablelist>
 </refsect1>
 
-- 
2.9.3


From 9a0920ce6ab777304e306f093b95ab86ab3f2009 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 23 Mar 2017 21:30:50 +0200
Subject: [PATCH v2 4/5] torture: add torture_assert_mem_not_equal_goto()

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12715

Signed-off-by: Uri Simchoni <uri at samba.org>
---
 lib/torture/torture.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/torture/torture.h b/lib/torture/torture.h
index b6d1301..668458a 100644
--- a/lib/torture/torture.h
+++ b/lib/torture/torture.h
@@ -367,6 +367,16 @@ void torture_result(struct torture_context *test,
 	} \
 	} while(0)
 
+#define torture_assert_mem_not_equal_goto(torture_ctx,got,expected,len,ret,label,cmt) \
+	do { const void *__got = (got), *__expected = (expected); \
+	if (memcmp(__got, __expected, len) == 0) { \
+		torture_result(torture_ctx, TORTURE_FAIL, \
+			       __location__": "#got" of len %d unexpectedly matches "#expected": %s", (int)len, cmt); \
+		ret = false; \
+		goto label; \
+	} \
+	} while(0)
+
 static inline void torture_dump_data_str_cb(const char *buf, void *private_data)
 {
 	char **dump = (char **)private_data;
-- 
2.9.3


From 64c7fea80cd18908ecc0cf0f723821dfe4a10e98 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 23 Mar 2017 21:32:04 +0200
Subject: [PATCH v2 5/5] selftest: tests for vfs_fruite file-id behavior

The test is in its own suite because it validates
our hackish workaround rather than some reference
implementation behavior.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12715

Signed-off-by: Uri Simchoni <uri at samba.org>
---
 source3/selftest/tests.py   |  4 ++-
 source4/torture/vfs/fruit.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
 source4/torture/vfs/vfs.c   |  1 +
 3 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 54f5b7b..336ec92 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -340,7 +340,7 @@ nbt = ["nbt.dgram" ]
 
 libsmbclient = ["libsmbclient"]
 
-vfs = ["vfs.fruit", "vfs.acl_xattr", "vfs.fruit_netatalk"]
+vfs = ["vfs.fruit", "vfs.acl_xattr", "vfs.fruit_netatalk", "vfs.fruit_file_id"]
 
 tests= base + raw + smb2 + rpc + unix + local + rap + nbt + libsmbclient + idmap + vfs
 
@@ -428,6 +428,8 @@ for t in tests:
         plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit_stream_depot --option=torture:share2=vfs_wo_fruit_stream_depot -U$USERNAME%$PASSWORD', 'streams_depot')
     elif t == "vfs.fruit_netatalk":
         plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit -U$USERNAME%$PASSWORD --option=torture:localdir=$SELFTEST_PREFIX/nt4_dc/share')
+    elif t == "vfs.fruit_file_id":
+        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit -U$USERNAME%$PASSWORD')
     elif t == "rpc.schannel_anon_setpw":
         plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$%', description="anonymous password set")
         plansmbtorture4testsuite(t, "nt4_dc_schannel", '//$SERVER_IP/tmp -U$%', description="anonymous password set (schannel enforced server-side)")
diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c
index d74a153..857e738 100644
--- a/source4/torture/vfs/fruit.c
+++ b/source4/torture/vfs/fruit.c
@@ -3914,6 +3914,63 @@ done:
 	return ret;
 }
 
+static bool test_zero_file_id(struct torture_context *tctx,
+			      struct smb2_tree *tree)
+{
+	const char *fname = "filtest_file_id";
+	struct smb2_create create = {0};
+	NTSTATUS status;
+	bool ret = true;
+	uint8_t zero_file_id[8] = {0};
+
+	torture_comment(tctx, "Testing zero file id\n");
+
+	ret = torture_setup_file(tctx, tree, fname, false);
+	torture_assert_goto(tctx, ret == true, ret, done, "torture_setup_file");
+
+	ZERO_STRUCT(create);
+	create.in.desired_access = SEC_FILE_READ_ATTRIBUTE;
+	create.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
+	create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+	create.in.create_disposition = NTCREATEX_DISP_OPEN;
+	create.in.fname = fname;
+	create.in.query_on_disk_id = true;
+
+	status = smb2_create(tree, tctx, &create);
+	torture_assert_ntstatus_equal_goto(tctx, status, NT_STATUS_OK, ret,
+					   done,
+					   "test file could not be opened");
+	torture_assert_mem_not_equal_goto(tctx, create.out.on_disk_id,
+					  zero_file_id, 8, ret, done,
+					  "unexpected zero file id");
+
+	smb2_util_close(tree, create.out.file.handle);
+
+	ret = enable_aapl(tctx, tree);
+	torture_assert(tctx, ret == true, "enable_aapl failed");
+
+	ZERO_STRUCT(create);
+	create.in.desired_access = SEC_FILE_READ_ATTRIBUTE;
+	create.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
+	create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+	create.in.create_disposition = NTCREATEX_DISP_OPEN;
+	create.in.fname = fname;
+	create.in.query_on_disk_id = true;
+
+	status = smb2_create(tree, tctx, &create);
+	torture_assert_ntstatus_equal_goto(
+	    tctx, status, NT_STATUS_OK, ret, done,
+	    "test file could not be opened with AAPL");
+	torture_assert_mem_equal_goto(tctx, create.out.on_disk_id, zero_file_id,
+				      8, ret, done, "non-zero file id");
+
+	smb2_util_close(tree, create.out.file.handle);
+
+done:
+	smb2_util_unlink(tree, fname);
+	return ret;
+}
+
 /*
  * Note: This test depends on "vfs objects = catia fruit streams_xattr".  For
  * some tests torture must be run on the host it tests and takes an additional
@@ -3968,3 +4025,18 @@ struct torture_suite *torture_vfs_fruit_netatalk(void)
 
 	return suite;
 }
+
+struct torture_suite *torture_vfs_fruit_file_id(void)
+{
+	struct torture_suite *suite =
+	    torture_suite_create(talloc_autofree_context(), "fruit_file_id");
+
+	suite->description =
+	    talloc_strdup(suite, "vfs_fruit tests for on-disk file ID that "
+				 "require fruit:zero_file_id=yes");
+
+	torture_suite_add_1smb2_test(suite, "zero file id if AAPL negotiated",
+				     test_zero_file_id);
+
+	return suite;
+}
diff --git a/source4/torture/vfs/vfs.c b/source4/torture/vfs/vfs.c
index a4f8125..710e93b 100644
--- a/source4/torture/vfs/vfs.c
+++ b/source4/torture/vfs/vfs.c
@@ -111,6 +111,7 @@ NTSTATUS torture_vfs_init(void)
 	torture_suite_add_suite(suite, torture_vfs_fruit());
 	torture_suite_add_suite(suite, torture_vfs_fruit_netatalk());
 	torture_suite_add_suite(suite, torture_acl_xattr());
+	torture_suite_add_suite(suite, torture_vfs_fruit_file_id());
 
 	torture_register_suite(suite);
 
-- 
2.9.3



More information about the samba-technical mailing list