Attachment 14834 Details for Bug 13770 – git-am fix for master

[patch] git-am fix for master

bug-13770-master.patch (text/plain), 12.68 KB, created by Jeremy Allison on 2019-02-07 02:16:36 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2019-02-07 02:16:36 UTC

Size: 12.68 KB

patch

obsolete

>From f50cf6a230bbbd21f4e09db5d873c09040b6183c Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Wed, 6 Feb 2019 17:49:16 -0800
>Subject: [PATCH 1/2] s3: VFS: vfs_fruit. Fix the NetAtalk deny mode
> compatibility code.
>
>This exhibited itself as a problem with OFD locks reported
>as:
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13770
>
>However, due to underlying bugs in the vfs_fruit
>code the file locks were not being properly applied.
>
>There are two problems in fruit_check_access().
>
>Problem #1:
>
>Inside fruit_check_access() we have:
>
>flags = fcntl(fsp->fh->fd, F_GETFL);
>..
>if (flags & (O_RDONLY|O_RDWR)) {
>
>We shouldn't be calling fcntl(fsp->fh->fd, ..) directly.
>fsp->fh->fd may be a made up number from an underlying
>VFS module that has no meaning to a system call.
>
>Secondly, in all POSIX systems - O_RDONLY is defined as
>*zero*. O_RDWR = 2.
>
>Which means flags & (O_RDONLY|O_RDWR) becomes (flags & 2),
>not what we actually thought.
>
>Problem #2:
>
>deny_mode is *not* a bitmask, it's a set of discrete values.
>
>Inside fruit_check_access() we have:
>
>if (deny_mode & DENY_READ) and also (deny_mode & DENY_WRITE)
>
>However, deny modes are defined as:
>
>/* deny modes */
>define DENY_DOS 0
>define DENY_ALL 1
>define DENY_WRITE 2
>define DENY_READ 3
>define DENY_NONE 4
>define DENY_FCB 7
>
>so if deny_mode = DENY_WRITE, or if deny_mode = DENY_READ
>then it's going to trigger both the if (deny_mode & DENY_READ)
>*and* the (deny_mode & DENY_WRITE) conditions.
>
>These problems allowed the original test test_netatalk_lock code to
>pass (which was added for BUG: https://bugzilla.samba.org/show_bug.cgi?id=13584
>to demonstrate the lock order violation).
>
>This patch refactors the fruit_check_access()
>code to be much simpler (IMHO) to understand.
>
>Firstly, pass in the SMB1/2 share mode, not old
>DOS deny modes.
>
>Secondly, read all the possible NetAtalk locks
>into local variables:
>
>netatalk_already_open_for_reading
>netatalk_already_open_with_deny_read
>netatalk_already_open_for_writing
>netatalk_already_open_with_deny_write
>
>Then do the share mode/access mode checks
>with the requested values against any stored
>netatalk modes/access modes.
>
>Finally add in NetATalk compatible locks
>that represent our share modes/access modes
>into the file, with an early return if we don't
>have FILE_READ_DATA (in which case we can't
>write locks anyway).
>
>The patch is easier to understand by looking
>at the completed patched fruit_check_access()
>function, rather than trying to look at the
>diff.
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/modules/vfs_fruit.c | 198 +++++++++++++++++-------------------
> 1 file changed, 92 insertions(+), 106 deletions(-)
>
>diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
>index 9f3fe24e5fc..e035ede5565 100644
>--- a/source3/modules/vfs_fruit.c
>+++ b/source3/modules/vfs_fruit.c
>@@ -2664,156 +2664,142 @@ static bool test_netatalk_lock(files_struct *fsp, off_t in_offset)
> static NTSTATUS fruit_check_access(vfs_handle_struct *handle,
> 				   files_struct *fsp,
> 				   uint32_t access_mask,
>-				   uint32_t deny_mode)
>+				   uint32_t share_mode)
> {
> 	NTSTATUS status = NT_STATUS_OK;
>-	bool open_for_reading, open_for_writing, deny_read, deny_write;
> 	off_t off;
>-	bool have_read = false;
>-	int flags;
>+	bool share_for_read = (share_mode & FILE_SHARE_READ);
>+	bool share_for_write = (share_mode & FILE_SHARE_WRITE);
>+	bool netatalk_already_open_for_reading = false;
>+	bool netatalk_already_open_for_writing = false;
>+	bool netatalk_already_open_with_deny_read = false;
>+	bool netatalk_already_open_with_deny_write = false;
> 
> 	/* FIXME: hardcoded data fork, add resource fork */
> 	enum apple_fork fork_type = APPLE_FORK_DATA;
> 
>-	DEBUG(10, ("fruit_check_access: %s, am: %s/%s, dm: %s/%s\n",
>+	DBG_DEBUG("fruit_check_access: %s, am: %s/%s, sm: 0x%x\n",
> 		  fsp_str_dbg(fsp),
> 		  access_mask & FILE_READ_DATA ? "READ" :"-",
> 		  access_mask & FILE_WRITE_DATA ? "WRITE" : "-",
>-		  deny_mode & DENY_READ ? "DENY_READ" : "-",
>-		  deny_mode & DENY_WRITE ? "DENY_WRITE" : "-"));
>+		  share_mode);
> 
> 	if (fsp->fh->fd == -1) {
> 		return NT_STATUS_OK;
> 	}
> 
>-	flags = fcntl(fsp->fh->fd, F_GETFL);
>-	if (flags == -1) {
>-		DBG_ERR("fcntl get flags [%s] fd [%d] failed [%s]\n",
>-			fsp_str_dbg(fsp), fsp->fh->fd, strerror(errno));
>-		return map_nt_error_from_unix(errno);
>-	}
>-
>-	if (flags & (O_RDONLY|O_RDWR)) {
>-		/*
>-		 * Applying fcntl read locks requires an fd opened for
>-		 * reading. This means we won't be applying locks for
>-		 * files openend write-only, but what can we do...
>-		 */
>-		have_read = true;
>-	}
>+	/* Read NetATalk opens and deny modes on the file. */
>+	netatalk_already_open_for_reading = test_netatalk_lock(fsp,
>+				access_to_netatalk_brl(fork_type,
>+					FILE_READ_DATA));
> 
>-	/*
>-	 * Check read access and deny read mode
>-	 */
>-	if ((access_mask & FILE_READ_DATA) || (deny_mode & DENY_READ)) {
>-		/* Check access */
>-		open_for_reading = test_netatalk_lock(
>-			fsp, access_to_netatalk_brl(fork_type, FILE_READ_DATA));
>+	netatalk_already_open_with_deny_read = test_netatalk_lock(fsp,
>+				denymode_to_netatalk_brl(fork_type,
>+					DENY_READ));
> 
>-		deny_read = test_netatalk_lock(
>-			fsp, denymode_to_netatalk_brl(fork_type, DENY_READ));
>+	netatalk_already_open_for_writing = test_netatalk_lock(fsp,
>+				access_to_netatalk_brl(fork_type,
>+					FILE_WRITE_DATA));
> 
>-		DEBUG(10, ("read: %s, deny_write: %s\n",
>-			  open_for_reading == true ? "yes" : "no",
>-			  deny_read == true ? "yes" : "no"));
>+	netatalk_already_open_with_deny_write = test_netatalk_lock(fsp,
>+				denymode_to_netatalk_brl(fork_type,
>+					DENY_WRITE));
> 
>-		if (((access_mask & FILE_READ_DATA) && deny_read)
>-		    || ((deny_mode & DENY_READ) && open_for_reading)) {
>-			return NT_STATUS_SHARING_VIOLATION;
>-		}
>+	/* If there are any conflicts - sharing violation. */
>+	if ((access_mask & FILE_READ_DATA) && netatalk_already_open_with_deny_read) {
>+		return NT_STATUS_SHARING_VIOLATION;
>+	}
> 
>-		/* Set locks */
>-		if ((access_mask & FILE_READ_DATA) && have_read) {
>-			struct byte_range_lock *br_lck = NULL;
>+	if (!share_for_read && netatalk_already_open_for_reading) {
>+		return NT_STATUS_SHARING_VIOLATION;
>+	}
> 
>-			off = access_to_netatalk_brl(fork_type, FILE_READ_DATA);
>-			br_lck = do_lock(
>-				handle->conn->sconn->msg_ctx, fsp,
>-				fsp->op->global->open_persistent_id, 1, off,
>-				READ_LOCK, POSIX_LOCK, false,
>-				&status, NULL);
>+	if ((access_mask & FILE_WRITE_DATA) && netatalk_already_open_with_deny_write) {
>+		return NT_STATUS_SHARING_VIOLATION;
>+	}
> 
>-			TALLOC_FREE(br_lck);
>+	if (!share_for_write && netatalk_already_open_for_writing) {
>+		return NT_STATUS_SHARING_VIOLATION;
>+	}
> 
>-			if (!NT_STATUS_IS_OK(status))  {
>-				return status;
>-			}
>-		}
>+	if (!(access_mask & FILE_READ_DATA)) {
>+		/*
>+		 * Nothing we can do here, we need read access
>+		 * to set locks.
>+		 */
>+		return NT_STATUS_OK;
>+	}
> 
>-		if ((deny_mode & DENY_READ) && have_read) {
>-			struct byte_range_lock *br_lck = NULL;
>+	/* Set NetAtalk locks matching our access */
>+	if (access_mask & FILE_READ_DATA) {
>+		struct byte_range_lock *br_lck = NULL;
> 
>-			off = denymode_to_netatalk_brl(fork_type, DENY_READ);
>-			br_lck = do_lock(
>-				handle->conn->sconn->msg_ctx, fsp,
>-				fsp->op->global->open_persistent_id, 1, off,
>-				READ_LOCK, POSIX_LOCK, false,
>-				&status, NULL);
>+		off = access_to_netatalk_brl(fork_type, FILE_READ_DATA);
>+		br_lck = do_lock(
>+			handle->conn->sconn->msg_ctx, fsp,
>+			fsp->op->global->open_persistent_id, 1, off,
>+			READ_LOCK, POSIX_LOCK, false,
>+			&status, NULL);
> 
>-			TALLOC_FREE(br_lck);
>+		TALLOC_FREE(br_lck);
> 
>-			if (!NT_STATUS_IS_OK(status)) {
>-				return status;
>-			}
>+		if (!NT_STATUS_IS_OK(status))  {
>+			return status;
> 		}
> 	}
> 
>-	/*
>-	 * Check write access and deny write mode
>-	 */
>-	if ((access_mask & FILE_WRITE_DATA) || (deny_mode & DENY_WRITE)) {
>-		/* Check access */
>-		open_for_writing = test_netatalk_lock(
>-			fsp, access_to_netatalk_brl(fork_type, FILE_WRITE_DATA));
>+	if (!share_for_read) {
>+		struct byte_range_lock *br_lck = NULL;
> 
>-		deny_write = test_netatalk_lock(
>-			fsp, denymode_to_netatalk_brl(fork_type, DENY_WRITE));
>+		off = denymode_to_netatalk_brl(fork_type, DENY_READ);
>+		br_lck = do_lock(
>+			handle->conn->sconn->msg_ctx, fsp,
>+			fsp->op->global->open_persistent_id, 1, off,
>+			READ_LOCK, POSIX_LOCK, false,
>+			&status, NULL);
> 
>-		DEBUG(10, ("write: %s, deny_write: %s\n",
>-			  open_for_writing == true ? "yes" : "no",
>-			  deny_write == true ? "yes" : "no"));
>+		TALLOC_FREE(br_lck);
> 
>-		if (((access_mask & FILE_WRITE_DATA) && deny_write)
>-		    || ((deny_mode & DENY_WRITE) && open_for_writing)) {
>-			return NT_STATUS_SHARING_VIOLATION;
>+		if (!NT_STATUS_IS_OK(status)) {
>+			return status;
> 		}
>+	}
> 
>-		/* Set locks */
>-		if ((access_mask & FILE_WRITE_DATA) && have_read) {
>-			struct byte_range_lock *br_lck = NULL;
>+	if (access_mask & FILE_WRITE_DATA) {
>+		struct byte_range_lock *br_lck = NULL;
> 
>-			off = access_to_netatalk_brl(fork_type, FILE_WRITE_DATA);
>-			br_lck = do_lock(
>-				handle->conn->sconn->msg_ctx, fsp,
>-				fsp->op->global->open_persistent_id, 1, off,
>-				READ_LOCK, POSIX_LOCK, false,
>-				&status, NULL);
>+		off = access_to_netatalk_brl(fork_type, FILE_WRITE_DATA);
>+		br_lck = do_lock(
>+			handle->conn->sconn->msg_ctx, fsp,
>+			fsp->op->global->open_persistent_id, 1, off,
>+			READ_LOCK, POSIX_LOCK, false,
>+			&status, NULL);
> 
>-			TALLOC_FREE(br_lck);
>+		TALLOC_FREE(br_lck);
> 
>-			if (!NT_STATUS_IS_OK(status)) {
>-				return status;
>-			}
>+		if (!NT_STATUS_IS_OK(status)) {
>+			return status;
> 		}
>-		if ((deny_mode & DENY_WRITE) && have_read) {
>-			struct byte_range_lock *br_lck = NULL;
>+	}
> 
>-			off = denymode_to_netatalk_brl(fork_type, DENY_WRITE);
>-			br_lck = do_lock(
>-				handle->conn->sconn->msg_ctx, fsp,
>-				fsp->op->global->open_persistent_id, 1, off,
>-				READ_LOCK, POSIX_LOCK, false,
>-				&status, NULL);
>+	if (!share_for_write) {
>+		struct byte_range_lock *br_lck = NULL;
> 
>-			TALLOC_FREE(br_lck);
>+		off = denymode_to_netatalk_brl(fork_type, DENY_WRITE);
>+		br_lck = do_lock(
>+			handle->conn->sconn->msg_ctx, fsp,
>+			fsp->op->global->open_persistent_id, 1, off,
>+			READ_LOCK, POSIX_LOCK, false,
>+			&status, NULL);
> 
>-			if (!NT_STATUS_IS_OK(status)) {
>-				return status;
>-			}
>+		TALLOC_FREE(br_lck);
>+
>+		if (!NT_STATUS_IS_OK(status)) {
>+			return status;
> 		}
> 	}
> 
>-	return status;
>+	return NT_STATUS_OK;
> }
> 
> static NTSTATUS check_aapl(vfs_handle_struct *handle,
>@@ -6121,7 +6107,7 @@ static NTSTATUS fruit_create_file(vfs_handle_struct *handle,
> 		status = fruit_check_access(
> 			handle, *result,
> 			access_mask,
>-			map_share_mode_to_deny_mode(share_access, 0));
>+			share_access);
> 		if (!NT_STATUS_IS_OK(status)) {
> 			goto fail;
> 		}
>-- 
>2.19.1
>
>
>From 0251ab28a9a9eb580c06f320725a77e5d7307105 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Wed, 6 Feb 2019 18:01:52 -0800
>Subject: [PATCH 2/2] s4: torture: vfs_fruit. Change
> test_fruit_locking_conflict() to match the vfs_fruit working server code.
>
>Originally added for BUG: https://bugzilla.samba.org/show_bug.cgi?id=13584
>to demonstrate a lock order violation, this test
>exposed problems in the mapping of SMB1/2 share modes
>and open modes to NetATalk modes once we moved to OFD locks.
>
>Change the test slightly (and add comments)
>so it demonstrates working NetATalk share modes
>on an open file.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13770
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source4/torture/vfs/fruit.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c
>index 1296ba82e93..73b92c9391f 100644
>--- a/source4/torture/vfs/fruit.c
>+++ b/source4/torture/vfs/fruit.c
>@@ -6399,6 +6399,7 @@ static bool test_fruit_locking_conflict(struct torture_context *tctx,
> 	CHECK_STATUS(status, NT_STATUS_OK);
> 	h = create.out.file.handle;
> 
>+	/* Add AD_FILELOCK_RSRC_DENY_WR lock. */
> 	el = (struct smb2_lock_element) {
> 		.offset = 0xfffffffffffffffc,
> 		.length = 1,
>@@ -6410,12 +6411,21 @@ static bool test_fruit_locking_conflict(struct torture_context *tctx,
> 		.in.locks = &el,
> 	};
> 
>+	/*
>+	 * Lock up to and including:
>+	 * AD_FILELOCK_OPEN_WR
>+	 * AD_FILELOCK_OPEN_RD
>+	 * This is designed to cause a NetAtalk
>+	 * locking conflict on the next open,
>+	 * even though the share modes are
>+	 * compatible.
>+	 */
> 	status = smb2_lock(tree, &lck);
> 	CHECK_STATUS(status, NT_STATUS_OK);
> 
> 	el = (struct smb2_lock_element) {
> 		.offset = 0,
>-		.length = 0x7fffffffffffffff,
>+		.length = 0x7ffffffffffffff7,
> 		.flags = SMB2_LOCK_FLAG_EXCLUSIVE,
> 	};
> 	status = smb2_lock(tree, &lck);
>@@ -6432,7 +6442,7 @@ static bool test_fruit_locking_conflict(struct torture_context *tctx,
> 	};
> 
> 	status = smb2_create(tree, mem_ctx, &create);
>-	CHECK_STATUS(status, NT_STATUS_FILE_LOCK_CONFLICT);
>+	CHECK_STATUS(status, NT_STATUS_SHARING_VIOLATION);
> 
> 	{
> 		struct smb2_close cl = {
>-- 
>2.19.1
>

Flags:

slow: review+

Actions: View

Attachments on bug 13770: 14811 | 14818 | 14833 | 14834 | 14837 | 14839