[PATCH] Fix bug #10564 - Lock order violation and file lost
Jeremy Allison
jra at samba.org
Thu May 1 12:21:00 MDT 2014
Volker, here is the patchset we discussed
on the phone earlier this week. It finally
removes open_file_fchmod() and protects
INTERNAL_OPEN_ONLY from causing deadlocks
when the share mode locking db has already
been locked across an atomic rename() operation.
Tested here and fixes the smb_panic call
reported by the caller.
Please review and push if appropriate !
Cheers,
Jeremy.
-------------- next part --------------
From 49a968afe56cc3b442dbccf2c26e3f8b5f088cc2 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 1 May 2014 10:58:51 -0700
Subject: [PATCH 1/4] s3 : smbd : Protect all possible code paths from fsp->op
== NULL.
In changes to come this will be possible for an INTERNAL_OPEN_ONLY.
The protection was already in place for some code paths, this
makes the coverage compete.
Bug 10564 - Lock order violation and file lost
https://bugzilla.samba.org/show_bug.cgi?id=10564
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/locking/brlock.c | 16 ++++++++++++++--
source3/modules/vfs_btrfs.c | 5 +++++
source3/modules/vfs_default.c | 9 +++++++++
source3/smbd/aio.c | 10 ++++++++++
source3/smbd/scavenger.c | 3 +++
5 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index ac22ba4..e0e042d 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -1563,12 +1563,18 @@ void brl_close_fnum(struct messaging_context *msg_ctx,
bool brl_mark_disconnected(struct files_struct *fsp)
{
uint32_t tid = fsp->conn->cnum;
- uint64_t smblctx = fsp->op->global->open_persistent_id;
+ uint64_t smblctx;
uint64_t fnum = fsp->fnum;
unsigned int i;
struct server_id self = messaging_server_id(fsp->conn->sconn->msg_ctx);
struct byte_range_lock *br_lck = NULL;
+ if (fsp->op == NULL) {
+ return false;
+ }
+
+ smblctx = fsp->op->global->open_persistent_id;
+
if (!fsp->op->global->durable) {
return false;
}
@@ -1623,12 +1629,18 @@ bool brl_mark_disconnected(struct files_struct *fsp)
bool brl_reconnect_disconnected(struct files_struct *fsp)
{
uint32_t tid = fsp->conn->cnum;
- uint64_t smblctx = fsp->op->global->open_persistent_id;
+ uint64_t smblctx;
uint64_t fnum = fsp->fnum;
unsigned int i;
struct server_id self = messaging_server_id(fsp->conn->sconn->msg_ctx);
struct byte_range_lock *br_lck = NULL;
+ if (fsp->op == NULL) {
+ return false;
+ }
+
+ smblctx = fsp->op->global->open_persistent_id;
+
if (!fsp->op->global->durable) {
return false;
}
diff --git a/source3/modules/vfs_btrfs.c b/source3/modules/vfs_btrfs.c
index 997a5de..c1e17b3 100644
--- a/source3/modules/vfs_btrfs.c
+++ b/source3/modules/vfs_btrfs.c
@@ -116,6 +116,11 @@ static struct tevent_req *btrfs_copy_chunk_send(struct vfs_handle_struct *handle
return tevent_req_post(req, ev);
}
+ if (src_fsp->op == NULL || dest_fsp->op == NULL) {
+ tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR);
+ return tevent_req_post(req, ev);
+ }
+
init_strict_lock_struct(src_fsp,
src_fsp->op->global->open_persistent_id,
src_off,
diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
index a129d81..0695357 100644
--- a/source3/modules/vfs_default.c
+++ b/source3/modules/vfs_default.c
@@ -1393,6 +1393,10 @@ static struct tevent_req *vfswrap_copy_chunk_send(struct vfs_handle_struct *hand
off_t this_num = MIN(sizeof(vfs_cc_state->buf),
num - vfs_cc_state->copied);
+ if (src_fsp->op == NULL) {
+ tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR);
+ return tevent_req_post(req, ev);
+ }
init_strict_lock_struct(src_fsp,
src_fsp->op->global->open_persistent_id,
src_off,
@@ -1426,6 +1430,11 @@ static struct tevent_req *vfswrap_copy_chunk_send(struct vfs_handle_struct *hand
src_off += ret;
+ if (dest_fsp->op == NULL) {
+ tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR);
+ return tevent_req_post(req, ev);
+ }
+
init_strict_lock_struct(dest_fsp,
dest_fsp->op->global->open_persistent_id,
dest_off,
diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
index 9c7c92c..2235c32 100644
--- a/source3/smbd/aio.c
+++ b/source3/smbd/aio.c
@@ -688,6 +688,11 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn,
return NT_STATUS_RETRY;
}
+ if (fsp->op == NULL) {
+ /* No AIO on internal opens. */
+ return NT_STATUS_RETRY;
+ }
+
if ((!min_aio_read_size || (smb_maxcnt < min_aio_read_size))
&& !SMB_VFS_AIO_FORCE(fsp)) {
/* Too small a read for aio request. */
@@ -839,6 +844,11 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
return NT_STATUS_RETRY;
}
+ if (fsp->op == NULL) {
+ /* No AIO on internal opens. */
+ return NT_STATUS_RETRY;
+ }
+
if ((!min_aio_write_size || (in_data.length < min_aio_write_size))
&& !SMB_VFS_AIO_FORCE(fsp)) {
/* Too small a write for aio request. */
diff --git a/source3/smbd/scavenger.c b/source3/smbd/scavenger.c
index e6e2878..122305e 100644
--- a/source3/smbd/scavenger.c
+++ b/source3/smbd/scavenger.c
@@ -418,6 +418,9 @@ void scavenger_schedule_disconnected(struct files_struct *fsp)
struct scavenger_message msg;
DATA_BLOB msg_blob;
+ if (fsp->op == NULL) {
+ return;
+ }
nttime_to_timeval(&disconnect_time, fsp->op->global->disconnect_time);
timeout_usec = 1000 * fsp->op->global->durable_timeout_msec;
until = timeval_add(&disconnect_time,
--
1.9.1.423.g4596e3a
From f81cee5537f243a54824cdc785240e46545fb748 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 1 May 2014 11:01:03 -0700
Subject: [PATCH 2/4] s3: smbd : Ensure file_new doesn't call into
smbXsrv_open_create() for INTERNAL_OPEN_ONLY.
This causes deadlocks which cause smbd to crash if the locking
database has already been locked for a compound operation we
need to be atomic (as in the file rename case).
Ensure INTERNAL_OPEN_ONLY opens are synonymous with req==NULL.
INTERNAL_OPEN_ONLY opens leave a NO_OPLOCK record in
the share mode database, so they can be detected by other
processes for share mode violation purposes (because
they're doing an operation on the file that may include
reads or writes they need to have real state inside the
locking database) but have an fnum of FNUM_FIELD_INVALID
and a local share_file_id of zero, as they will never be
seen on the wire.
Ensure validate_my_share_entries() ignores
INTERNAL_OPEN_ONLY records (share_file_id == 0).
Bug 10564 - Lock order violation and file lost
https://bugzilla.samba.org/show_bug.cgi?id=10564
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/files.c | 4 +++-
source3/smbd/open.c | 14 +++++++++++---
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/source3/smbd/files.c b/source3/smbd/files.c
index 8496806..5676bc9 100644
--- a/source3/smbd/files.c
+++ b/source3/smbd/files.c
@@ -93,7 +93,7 @@ NTSTATUS file_new(struct smb_request *req, connection_struct *conn,
GetTimeOfDay(&fsp->open_time);
- if (sconn->conn) {
+ if (req) {
struct smbXsrv_open *op = NULL;
NTTIME now = timeval_to_nttime(&fsp->open_time);
@@ -108,6 +108,8 @@ NTSTATUS file_new(struct smb_request *req, connection_struct *conn,
op->compat = fsp;
fsp->fnum = op->local_id;
fsp->fh->gen_id = smbXsrv_open_hash(op);
+ } else {
+ /* INTERNAL_OPEN_ONLY */
}
/*
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index d05c9ec..585d6c9 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1080,6 +1080,11 @@ static void validate_my_share_entries(struct smbd_server_connection *sconn,
return;
}
+ if (share_entry->share_file_id == 0) {
+ /* INTERNAL_OPEN_ONLY */
+ return;
+ }
+
if (!is_valid_share_mode_entry(share_entry)) {
return;
}
@@ -2125,9 +2130,12 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
create_options, (unsigned int)unx_mode, oplock_request,
(unsigned int)private_flags));
- if ((req == NULL) && ((oplock_request & INTERNAL_OPEN_ONLY) == 0)) {
- DEBUG(0, ("No smb request but not an internal only open!\n"));
- return NT_STATUS_INTERNAL_ERROR;
+ if (req == NULL) {
+ /* Ensure req == NULL means INTERNAL_OPEN_ONLY */
+ SMB_ASSERT(((oplock_request & INTERNAL_OPEN_ONLY) != 0));
+ } else {
+ /* And req != NULL means no INTERNAL_OPEN_ONLY */
+ SMB_ASSERT(((oplock_request & INTERNAL_OPEN_ONLY) == 0));
}
/*
--
1.9.1.423.g4596e3a
From 8b8edbbd0e90213bf204ef684abf06f8e1f69f9e Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 1 May 2014 11:07:44 -0700
Subject: [PATCH 3/4] s3: smbd: change file_set_dosmode() to use
get_file_handle_for_metadata() instead of open_file_fchmod().
get_file_handle_for_metadata() is a new function that
finds an existing open handle (fsp->fh->fd != -1) for
a given dev/ino if there is one available, and uses
INTERNAL_OPEN_ONLY with WRITE_DATA access if not.
Allows open_file_fchmod() to be removed next.
Bug 10564 - Lock order violation and file lost
https://bugzilla.samba.org/show_bug.cgi?id=10564
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/dosmode.c | 103 ++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 90 insertions(+), 13 deletions(-)
diff --git a/source3/smbd/dosmode.c b/source3/smbd/dosmode.c
index 831f6d0..22ab4eb 100644
--- a/source3/smbd/dosmode.c
+++ b/source3/smbd/dosmode.c
@@ -26,6 +26,11 @@
#include "smbd/smbd.h"
#include "lib/param/loadparm.h"
+static NTSTATUS get_file_handle_for_metadata(connection_struct *conn,
+ struct smb_filename *smb_fname,
+ files_struct **ret_fsp,
+ bool *need_close);
+
static void dos_mode_debug_print(uint32_t mode)
{
DEBUG(8,("dos_mode returning "));
@@ -420,6 +425,7 @@ static bool set_ea_dos_attribute(connection_struct *conn,
SAMBA_XATTR_DOS_ATTRIB, blob.data, blob.length,
0) == -1) {
bool ret = false;
+ bool need_close = false;
files_struct *fsp = NULL;
if((errno != EPERM) && (errno != EACCES)) {
@@ -451,14 +457,17 @@ static bool set_ea_dos_attribute(connection_struct *conn,
}
/*
- * We need to open the file with write access whilst
- * still in our current user context. This ensures we
- * are not violating security in doing the setxattr.
+ * We need to get an open file handle to do the
+ * metadata operation under root.
*/
- if (!NT_STATUS_IS_OK(open_file_fchmod(conn, smb_fname,
- &fsp)))
+ if (!NT_STATUS_IS_OK(get_file_handle_for_metadata(conn,
+ smb_fname,
+ &fsp,
+ &need_close))) {
return false;
+ }
+
become_root();
if (SMB_VFS_FSETXATTR(fsp,
SAMBA_XATTR_DOS_ATTRIB, blob.data,
@@ -466,7 +475,9 @@ static bool set_ea_dos_attribute(connection_struct *conn,
ret = true;
}
unbecome_root();
- close_file(NULL, fsp, NORMAL_CLOSE);
+ if (need_close) {
+ close_file(NULL, fsp, NORMAL_CLOSE);
+ }
return ret;
}
DEBUG(10,("set_ea_dos_attribute: set EA 0x%x on file %s\n",
@@ -777,6 +788,7 @@ int file_set_dosmode(connection_struct *conn, struct smb_filename *smb_fname,
uint32_t old_mode;
struct timespec new_create_timespec;
files_struct *fsp = NULL;
+ bool need_close = false;
if (!CAN_WRITE(conn)) {
errno = EROFS;
@@ -945,17 +957,23 @@ int file_set_dosmode(connection_struct *conn, struct smb_filename *smb_fname,
}
/*
- * We need to open the file with write access whilst
- * still in our current user context. This ensures we
- * are not violating security in doing the fchmod.
+ * We need to get an open file handle to do the
+ * metadata operation under root.
*/
- if (!NT_STATUS_IS_OK(open_file_fchmod(conn, smb_fname,
- &fsp)))
- return -1;
+
+ if (!NT_STATUS_IS_OK(get_file_handle_for_metadata(conn,
+ smb_fname,
+ &fsp,
+ &need_close))) {
+ return false;
+ }
+
become_root();
ret = SMB_VFS_FCHMOD(fsp, unixmode);
unbecome_root();
- close_file(NULL, fsp, NORMAL_CLOSE);
+ if (need_close) {
+ close_file(NULL, fsp, NORMAL_CLOSE);
+ }
if (!newfile) {
notify_fname(conn, NOTIFY_ACTION_MODIFIED,
FILE_NOTIFY_CHANGE_ATTRIBUTES,
@@ -1191,3 +1209,62 @@ struct timespec get_change_timespec(connection_struct *conn,
{
return smb_fname->st.st_ex_mtime;
}
+
+/****************************************************************************
+ Get a real open file handle we can do meta-data operations on. As it's
+ going to be used under root access only on meta-data we should look for
+ any existing open file handle first, and use that in preference (also to
+ avoid kernel self-oplock breaks). If not use an INTERNAL_OPEN_ONLY handle.
+****************************************************************************/
+
+static NTSTATUS get_file_handle_for_metadata(connection_struct *conn,
+ struct smb_filename *smb_fname,
+ files_struct **ret_fsp,
+ bool *need_close)
+{
+ NTSTATUS status;
+ files_struct *fsp;
+ struct file_id file_id;
+
+ *need_close = false;
+
+ if (!VALID_STAT(smb_fname->st)) {
+ return NT_STATUS_INVALID_PARAMETER;
+ }
+
+ file_id = vfs_file_id_from_sbuf(conn, &smb_fname->st);
+
+ for(fsp = file_find_di_first(conn->sconn, file_id);
+ fsp;
+ fsp = file_find_di_next(fsp)) {
+ if (fsp->fh->fd != -1) {
+ *ret_fsp = fsp;
+ return NT_STATUS_OK;
+ }
+ }
+
+ /* Opens an INTERNAL_OPEN_ONLY write handle. */
+ status = SMB_VFS_CREATE_FILE(
+ conn, /* conn */
+ NULL, /* req */
+ 0, /* root_dir_fid */
+ smb_fname, /* fname */
+ FILE_WRITE_DATA, /* access_mask */
+ (FILE_SHARE_READ | FILE_SHARE_WRITE | /* share_access */
+ FILE_SHARE_DELETE),
+ FILE_OPEN, /* create_disposition*/
+ 0, /* create_options */
+ 0, /* file_attributes */
+ INTERNAL_OPEN_ONLY, /* oplock_request */
+ 0, /* allocation_size */
+ 0, /* private_flags */
+ NULL, /* sd */
+ NULL, /* ea_list */
+ ret_fsp, /* result */
+ NULL); /* pinfo */
+
+ if (NT_STATUS_IS_OK(status)) {
+ *need_close = true;
+ }
+ return status;
+}
--
1.9.1.423.g4596e3a
From b32c71f04430adf570fc04345e23f56ac9fa4bad Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 1 May 2014 11:11:20 -0700
Subject: [PATCH 4/4] s3: smbd: Remove open_file_fchmod().
No longer used (hurrah!).
Bug 10564 - Lock order violation and file lost
https://bugzilla.samba.org/show_bug.cgi?id=10564
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/open.c | 33 ---------------------------------
source3/smbd/proto.h | 3 ---
2 files changed, 36 deletions(-)
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 585d6c9..a8c6d24 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -2870,39 +2870,6 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
return NT_STATUS_OK;
}
-
-/****************************************************************************
- Open a file for for write to ensure that we can fchmod it.
-****************************************************************************/
-
-NTSTATUS open_file_fchmod(connection_struct *conn,
- struct smb_filename *smb_fname,
- files_struct **result)
-{
- if (!VALID_STAT(smb_fname->st)) {
- return NT_STATUS_INVALID_PARAMETER;
- }
-
- return SMB_VFS_CREATE_FILE(
- conn, /* conn */
- NULL, /* req */
- 0, /* root_dir_fid */
- smb_fname, /* fname */
- FILE_WRITE_DATA, /* access_mask */
- (FILE_SHARE_READ | FILE_SHARE_WRITE | /* share_access */
- FILE_SHARE_DELETE),
- FILE_OPEN, /* create_disposition*/
- 0, /* create_options */
- 0, /* file_attributes */
- INTERNAL_OPEN_ONLY, /* oplock_request */
- 0, /* allocation_size */
- 0, /* private_flags */
- NULL, /* sd */
- NULL, /* ea_list */
- result, /* result */
- NULL); /* pinfo */
-}
-
static NTSTATUS mkdir_internal(connection_struct *conn,
struct smb_filename *smb_dname,
uint32 file_attributes)
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index bc15f15..f598816 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -621,9 +621,6 @@ NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
SMB_STRUCT_STAT *psbuf);
bool is_stat_open(uint32 access_mask);
bool is_deferred_open_async(const void *ptr);
-NTSTATUS open_file_fchmod(connection_struct *conn,
- struct smb_filename *smb_fname,
- files_struct **result);
NTSTATUS create_directory(connection_struct *conn, struct smb_request *req,
struct smb_filename *smb_dname);
void msg_file_was_renamed(struct messaging_context *msg,
--
1.9.1.423.g4596e3a
More information about the samba-technical
mailing list