Attachment 16141 Details for Bug 14428 – Patches for v4-12-test

[patch] Patches for v4-12-test

tmp412.diff.txt (text/plain), 12.57 KB, created by Stefan Metzmacher on 2020-07-28 07:18:19 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stefan Metzmacher

Created: 2020-07-28 07:18:19 UTC

Size: 12.57 KB

patch

obsolete

>From e2fa20763af2f132e3f921bd7b1a8160ab6dbdf6 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Thu, 2 Jul 2020 14:09:15 +0200
>Subject: [PATCH 1/7] smbd: increase loglevel when leases_db_del() with
> anything then NT_STATUS_NOT_FOUND
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14428
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(cherry picked from commit fbb8bbe1243eb2a0351dc2422929278f85a99e26)
>---
> source3/locking/locking.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/source3/locking/locking.c b/source3/locking/locking.c
>index 1220cb3a2be1..2d9569809e44 100644
>--- a/source3/locking/locking.c
>+++ b/source3/locking/locking.c
>@@ -728,8 +728,13 @@ NTSTATUS remove_lease_if_stale(struct share_mode_lock *lck,
> 
> 	status = leases_db_del(client_guid, lease_key, &d->id);
> 	if (!NT_STATUS_IS_OK(status)) {
>-		DBG_DEBUG("leases_db_del failed: %s\n",
>-			  nt_errstr(status));
>+		int level = DBGLVL_DEBUG;
>+
>+		if (!NT_STATUS_EQUAL(status, NT_STATUS_NOT_FOUND)) {
>+			level = DBGLVL_ERR;
>+		}
>+		DBG_PREFIX(level, ("leases_db_del failed: %s\n",
>+			   nt_errstr(status)));
> 	}
> 	return status;
> }
>-- 
>2.17.1
>
>
>From dd816924ac282605d8d37b95d60f8691f54b8362 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Thu, 2 Jul 2020 14:10:05 +0200
>Subject: [PATCH 2/7] s3/leases: log NDR decoding failure with level 0 in
> leases_db_get_fn()
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14428
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>(cherry picked from commit 383a2457bd6cbe0acd571a8d601f8bdc5365f0b4)
>---
> source3/locking/leases_db.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/source3/locking/leases_db.c b/source3/locking/leases_db.c
>index a12b421d2603..2e2ccb150ac3 100644
>--- a/source3/locking/leases_db.c
>+++ b/source3/locking/leases_db.c
>@@ -549,8 +549,8 @@ static void leases_db_get_fn(TDB_DATA key, TDB_DATA data, void *private_data)
> 		&blob, value, value,
> 		(ndr_pull_flags_fn_t)ndr_pull_leases_db_value);
> 	if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
>-		DBG_DEBUG("ndr_pull_struct_blob_failed: %s\n",
>-			  ndr_errstr(ndr_err));
>+		DBG_ERR("ndr_pull_struct_blob_failed: %s\n",
>+			ndr_errstr(ndr_err));
> 		TALLOC_FREE(value);
> 		state->status = ndr_map_error2ntstatus(ndr_err);
> 		return;
>-- 
>2.17.1
>
>
>From 0c23e5489e9783299e51062a75cfe150a2a3d084 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Thu, 2 Jul 2020 14:08:44 +0200
>Subject: [PATCH 3/7] smbd: inverse if/else logic in get_lease_type()
>
>No change in behaviour.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14428
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(cherry picked from commit e4328db1c94837a8ea5652971cea20055d3d24ff)
>---
> source3/smbd/oplock.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
>diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
>index 2c4449b10b26..c63f97aac0ad 100644
>--- a/source3/smbd/oplock.c
>+++ b/source3/smbd/oplock.c
>@@ -171,24 +171,24 @@ static void downgrade_file_oplock(files_struct *fsp)
> 
> uint32_t get_lease_type(const struct share_mode_entry *e, struct file_id id)
> {
>-	if (e->op_type == LEASE_OPLOCK) {
>-		NTSTATUS status;
>-		uint32_t current_state;
>+	NTSTATUS status;
>+	uint32_t current_state;
> 
>-		status = leases_db_get(
>-			&e->client_guid,
>-			&e->lease_key,
>-			&id,
>-			&current_state,
>-			NULL,	/* breaking */
>-			NULL,	/* breaking_to_requested */
>-			NULL,	/* breaking_to_required */
>-			NULL,	/* lease_version */
>-			NULL);	/* epoch */
>-		SMB_ASSERT(NT_STATUS_IS_OK(status));
>-		return current_state;
>-	}
>-	return map_oplock_to_lease_type(e->op_type);
>+	if (e->op_type != LEASE_OPLOCK) {
>+		return map_oplock_to_lease_type(e->op_type);
>+	}
>+
>+	status = leases_db_get(&e->client_guid,
>+			       &e->lease_key,
>+			       &id,
>+			       &current_state,
>+			       NULL,	/* breaking */
>+			       NULL,	/* breaking_to_requested */
>+			       NULL,	/* breaking_to_required */
>+			       NULL,	/* lease_version */
>+			       NULL);	/* epoch */
>+	SMB_ASSERT(NT_STATUS_IS_OK(status));
>+	return current_state;
> }
> 
> /****************************************************************************
>-- 
>2.17.1
>
>
>From 39394ffea15f0fa3d49e85f984473d7033ab7e28 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Thu, 2 Jul 2020 14:45:59 +0200
>Subject: [PATCH 4/7] smbd: let get_lease_type() take a non-const
> share_mode_entry
>
>We're going to add a call to share_entry_stale_pid(share_mode_entry) which takes
>a non-const pointer (in order to eventually set e->state = true).
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14428
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(cherry picked from commit 3f4a865821da27efbed4f7c38ad3efbcaae77a02)
>---
> source3/smbd/oplock.c | 2 +-
> source3/smbd/proto.h  | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
>index c63f97aac0ad..97ab05b2ba67 100644
>--- a/source3/smbd/oplock.c
>+++ b/source3/smbd/oplock.c
>@@ -169,7 +169,7 @@ static void downgrade_file_oplock(files_struct *fsp)
> 	TALLOC_FREE(fsp->oplock_timeout);
> }
> 
>-uint32_t get_lease_type(const struct share_mode_entry *e, struct file_id id)
>+uint32_t get_lease_type(struct share_mode_entry *e, struct file_id id)
> {
> 	NTSTATUS status;
> 	uint32_t current_state;
>diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
>index 60941ce6c1b2..ac021ad93fe1 100644
>--- a/source3/smbd/proto.h
>+++ b/source3/smbd/proto.h
>@@ -736,7 +736,7 @@ NTSTATUS create_file_default(connection_struct *conn,
> 
> /* The following definitions come from smbd/oplock.c  */
> 
>-uint32_t get_lease_type(const struct share_mode_entry *e, struct file_id id);
>+uint32_t get_lease_type(struct share_mode_entry *e, struct file_id id);
> 
> void break_kernel_oplock(struct messaging_context *msg_ctx, files_struct *fsp);
> NTSTATUS set_file_oplock(files_struct *fsp);
>-- 
>2.17.1
>
>
>From abb4a240e9a11ef39cbbccb1cd04793caec71c4a Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Thu, 2 Jul 2020 14:47:12 +0200
>Subject: [PATCH 5/7] smbd: check for stale pid in get_lease_type()
>
>If leases_db_get() failed the leases_db record might have been cleaned up for
>stale processes. Check if the share-mode-entry owner is stale in this case and
>return a 0 lease state. In any other case, log a debug messages and panic.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14428
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>
>Autobuild-User(master): Stefan Metzmacher <metze@samba.org>
>Autobuild-Date(master): Thu Jul  2 16:45:42 UTC 2020 on sn-devel-184
>
>(cherry picked from commit 05d4466a6d1ad048fa86aea09ec0a56a7b961369)
>---
> source3/smbd/oplock.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
>diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
>index 97ab05b2ba67..1c1510f3aabc 100644
>--- a/source3/smbd/oplock.c
>+++ b/source3/smbd/oplock.c
>@@ -171,6 +171,8 @@ static void downgrade_file_oplock(files_struct *fsp)
> 
> uint32_t get_lease_type(struct share_mode_entry *e, struct file_id id)
> {
>+	struct GUID_txt_buf guid_strbuf;
>+	struct file_id_buf file_id_strbuf;
> 	NTSTATUS status;
> 	uint32_t current_state;
> 
>@@ -187,8 +189,22 @@ uint32_t get_lease_type(struct share_mode_entry *e, struct file_id id)
> 			       NULL,	/* breaking_to_required */
> 			       NULL,	/* lease_version */
> 			       NULL);	/* epoch */
>-	SMB_ASSERT(NT_STATUS_IS_OK(status));
>-	return current_state;
>+	if (NT_STATUS_IS_OK(status)) {
>+		return current_state;
>+	}
>+
>+	if (share_entry_stale_pid(e)) {
>+		return 0;
>+	}
>+	DBG_ERR("leases_db_get for client_guid [%s] "
>+		"lease_key [%"PRIu64"/%"PRIu64"] "
>+		"file_id [%s] failed: %s\n",
>+		GUID_buf_string(&e->client_guid, &guid_strbuf),
>+		e->lease_key.data[0],
>+		e->lease_key.data[1],
>+		file_id_str_buf(id, &file_id_strbuf),
>+		nt_errstr(status));
>+	smb_panic("leases_db_get() failed");
> }
> 
> /****************************************************************************
>-- 
>2.17.1
>
>
>From 14358848397e52f5ae6fba2ec63d5e78da41de52 Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Mon, 6 Jul 2020 14:03:39 +0200
>Subject: [PATCH 6/7] s3:leases: log errors with level 0 in
> leases_db_do_locked_fn()
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14428
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>(cherry picked from commit 217693682d5bbd0f2d6b5331f47b2a6348840898)
>---
> source3/locking/leases_db.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/source3/locking/leases_db.c b/source3/locking/leases_db.c
>index 2e2ccb150ac3..855d6143ad71 100644
>--- a/source3/locking/leases_db.c
>+++ b/source3/locking/leases_db.c
>@@ -121,7 +121,7 @@ static void leases_db_do_locked_fn(
> 			value,
> 			(ndr_pull_flags_fn_t)ndr_pull_leases_db_value);
> 		if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
>-			DBG_DEBUG("ndr_pull_struct_blob_failed: %s\n",
>+			DBG_ERR("ndr_pull_struct_blob_failed: %s\n",
> 				  ndr_errstr(ndr_err));
> 			state->status = ndr_map_error2ntstatus(ndr_err);
> 			goto done;
>@@ -137,7 +137,7 @@ static void leases_db_do_locked_fn(
> 	if (value->num_files == 0) {
> 		state->status = dbwrap_record_delete(rec);
> 		if (!NT_STATUS_IS_OK(state->status)) {
>-			DBG_DEBUG("dbwrap_record_delete returned %s\n",
>+			DBG_ERR("dbwrap_record_delete returned %s\n",
> 				  nt_errstr(state->status));
> 		}
> 		goto done;
>@@ -149,7 +149,7 @@ static void leases_db_do_locked_fn(
> 		value,
> 		(ndr_push_flags_fn_t)ndr_push_leases_db_value);
> 	if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
>-		DBG_DEBUG("ndr_push_struct_blob_failed: %s\n",
>+		DBG_ERR("ndr_push_struct_blob_failed: %s\n",
> 			  ndr_errstr(ndr_err));
> 		state->status = ndr_map_error2ntstatus(ndr_err);
> 		goto done;
>@@ -164,7 +164,7 @@ static void leases_db_do_locked_fn(
> 
> 	state->status = dbwrap_record_store(rec, db_value, 0);
> 	if (!NT_STATUS_IS_OK(state->status)) {
>-		DBG_DEBUG("dbwrap_record_store returned %s\n",
>+		DBG_ERR("dbwrap_record_store returned %s\n",
> 			  nt_errstr(state->status));
> 	}
> 
>-- 
>2.17.1
>
>
>From 9cabdfaa07177aad2025b89a49f4d9ae92906a6e Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>Date: Mon, 6 Jul 2020 08:58:22 +0200
>Subject: [PATCH 7/7] s3:smbd: check for stale pid in delay_for_oplock_fn()
> when leases_db_get() fails
>
>If leases_db_get() failed the leases_db record might have been cleaned up for
>stale processes. Check if the share-mode-entry owner is stale in this case and
>return ignore the entry. In any other case, log a debug messages and panic.
>
>Commit 05d4466a6d1ad048fa86aea09ec0a56a7b961369
>"smbd: check for stale pid in get_lease_type()" fixed only one half of
>this.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14428
>
>Signed-off-by: Stefan Metzmacher <metze@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>
>Autobuild-User(master): Jeremy Allison <jra@samba.org>
>Autobuild-Date(master): Tue Jul  7 02:47:46 UTC 2020 on sn-devel-184
>
>(cherry picked from commit 58adf349edfd3001ad071cc7ed8cfc551f67f8a2)
>---
> source3/smbd/open.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
>diff --git a/source3/smbd/open.c b/source3/smbd/open.c
>index 03e66d30a3e1..4a31b8226463 100644
>--- a/source3/smbd/open.c
>+++ b/source3/smbd/open.c
>@@ -2380,7 +2380,42 @@ static bool delay_for_oplock_fn(
> 			NULL, /* breaking_to_required */
> 			NULL, /* lease_version */
> 			NULL); /* epoch */
>-		SMB_ASSERT(NT_STATUS_IS_OK(status));
>+
>+		/*
>+		 * leases_db_get() can return NT_STATUS_NOT_FOUND
>+		 * if the share_mode_entry e is stale and the
>+		 * lease record was already removed. In this case return
>+		 * false so the traverse continues.
>+		 */
>+
>+		if (NT_STATUS_EQUAL(status, NT_STATUS_NOT_FOUND) &&
>+		    share_entry_stale_pid(e))
>+		{
>+			struct GUID_txt_buf guid_strbuf;
>+			struct file_id_buf file_id_strbuf;
>+			DBG_DEBUG("leases_db_get for client_guid [%s] "
>+				  "lease_key [%"PRIu64"/%"PRIu64"] "
>+				  "file_id [%s] failed for stale "
>+				  "share_mode_entry\n",
>+				  GUID_buf_string(&e->client_guid, &guid_strbuf),
>+				  e->lease_key.data[0],
>+				  e->lease_key.data[1],
>+				  file_id_str_buf(fsp->file_id, &file_id_strbuf));
>+			return false;
>+		}
>+		if (!NT_STATUS_IS_OK(status)) {
>+			struct GUID_txt_buf guid_strbuf;
>+			struct file_id_buf file_id_strbuf;
>+			DBG_ERR("leases_db_get for client_guid [%s] "
>+				"lease_key [%"PRIu64"/%"PRIu64"] "
>+				"file_id [%s] failed: %s\n",
>+				GUID_buf_string(&e->client_guid, &guid_strbuf),
>+				e->lease_key.data[0],
>+				e->lease_key.data[1],
>+				file_id_str_buf(fsp->file_id, &file_id_strbuf),
>+				nt_errstr(status));
>+			smb_panic("leases_db_get() failed");
>+		}
> 	}
> 
> 	if (!state->got_handle_lease &&
>-- 
>2.17.1
>

Flags:

slow: review+
metze: ci-passed+

Actions: View

Attachments on bug 14428: 16113 | 16114 | 16141 | 16144 | 16190