Bug 11138 - Samba does not break leases on rename of parent directory
Samba does not break leases on rename of parent directory
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.2.0rc4
All All
: P5 major
: 4.3
Assigned To: Ralph Böhme
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-06 07:24 UTC by Steve French
Modified: 2015-12-04 14:24 UTC (History)
3 users (show)

See Also:


Attachments
wireshark trace - samba does not issue lease break to open child files on rename of parent (18.01 KB, application/x-pcapng)
2015-03-06 07:25 UTC, Steve French
no flags Details
wireshark trace -Windows 8.1 does issue lease break to open child file on rename of parent (10.20 KB, application/x-pcapng)
2015-03-06 07:28 UTC, Steve French
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Steve French 2015-03-06 07:24:07 UTC
In debugging Microsoft functional test failures against Samba, I noticed that lease breaks are not issued on renames of the parent directory (leases are enabled, as are durable handles, kernel oplocks are disabled in my smb.conf for testing leasev2 and durable handles obviously).

I created a modified torture test (not very useful by itself unfortunately) which opens a file with a RWH lease and just sits for 15 seconds.  I then rename the parent directory from a cifs.ko mount.  In Samba 4.2 I don't see a lease break on the child file's lease, but in Windows 8.1 server I do.
Comment 1 Steve French 2015-03-06 07:25:01 UTC
Created attachment 10826 [details]
wireshark trace - samba does not issue lease break to open child files on rename of parent
Comment 2 Steve French 2015-03-06 07:28:29 UTC
Created attachment 10827 [details]
wireshark trace -Windows 8.1 does issue lease break to open child file on rename of parent

Client is smbtorture (smb2.lease lightly modified) See frame 40 (lease break issued by server due to rename of parent directory)
Comment 3 Steve French 2015-03-06 07:29:48 UTC
Comment on attachment 10826 [details]
wireshark trace - samba does not issue lease break to open child files on rename of parent

client in both of these is a modified lease.c (source4/smbtorture)
Comment 4 Steve French 2015-03-06 21:24:09 UTC
Various Microsoft tests generate it - but in case you prefer smbtorture, I do a rename of the directory smb2_dir (from a cifs mount) while the smbtorture test below is blocked (I added a sleep of 12 seconds in the middle of the below) in order to demonstrate the problem

diff --git a/source4/torture/smb2/lease.c b/source4/torture/smb2/lease.c
index 4b435a1..96df6fe 100644
--- a/source4/torture/smb2/lease.c
+++ b/source4/torture/smb2/lease.c
@@ -3882,6 +3882,206 @@ static bool test_lease_dynamic_share(struct torture_context *tctx,
 	return ret;
 }
 
+#define DNAME	"smb2_dir"
+#define FNAME   "smb2_dir\\smb2_lease_file_for_rename_test"
+#define NFILES	10
+
+struct file_elem {
+	char *name;
+	NTTIME create_time;
+	bool found;
+};
+
+static bool dir_rename_of_parent(struct torture_context *tctx,
+			      struct smb2_tree *tree)
+{
+	struct smb2_create create;
+	char **strs = NULL;
+	NTSTATUS status;
+	bool ret;
+	int i;
+	int nfiles = NFILES;
+	TALLOC_CTX *mem_ctx = talloc_new(tctx);
+	struct smb2_handle *h_out;
+	struct file_elem files[NFILES] = {};
+
+	smb2_deltree(tree, DNAME);
+
+	ZERO_STRUCT(create);
+	create.in.desired_access = SEC_RIGHTS_DIR_ALL;
+	create.in.create_options = NTCREATEX_OPTIONS_DIRECTORY;
+	create.in.file_attributes = FILE_ATTRIBUTE_DIRECTORY;
+	create.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
+				 NTCREATEX_SHARE_ACCESS_WRITE |
+				 NTCREATEX_SHARE_ACCESS_DELETE;
+	create.in.create_disposition = NTCREATEX_DISP_CREATE;
+	create.in.fname = DNAME;
+
+	status = smb2_create(tree, mem_ctx, &create);
+	torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "");
+	*h_out = create.out.file.handle;
+
+	ZERO_STRUCT(create);
+	create.in.desired_access = SEC_RIGHTS_FILE_ALL;
+	create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
+	create.in.create_disposition = NTCREATEX_DISP_CREATE;
+
+/* create file, open file with lease, rename parent directory */
+
+	strs = generate_unique_strs(mem_ctx, 8, nfiles);
+	if (strs == NULL) {
+		status = NT_STATUS_OBJECT_NAME_COLLISION;
+		goto done;
+	}
+	for (i = 0; i < nfiles; i++) {
+		files[i].name = strs[i];
+		create.in.fname = talloc_asprintf(mem_ctx, "%s\\%s",
+		    DNAME, files[i].name);
+		status = smb2_create(tree, mem_ctx, &create);
+		torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "");
+		files[i].create_time = create.out.create_time;
+		smb2_util_close(tree, create.out.file.handle);
+	}
+ done:
+	return true;
+}
+
+static bool create_dir(struct torture_context *tctx, struct smb2_tree *tree)
+{
+	NTSTATUS status;
+	struct smb2_create io;
+	struct smb2_handle handle;
+	union smb_fileinfo q;
+	union smb_setfileinfo set;
+	struct security_descriptor *sd, *sd_orig;
+	const char *owner_sid;
+	uint32_t perms = 0;
+	bool ret;
+
+	torture_comment(tctx, "Creating Directory for testing: %s\n", DNAME);
+
+	ZERO_STRUCT(io);
+	io.level = RAW_OPEN_SMB2;
+	io.in.create_flags = 0;
+	io.in.desired_access =
+		SEC_STD_READ_CONTROL |
+		SEC_STD_WRITE_DAC |
+		SEC_STD_WRITE_OWNER;
+	io.in.create_options = NTCREATEX_OPTIONS_DIRECTORY;
+	io.in.file_attributes = FILE_ATTRIBUTE_DIRECTORY;
+	io.in.share_access =
+		NTCREATEX_SHARE_ACCESS_READ |
+		NTCREATEX_SHARE_ACCESS_WRITE;
+	io.in.alloc_size = 0;
+	io.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
+	io.in.impersonation_level = NTCREATEX_IMPERSONATION_ANONYMOUS;
+	io.in.security_flags = 0;
+	io.in.fname = DNAME;
+	status = smb2_create(tree, tctx, &io);
+	CHECK_STATUS(status, NT_STATUS_OK);
+	handle = io.out.file.handle;
+
+	torture_comment(tctx, "get the original sd\n");
+	q.query_secdesc.level = RAW_FILEINFO_SEC_DESC;
+	q.query_secdesc.in.file.handle = handle;
+	q.query_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER;
+	status = smb2_getinfo_file(tree, tctx, &q);
+	CHECK_STATUS(status, NT_STATUS_OK);
+	sd_orig = q.query_secdesc.out.sd;
+
+	owner_sid = dom_sid_string(tctx, sd_orig->owner_sid);
+
+	/*
+	 * We create an SD that allows us to do most things but we do not
+	 * get DELETE and DELETE CHILD access!
+	 */
+
+	perms = SEC_STD_SYNCHRONIZE | SEC_STD_WRITE_OWNER |
+		SEC_STD_WRITE_DAC | SEC_STD_READ_CONTROL | 
+		SEC_DIR_WRITE_ATTRIBUTE | SEC_DIR_READ_ATTRIBUTE | 
+		SEC_DIR_TRAVERSE | SEC_DIR_WRITE_EA | 
+		SEC_FILE_READ_EA | SEC_FILE_APPEND_DATA |
+		SEC_FILE_WRITE_DATA | SEC_FILE_READ_DATA;
+
+	torture_comment(tctx, "Setting permissions on dir to 0x1e01bf\n");
+	sd = security_descriptor_dacl_create(tctx,
+					0, owner_sid, NULL,
+					owner_sid,
+					SEC_ACE_TYPE_ACCESS_ALLOWED,
+					perms,
+					SEC_ACE_FLAG_OBJECT_INHERIT,
+					NULL);
+
+	set.set_secdesc.level = RAW_SFILEINFO_SEC_DESC;
+	set.set_secdesc.in.file.handle = handle;
+	set.set_secdesc.in.secinfo_flags = SECINFO_DACL | SECINFO_OWNER;
+	set.set_secdesc.in.sd = sd;
+
+	status = smb2_setinfo_file(tree, &set);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	status = smb2_util_close(tree, handle);
+done:
+	return true;
+}
+
+static bool sample_test_doc_create(struct torture_context *tctx, struct smb2_tree *tree)
+{
+	struct smb2_create io;
+	NTSTATUS status;
+	uint32_t perms = 0;
+	struct smb2_lease ls;
+	bool ret;
+
+	smb2_deltree(tree, DNAME);
+
+	create_dir(tctx, tree);
+
+	torture_comment(tctx, "Create file with DeleteOnClose on non-existent file (CREATE) \n");
+	torture_comment(tctx, "We expect NT_STATUS_OK\n");
+
+	perms = SEC_STD_SYNCHRONIZE | SEC_STD_READ_CONTROL | SEC_STD_DELETE | 
+		SEC_DIR_WRITE_ATTRIBUTE | SEC_DIR_READ_ATTRIBUTE | 
+		SEC_DIR_WRITE_EA | SEC_FILE_APPEND_DATA |
+		SEC_FILE_WRITE_DATA;
+
+	ZERO_STRUCT(io);
+	smb2_lease_create(&io, &ls, false, FNAME, LEASE2, smb2_util_lease_state("RHW"));
+	io.in.desired_access	 = perms;
+	io.in.file_attributes	 = 0;
+	io.in.create_disposition = NTCREATEX_DISP_CREATE;
+	io.in.share_access	 = NTCREATEX_SHARE_ACCESS_DELETE;
+	io.in.create_options     = NTCREATEX_OPTIONS_DELETE_ON_CLOSE | 
+				   NTCREATEX_OPTIONS_NON_DIRECTORY_FILE;
+	io.in.fname              = FNAME;
+
+	status = smb2_create(tree, tctx, &io);
+	CHECK_STATUS(status, NT_STATUS_OK);
+
+	smb_msleep(12000); /* wait 12 seconds to give me time to rename parent from another client */
+
+	/* While this is blocked sleeping - do a "mv smb2_dir somedirname" from any cifs/smb2/smb3 client (I used Linux cifs.ko) */
+
+	status = smb2_util_close(tree, io.out.file.handle);
+
+	/* Check it was deleted */
+	ZERO_STRUCT(io);
+	io.in.desired_access	 = perms;
+	io.in.file_attributes	 = 0;
+	io.in.create_disposition = NTCREATEX_DISP_OPEN;
+	io.in.share_access	 = NTCREATEX_SHARE_ACCESS_DELETE;
+	io.in.create_options     = 0;
+	io.in.fname              = FNAME;
+
+	torture_comment(tctx, "Testing if the file was deleted when closed\n");
+	torture_comment(tctx, "We expect NT_STATUS_OBJECT_NAME_NOT_FOUND\n");
+
+	status = smb2_create(tree, tctx, &io);
+	CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND);
+done:
+	return true;
+}
+
 struct torture_suite *torture_smb2_lease_init(void)
 {
 	struct torture_suite *suite =
@@ -3919,6 +4119,7 @@ struct torture_suite *torture_smb2_lease_init(void)
 	torture_suite_add_1smb2_test(suite, "v2_rename", test_lease_v2_rename);
 	torture_suite_add_1smb2_test(suite, "dynamic_share", test_lease_dynamic_share);
 	torture_suite_add_1smb2_test(suite, "timeout", test_lease_timeout);
+	torture_suite_add_1smb2_test(suite, "rename_parent", sample_test_doc_create);
 
 	suite->description = talloc_strdup(suite, "SMB2-LEASE tests");
Comment 5 Steve French 2015-03-06 21:24:51 UTC
In the test above, I cut-and-pasted from a delete on close test, but delete on close doesn't have things to do with it.
Comment 6 Steve French 2015-03-06 23:33:48 UTC
Metze suggested setting

"strict rename = yes"

in smb.conf and retrying these tests
Comment 7 Steve French 2015-03-07 02:38:58 UTC
Adding "strict rename = yes" to the share definition did not alter the test case behavior.

The Samba behavior seems to violate MS-SMB2 (see e.g. 3.3.1.4)

HANDLE caching permits one or more SMB2 clients to delay closing handles it holds open, or to defer sending opens. Before processing one of the following operations, the underlying object store MUST request that the server revoke HANDLE caching, and the object store MUST wait for acknowledgment before proceeding with the operation:
HANDLE caching on a file:
 A file is opened with an access or share mode incompatible with opens from different ClientIds or local applications as described in [MS-FSA] section 2.1.5.1.2.
 The parent directory is being renamed.
Comment 8 Ralph Böhme 2015-11-20 07:49:58 UTC
This should be taken care of by f435f1b3acb75c065166e3077c01acbd88601f34:

Author:     Jeremy Allison <jra@samba.org>
AuthorDate: Thu Dec 4 21:19:32 2014 -0800
Commit:     Jeremy Allison <jra@samba.org>
CommitDate: Fri Dec 5 18:37:10 2014 +0100

    s3: leases: Make SMB2 setinfo SMB2_FILE_RENAME_INFORMATION_INTERNAL async.
    
    If there are any RH leases we must break them to read
    and must wait for the client response before doing the rename.
    
    Pair-Programmed-With: Stefan Metzmacher <metze@samba.org>
    
    Signed-off-by: Jeremy Allison <jra@samba.org>
    Signed-off-by: Stefan Metzmacher <metze@samba.org>

Are you sure the Samba version you tested got that one?
Comment 9 Ralph Böhme 2015-12-04 14:24:36 UTC
This is definitely fixed in master.