The Samba-Bugzilla – Attachment 9209 Details for
Bug 10138
smbd doesn't always clean up share modes after hard crash.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for 4.0.next
bug-10138-4.0.patchset (text/plain), 15.04 KB, created by
Jeremy Allison
on 2013-09-12 19:22:06 UTC
(
hide
)
Description:
git-am fix for 4.0.next
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2013-09-12 19:22:06 UTC
Size:
15.04 KB
patch
obsolete
>From 33f3ad152b9d005737b9920f18ba38ca295a500c Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >Date: Thu, 22 Aug 2013 08:49:07 +0000 >Subject: [PATCH 1/5] smbd: Simplify find_oplock_types > >Signed-off-by: Volker Lendecke <vl@samba.org> >Reviewed-by: Michael Adam <obnox@samba.org> >(cherry picked from commit 94b320527eee0c7ba1d3818816e7d59cb863bf3f) >--- > source3/smbd/open.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > >diff --git a/source3/smbd/open.c b/source3/smbd/open.c >index 16ca34a..c498c66 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -1243,19 +1243,20 @@ static void find_oplock_types(files_struct *fsp, > } > > for (i=0; i<lck->data->num_share_modes; i++) { >- if (!is_valid_share_mode_entry(&lck->data->share_modes[i])) { >+ struct share_mode_entry *e = &lck->data->share_modes[i]; >+ >+ if (!is_valid_share_mode_entry(e)) { > continue; > } > >- if (lck->data->share_modes[i].op_type == NO_OPLOCK && >- is_stat_open(lck->data->share_modes[i].access_mask)) { >+ if (e->op_type == NO_OPLOCK && is_stat_open(e->access_mask)) { > /* We ignore stat opens in the table - they > always have NO_OPLOCK and never get or > cause breaks. JRA. */ > continue; > } > >- if (BATCH_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) { >+ if (BATCH_OPLOCK_TYPE(e->op_type)) { > /* batch - can only be one. */ > if (share_mode_stale_pid(lck->data, i)) { > DEBUG(10, ("Found stale batch oplock\n")); >@@ -1264,10 +1265,10 @@ static void find_oplock_types(files_struct *fsp, > if (*pp_ex_or_batch || *pp_batch || *got_level2 || *got_no_oplock) { > smb_panic("Bad batch oplock entry."); > } >- *pp_batch = &lck->data->share_modes[i]; >+ *pp_batch = e; > } > >- if (EXCLUSIVE_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) { >+ if (EXCLUSIVE_OPLOCK_TYPE(e->op_type)) { > if (share_mode_stale_pid(lck->data, i)) { > DEBUG(10, ("Found stale duplicate oplock\n")); > continue; >@@ -1276,10 +1277,10 @@ static void find_oplock_types(files_struct *fsp, > if (*pp_ex_or_batch || *got_level2 || *got_no_oplock) { > smb_panic("Bad exclusive or batch oplock entry."); > } >- *pp_ex_or_batch = &lck->data->share_modes[i]; >+ *pp_ex_or_batch = e; > } > >- if (LEVEL_II_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) { >+ if (LEVEL_II_OPLOCK_TYPE(e->op_type)) { > if (*pp_batch || *pp_ex_or_batch) { > if (share_mode_stale_pid(lck->data, i)) { > DEBUG(10, ("Found stale LevelII " >@@ -1291,7 +1292,7 @@ static void find_oplock_types(files_struct *fsp, > *got_level2 = true; > } > >- if (lck->data->share_modes[i].op_type == NO_OPLOCK) { >+ if (e->op_type == NO_OPLOCK) { > if (*pp_batch || *pp_ex_or_batch) { > if (share_mode_stale_pid(lck->data, i)) { > DEBUG(10, ("Found stale NO_OPLOCK " >-- >1.8.4 > > >From 563d7422f88c2c062a8b9f4077ea552c24270bfb Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >Date: Sun, 1 Sep 2013 11:07:19 +0200 >Subject: [PATCH 2/5] smbd: Don't store in-memory only flags in locking.tdb > >Hey, pidl knows the [skip] attribute ... :-) > >Signed-off-by: Volker Lendecke <vl@samba.org> >Reviewed-by: Michael Adam <obnox@samba.org> >(cherry picked from commit 696bc569b17f024f840774e3d59761229836a310) >--- > source3/librpc/idl/open_files.idl | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/source3/librpc/idl/open_files.idl b/source3/librpc/idl/open_files.idl >index fa87bc7..a2e386f 100644 >--- a/source3/librpc/idl/open_files.idl >+++ b/source3/librpc/idl/open_files.idl >@@ -42,8 +42,8 @@ interface open_files > [size_is(num_delete_tokens)] delete_token delete_tokens[]; > timespec old_write_time; > timespec changed_write_time; >- uint8 fresh; >- uint8 modified; >+ [skip] boolean8 fresh; >+ [skip] boolean8 modified; > [ignore] db_record *record; > } share_mode_data; > >-- >1.8.4 > > >From 738d40dbdcc1ea779d4583440540a7907f555c81 Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >Date: Fri, 30 Aug 2013 12:27:36 +0000 >Subject: [PATCH 3/5] smbd: Rename parameter "i" to "idx" > >We'll need "i" in a later checkin ... :-) > >Signed-off-by: Volker Lendecke <vl@samba.org> >Reviewed-by: Michael Adam <obnox@samba.org> >(cherry picked from commit 5006db98aaf1efe119f1da8be091587a9bc2b952) > >Conflicts: > source3/locking/proto.h >--- > source3/locking/locking.c | 12 ++++++------ > source3/locking/proto.h | 2 +- > 2 files changed, 7 insertions(+), 7 deletions(-) > >diff --git a/source3/locking/locking.c b/source3/locking/locking.c >index a7fc50c..c009210 100644 >--- a/source3/locking/locking.c >+++ b/source3/locking/locking.c >@@ -639,24 +639,24 @@ bool is_deferred_open_entry(const struct share_mode_entry *e) > * from d->share_modes. Modifies d->num_share_modes, watch out in > * routines iterating over that array. > */ >-bool share_mode_stale_pid(struct share_mode_data *d, unsigned i) >+bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx) > { > struct share_mode_entry *e; > >- if (i > d->num_share_modes) { >+ if (idx > d->num_share_modes) { > DEBUG(1, ("Asking for index %u, only %u around\n", >- i, (unsigned)d->num_share_modes)); >+ idx, (unsigned)d->num_share_modes)); > return false; > } >- e = &d->share_modes[i]; >+ e = &d->share_modes[idx]; > if (serverid_exists(&e->pid)) { > DEBUG(10, ("PID %s (index %u out of %u) still exists\n", >- procid_str_static(&e->pid), i, >+ procid_str_static(&e->pid), idx, > (unsigned)d->num_share_modes)); > return false; > } > DEBUG(10, ("PID %s (index %u out of %u) does not exist anymore\n", >- procid_str_static(&e->pid), i, >+ procid_str_static(&e->pid), idx, > (unsigned)d->num_share_modes)); > *e = d->share_modes[d->num_share_modes-1]; > d->num_share_modes -= 1; >diff --git a/source3/locking/proto.h b/source3/locking/proto.h >index bb7255d..47d9b80 100644 >--- a/source3/locking/proto.h >+++ b/source3/locking/proto.h >@@ -171,7 +171,7 @@ void get_file_infos(struct file_id id, > struct timespec *write_time); > bool is_valid_share_mode_entry(const struct share_mode_entry *e); > bool is_deferred_open_entry(const struct share_mode_entry *e); >-bool share_mode_stale_pid(struct share_mode_data *d, unsigned i); >+bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx); > void set_share_mode(struct share_mode_lock *lck, files_struct *fsp, > uid_t uid, uint64_t mid, uint16 op_type); > void add_deferred_open(struct share_mode_lock *lck, uint64_t mid, >-- >1.8.4 > > >From 93e8c1ddda131e37b48f1655bdfb9f1966d6426a Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >Date: Fri, 30 Aug 2013 12:49:43 +0000 >Subject: [PATCH 4/5] smbd: Fix flawed share_mode_stale_pid API > >The comment for this routine said: > >> Modifies d->num_share_modes, watch out in routines iterating over >> that array. > >Well, it turns out that *every* caller of this API got it wrong. So I >think it's better to change the routine. > >This leaves the array untouched while iterating but filters out the >deleted ones while saving them back to disk. > >Signed-off-by: Volker Lendecke <vl@samba.org> >Reviewed-by: Michael Adam <obnox@samba.org> >(cherry picked from commit 7d91ffc6fdc3b371564e14f09822a96264ea372a) >--- > source3/librpc/idl/open_files.idl | 6 ++++++ > source3/locking/locking.c | 35 ++++++++++++++++++++++++++--------- > source3/locking/share_mode_lock.c | 24 ++++++++++++++++++++++++ > 3 files changed, 56 insertions(+), 9 deletions(-) > >diff --git a/source3/librpc/idl/open_files.idl b/source3/librpc/idl/open_files.idl >index a2e386f..686bc02 100644 >--- a/source3/librpc/idl/open_files.idl >+++ b/source3/librpc/idl/open_files.idl >@@ -23,6 +23,12 @@ interface open_files > uint32 uid; > uint16 flags; > uint32 name_hash; >+ >+ /* >+ * In-memory flag indicating a non-existing pid. We don't want >+ * to store this share_mode_entry on disk. >+ */ >+ [skip] boolean8 stale; > } share_mode_entry; > > typedef [public] struct { >diff --git a/source3/locking/locking.c b/source3/locking/locking.c >index c009210..be2c92d 100644 >--- a/source3/locking/locking.c >+++ b/source3/locking/locking.c >@@ -617,6 +617,10 @@ bool is_valid_share_mode_entry(const struct share_mode_entry *e) > { > int num_props = 0; > >+ if (e->stale) { >+ return false; >+ } >+ > num_props += ((e->op_type == NO_OPLOCK) ? 1 : 0); > num_props += (EXCLUSIVE_OPLOCK_TYPE(e->op_type) ? 1 : 0); > num_props += (LEVEL_II_OPLOCK_TYPE(e->op_type) ? 1 : 0); >@@ -635,9 +639,7 @@ bool is_deferred_open_entry(const struct share_mode_entry *e) > /* > * In case d->share_modes[i] conflicts with something or otherwise is > * being used, we need to make sure the corresponding process still >- * exists. This routine checks it and potentially removes the entry >- * from d->share_modes. Modifies d->num_share_modes, watch out in >- * routines iterating over that array. >+ * exists. > */ > bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx) > { >@@ -658,17 +660,32 @@ bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx) > DEBUG(10, ("PID %s (index %u out of %u) does not exist anymore\n", > procid_str_static(&e->pid), idx, > (unsigned)d->num_share_modes)); >- *e = d->share_modes[d->num_share_modes-1]; >- d->num_share_modes -= 1; > >- if (d->num_share_modes == 0 && >- d->num_delete_tokens) { >+ e->stale = true; >+ >+ if (d->num_delete_tokens != 0) { >+ uint32_t i, num_stale; >+ > /* > * We cannot have any delete tokens > * if there are no valid share modes. > */ >- TALLOC_FREE(d->delete_tokens); >- d->num_delete_tokens = 0; >+ >+ num_stale = 0; >+ >+ for (i=0; i<d->num_share_modes; i++) { >+ if (d->share_modes[i].stale) { >+ num_stale += 1; >+ } >+ } >+ >+ if (num_stale == d->num_share_modes) { >+ /* >+ * No non-stale share mode found >+ */ >+ TALLOC_FREE(d->delete_tokens); >+ d->num_delete_tokens = 0; >+ } > } > > d->modified = true; >diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c >index 266be65..6782f59 100644 >--- a/source3/locking/share_mode_lock.c >+++ b/source3/locking/share_mode_lock.c >@@ -118,6 +118,7 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx, > { > struct share_mode_data *d; > enum ndr_err_code ndr_err; >+ uint32_t i; > DATA_BLOB blob; > > d = talloc(mem_ctx, struct share_mode_data); >@@ -137,6 +138,14 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx, > goto fail; > } > >+ /* >+ * Initialize the values that are [skip] in the idl. The NDR code does >+ * not initialize them. >+ */ >+ >+ for (i=0; i<d->num_share_modes; i++) { >+ d->share_modes[i].stale = false; >+ } > d->modified = false; > d->fresh = false; > >@@ -159,12 +168,27 @@ static TDB_DATA unparse_share_modes(struct share_mode_data *d) > { > DATA_BLOB blob; > enum ndr_err_code ndr_err; >+ uint32_t i; > > if (DEBUGLEVEL >= 10) { > DEBUG(10, ("unparse_share_modes:\n")); > NDR_PRINT_DEBUG(share_mode_data, d); > } > >+ i = 0; >+ while (i < d->num_share_modes) { >+ if (d->share_modes[i].stale) { >+ /* >+ * Remove the stale entries before storing >+ */ >+ struct share_mode_entry *m = d->share_modes; >+ m[i] = m[d->num_share_modes-1]; >+ d->num_share_modes -= 1; >+ } else { >+ i += 1; >+ } >+ } >+ > if (d->num_share_modes == 0) { > DEBUG(10, ("No used share mode found\n")); > return make_tdb_data(NULL, 0); >-- >1.8.4 > > >From 2e3a1a0cc8d134620c7d52e31490cca619fe6d72 Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >Date: Sun, 1 Sep 2013 18:54:59 +0200 >Subject: [PATCH 5/5] torture3: Trigger a nasty cleanup bug in smbd > >Signed-off-by: Volker Lendecke <vl@samba.org> >Reviewed-by: Michael Adam <obnox@samba.org> > >Autobuild-User(master): Michael Adam <obnox@samba.org> >Autobuild-Date(master): Tue Sep 3 19:13:14 CEST 2013 on sn-devel-104 > >(cherry picked from commit ade8477f98fcffcc6e3c5ea31618b49d0c1bba95) >--- > source3/selftest/tests.py | 1 + > source3/torture/proto.h | 1 + > source3/torture/test_cleanup.c | 70 ++++++++++++++++++++++++++++++++++++++++++ > source3/torture/torture.c | 1 + > 4 files changed, 73 insertions(+) > >diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py >index 4453c4d..c293e6d 100755 >--- a/source3/selftest/tests.py >+++ b/source3/selftest/tests.py >@@ -63,6 +63,7 @@ tests = ["FDPASS", "LOCK1", "LOCK2", "LOCK3", "LOCK4", "LOCK5", "LOCK6", "LOCK7" > "SMB2-SESSION-REAUTH", "SMB2-SESSION-RECONNECT", > "CLEANUP1", > "CLEANUP2", >+ "CLEANUP4", > "BAD-NBT-SESSION"] > > for t in tests: >diff --git a/source3/torture/proto.h b/source3/torture/proto.h >index 0c6fc70..dde28ef 100644 >--- a/source3/torture/proto.h >+++ b/source3/torture/proto.h >@@ -104,6 +104,7 @@ bool run_local_sprintf_append(int dummy); > bool run_cleanup1(int dummy); > bool run_cleanup2(int dummy); > bool run_cleanup3(int dummy); >+bool run_cleanup4(int dummy); > bool run_ctdb_conn(int dummy); > bool run_msg_test(int dummy); > bool run_notify_bench2(int dummy); >diff --git a/source3/torture/test_cleanup.c b/source3/torture/test_cleanup.c >index d9dce40..319a55f 100644 >--- a/source3/torture/test_cleanup.c >+++ b/source3/torture/test_cleanup.c >@@ -329,3 +329,73 @@ bool run_cleanup3(int dummy) > > return true; > } >+ >+bool run_cleanup4(int dummy) >+{ >+ struct cli_state *cli1, *cli2; >+ const char *fname = "\\cleanup4"; >+ uint16_t fnum1, fnum2; >+ NTSTATUS status; >+ >+ printf("CLEANUP4: Checking that a conflicting share mode is cleaned " >+ "up\n"); >+ >+ if (!torture_open_connection(&cli1, 0)) { >+ return false; >+ } >+ if (!torture_open_connection(&cli2, 0)) { >+ return false; >+ } >+ >+ status = cli_ntcreate( >+ cli1, fname, 0, >+ FILE_GENERIC_READ|DELETE_ACCESS, >+ FILE_ATTRIBUTE_NORMAL, >+ FILE_SHARE_READ|FILE_SHARE_DELETE, >+ FILE_OVERWRITE_IF, 0, 0, &fnum1); >+ if (!NT_STATUS_IS_OK(status)) { >+ printf("creating file failed: %s\n", >+ nt_errstr(status)); >+ return false; >+ } >+ >+ status = cli_ntcreate( >+ cli2, fname, 0, >+ FILE_GENERIC_READ|DELETE_ACCESS, >+ FILE_ATTRIBUTE_NORMAL, >+ FILE_SHARE_READ|FILE_SHARE_DELETE, >+ FILE_OPEN, 0, 0, &fnum2); >+ if (!NT_STATUS_IS_OK(status)) { >+ printf("opening file 1st time failed: %s\n", >+ nt_errstr(status)); >+ return false; >+ } >+ >+ status = smbXcli_conn_samba_suicide(cli1->conn, 1); >+ if (!NT_STATUS_IS_OK(status)) { >+ printf("smbXcli_conn_samba_suicide failed: %s\n", >+ nt_errstr(status)); >+ return false; >+ } >+ >+ /* >+ * The next open will conflict with both opens above. The first open >+ * above will be correctly cleaned up. A bug in smbd iterating over >+ * the share mode array made it skip the share conflict check for the >+ * second open. Trigger this bug. >+ */ >+ >+ status = cli_ntcreate( >+ cli2, fname, 0, >+ FILE_GENERIC_WRITE|DELETE_ACCESS, >+ FILE_ATTRIBUTE_NORMAL, >+ FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, >+ FILE_OPEN, 0, 0, &fnum2); >+ if (!NT_STATUS_EQUAL(status, NT_STATUS_SHARING_VIOLATION)) { >+ printf("opening file 2nd time returned: %s\n", >+ nt_errstr(status)); >+ return false; >+ } >+ >+ return true; >+} >diff --git a/source3/torture/torture.c b/source3/torture/torture.c >index 21dcf9b..107106c 100644 >--- a/source3/torture/torture.c >+++ b/source3/torture/torture.c >@@ -9508,6 +9508,7 @@ static struct { > { "CLEANUP1", run_cleanup1 }, > { "CLEANUP2", run_cleanup2 }, > { "CLEANUP3", run_cleanup3 }, >+ { "CLEANUP4", run_cleanup4 }, > { "LOCAL-SUBSTITUTE", run_local_substitute, 0}, > { "LOCAL-GENCACHE", run_local_gencache, 0}, > { "LOCAL-TALLOC-DICT", run_local_talloc_dict, 0}, >-- >1.8.4 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
vl
:
review+
Actions:
View
Attachments on
bug 10138
:
9208
| 9209 |
9247