[PATCH] Fix a bug introduced with serverid.tdb removal
Jeremy Allison
jra at samba.org
Tue Dec 12 18:41:25 UTC 2017
On Tue, Dec 12, 2017 at 09:05:43AM +0100, Volker Lendecke via samba-technical wrote:
> Hi!
>
> cleanupd would spin 100%. Attached find a fix.
>
> Review appreciated!
Oh, good catch Volker. I'm sorry for not seeing this in the initial
review, but this code is somewhat subtle. Thanks for the additional
test !
Reviewed-by: Jeremy Allison <jra at samba.org>
Pushed.
> --
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de
> From b52cd24cc5dbabc186f7b3ca80a0648ab339eb7e Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 8 Dec 2017 17:18:33 +0100
> Subject: [PATCH 1/3] messaging: Don't do self-sends in messaging_send_all
>
> This leads to cleanupd doing endless MSG_SMB_UNLOCK calls, as it triggers
> itself in the send_all. This worked correctly before the serverid.tdb removal
> because cleanupd did not register in serverid.tdb (which was a bug, but it
> helped us there).
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/messages.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/source3/lib/messages.c b/source3/lib/messages.c
> index a0a3f9fb1ba..561616df6e4 100644
> --- a/source3/lib/messages.c
> +++ b/source3/lib/messages.c
> @@ -857,6 +857,11 @@ static int send_all_fn(pid_t pid, void *private_data)
> struct send_all_state *state = private_data;
> NTSTATUS status;
>
> + if (pid == getpid()) {
> + DBG_DEBUG("Skip ourselves in messaging_send_all\n");
> + return 0;
> + }
> +
> status = messaging_send_buf(state->msg_ctx, pid_to_procid(pid),
> state->msg_type, state->buf, state->len);
> if (!NT_STATUS_IS_OK(status)) {
> --
> 2.11.0
>
>
> From b02608487499cd8e287135405492d8573d3f58b1 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 8 Dec 2017 17:21:37 +0100
> Subject: [PATCH 2/3] messaging: Ignore messages from ourselves
>
> For non-clustered messaging this should have never gone through the socket, we
> should have caught it before in messaging_send_iov_from.
>
> It can come in on a socket from ctdb when broadcasting in clustered mode. There
> ctdb does the broadcasting.
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/lib/messages.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/source3/lib/messages.c b/source3/lib/messages.c
> index 561616df6e4..464233fda2c 100644
> --- a/source3/lib/messages.c
> +++ b/source3/lib/messages.c
> @@ -399,6 +399,11 @@ static void messaging_recv_cb(struct tevent_context *ev,
> (unsigned)rec.msg_type, rec.buf.length, num_fds,
> server_id_str_buf(rec.src, &idbuf));
>
> + if (server_id_same_process(&rec.src, &msg_ctx->id)) {
> + DBG_DEBUG("Ignoring self-send\n");
> + goto close_fail;
> + }
> +
> messaging_dispatch_rec(msg_ctx, ev, &rec);
> return;
>
> --
> 2.11.0
>
>
> From 19afd6a3bad8c44704bf0f9b5a82467324cd47fb Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 11 Dec 2017 15:58:26 +0100
> Subject: [PATCH 3/3] torture: Check messaging_send_all
>
> We must make sure not to receive our own broadcast
>
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
> source3/selftest/tests.py | 1 +
> source3/torture/proto.h | 1 +
> source3/torture/test_messaging_send_all.c | 279 ++++++++++++++++++++++++++++++
> source3/torture/torture.c | 1 +
> source3/wscript_build | 1 +
> 5 files changed, 283 insertions(+)
> create mode 100644 source3/torture/test_messaging_send_all.c
>
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index 1b3576827a3..f8d2a4de9ba 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -147,6 +147,7 @@ local_tests = [
> "LOCAL-MESSAGING-FDPASS2",
> "LOCAL-MESSAGING-FDPASS2a",
> "LOCAL-MESSAGING-FDPASS2b",
> + "LOCAL-MESSAGING-SEND-ALL",
> "LOCAL-PTHREADPOOL-TEVENT",
> "LOCAL-CANONICALIZE-PATH",
> "LOCAL-DBWRAP-WATCH1",
> diff --git a/source3/torture/proto.h b/source3/torture/proto.h
> index 83e0c7494d8..12c76a72d62 100644
> --- a/source3/torture/proto.h
> +++ b/source3/torture/proto.h
> @@ -124,6 +124,7 @@ bool run_messaging_fdpass1(int dummy);
> bool run_messaging_fdpass2(int dummy);
> bool run_messaging_fdpass2a(int dummy);
> bool run_messaging_fdpass2b(int dummy);
> +bool run_messaging_send_all(int dummy);
> bool run_oplock_cancel(int dummy);
> bool run_pthreadpool_tevent(int dummy);
> bool run_g_lock1(int dummy);
> diff --git a/source3/torture/test_messaging_send_all.c b/source3/torture/test_messaging_send_all.c
> new file mode 100644
> index 00000000000..8512fa4bffd
> --- /dev/null
> +++ b/source3/torture/test_messaging_send_all.c
> @@ -0,0 +1,279 @@
> +/*
> + * Unix SMB/CIFS implementation.
> + * Test for a messaging_send_all bug
> + * Copyright (C) Volker Lendecke 2017
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "includes.h"
> +#include "torture/proto.h"
> +#include "lib/util/tevent_unix.h"
> +#include "messages.h"
> +#include "lib/async_req/async_sock.h"
> +#include "lib/util/sys_rw.h"
> +
> +static pid_t fork_responder(struct messaging_context *msg_ctx,
> + int exit_pipe[2])
> +{
> + struct tevent_context *ev = messaging_tevent_context(msg_ctx);
> + struct tevent_req *req;
> + pid_t child_pid;
> + int ready_pipe[2];
> + char c = 0;
> + bool ok;
> + int ret, err;
> + NTSTATUS status;
> + ssize_t nwritten;
> +
> + ret = pipe(ready_pipe);
> + if (ret == -1) {
> + perror("pipe failed");
> + return -1;
> + }
> +
> + child_pid = fork();
> + if (child_pid == -1) {
> + perror("fork failed");
> + close(ready_pipe[0]);
> + close(ready_pipe[1]);
> + return -1;
> + }
> +
> + if (child_pid != 0) {
> + ssize_t nread;
> + close(ready_pipe[1]);
> + nread = read(ready_pipe[0], &c, 1);
> + close(ready_pipe[0]);
> + if (nread != 1) {
> + perror("read failed");
> + return -1;
> + }
> + return child_pid;
> + }
> +
> + close(ready_pipe[0]);
> + close(exit_pipe[1]);
> +
> + status = messaging_reinit(msg_ctx);
> + if (!NT_STATUS_IS_OK(status)) {
> + fprintf(stderr, "messaging_reinit failed: %s\n",
> + nt_errstr(status));
> + close(ready_pipe[1]);
> + exit(1);
> + }
> +
> + nwritten = sys_write(ready_pipe[1], &c, 1);
> + if (nwritten != 1) {
> + fprintf(stderr, "write failed: %s\n", strerror(errno));
> + exit(1);
> + }
> +
> + close(ready_pipe[1]);
> +
> + req = wait_for_read_send(ev, ev, exit_pipe[0], false);
> + if (req == NULL) {
> + fprintf(stderr, "wait_for_read_send failed\n");
> + exit(1);
> + }
> +
> + ok = tevent_req_poll_unix(req, ev, &err);
> + if (!ok) {
> + fprintf(stderr, "tevent_req_poll_unix failed: %s\n",
> + strerror(err));
> + exit(1);
> + }
> +
> + exit(0);
> +}
> +
> +struct messaging_send_all_state {
> + struct tevent_context *ev;
> + struct messaging_context *msg;
> + pid_t *senders;
> + size_t num_received;
> +};
> +
> +static void collect_pong_received(struct tevent_req *subreq);
> +
> +static struct tevent_req *collect_pong_send(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev,
> + struct messaging_context *msg,
> + const pid_t *senders,
> + size_t num_senders)
> +{
> + struct tevent_req *req, *subreq;
> + struct messaging_send_all_state *state;
> +
> + req = tevent_req_create(mem_ctx, &state,
> + struct messaging_send_all_state);
> + if (req == NULL) {
> + return NULL;
> + }
> + state->senders = talloc_memdup(
> + state, senders, num_senders * sizeof(pid_t));
> + if (tevent_req_nomem(state->senders, req)) {
> + return tevent_req_post(req, ev);
> + }
> + state->ev = ev;
> + state->msg = msg;
> +
> + subreq = messaging_read_send(state, state->ev, state->msg, MSG_PONG);
> + if (tevent_req_nomem(subreq, req)) {
> + return tevent_req_post(req, ev);
> + }
> + tevent_req_set_callback(subreq, collect_pong_received, req);
> + return req;
> +}
> +
> +static void collect_pong_received(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + struct messaging_send_all_state *state = tevent_req_data(
> + req, struct messaging_send_all_state);
> + size_t num_senders = talloc_array_length(state->senders);
> + size_t i;
> + struct messaging_rec *rec;
> + int ret;
> +
> + ret = messaging_read_recv(subreq, state, &rec);
> + TALLOC_FREE(subreq);
> + if (tevent_req_error(req, ret)) {
> + return;
> + }
> +
> + /*
> + * We need to make sure we don't receive our own broadcast!
> + */
> +
> + if (rec->src.pid == (uint64_t)getpid()) {
> + fprintf(stderr, "Received my own broadcast!\n");
> + tevent_req_error(req, EMULTIHOP);
> + return;
> + }
> +
> + for (i=0; i<num_senders; i++) {
> + if (state->senders[i] == (pid_t)rec->src.pid) {
> + printf("got message from %"PRIu64"\n", rec->src.pid);
> + state->senders[i] = 0;
> + state->num_received += 1;
> + break;
> + }
> + }
> +
> + if (state->num_received == num_senders) {
> + printf("done\n");
> + tevent_req_done(req);
> + return;
> + }
> +
> + subreq = messaging_read_send(state, state->ev, state->msg, MSG_PONG);
> + if (tevent_req_nomem(subreq, req)) {
> + return;
> + }
> + tevent_req_set_callback(subreq, collect_pong_received, req);
> +}
> +
> +static int collect_pong_recv(struct tevent_req *req)
> +{
> + return tevent_req_simple_recv_unix(req);
> +}
> +
> +extern int torture_nprocs;
> +
> +bool run_messaging_send_all(int dummy)
> +{
> + struct tevent_context *ev = NULL;
> + struct messaging_context *msg_ctx = NULL;
> + int exit_pipe[2];
> + pid_t children[MAX(5, torture_nprocs)];
> + struct tevent_req *req;
> + size_t i;
> + bool ok;
> + int ret, err;
> +
> + ev = samba_tevent_context_init(talloc_tos());
> + if (ev == NULL) {
> + fprintf(stderr, "tevent_context_init failed\n");
> + return false;
> + }
> + msg_ctx = messaging_init(ev, ev);
> + if (msg_ctx == NULL) {
> + fprintf(stderr, "messaging_init failed\n");
> + return false;
> + }
> + ret = pipe(exit_pipe);
> + if (ret != 0) {
> + perror("parent: pipe failed for exit_pipe");
> + return false;
> + }
> +
> + for (i=0; i<ARRAY_SIZE(children); i++) {
> + children[i] = fork_responder(msg_ctx, exit_pipe);
> + if (children[i] == -1) {
> + fprintf(stderr, "fork_responder(%zu) failed\n", i);
> + return false;
> + }
> + }
> +
> + req = collect_pong_send(ev, ev, msg_ctx, children,
> + ARRAY_SIZE(children));
> + if (req == NULL) {
> + perror("collect_pong failed");
> + return false;
> + }
> +
> + ok = tevent_req_set_endtime(req, ev,
> + tevent_timeval_current_ofs(10, 0));
> + if (!ok) {
> + perror("tevent_req_set_endtime failed");
> + return false;
> + }
> +
> + messaging_send_all(msg_ctx, MSG_PING, NULL, 0);
> +
> + ok = tevent_req_poll_unix(req, ev, &err);
> + if (!ok) {
> + perror("tevent_req_poll_unix failed");
> + return false;
> + }
> +
> + ret = collect_pong_recv(req);
> + TALLOC_FREE(req);
> +
> + if (ret != 0) {
> + fprintf(stderr, "collect_pong_send returned %s\n",
> + strerror(ret));
> + return false;
> + }
> +
> + close(exit_pipe[1]);
> +
> + for (i=0; i<ARRAY_SIZE(children); i++) {
> + pid_t child;
> + int status;
> +
> + do {
> + child = waitpid(children[i], &status, 0);
> + } while ((child == -1) && (errno == EINTR));
> +
> + if (child != children[i]) {
> + printf("waitpid(%d) failed\n", children[i]);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 87276c09cb3..232e8ff2bbb 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -11559,6 +11559,7 @@ static struct {
> { "LOCAL-MESSAGING-FDPASS2", run_messaging_fdpass2, 0 },
> { "LOCAL-MESSAGING-FDPASS2a", run_messaging_fdpass2a, 0 },
> { "LOCAL-MESSAGING-FDPASS2b", run_messaging_fdpass2b, 0 },
> + { "LOCAL-MESSAGING-SEND-ALL", run_messaging_send_all, 0 },
> { "LOCAL-BASE64", run_local_base64, 0},
> { "LOCAL-RBTREE", run_local_rbtree, 0},
> { "LOCAL-MEMCACHE", run_local_memcache, 0},
> diff --git a/source3/wscript_build b/source3/wscript_build
> index 8b46caee02c..d4bc3cb1700 100644
> --- a/source3/wscript_build
> +++ b/source3/wscript_build
> @@ -1169,6 +1169,7 @@ bld.SAMBA3_BINARY('smbtorture' + bld.env.suffix3,
> torture/test_buffersize.c
> torture/test_messaging_read.c
> torture/test_messaging_fd_passing.c
> + torture/test_messaging_send_all.c
> torture/test_oplock_cancel.c
> torture/test_pthreadpool_tevent.c
> torture/bench_pthreadpool.c
> --
> 2.11.0
>
More information about the samba-technical
mailing list