[Tarantool-patches] [PATCH v7 0/2] fiber: Handle stack madvise/mprotect errors

[Tarantool-patches] [PATCH v7 0/2] fiber: Handle stack madvise/mprotect errors

[Cyrill Gorcunov]

Changes in v7:
 - address Vlad's comments
 - add test for slab leak when mprotect fails on exit path

Still the beautifying of errinj is send as a separate patch
so diff is pretty big for now.

Cyrill Gorcunov (2):
  fiber: set diagnostics at madvise/mprotect failure
  fiber: leak slab if unable to bring prots back

 src/lib/core/errinj.h        |   2 +
 src/lib/core/fiber.c         |  93 +++++++++++++++++++-----
 test/box/errinj.result       | 136 ++++++++++++++++++-----------------
 test/unit/CMakeLists.txt     |   4 ++
 test/unit/fiber_stack.c      | 102 ++++++++++++++++++++++++++
 test/unit/fiber_stack.result |  10 +++
 test/unit/suite.ini          |   2 +-
 7 files changed, 264 insertions(+), 85 deletions(-)
 create mode 100644 test/unit/fiber_stack.c
 create mode 100644 test/unit/fiber_stack.result

-- 
2.20.1

[Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Cyrill Gorcunov]

Both madvise and mprotect calls can fail due to various
reasons, mostly because of lack of free memory in the
system.

We log such cases via say_x helpers but this is not enough.
In particular tarantool/memcached relies on diag error to be
set to detect an error condition:

 | expire_fiber = fiber_new(name, memcached_expire_loop);
 | const box_error_t *err = box_error_last();
 | if (err) {
 |	say_error("Can't start the expire fiber");
 |	say_error("%s", box_error_message(err));
 |	return -1;
 | }

Thus lets use diag_set() helper here and instead of macros
use inline functions for better readability.

Fixes #4722

Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/errinj.h        |   2 +
 src/lib/core/fiber.c         |  84 +++++++++++++++++-----
 test/box/errinj.result       | 136 ++++++++++++++++++-----------------
 test/unit/CMakeLists.txt     |   4 ++
 test/unit/fiber_stack.c      |  79 ++++++++++++++++++++
 test/unit/fiber_stack.result |   8 +++
 test/unit/suite.ini          |   2 +-
 7 files changed, 231 insertions(+), 84 deletions(-)
 create mode 100644 test/unit/fiber_stack.c
 create mode 100644 test/unit/fiber_stack.result

diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 672da2119..ed0cba903 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -135,6 +135,8 @@ struct errinj {
 	_(ERRINJ_COIO_SENDFILE_CHUNK, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_SWIM_FD_ONLY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
+	_(ERRINJ_FIBER_MADVISE, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_FIBER_MPROTECT, ERRINJ_INT, {.iparam = -1})
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 00ae8cded..f10687b29 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -41,6 +41,7 @@
 #include "assoc.h"
 #include "memory.h"
 #include "trigger.h"
+#include "errinj.h"
 
 #if ENABLE_FIBER_TOP
 #include <x86intrin.h> /* __rdtscp() */
@@ -173,21 +174,40 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
 #define ASAN_FINISH_SWITCH_FIBER(var_name)
 #endif
 
-#define fiber_madvise(addr, len, advice)				\
-({									\
-	int err = madvise((addr), (len), (advice));			\
-	if (err)							\
-		say_syserror("madvise");				\
-	err;								\
-})
-
-#define fiber_mprotect(addr, len, prot)					\
-({									\
-	int err = mprotect((addr), (len), (prot));			\
-	if (err)							\
-		say_syserror("mprotect");				\
-	err;								\
-})
+static inline int
+fiber_madvise(void *addr, size_t len, int advice)
+{
+	int rc = 0;
+
+	ERROR_INJECT(ERRINJ_FIBER_MADVISE, {
+		errno = ENOMEM;
+		rc = -1;
+	});
+
+	if (rc != 0 || madvise(addr, len, advice) != 0) {
+		diag_set(SystemError, "fiber madvise failed");
+		return -1;
+	}
+	return 0;
+}
+
+static inline int
+fiber_mprotect(void *addr, size_t len, int prot)
+{
+	int rc = 0;
+
+	struct errinj *inj = errinj(ERRINJ_FIBER_MPROTECT, ERRINJ_INT);
+	if (inj != NULL && inj->iparam == prot) {
+		errno = ENOMEM;
+		rc = -1;
+	}
+
+	if (rc != 0 || mprotect(addr, len, prot) != 0) {
+		diag_set(SystemError, "fiber mprotect failed");
+		return -1;
+	}
+	return 0;
+}
 
 #if ENABLE_FIBER_TOP
 static __thread bool fiber_top_enabled = false;
@@ -951,6 +971,12 @@ fiber_stack_recycle(struct fiber *fiber)
 		start = page_align_up(fiber->stack_watermark);
 		end = fiber->stack + fiber->stack_size;
 	}
+
+	/*
+	 * Ignore errors on MADV_DONTNEED because this is
+	 * just a hint for OS and not critical for
+	 * functionality.
+	 */
 	fiber_madvise(start, end - start, MADV_DONTNEED);
 	stack_put_watermark(fiber->stack_watermark);
 }
@@ -969,7 +995,9 @@ fiber_stack_watermark_create(struct fiber *fiber)
 
 	/*
 	 * We don't expect the whole stack usage in regular
-	 * loads, let's try to minimize rss pressure.
+	 * loads, let's try to minimize rss pressure. But do
+	 * not exit if MADV_DONTNEED failed, it is just a hint
+	 * for OS, not critical one.
 	 */
 	fiber_madvise(fiber->stack, fiber->stack_size, MADV_DONTNEED);
 
@@ -1007,6 +1035,8 @@ fiber_stack_watermark_create(struct fiber *fiber)
 static void
 fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
 {
+	static const int mprotect_flags = PROT_READ | PROT_WRITE;
+
 	if (fiber->stack != NULL) {
 		VALGRIND_STACK_DEREGISTER(fiber->stack_id);
 #if ENABLE_ASAN
@@ -1017,7 +1047,22 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
 			guard = page_align_down(fiber->stack - page_size);
 		else
 			guard = page_align_up(fiber->stack + fiber->stack_size);
-		fiber_mprotect(guard, page_size, PROT_READ | PROT_WRITE);
+
+		if (fiber_mprotect(guard, page_size, mprotect_flags) != 0) {
+			/*
+			 * FIXME: We need some intelligent handling:
+			 * say put this slab into a queue and retry
+			 * to setup the original protection back in
+			 * background.
+			 *
+			 * Note that in case if we're called from
+			 * fiber_stack_create() the @a mprotect_flags is
+			 * the same as the slab been created with, so
+			 * calling mprotect for VMA with same flags
+			 * won't fail.
+			 */
+			diag_log();
+		}
 		slab_put(slabc, fiber->stack_slab);
 	}
 }
@@ -1064,6 +1109,11 @@ fiber_stack_create(struct fiber *fiber, struct slab_cache *slabc,
 						  fiber->stack_size);
 
 	if (fiber_mprotect(guard, page_size, PROT_NONE)) {
+		/*
+		 * Write an error into the log since a guard
+		 * page is critical for functionality.
+		 */
+		diag_log();
 		fiber_stack_destroy(fiber, slabc);
 		return -1;
 	}
diff --git a/test/box/errinj.result b/test/box/errinj.result
index babe36b1b..16f6b40c2 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -23,132 +23,136 @@ errinj.info()
 ---
 - ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT:
     state: 0
-  ERRINJ_WAL_WRITE:
-    state: false
-  ERRINJ_RELAY_BREAK_LSN:
+  ERRINJ_WAL_BREAK_LSN:
     state: -1
-  ERRINJ_HTTPC_EXECUTE:
-    state: false
   ERRINJ_VYRUN_DATA_READ:
     state: false
-  ERRINJ_SWIM_FD_ONLY:
-    state: false
-  ERRINJ_SQL_NAME_NORMALIZATION:
-    state: false
   ERRINJ_VY_SCHED_TIMEOUT:
     state: 0
-  ERRINJ_COIO_SENDFILE_CHUNK:
-    state: -1
   ERRINJ_HTTP_RESPONSE_ADD_WAIT:
     state: false
-  ERRINJ_WAL_WRITE_PARTIAL:
-    state: -1
-  ERRINJ_VY_GC:
-    state: false
-  ERRINJ_WAL_DELAY:
-    state: false
-  ERRINJ_INDEX_ALLOC:
-    state: false
   ERRINJ_WAL_WRITE_EOF:
     state: false
-  ERRINJ_WAL_SYNC:
-    state: false
-  ERRINJ_BUILD_INDEX:
-    state: -1
   ERRINJ_BUILD_INDEX_DELAY:
     state: false
-  ERRINJ_VY_RUN_FILE_RENAME:
-    state: false
-  ERRINJ_VY_COMPACTION_DELAY:
-    state: false
-  ERRINJ_VY_DUMP_DELAY:
-    state: false
   ERRINJ_VY_DELAY_PK_LOOKUP:
     state: false
-  ERRINJ_VY_TASK_COMPLETE:
-    state: false
-  ERRINJ_PORT_DUMP:
+  ERRINJ_VY_POINT_ITER_WAIT:
     state: false
-  ERRINJ_WAL_BREAK_LSN:
-    state: -1
   ERRINJ_WAL_IO:
     state: false
-  ERRINJ_WAL_FALLOCATE:
-    state: 0
-  ERRINJ_DYN_MODULE_COUNT:
-    state: 0
   ERRINJ_VY_INDEX_FILE_RENAME:
     state: false
   ERRINJ_TUPLE_FORMAT_COUNT:
     state: -1
   ERRINJ_TUPLE_ALLOC:
     state: false
-  ERRINJ_VY_RUN_WRITE_DELAY:
+  ERRINJ_VY_RUN_FILE_RENAME:
     state: false
   ERRINJ_VY_READ_PAGE:
     state: false
   ERRINJ_RELAY_REPORT_INTERVAL:
     state: 0
-  ERRINJ_VY_LOG_FILE_RENAME:
-    state: false
-  ERRINJ_VY_READ_PAGE_TIMEOUT:
-    state: 0
+  ERRINJ_RELAY_BREAK_LSN:
+    state: -1
   ERRINJ_XLOG_META:
     state: false
-  ERRINJ_SIO_READ_MAX:
-    state: -1
   ERRINJ_SNAP_COMMIT_DELAY:
     state: false
-  ERRINJ_WAL_WRITE_DISK:
+  ERRINJ_VY_RUN_WRITE:
     state: false
-  ERRINJ_SNAP_WRITE_DELAY:
+  ERRINJ_BUILD_INDEX:
+    state: -1
+  ERRINJ_RELAY_FINAL_JOIN:
+    state: false
+  ERRINJ_REPLICA_JOIN_DELAY:
     state: false
   ERRINJ_LOG_ROTATE:
     state: false
-  ERRINJ_VY_RUN_WRITE:
+  ERRINJ_MEMTX_DELAY_GC:
     state: false
-  ERRINJ_CHECK_FORMAT_DELAY:
+  ERRINJ_XLOG_GARBAGE:
+    state: false
+  ERRINJ_VY_READ_PAGE_DELAY:
+    state: false
+  ERRINJ_SWIM_FD_ONLY:
+    state: false
+  ERRINJ_WAL_WRITE:
+    state: false
+  ERRINJ_HTTPC_EXECUTE:
+    state: false
+  ERRINJ_SQL_NAME_NORMALIZATION:
+    state: false
+  ERRINJ_WAL_WRITE_PARTIAL:
+    state: -1
+  ERRINJ_VY_GC:
+    state: false
+  ERRINJ_WAL_DELAY:
+    state: false
+  ERRINJ_XLOG_READ:
+    state: -1
+  ERRINJ_WAL_SYNC:
+    state: false
+  ERRINJ_VY_TASK_COMPLETE:
     state: false
+  ERRINJ_PORT_DUMP:
+    state: false
+  ERRINJ_COIO_SENDFILE_CHUNK:
+    state: -1
+  ERRINJ_DYN_MODULE_COUNT:
+    state: 0
+  ERRINJ_SIO_READ_MAX:
+    state: -1
+  ERRINJ_FIBER_MPROTECT:
+    state: -1
+  ERRINJ_FIBER_MADVISE:
+    state: false
+  ERRINJ_RELAY_TIMEOUT:
+    state: 0
+  ERRINJ_VY_DUMP_DELAY:
+    state: false
+  ERRINJ_VY_SQUASH_TIMEOUT:
+    state: 0
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
-  ERRINJ_RELAY_FINAL_JOIN:
+  ERRINJ_RELAY_SEND_DELAY:
     state: false
-  ERRINJ_REPLICA_JOIN_DELAY:
+  ERRINJ_VY_COMPACTION_DELAY:
     state: false
-  ERRINJ_RELAY_FINAL_SLEEP:
+  ERRINJ_VY_LOG_FILE_RENAME:
     state: false
   ERRINJ_VY_RUN_DISCARD:
     state: false
   ERRINJ_WAL_ROTATE:
     state: false
-  ERRINJ_RELAY_EXIT_DELAY:
+  ERRINJ_VY_READ_PAGE_TIMEOUT:
     state: 0
-  ERRINJ_VY_POINT_ITER_WAIT:
+  ERRINJ_VY_INDEX_DUMP:
+    state: -1
+  ERRINJ_TUPLE_FIELD:
     state: false
-  ERRINJ_MEMTX_DELAY_GC:
+  ERRINJ_SNAP_WRITE_DELAY:
     state: false
   ERRINJ_IPROTO_TX_DELAY:
     state: false
-  ERRINJ_XLOG_READ:
-    state: -1
-  ERRINJ_TUPLE_FIELD:
+  ERRINJ_RELAY_EXIT_DELAY:
+    state: 0
+  ERRINJ_RELAY_FINAL_SLEEP:
     state: false
-  ERRINJ_XLOG_GARBAGE:
+  ERRINJ_WAL_WRITE_DISK:
     state: false
-  ERRINJ_VY_INDEX_DUMP:
-    state: -1
-  ERRINJ_VY_READ_PAGE_DELAY:
+  ERRINJ_CHECK_FORMAT_DELAY:
     state: false
   ERRINJ_TESTING:
     state: false
-  ERRINJ_RELAY_SEND_DELAY:
+  ERRINJ_VY_RUN_WRITE_DELAY:
     state: false
-  ERRINJ_VY_SQUASH_TIMEOUT:
+  ERRINJ_WAL_FALLOCATE:
     state: 0
   ERRINJ_VY_LOG_FLUSH:
     state: false
-  ERRINJ_RELAY_TIMEOUT:
-    state: 0
+  ERRINJ_INDEX_ALLOC:
+    state: false
 ...
 errinj.set("some-injection", true)
 ---
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 4a57597e9..c037ac539 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -74,6 +74,10 @@ add_executable(fiber.test fiber.cc)
 set_source_files_properties(fiber.cc PROPERTIES COMPILE_FLAGS -O0)
 target_link_libraries(fiber.test core unit)
 
+add_executable(fiber_stack.test fiber_stack.c)
+set_source_files_properties(fiber_stack.c PROPERTIES COMPILE_FLAGS -O0)
+target_link_libraries(fiber_stack.test core unit)
+
 if (NOT ENABLE_GCOV)
     # This test is known to be broken with GCOV
     add_executable(guard.test guard.cc)
diff --git a/test/unit/fiber_stack.c b/test/unit/fiber_stack.c
new file mode 100644
index 000000000..103dfaf0d
--- /dev/null
+++ b/test/unit/fiber_stack.c
@@ -0,0 +1,79 @@
+#include "memory.h"
+#include "fiber.h"
+#include "unit.h"
+#include "trivia/util.h"
+#include "errinj.h"
+
+static struct fiber_attr default_attr;
+
+static int
+noop_f(va_list ap)
+{
+	return 0;
+}
+
+static int
+main_f(va_list ap)
+{
+	struct errinj *inj;
+	struct fiber *fiber;
+
+	header();
+	plan(4);
+
+	/*
+	 * Set non-default stack size to prevent reusing of an
+	 * existing fiber.
+	 */
+	struct fiber_attr *fiber_attr = fiber_attr_new();
+	fiber_attr_setstacksize(fiber_attr, default_attr.stack_size * 2);
+
+	/*
+	 * Clear the fiber's diagnostics area to check that failed
+	 * fiber_new() sets an error.
+	 */
+	diag_clear(diag_get());
+
+	/*
+	 * Check guard page setup via mprotect. We can't test the fiber
+	 * destroy path since it clears fiber's diag.
+	 */
+	inj = errinj(ERRINJ_FIBER_MPROTECT, ERRINJ_INT);
+	inj->iparam = PROT_NONE;
+	fiber = fiber_new_ex("test_mprotect", fiber_attr, noop_f);
+	inj->iparam = -1;
+
+	ok(fiber == NULL, "mprotect: failed to setup fiber guard page");
+	ok(diag_get() != NULL, "mprotect: diag is armed after error");
+
+	/*
+	 * Check madvise. We can't test the fiber destroy
+	 * path since it is cleaning error.
+	 */
+	diag_clear(diag_get());
+	inj = errinj(ERRINJ_FIBER_MADVISE, ERRINJ_BOOL);
+	inj->bparam = true;
+	fiber = fiber_new_ex("test_madvise", fiber_attr, noop_f);
+	inj->bparam = false;
+
+	ok(fiber != NULL, "madvise: non critical error on madvise hint");
+	ok(diag_get() != NULL, "madvise: diag is armed after error");
+
+	footer();
+
+	ev_break(loop(), EVBREAK_ALL);
+	return check_plan();
+}
+
+int main()
+{
+	memory_init();
+	fiber_init(fiber_c_invoke);
+	fiber_attr_create(&default_attr);
+	struct fiber *f = fiber_new("main", main_f);
+	fiber_wakeup(f);
+	ev_run(loop(), 0);
+	fiber_free();
+	memory_free();
+	return 0;
+}
diff --git a/test/unit/fiber_stack.result b/test/unit/fiber_stack.result
new file mode 100644
index 000000000..43ff74b2f
--- /dev/null
+++ b/test/unit/fiber_stack.result
@@ -0,0 +1,8 @@
+SystemError fiber mprotect failed: Cannot allocate memory
+	*** main_f ***
+1..4
+ok 1 - mprotect: failed to setup fiber guard page
+ok 2 - mprotect: diag is armed after error
+ok 3 - madvise: non critical error on madvise hint
+ok 4 - madvise: diag is armed after error
+	*** main_f: done ***
diff --git a/test/unit/suite.ini b/test/unit/suite.ini
index 89c8499fc..d429c95e9 100644
--- a/test/unit/suite.ini
+++ b/test/unit/suite.ini
@@ -1,5 +1,5 @@
 [default]
 core = unittest
 description = unit tests
-release_disabled = swim_errinj.test
+release_disabled = fiber_stack.test swim_errinj.test
 is_parallel = True
-- 
2.20.1

[Tarantool-patches] [PATCH v7 2/2] fiber: leak slab if unable to bring prots back

[Cyrill Gorcunov]

In case if we unable to revert guard page back to
read|write we should never use such slab again.

Initially I thought of just put panic here and
exit but it is too destructive. I think better
print an error and continue. If node admin ignore
this message then one moment at future there won't
be slab left for use and creating new fibers get
prohibited.

In future (hopefully near one) we plan to drop
guard pages to prevent VMA fracturing and use
stack marks instead.

Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/fiber.c         | 11 +++++++++--
 test/unit/fiber_stack.c      | 29 ++++++++++++++++++++++++++---
 test/unit/fiber_stack.result |  4 +++-
 3 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index f10687b29..73de7765b 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1055,15 +1055,22 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
 			 * to setup the original protection back in
 			 * background.
 			 *
+			 * For now lets keep such slab referenced and
+			 * leaked: if mprotect failed we must not allow
+			 * to reuse such slab with PROT_NONE'ed page
+			 * inside.
+			 *
 			 * Note that in case if we're called from
 			 * fiber_stack_create() the @a mprotect_flags is
 			 * the same as the slab been created with, so
 			 * calling mprotect for VMA with same flags
 			 * won't fail.
 			 */
-			diag_log();
+			say_syserror("fiber: Can't put guard page to slab. "
+				     "Leak %zu bytes", (size_t)fiber->stack_size);
+		} else {
+			slab_put(slabc, fiber->stack_slab);
 		}
-		slab_put(slabc, fiber->stack_slab);
 	}
 }
 
diff --git a/test/unit/fiber_stack.c b/test/unit/fiber_stack.c
index 103dfaf0d..5eb128ea1 100644
--- a/test/unit/fiber_stack.c
+++ b/test/unit/fiber_stack.c
@@ -19,7 +19,7 @@ main_f(va_list ap)
 	struct fiber *fiber;
 
 	header();
-	plan(4);
+	plan(5);
 
 	/*
 	 * Set non-default stack size to prevent reusing of an
@@ -47,8 +47,7 @@ main_f(va_list ap)
 	ok(diag_get() != NULL, "mprotect: diag is armed after error");
 
 	/*
-	 * Check madvise. We can't test the fiber destroy
-	 * path since it is cleaning error.
+	 * Check madvise error on fiber creation.
 	 */
 	diag_clear(diag_get());
 	inj = errinj(ERRINJ_FIBER_MADVISE, ERRINJ_BOOL);
@@ -59,6 +58,30 @@ main_f(va_list ap)
 	ok(fiber != NULL, "madvise: non critical error on madvise hint");
 	ok(diag_get() != NULL, "madvise: diag is armed after error");
 
+	/*
+	 * Check if we leak on fiber descrution.
+	 * We will print an error and result get
+	 * compared by testing engine.
+	 */
+	fiber_attr_delete(fiber_attr);
+	fiber_attr = fiber_attr_new();
+	fiber_attr->flags |= FIBER_CUSTOM_STACK;
+	fiber_attr->stack_size = 64 << 10;
+
+	diag_clear(diag_get());
+	fiber = fiber_new_ex("test_madvise", fiber_attr, noop_f);
+	ok(fiber != NULL, "fiber with custom stack");
+	fiber_set_joinable(fiber, true);
+
+	inj = errinj(ERRINJ_FIBER_MPROTECT, ERRINJ_INT);
+	inj->iparam = PROT_READ | PROT_WRITE;
+
+	fiber_start(fiber);
+	fiber_join(fiber);
+
+	inj->iparam = -1;
+
+	fiber_attr_delete(fiber_attr);
 	footer();
 
 	ev_break(loop(), EVBREAK_ALL);
diff --git a/test/unit/fiber_stack.result b/test/unit/fiber_stack.result
index 43ff74b2f..f0f5a59c2 100644
--- a/test/unit/fiber_stack.result
+++ b/test/unit/fiber_stack.result
@@ -1,8 +1,10 @@
 SystemError fiber mprotect failed: Cannot allocate memory
+fiber: Can't put guard page to slab. Leak 57344 bytes: Cannot allocate memory
 	*** main_f ***
-1..4
+1..5
 ok 1 - mprotect: failed to setup fiber guard page
 ok 2 - mprotect: diag is armed after error
 ok 3 - madvise: non critical error on madvise hint
 ok 4 - madvise: diag is armed after error
+ok 5 - fiber with custom stack
 	*** main_f: done ***
-- 
2.20.1

Re: [Tarantool-patches] [PATCH v7 0/2] fiber: Handle stack madvise/mprotect errors

[Cyrill Gorcunov]

On Thu, Feb 13, 2020 at 11:56:16PM +0300, Cyrill Gorcunov wrote:
> Changes in v7:
>  - address Vlad's comments
>  - add test for slab leak when mprotect fails on exit path
> 
> Still the beautifying of errinj is send as a separate patch
> so diff is pretty big for now.

issue https://github.com/tarantool/tarantool/issues/4722
branch https://github.com/tarantool/tarantool/tree/gorcunov/gh-4722-mprotect-diag-error-7

Re: [Tarantool-patches] [PATCH v7 2/2] fiber: leak slab if unable to bring prots back

[Vladislav Shpilevoy]

Thanks for the fixes!

See 2 comments below.

On 13/02/2020 21:56, Cyrill Gorcunov wrote:
> In case if we unable to revert guard page back to
> read|write we should never use such slab again.
> 
> Initially I thought of just put panic here and
> exit but it is too destructive. I think better
> print an error and continue. If node admin ignore
> this message then one moment at future there won't
> be slab left for use and creating new fibers get
> prohibited.
> 
> In future (hopefully near one) we plan to drop
> guard pages to prevent VMA fracturing and use
> stack marks instead.
> 
> Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/lib/core/fiber.c         | 11 +++++++++--
>  test/unit/fiber_stack.c      | 29 ++++++++++++++++++++++++++---
>  test/unit/fiber_stack.result |  4 +++-
>  3 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/test/unit/fiber_stack.c b/test/unit/fiber_stack.c
> index 103dfaf0d..5eb128ea1 100644
> --- a/test/unit/fiber_stack.c
> +++ b/test/unit/fiber_stack.c
> @@ -59,6 +58,30 @@ main_f(va_list ap)
>  	ok(fiber != NULL, "madvise: non critical error on madvise hint");
>  	ok(diag_get() != NULL, "madvise: diag is armed after error");
>  
> +	/*
> +	 * Check if we leak on fiber descrution.

1. You probably have meant 'destruction'.

> +	 * We will print an error and result get
> +	 * compared by testing engine.
> +	 */
> +	fiber_attr_delete(fiber_attr);
> +	fiber_attr = fiber_attr_new();
> +	fiber_attr->flags |= FIBER_CUSTOM_STACK;
> +	fiber_attr->stack_size = 64 << 10;
> +
> +	diag_clear(diag_get());
> +	fiber = fiber_new_ex("test_madvise", fiber_attr, noop_f);
> +	ok(fiber != NULL, "fiber with custom stack");
> +	fiber_set_joinable(fiber, true);
> +
> +	inj = errinj(ERRINJ_FIBER_MPROTECT, ERRINJ_INT);
> +	inj->iparam = PROT_READ | PROT_WRITE;
> +
> +	fiber_start(fiber);
> +	fiber_join(fiber);
> +
> +	inj->iparam = -1;
> +
> +	fiber_attr_delete(fiber_attr);

2. When I run the test without the fix, I got 'Bus error'.
Was it intended? In my email I proposed to compare memory
statistics, but crash also looks ok, maybe. If this is what
you meant. And then there should be a comment saying, that we
expect fiber_join() to crash when no leak is done, because it
would put the slab into the cash, where it would be read/written
against protection. This was not obvious until I reverted your
patch and run the test.

IMO, problem of that way of testing is that if internals of
core/fiber.c one day will stop touching stack of a just
destroyed fiber (for example, a pointer at it would be cached
somewhere for newer fibers), this test will happily pass, even
though it perhaps should not. It will pass regardless of what
happens with a fiber stack unless something tries to read/write
it.

>  	footer();
>  
>  	ev_break(loop(), EVBREAK_ALL);
> diff --git a/test/unit/fiber_stack.result b/test/unit/fiber_stack.result
> index 43ff74b2f..f0f5a59c2 100644
> --- a/test/unit/fiber_stack.result
> +++ b/test/unit/fiber_stack.result
> @@ -1,8 +1,10 @@
>  SystemError fiber mprotect failed: Cannot allocate memory
> +fiber: Can't put guard page to slab. Leak 57344 bytes: Cannot allocate memory
>  	*** main_f ***
> -1..4
> +1..5
>  ok 1 - mprotect: failed to setup fiber guard page
>  ok 2 - mprotect: diag is armed after error
>  ok 3 - madvise: non critical error on madvise hint
>  ok 4 - madvise: diag is armed after error
> +ok 5 - fiber with custom stack
>  	*** main_f: done ***
> 

Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Vladislav Shpilevoy]

Thanks for the fixes!

> @@ -1007,6 +1035,8 @@ fiber_stack_watermark_create(struct fiber *fiber)
>  static void
>  fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
>  {
> +	static const int mprotect_flags = PROT_READ | PROT_WRITE;
> +

Why is it static? From what I know, when it is static we don't
have a guarantee, that it won't occupy memory in the .data section.
Even though here it is clearly not necessary. Wouldn't just const
be enough? Why was not it possible to leave these flags inlined in
fiber_mprotect() call? They are not used anywhere else. PROT_NONE,
for example, is left inlined.

>  	if (fiber->stack != NULL) {
>  		VALGRIND_STACK_DEREGISTER(fiber->stack_id);
>  #if ENABLE_ASAN
> @@ -1017,7 +1047,22 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
>  			guard = page_align_down(fiber->stack - page_size);
>  		else
>  			guard = page_align_up(fiber->stack + fiber->stack_size);
> -		fiber_mprotect(guard, page_size, PROT_READ | PROT_WRITE);
> +
> +		if (fiber_mprotect(guard, page_size, mprotect_flags) != 0) {
> +			/*
> +			 * FIXME: We need some intelligent handling:
> +			 * say put this slab into a queue and retry
> +			 * to setup the original protection back in
> +			 * background.
> +			 *
> +			 * Note that in case if we're called from
> +			 * fiber_stack_create() the @a mprotect_flags is
> +			 * the same as the slab been created with, so
> +			 * calling mprotect for VMA with same flags
> +			 * won't fail.
> +			 */
> +			diag_log();
> +		}
>  		slab_put(slabc, fiber->stack_slab);
>  	}
>  }
> @@ -1064,6 +1109,11 @@ fiber_stack_create(struct fiber *fiber, struct slab_cache *slabc,
>  						  fiber->stack_size);
>  
>  	if (fiber_mprotect(guard, page_size, PROT_NONE)) {
> +		/*
> +		 * Write an error into the log since a guard
> +		 * page is critical for functionality.
> +		 */
> +		diag_log();
>  		fiber_stack_destroy(fiber, slabc);
>  		return -1;
>  	}

Re: [Tarantool-patches] [PATCH v7 0/2] fiber: Handle stack madvise/mprotect errors

[Vladislav Shpilevoy]

Thanks for the fixes!

Please, put branch and issue link here next time.
It is really hard to find that branch just by commit
title, or by searching for older versions of the
branch in email history. Especially taking into
account that there are 7 versions of mprotect branches.

Btw, you can force push into one branch, it is ok, and
is even more convenient. At least from reviewer's point
of view.

On 13/02/2020 21:56, Cyrill Gorcunov wrote:
> Changes in v7:
>  - address Vlad's comments
>  - add test for slab leak when mprotect fails on exit path
> 
> Still the beautifying of errinj is send as a separate patch
> so diff is pretty big for now.
> 
> Cyrill Gorcunov (2):
>   fiber: set diagnostics at madvise/mprotect failure
>   fiber: leak slab if unable to bring prots back
> 
>  src/lib/core/errinj.h        |   2 +
>  src/lib/core/fiber.c         |  93 +++++++++++++++++++-----
>  test/box/errinj.result       | 136 ++++++++++++++++++-----------------
>  test/unit/CMakeLists.txt     |   4 ++
>  test/unit/fiber_stack.c      | 102 ++++++++++++++++++++++++++
>  test/unit/fiber_stack.result |  10 +++
>  test/unit/suite.ini          |   2 +-
>  7 files changed, 264 insertions(+), 85 deletions(-)
>  create mode 100644 test/unit/fiber_stack.c
>  create mode 100644 test/unit/fiber_stack.result
> 

Re: [Tarantool-patches] [PATCH v7 0/2] fiber: Handle stack madvise/mprotect errors

[Cyrill Gorcunov]

On Fri, Feb 14, 2020 at 12:26:05AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> Please, put branch and issue link here next time.
> It is really hard to find that branch just by commit
> title, or by searching for older versions of the
> branch in email history. Especially taking into
> account that there are 7 versions of mprotect branches.

I did so in reply to this email ('cause I managed to forgot, sorry)

> 
> Btw, you can force push into one branch, it is ok, and
> is even more convenient. At least from reviewer's point
> of view.

Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Cyrill Gorcunov]

On Fri, Feb 14, 2020 at 12:26:04AM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > @@ -1007,6 +1035,8 @@ fiber_stack_watermark_create(struct fiber *fiber)
> >  static void
> >  fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
> >  {
> > +	static const int mprotect_flags = PROT_READ | PROT_WRITE;
> > +
> 
> Why is it static? From what I know, when it is static we don't
> have a guarantee, that it won't occupy memory in the .data section.

it is not about memory occupation but rather init it once instead
of doing so during function prologue. It actually depends on compiler
and modern ones would simply optimize this assignment.

Actually there is one hidden idea -- R|W is initial flags the 'small'
uses when allocates memory that's why I mention this variable in
comment.

> Even though here it is clearly not necessary. Wouldn't just const
> be enough? Why was not it possible to leave these flags inlined in
> fiber_mprotect() call? They are not used anywhere else. PROT_NONE,
> for example, is left inlined.

PROT_NONE is special, it is unrelated to 'small'.

	Cyrill

Re: [Tarantool-patches] [PATCH v7 2/2] fiber: leak slab if unable to bring prots back

[Cyrill Gorcunov]

On Fri, Feb 14, 2020 at 12:26:04AM +0100, Vladislav Shpilevoy wrote:
> > --- a/test/unit/fiber_stack.c
> > +++ b/test/unit/fiber_stack.c
> > @@ -59,6 +58,30 @@ main_f(va_list ap)
> >  	ok(fiber != NULL, "madvise: non critical error on madvise hint");
> >  	ok(diag_get() != NULL, "madvise: diag is armed after error");
> >  
> > +	/*
> > +	 * Check if we leak on fiber descrution.
> 
> 1. You probably have meant 'destruction'.

Sharp eyes =) Thanks!

> > +
> > +	fiber_attr_delete(fiber_attr);
> 
> 2. When I run the test without the fix, I got 'Bus error'.
> Was it intended? In my email I proposed to compare memory

Hmm. Actually not, should not be. I'll dig into, thanks!

> statistics, but crash also looks ok, maybe. If this is what
> you meant. And then there should be a comment saying, that we
> expect fiber_join() to crash when no leak is done, because it
> would put the slab into the cash, where it would be read/written
> against protection. This was not obvious until I reverted your
> patch and run the test.

I'll try to play with statistics again, the error message
was just simplier and faster.

> IMO, problem of that way of testing is that if internals of
> core/fiber.c one day will stop touching stack of a just
> destroyed fiber (for example, a pointer at it would be cached
> somewhere for newer fibers), this test will happily pass, even
> though it perhaps should not. It will pass regardless of what
> happens with a fiber stack unless something tries to read/write
> it.

Thanks for comments!

	Cyrill

Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Vladislav Shpilevoy]

Thanks for the answers!

>>> @@ -1007,6 +1035,8 @@ fiber_stack_watermark_create(struct fiber *fiber)
>>>  static void
>>>  fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
>>>  {
>>> +	static const int mprotect_flags = PROT_READ | PROT_WRITE;
>>> +
>>
>> Why is it static? From what I know, when it is static we don't
>> have a guarantee, that it won't occupy memory in the .data section.
> 
> it is not about memory occupation but rather init it once instead
> of doing so during function prologue. It actually depends on compiler
> and modern ones would simply optimize this assignment.

Sorry, I don't understand. What do you mean init it only once?
PROT_READ | PROT_WRITE is a constant. There is nothing to init or
to calculate. This is just a one constant integer, which does not
need to be saved into a variable before being passed to a function,
especially into a static variable.

> Actually there is one hidden idea -- R|W is initial flags the 'small'
> uses when allocates memory that's why I mention this variable in
> comment.

You don't really need a static variable to mention its value in a
comment. Just say that you use READ|WRITE because this is what
usually can be done with normal memory.

Otherwise I am missing something.

>> Even though here it is clearly not necessary. Wouldn't just const
>> be enough? Why was not it possible to leave these flags inlined in
>> fiber_mprotect() call? They are not used anywhere else. PROT_NONE,
>> for example, is left inlined.
> 
> PROT_NONE is special, it is unrelated to 'small'.

Strictly speaking, core/fiber.c can't assume anything about small
internals, including READ|WRITE flags. Because fiber is not a
part of small. But I propose not to overcomplicate this.

> 	Cyrill

Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Cyrill Gorcunov]

On Fri, Feb 14, 2020 at 11:27:50PM +0100, Vladislav Shpilevoy wrote:
> > 
> > it is not about memory occupation but rather init it once instead
> > of doing so during function prologue. It actually depends on compiler
> > and modern ones would simply optimize this assignment.
> 
> Sorry, I don't understand. What do you mean init it only once?
> PROT_READ | PROT_WRITE is a constant. There is nothing to init or
> to calculate. This is just a one constant integer, which does not
> need to be saved into a variable before being passed to a function,
> especially into a static variable.

It highly depends on compiler. It might be optimized and not allocated
on the stack at all or it might be allocated and loaded in the procedure
prologue (which will happen with optimization disabled).

> > Actually there is one hidden idea -- R|W is initial flags the 'small'
> > uses when allocates memory that's why I mention this variable in
> > comment.
> 
> You don't really need a static variable to mention its value in a
> comment. Just say that you use READ|WRITE because this is what
> usually can be done with normal memory.

And I'll have to modify both -- comment and code if we need
to change the permissions. Thanks but no. We could do better
and keep R|W in one place.

But if you bothers this approach I could just inline R|W, no problem.

Still there is a really big question remains -- what to do with
the issue. I suspect Kostya has something different in mind. So
for now I'm giving up cooking this series (until I'll be sure
that I really understand the next step to walk).

Thanks a huge for all your comments, Vlad!

> Otherwise I am missing something.
> 
> >> Even though here it is clearly not necessary. Wouldn't just const
> >> be enough? Why was not it possible to leave these flags inlined in
> >> fiber_mprotect() call? They are not used anywhere else. PROT_NONE,
> >> for example, is left inlined.
> > 
> > PROT_NONE is special, it is unrelated to 'small'.
> 
> Strictly speaking, core/fiber.c can't assume anything about small
> internals, including READ|WRITE flags. Because fiber is not a
> part of small. But I propose not to overcomplicate this.

Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Vladislav Shpilevoy]

On 15/02/2020 07:57, Cyrill Gorcunov wrote:
> On Fri, Feb 14, 2020 at 11:27:50PM +0100, Vladislav Shpilevoy wrote:
>>>
>>> it is not about memory occupation but rather init it once instead
>>> of doing so during function prologue. It actually depends on compiler
>>> and modern ones would simply optimize this assignment.
>>
>> Sorry, I don't understand. What do you mean init it only once?
>> PROT_READ | PROT_WRITE is a constant. There is nothing to init or
>> to calculate. This is just a one constant integer, which does not
>> need to be saved into a variable before being passed to a function,
>> especially into a static variable.
> 
> It highly depends on compiler. It might be optimized and not allocated
> on the stack at all or it might be allocated and loaded in the procedure
> prologue (which will happen with optimization disabled).

This is what I don't like - dependency on whether a compiler will
optimize something or not.

>>> Actually there is one hidden idea -- R|W is initial flags the 'small'
>>> uses when allocates memory that's why I mention this variable in
>>> comment.
>>
>> You don't really need a static variable to mention its value in a
>> comment. Just say that you use READ|WRITE because this is what
>> usually can be done with normal memory.
> 
> And I'll have to modify both -- comment and code if we need
> to change the permissions. Thanks but no. We could do better
> and keep R|W in one place.

I can't imagine why would we need other flags here, but anyway if
that will happen, I am sure it will change much more than just a
comment. However, this is subjective.

> But if you bothers this approach I could just inline R|W, no problem.

Usually for similar things, when it is wanted to have a constant
expression as a single identifier, enum works well, and we use
that a lot, for the same reasons. So I propose you to consider a
enum then. Anonymous enum in fiber.c.

> Still there is a really big question remains -- what to do with
> the issue. I suspect Kostya has something different in mind. So
> for now I'm giving up cooking this series (until I'll be sure
> that I really understand the next step to walk).

Ok. Btw, I don't understand what is wrong with the current
solution.

Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at madvise/mprotect failure

[Cyrill Gorcunov]

On Sat, Feb 15, 2020 at 04:41:39PM +0100, Vladislav Shpilevoy wrote:
> > 
> > It highly depends on compiler. It might be optimized and not allocated
> > on the stack at all or it might be allocated and loaded in the procedure
> > prologue (which will happen with optimization disabled).
> 
> This is what I don't like - dependency on whether a compiler will
> optimize something or not.

I mean the different thing. If variable declared as const inside
prologue it might be or might be not rolled in without load. To
prevent possible load I use static var.

> > 
> > And I'll have to modify both -- comment and code if we need
> > to change the permissions. Thanks but no. We could do better
> > and keep R|W in one place.
> 
> I can't imagine why would we need other flags here, but anyway if
> that will happen, I am sure it will change much more than just a
> comment. However, this is subjective.

Yup.

> 
> > But if you bothers this approach I could just inline R|W, no problem.
> 
> Usually for similar things, when it is wanted to have a constant
> expression as a single identifier, enum works well, and we use
> that a lot, for the same reasons. So I propose you to consider a
> enum then. Anonymous enum in fiber.c.

Sure, thanks

> > Still there is a really big question remains -- what to do with
> > the issue. I suspect Kostya has something different in mind. So
> > for now I'm giving up cooking this series (until I'll be sure
> > that I really understand the next step to walk).
> 
> Ok. Btw, I don't understand what is wrong with the current
> solution.

I though of extending code on the top of this series to drop
r|w in release builds completely, but not right now. So lets
wait for Kostya to respond.