From bb29835868ed118b7eaac9293acc6cc4c1de1692 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 6 Feb 2019 17:49:16 -0800 Subject: [PATCH 1/4] s3: VFS: vfs_fruit. Fix the NetAtalk deny mode compatibility code. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ralph Böhme (cherry picked from commit 3204dc66f6801a7c8c87c48f601e0ebdee9e3d40) --- source3/modules/vfs_fruit.c | 202 +++++++++++++++++------------------- 1 file changed, 96 insertions(+), 106 deletions(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 9f3fe24e5fc..c801f98eafb 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -2664,156 +2664,146 @@ 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 +6111,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.20.1.791.gb4d0f1c61a-goog From 603cb2d257d1309885719e42a920d671cc900eae Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 6 Feb 2019 18:01:52 -0800 Subject: [PATCH 2/4] s4: torture: vfs_fruit. Change test_fruit_locking_conflict() to match the vfs_fruit working server code. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Ralph Böhme Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Fri Feb 8 23:26:46 CET 2019 on sn-devel-144 (cherry picked from commit 28990e4ba23695ecf264117efad90cc4e573302e) --- source4/torture/vfs/fruit.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c index 3b01cf876d0..e460ce20336 100644 --- a/source4/torture/vfs/fruit.c +++ b/source4/torture/vfs/fruit.c @@ -6325,7 +6325,8 @@ done: } static bool test_fruit_locking_conflict(struct torture_context *tctx, - struct smb2_tree *tree) + struct smb2_tree *tree, + struct smb2_tree *tree2) { TALLOC_CTX *mem_ctx; struct smb2_create create; @@ -6363,6 +6364,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, @@ -6374,12 +6376,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); @@ -6395,8 +6406,13 @@ static bool test_fruit_locking_conflict(struct torture_context *tctx, .in.fname = fname, }; - status = smb2_create(tree, mem_ctx, &create); - CHECK_STATUS(status, NT_STATUS_FILE_LOCK_CONFLICT); + /* + * Open on the second tree - ensure we are + * emulating trying to access with a NetATalk + * process with an existing open/deny mode. + */ + status = smb2_create(tree2, mem_ctx, &create); + CHECK_STATUS(status, NT_STATUS_SHARING_VIOLATION); { struct smb2_close cl = { @@ -6420,7 +6436,7 @@ struct torture_suite *torture_vfs_fruit_netatalk(TALLOC_CTX *ctx) torture_suite_add_1smb2_test(suite, "read netatalk metadata", test_read_netatalk_metadata); torture_suite_add_1smb2_test(suite, "stream names with locally created xattr", test_stream_names_local); - torture_suite_add_1smb2_test( + torture_suite_add_2smb2_test( suite, "locking conflict", test_fruit_locking_conflict); return suite; -- 2.20.1.791.gb4d0f1c61a-goog From 9ffc5f528313d0df7af0dbd4e5ba5f5ab7208ded Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Wed, 30 Jan 2019 18:09:52 +0100 Subject: [PATCH 3/4] s3:vfs: Initialize pid to 0 in test_netatalk_lock() BUG: https://bugzilla.samba.org/show_bug.cgi?id=13770 Signed-off-by: Andreas Schneider Reviewed-by: Jeremy Allison (cherry picked from commit 2ff2594b2bd878928cec30bc72a95a6d38bee154) --- source3/modules/vfs_fruit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index c801f98eafb..f54038f53d4 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -2647,7 +2647,7 @@ static bool test_netatalk_lock(files_struct *fsp, off_t in_offset) off_t offset = in_offset; off_t len = 1; int type = F_WRLCK; - pid_t pid; + pid_t pid = 0; result = SMB_VFS_GETLOCK(fsp, &offset, &len, &type, &pid); if (result == false) { -- 2.20.1.791.gb4d0f1c61a-goog From ffd2ade8f667771741668dcffec376fab7eb4adb Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Wed, 30 Jan 2019 18:45:34 +0100 Subject: [PATCH 4/4] s3:vfs: Correctly check if OFD locks should be enabled or not Also the smb.conf options should only be checked once and a reload of the config should not switch to a different locking mode. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13770 Signed-off-by: Andreas Schneider Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Sat Feb 9 03:43:50 CET 2019 on sn-devel-144 (cherry picked from commit 7ff94b18e2e39567ef7a208084cc5c914c39d3bd) --- source3/include/proto.h | 2 +- source3/lib/util.c | 7 ++----- source3/modules/vfs_default.c | 14 ++++---------- source3/smbd/files.c | 9 +++++++++ 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/source3/include/proto.h b/source3/include/proto.h index 715bc56e286..9d6192967ba 100644 --- a/source3/include/proto.h +++ b/source3/include/proto.h @@ -360,7 +360,7 @@ void set_namearray(name_compare_entry **ppname_array, const char *namelist); void free_namearray(name_compare_entry *name_array); bool fcntl_lock(int fd, int op, off_t offset, off_t count, int type); bool fcntl_getlock(int fd, int op, off_t *poffset, off_t *pcount, int *ptype, pid_t *ppid); -int map_process_lock_to_ofd_lock(int op, bool *use_ofd_locks); +int map_process_lock_to_ofd_lock(int op); bool is_myname(const char *s); void ra_lanman_string( const char *native_lanman ); const char *get_remote_arch_str(void); diff --git a/source3/lib/util.c b/source3/lib/util.c index 5dbd67349fa..7530ea67973 100644 --- a/source3/lib/util.c +++ b/source3/lib/util.c @@ -1079,7 +1079,7 @@ bool fcntl_getlock(int fd, int op, off_t *poffset, off_t *pcount, int *ptype, pi } #if defined(HAVE_OFD_LOCKS) -int map_process_lock_to_ofd_lock(int op, bool *use_ofd_locks) +int map_process_lock_to_ofd_lock(int op) { switch (op) { case F_GETLK: @@ -1095,16 +1095,13 @@ int map_process_lock_to_ofd_lock(int op, bool *use_ofd_locks) op = F_OFD_SETLKW; break; default: - *use_ofd_locks = false; return -1; } - *use_ofd_locks = true; return op; } #else /* HAVE_OFD_LOCKS */ -int map_process_lock_to_ofd_lock(int op, bool *use_ofd_locks) +int map_process_lock_to_ofd_lock(int op) { - *use_ofd_locks = false; return op; } #endif /* HAVE_OFD_LOCKS */ diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index a27d33a6bea..cb5537e096e 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -2553,11 +2553,8 @@ static bool vfswrap_lock(vfs_handle_struct *handle, files_struct *fsp, int op, o START_PROFILE(syscall_fcntl_lock); - if (fsp->use_ofd_locks || !lp_parm_bool(SNUM(fsp->conn), - "smbd", - "force process locks", - false)) { - op = map_process_lock_to_ofd_lock(op, &fsp->use_ofd_locks); + if (fsp->use_ofd_locks) { + op = map_process_lock_to_ofd_lock(op); } result = fcntl_lock(fsp->fh->fd, op, offset, count, type); @@ -2581,11 +2578,8 @@ static bool vfswrap_getlock(vfs_handle_struct *handle, files_struct *fsp, off_t START_PROFILE(syscall_fcntl_getlock); - if (fsp->use_ofd_locks || !lp_parm_bool(SNUM(fsp->conn), - "smbd", - "force process locks", - false)) { - op = map_process_lock_to_ofd_lock(op, &fsp->use_ofd_locks); + if (fsp->use_ofd_locks) { + op = map_process_lock_to_ofd_lock(op); } result = fcntl_getlock(fsp->fh->fd, op, poffset, pcount, ptype, ppid); diff --git a/source3/smbd/files.c b/source3/smbd/files.c index 397baea84cb..99b4937c99b 100644 --- a/source3/smbd/files.c +++ b/source3/smbd/files.c @@ -51,6 +51,15 @@ NTSTATUS fsp_new(struct connection_struct *conn, TALLOC_CTX *mem_ctx, goto fail; } +#if defined(HAVE_OFD_LOCKS) + fsp->use_ofd_locks = true; + if (lp_parm_bool(SNUM(conn), + "smbd", + "force process locks", + false)) { + fsp->use_ofd_locks = false; + } +#endif fsp->fh->ref_count = 1; fsp->fh->fd = -1; -- 2.20.1.791.gb4d0f1c61a-goog