[PATCH] Fix for crash "PANIC ...: assert failed: lease_type_is_exclusive(e_lease_type)"
Jeremy Allison
jra at samba.org
Fri May 26 16:01:06 UTC 2017
On Fri, May 26, 2017 at 04:10:52PM +0200, Ralph Böhme wrote:
> Hi!
>
> Attached is a patch for bug
> <https://bugzilla.samba.org/show_bug.cgi?id=12798>
>
> The crash is triggered by stat-opens. The code before the lease optimisation had
> the same issue, but it was not triggered with modern clients since Samba 4.5
> when leases where used.
>
> Please review & push if okay.
>
> There's a second patch attached with an additional test for leases and
> stat-opens. I stumbled across this while working out the reproducer for the
> bug. Looks like we only grant R leases if there's an existing (stat-)open on a
> file, while Windows happily grants RWH if requested.
>
> The test is marked as known fail. I don't have the time currently to work out
> why we're behaving differently compared to Windows and I don't want to let this
> test rot in my private attic.
>
> All patches are signed-off by metze and myself, so if noone pushes or objects,
> I'll push on Monday.
Great work Ralph and Metze ! RB+. I'll push today.
How did you work out what the reproducer was ?
Jeremy.
> From 58974024c18314e99633516a8cf953980a8862e8 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 26 May 2017 11:35:52 +0200
> Subject: [PATCH 1/3] s3/locking: make find_share_mode_entry public
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12798
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Reviewed-by: Stefan Metzmacher <metze at samba.org>
> ---
> source3/locking/locking.c | 2 +-
> source3/locking/proto.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/source3/locking/locking.c b/source3/locking/locking.c
> index 51c1640..037a987 100644
> --- a/source3/locking/locking.c
> +++ b/source3/locking/locking.c
> @@ -859,7 +859,7 @@ bool set_share_mode(struct share_mode_lock *lck, struct files_struct *fsp,
> return true;
> }
>
> -static struct share_mode_entry *find_share_mode_entry(
> +struct share_mode_entry *find_share_mode_entry(
> struct share_mode_lock *lck, files_struct *fsp)
> {
> struct share_mode_data *d = lck->data;
> diff --git a/source3/locking/proto.h b/source3/locking/proto.h
> index 5d2326a..6fb2bf2 100644
> --- a/source3/locking/proto.h
> +++ b/source3/locking/proto.h
> @@ -176,6 +176,8 @@ bool share_mode_stale_pid(struct share_mode_data *d, uint32_t idx);
> bool set_share_mode(struct share_mode_lock *lck, struct files_struct *fsp,
> uid_t uid, uint64_t mid, uint16_t op_type,
> uint32_t lease_idx);
> +struct share_mode_entry *find_share_mode_entry(struct share_mode_lock *lck,
> + files_struct *fsp);
> void remove_stale_share_mode_entries(struct share_mode_data *d);
> bool del_share_mode(struct share_mode_lock *lck, files_struct *fsp);
> bool mark_share_mode_disconnected(struct share_mode_lock *lck,
> --
> 2.9.3
>
>
> From 0526a177b12fd706a7b4e4b756b9f76625e94343 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 26 May 2017 11:57:08 +0200
> Subject: [PATCH 2/3] s3/smbd: fix exclusive lease optimisation
>
> We need to expect any amount of "stat" opens on the file without
> triggering an assert.
>
> This is the correct fix for bug #11844. I guess we haven't seens this
> very often before bug #12766 got fixed, because most clients were using
> LEASES instead of OPLOCKS.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12798
>
> See also:
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11844
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12766
>
> Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
> source3/smbd/oplock.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
> index aa060ad..1b2a87b 100644
> --- a/source3/smbd/oplock.c
> +++ b/source3/smbd/oplock.c
> @@ -26,6 +26,7 @@
> #include "smbd/globals.h"
> #include "messages.h"
> #include "../librpc/gen_ndr/open_files.h"
> +#include "../librpc/gen_ndr/ndr_open_files.h"
>
> /*
> * helper function used by the kernel oplock backends to post the break message
> @@ -167,17 +168,38 @@ bool update_num_read_oplocks(files_struct *fsp, struct share_mode_lock *lck)
> uint32_t i;
>
> if (fsp_lease_type_is_exclusive(fsp)) {
> + const struct share_mode_entry *e = NULL;
> + uint32_t e_lease_type = 0;
> +
> /*
> * If we're fully exclusive, we don't need a brlock entry
> */
> remove_stale_share_mode_entries(d);
>
> - for (i=0; i<d->num_share_modes; i++) {
> - struct share_mode_entry *e = &d->share_modes[i];
> - uint32_t e_lease_type = get_lease_type(d, e);
> + e = find_share_mode_entry(lck, fsp);
> + if (e != NULL) {
> + e_lease_type = get_lease_type(d, e);
> + }
> +
> + if (!lease_type_is_exclusive(e_lease_type)) {
> + char *timestr = NULL;
>
> - SMB_ASSERT(lease_type_is_exclusive(e_lease_type));
> + timestr = timeval_string(talloc_tos(),
> + &fsp->open_time,
> + true);
> +
> + NDR_PRINT_DEBUG(share_mode_data, d);
> + DBG_ERR("file [%s] file_id [%s] gen_id [%lu] "
> + "open_time[%s] lease_type [0x%x] "
> + "oplock_type [0x%x]\n",
> + fsp_str_dbg(fsp),
> + file_id_string_tos(&fsp->file_id),
> + fsp->fh->gen_id, timestr,
> + e_lease_type, fsp->oplock_type);
> +
> + smb_panic("Found non-exclusive lease");
> }
> +
> return true;
> }
>
> --
> 2.9.3
>
>
> From 63b6d2c54efafa691cab1212483262adb379ee16 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 26 May 2017 15:35:54 +0200
> Subject: [PATCH 3/3] s4/torture: test for bug 12798
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12798
>
> Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
> source4/torture/smb2/lease.c | 82 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
>
> diff --git a/source4/torture/smb2/lease.c b/source4/torture/smb2/lease.c
> index b72d76c..10761b8 100644
> --- a/source4/torture/smb2/lease.c
> +++ b/source4/torture/smb2/lease.c
> @@ -993,6 +993,87 @@ done:
> return ret;
> }
>
> +static bool test_lease_statopen2(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + TALLOC_CTX *mem_ctx = talloc_new(tctx);
> + struct smb2_create io;
> + struct smb2_lease ls;
> + struct smb2_handle h1 = {{0}};
> + struct smb2_handle h2 = {{0}};
> + struct smb2_handle h3 = {{0}};
> + NTSTATUS status;
> + const char *fname = "lease_statopen2.dat";
> + bool ret = true;
> + uint32_t caps;
> +
> + caps = smb2cli_conn_server_capabilities(
> + tree->session->transport->conn);
> + if (!(caps & SMB2_CAP_LEASING)) {
> + torture_skip(tctx, "leases are not supported");
> + }
> +
> + smb2_util_unlink(tree, fname);
> + ZERO_STRUCT(break_info);
> + tree->session->transport->lease.handler = torture_lease_handler;
> + tree->session->transport->lease.private_data = tree;
> +
> + status = torture_smb2_testfile(tree, fname, &h1);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + smb2_util_close(tree, h1);
> + ZERO_STRUCT(h1);
> +
> + /* Open file with RWH lease. */
> + smb2_lease_create_share(&io, &ls, false, fname,
> + smb2_util_share_access("RWD"),
> + LEASE1,
> + smb2_util_lease_state("RWH"));
> + io.in.desired_access = SEC_FILE_WRITE_DATA;
> + status = smb2_create(tree, mem_ctx, &io);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + h1 = io.out.file.handle;
> + CHECK_LEASE(&io, "RWH", true, LEASE1, 0);
> +
> + /* Stat open */
> + ZERO_STRUCT(io);
> + io.in.desired_access = FILE_READ_ATTRIBUTES;
> + io.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
> + io.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
> + io.in.create_disposition = NTCREATEX_DISP_OPEN;
> + io.in.fname = fname;
> + status = smb2_create(tree, mem_ctx, &io);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + h2 = io.out.file.handle;
> +
> + /* Open file with RWH lease. */
> + smb2_lease_create_share(&io, &ls, false, fname,
> + smb2_util_share_access("RWD"),
> + LEASE1,
> + smb2_util_lease_state("RWH"));
> + io.in.desired_access = SEC_FILE_WRITE_DATA;
> + status = smb2_create(tree, mem_ctx, &io);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + h3 = io.out.file.handle;
> + CHECK_LEASE(&io, "RWH", true, LEASE1, 0);
> +
> +done:
> + if (!smb2_util_handle_empty(h3)) {
> + smb2_util_close(tree, h3);
> + }
> + if (!smb2_util_handle_empty(h2)) {
> + smb2_util_close(tree, h2);
> + }
> + if (!smb2_util_handle_empty(h1)) {
> + smb2_util_close(tree, h1);
> + }
> + smb2_util_unlink(tree, fname);
> + talloc_free(mem_ctx);
> + return ret;
> +}
>
> static void torture_oplock_break_callback(struct smb2_request *req)
> {
> @@ -3920,6 +4001,7 @@ struct torture_suite *torture_smb2_lease_init(TALLOC_CTX *ctx)
> torture_suite_add_1smb2_test(suite, "nobreakself",
> test_lease_nobreakself);
> torture_suite_add_1smb2_test(suite, "statopen", test_lease_statopen);
> + torture_suite_add_1smb2_test(suite, "statopen2", test_lease_statopen2);
> torture_suite_add_1smb2_test(suite, "upgrade", test_lease_upgrade);
> torture_suite_add_1smb2_test(suite, "upgrade2", test_lease_upgrade2);
> torture_suite_add_1smb2_test(suite, "upgrade3", test_lease_upgrade3);
> --
> 2.9.3
>
> From e48fa7f68a82ce21fa6f709a9e6ced5f1669bef2 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Fri, 26 May 2017 15:42:46 +0200
> Subject: [PATCH] s4/torture: add a leases test with stat open
>
> This test passes against Windows 2016 but currently fails against Samba
> for some reason. The test does the following:
>
> 1. A stat open on a file, then
> 2. a second open with a RWH-lease request
>
> Windows grants a RWH-lease in step 2, while Samba only grants a
> R-lease. Go figure...
>
> Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
>
> Signed-off-by: Ralph Boehme <slow at samba.org>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
> selftest/knownfail | 1 +
> source4/torture/smb2/lease.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 68 insertions(+)
>
> diff --git a/selftest/knownfail b/selftest/knownfail
> index 2cc9c70..286d3c0 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -173,6 +173,7 @@
> ^samba3.smb2.replay.replay3
> ^samba3.smb2.replay.replay4
> ^samba3.smb2.lock.*replay
> +^samba3.smb2.lease.statopen3
> ^samba4.smb2.ioctl.compress_notsup.*\(ad_dc_ntvfs\)
> ^samba3.raw.session.*reauth2 # maybe fix this?
> ^samba3.rpc.lsa.secrets.seal # This gives NT_STATUS_LOCAL_USER_SESSION_KEY
> diff --git a/source4/torture/smb2/lease.c b/source4/torture/smb2/lease.c
> index 10761b8..6212c08 100644
> --- a/source4/torture/smb2/lease.c
> +++ b/source4/torture/smb2/lease.c
> @@ -1075,6 +1075,72 @@ done:
> return ret;
> }
>
> +static bool test_lease_statopen3(struct torture_context *tctx,
> + struct smb2_tree *tree)
> +{
> + TALLOC_CTX *mem_ctx = talloc_new(tctx);
> + struct smb2_create io;
> + struct smb2_lease ls;
> + struct smb2_handle h1 = {{0}};
> + struct smb2_handle h2 = {{0}};
> + NTSTATUS status;
> + const char *fname = "lease_statopen3.dat";
> + bool ret = true;
> + uint32_t caps;
> +
> + caps = smb2cli_conn_server_capabilities(
> + tree->session->transport->conn);
> + if (!(caps & SMB2_CAP_LEASING)) {
> + torture_skip(tctx, "leases are not supported");
> + }
> +
> + smb2_util_unlink(tree, fname);
> + ZERO_STRUCT(break_info);
> + tree->session->transport->lease.handler = torture_lease_handler;
> + tree->session->transport->lease.private_data = tree;
> +
> + status = torture_smb2_testfile(tree, fname, &h1);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + smb2_util_close(tree, h1);
> + ZERO_STRUCT(h1);
> +
> + /* Stat open */
> + ZERO_STRUCT(io);
> + io.in.desired_access = FILE_READ_ATTRIBUTES;
> + io.in.share_access = NTCREATEX_SHARE_ACCESS_MASK;
> + io.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
> + io.in.create_disposition = NTCREATEX_DISP_OPEN;
> + io.in.fname = fname;
> + status = smb2_create(tree, mem_ctx, &io);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + h1 = io.out.file.handle;
> +
> + /* Open file with RWH lease. */
> + smb2_lease_create_share(&io, &ls, false, fname,
> + smb2_util_share_access("RWD"),
> + LEASE1,
> + smb2_util_lease_state("RWH"));
> + io.in.desired_access = SEC_FILE_WRITE_DATA;
> + status = smb2_create(tree, mem_ctx, &io);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2_create failed\n");
> + h2 = io.out.file.handle;
> + CHECK_LEASE(&io, "RWH", true, LEASE1, 0);
> +
> +done:
> + if (!smb2_util_handle_empty(h1)) {
> + smb2_util_close(tree, h1);
> + }
> + if (!smb2_util_handle_empty(h2)) {
> + smb2_util_close(tree, h2);
> + }
> + smb2_util_unlink(tree, fname);
> + talloc_free(mem_ctx);
> + return ret;
> +}
> +
> static void torture_oplock_break_callback(struct smb2_request *req)
> {
> NTSTATUS status;
> @@ -4002,6 +4068,7 @@ struct torture_suite *torture_smb2_lease_init(TALLOC_CTX *ctx)
> test_lease_nobreakself);
> torture_suite_add_1smb2_test(suite, "statopen", test_lease_statopen);
> torture_suite_add_1smb2_test(suite, "statopen2", test_lease_statopen2);
> + torture_suite_add_1smb2_test(suite, "statopen3", test_lease_statopen3);
> torture_suite_add_1smb2_test(suite, "upgrade", test_lease_upgrade);
> torture_suite_add_1smb2_test(suite, "upgrade2", test_lease_upgrade2);
> torture_suite_add_1smb2_test(suite, "upgrade3", test_lease_upgrade3);
> --
> 2.9.3
>
More information about the samba-technical
mailing list