The Samba-Bugzilla – Attachment 9885 Details for
Bug 10564
Lock order violation and file lost
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for 4.1.next.
4.1.patchset (text/plain), 16.12 KB, created by
Jeremy Allison
on 2014-05-01 18:56:40 UTC
(
hide
)
Description:
git-am fix for 4.1.next.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2014-05-01 18:56:40 UTC
Size:
16.12 KB
patch
obsolete
>From 3da00ac7c9da8c6c413e72674041dc6847eb127c Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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@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 78c2538..df689a7 100644 >--- a/source3/locking/brlock.c >+++ b/source3/locking/brlock.c >@@ -1526,12 +1526,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; > } >@@ -1586,12 +1592,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 4ecf7ab..f062eba 100644 >--- a/source3/modules/vfs_btrfs.c >+++ b/source3/modules/vfs_btrfs.c >@@ -98,6 +98,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 82d059c..f6200ed 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 eec29f6..44d771e 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 8c960dfc85e61e8a3dd3e9b761301ca48af3c661 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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 can be detected by other >processes for share mode violation purposes (that >they're doing an operation on the file 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@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 d94ee11..34ec879 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 6e4f690..15fec95 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -1079,6 +1079,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; > } >@@ -2088,9 +2093,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 81cda374cb87d37bcac77a99fd490d85926f0e9d Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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@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 2d07dd9..efadcb6 100644 >--- a/source3/smbd/dosmode.c >+++ b/source3/smbd/dosmode.c >@@ -25,6 +25,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 uint32_t filter_mode_by_protocol(uint32_t mode) > { > if (get_Protocol() <= PROTOCOL_LANMAN2) { >@@ -387,6 +392,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)) { >@@ -418,14 +424,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, >@@ -433,7 +442,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", >@@ -711,6 +722,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; >@@ -879,17 +891,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, >@@ -1125,3 +1143,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 192c4b03048e31070b318d181ba6b246998f11e3 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@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@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 15fec95..5f7bff9 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -2838,39 +2838,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 1e0d06d..6153a49 100644 >--- a/source3/smbd/proto.h >+++ b/source3/smbd/proto.h >@@ -620,9 +620,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 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=9885">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Actions:
View
Attachments on
bug 10564
:
9872
|
9873
|
9874
|
9876
|
9878
| 9885 |
9895
|
9896