[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