[Patches] g_lock conflict detection broken when processing stale entries (bug #)
Jeremy Allison
jra at samba.org
Wed Dec 20 21:19:35 UTC 2017
On Wed, Dec 20, 2017 at 10:15:53AM +0100, Stefan Metzmacher via samba-technical wrote:
> Hi,
>
> here're patches for https://bugzilla.samba.org/show_bug.cgi?id=13195
>
> Please review and push:-)
Oh, really good catch ! Glad that went in :-).
> From 8fc9defd720e82fd20f7a6421296e900ff5f1476 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Wed, 20 Dec 2017 09:44:40 +0100
> Subject: [PATCH 1/2] torture3: add LOCAL-G-LOCK6 test
>
> This is a regression test for bug #13195.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13195
>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
> selftest/knownfail.d/local-g-lock6 | 1 +
> source3/selftest/tests.py | 1 +
> source3/torture/proto.h | 1 +
> source3/torture/test_g_lock.c | 147 +++++++++++++++++++++++++++++++++++++
> source3/torture/torture.c | 1 +
> 5 files changed, 151 insertions(+)
> create mode 100644 selftest/knownfail.d/local-g-lock6
>
> diff --git a/selftest/knownfail.d/local-g-lock6 b/selftest/knownfail.d/local-g-lock6
> new file mode 100644
> index 0000000..14fd5c8
> --- /dev/null
> +++ b/selftest/knownfail.d/local-g-lock6
> @@ -0,0 +1 @@
> +^samba3.smbtorture_s3.LOCAL-G-LOCK6.smbtorture
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index f8d2a4d..5b8ab79 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -158,6 +158,7 @@ local_tests = [
> "LOCAL-G-LOCK3",
> "LOCAL-G-LOCK4",
> "LOCAL-G-LOCK5",
> + "LOCAL-G-LOCK6",
> "LOCAL-NAMEMAP-CACHE1",
> "LOCAL-hex_encode_buf",
> "LOCAL-remove_duplicate_addrs2"]
> diff --git a/source3/torture/proto.h b/source3/torture/proto.h
> index 12c76a7..7ab2fe1 100644
> --- a/source3/torture/proto.h
> +++ b/source3/torture/proto.h
> @@ -132,6 +132,7 @@ bool run_g_lock2(int dummy);
> bool run_g_lock3(int dummy);
> bool run_g_lock4(int dummy);
> bool run_g_lock5(int dummy);
> +bool run_g_lock6(int dummy);
> bool run_g_lock_ping_pong(int dummy);
> bool run_local_namemap_cache1(int dummy);
>
> diff --git a/source3/torture/test_g_lock.c b/source3/torture/test_g_lock.c
> index 253d20c..7d11471 100644
> --- a/source3/torture/test_g_lock.c
> +++ b/source3/torture/test_g_lock.c
> @@ -695,6 +695,153 @@ bool run_g_lock5(int dummy)
> return true;
> }
>
> +struct lock6_parser_state {
> + size_t num_locks;
> +};
> +
> +static void lock6_parser(const struct g_lock_rec *locks,
> + size_t num_locks,
> + const uint8_t *data,
> + size_t datalen,
> + void *private_data)
> +{
> + struct lock6_parser_state *state = private_data;
> + state->num_locks = num_locks;
> +}
> +
> +/*
> + * Test cleanup with contention and stale locks
> + */
> +
> +bool run_g_lock6(int dummy)
> +{
> + struct tevent_context *ev = NULL;
> + struct messaging_context *msg = NULL;
> + struct g_lock_ctx *ctx = NULL;
> + const char *lockname = "lock6";
> + pid_t child;
> + int exit_pipe[2], ready_pipe[2];
> + NTSTATUS status;
> + size_t i, nprocs;
> + int ret;
> + bool ok;
> + ssize_t nread;
> + char c;
> +
> + if ((pipe(exit_pipe) != 0) || (pipe(ready_pipe) != 0)) {
> + perror("pipe failed");
> + return false;
> + }
> +
> + ok = get_g_lock_ctx(talloc_tos(), &ev, &msg, &ctx);
> + if (!ok) {
> + fprintf(stderr, "get_g_lock_ctx failed");
> + return false;
> + }
> +
> + nprocs = 2;
> + for (i=0; i<nprocs; i++) {
> +
> + child = fork();
> +
> + if (child == -1) {
> + perror("fork failed");
> + return false;
> + }
> +
> + if (child == 0) {
> + TALLOC_FREE(ctx);
> +
> + status = reinit_after_fork(msg, ev, false, "");
> +
> + close(ready_pipe[0]);
> + close(exit_pipe[1]);
> +
> + ok = get_g_lock_ctx(talloc_tos(), &ev, &msg, &ctx);
> + if (!ok) {
> + fprintf(stderr, "get_g_lock_ctx failed");
> + exit(1);
> + }
> + status = g_lock_lock(ctx, lockname, G_LOCK_READ,
> + (struct timeval) { .tv_sec = 1 });
> + if (!NT_STATUS_IS_OK(status)) {
> + fprintf(stderr,
> + "child g_lock_lock failed %s\n",
> + nt_errstr(status));
> + exit(1);
> + }
> + if (i == 0) {
> + exit(0);
> + }
> + close(ready_pipe[1]);
> + nread = sys_read(exit_pipe[0], &c, sizeof(c));
> + if (nread != 0) {
> + fprintf(stderr, "sys_read returned %zu (%s)\n",
> + nread, strerror(errno));
> + exit(1);
> + }
> + exit(0);
> + }
> + }
> +
> + close(ready_pipe[1]);
> +
> + nread = sys_read(ready_pipe[0], &c, sizeof(c));
> + if (nread != 0) {
> + fprintf(stderr, "sys_read returned %zu (%s)\n",
> + nread, strerror(errno));
> + return false;
> + }
> +
> + {
> + int child_status;
> + ret = waitpid(-1, &child_status, 0);
> + if (ret == -1) {
> + perror("waitpid failed");
> + return false;
> + }
> + }
> +
> + {
> + struct lock6_parser_state state;
> +
> + status = g_lock_dump(ctx, lockname, lock6_parser, &state);
> + if (!NT_STATUS_IS_OK(status)) {
> + fprintf(stderr, "g_lock_dump returned %s\n",
> + nt_errstr(status));
> + return false;
> + }
> +
> + if (state.num_locks != nprocs) {
> + fprintf(stderr, "nlocks=%zu, expected %zu\n",
> + state.num_locks, nprocs);
> + return false;
> + }
> +
> + status = g_lock_lock(ctx, lockname, G_LOCK_WRITE,
> + (struct timeval) { .tv_sec = 1 });
> + if (!NT_STATUS_EQUAL(status, NT_STATUS_IO_TIMEOUT)) {
> + fprintf(stderr, "g_lock_lock should have failed with %s - %s\n",
> + nt_errstr(NT_STATUS_IO_TIMEOUT),
> + nt_errstr(status));
> + return false;
> + }
> + }
> +
> + close(exit_pipe[1]);
> +
> + {
> + int child_status;
> + ret = waitpid(-1, &child_status, 0);
> + if (ret == -1) {
> + perror("waitpid failed");
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> extern int torture_numops;
> extern int torture_nprocs;
>
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 232e8ff..1a9cf5b 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -11585,6 +11585,7 @@ static struct {
> { "LOCAL-G-LOCK3", run_g_lock3, 0 },
> { "LOCAL-G-LOCK4", run_g_lock4, 0 },
> { "LOCAL-G-LOCK5", run_g_lock5, 0 },
> + { "LOCAL-G-LOCK6", run_g_lock6, 0 },
> { "LOCAL-G-LOCK-PING-PONG", run_g_lock_ping_pong, 0 },
> { "LOCAL-CANONICALIZE-PATH", run_local_canonicalize_path, 0 },
> { "LOCAL-NAMEMAP-CACHE1", run_local_namemap_cache1, 0 },
> --
> 1.9.1
>
>
> From af59fdb8ddbe58a58b9db0b6624741763ac41e64 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Wed, 20 Dec 2017 08:25:19 +0100
> Subject: [PATCH 2/2] g_lock: fix cleanup of stale entries in g_lock_trylock()
>
> g_lock_trylock() always incremented the counter 'i', even after cleaning a stale
> entry at position 'i', which means it skipped checking for a conflict against
> the new entry at position 'i'.
>
> As result a process could get a write lock, while there're still
> some read lock holders. Once we get into that problem, also more than
> one write lock are possible.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13195
>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
> selftest/knownfail.d/local-g-lock6 | 1 -
> source3/lib/g_lock.c | 4 +++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
> delete mode 100644 selftest/knownfail.d/local-g-lock6
>
> diff --git a/selftest/knownfail.d/local-g-lock6 b/selftest/knownfail.d/local-g-lock6
> deleted file mode 100644
> index 14fd5c8..0000000
> --- a/selftest/knownfail.d/local-g-lock6
> +++ /dev/null
> @@ -1 +0,0 @@
> -^samba3.smbtorture_s3.LOCAL-G-LOCK6.smbtorture
> diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
> index 50ea566..bd63dad 100644
> --- a/source3/lib/g_lock.c
> +++ b/source3/lib/g_lock.c
> @@ -230,7 +230,7 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
> }
> }
>
> - for (i=0; i<lck.num_recs; i++) {
> + for (i=0; i<lck.num_recs;) {
> struct g_lock_rec lock;
>
> g_lock_get_rec(&lck, i, &lock);
> @@ -269,7 +269,9 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
> */
> g_lock_rec_del(&lck, i);
> modified = true;
> + continue;
> }
> + i++;
> }
>
> modified = true;
> --
> 1.9.1
>
More information about the samba-technical
mailing list