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

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

[Cyrill Gorcunov]

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

Cyrill Gorcunov (2):
  fiber: use diag_ logger in fiber_madvise/mprotect failures
  fiber: exit with panic if we unable to revert guard page

 src/lib/core/fiber.c | 74 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 17 deletions(-)

-- 
2.20.1

[Tarantool-patches] [PATCH v2 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures

[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>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/fiber.c | 70 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 17 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 00ae8cded..b51f46f2f 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -173,21 +173,27 @@ 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)
+{
+	if (madvise(addr, len, advice) != 0) {
+		diag_set(SystemError, "fiber madvise %p:%zu:%d failed",
+			 addr, len, advice);
+		return -1;
+	}
+	return 0;
+}
+
+static inline int
+fiber_mprotect(void *addr, size_t len, int prot)
+{
+	if (mprotect(addr, len, prot) != 0) {
+		diag_set(SystemError, "fiber mprotect %p:%zu:%d failed",
+			 addr, len, prot);
+		return -1;
+	}
+	return 0;
+}
 
 #if ENABLE_FIBER_TOP
 static __thread bool fiber_top_enabled = false;
@@ -951,6 +957,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 +981,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 +1021,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 +1033,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 @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 +1095,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;
 	}
-- 
2.20.1

[Tarantool-patches] [PATCH v2 2/2] fiber: exit with panic if we unable to revert guard page

[Cyrill Gorcunov]

At the moment we setup fiber's stack with a guard page
which is used to detect stack overrun. This page is just
a regular page taken from a slab with PROT_NONE attribute.

Once fiber is no longer needed we try to revert this
attribute back to PROT_READ | PROT_WRITE. Still there
is a pretty small chance that this attempt get failed.

Thus in such case we should not allow to proceed but rather
lets panic, otherwise the slab won't longer be solid r/w memory
area and attempt to write into this page will cause
an unpredictable exception.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/fiber.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index b51f46f2f..fdad7607c 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1041,13 +1041,17 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
 			 * to setup the original protection back in
 			 * background.
 			 *
+			 * For now lets exit with panic: if mprotect
+			 * failed we must not allow to reuse such slab
+			 * with PROT_NONE'ed page somewhere inside.
+			 *
 			 * Note that in case if we're called from
 			 * fiber_stack_create() the @mprotect_flags is
 			 * the same as the slab been created with, so
 			 * calling mprotect for VMA with same flags
 			 * won't fail.
 			 */
-			diag_log();
+			panic_syserror("fiber: Can't put guard page to slab");
 		}
 		slab_put(slabc, fiber->stack_slab);
 	}
-- 
2.20.1

Re: [Tarantool-patches] [PATCH v2 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures

[Alexander Turenko]

Everything looks correct.

However, I don't believe my eyes, so I wrote a test case (see below).

Don't sure whether it is good to have error injections for such
functions. Maybe LD_PRELOAD way is better (see [1]). Anyway, I checked
that the behaviour was incorrect using the test and that it becomes
correct.

Please, consider the shared test and decide whether it worth to add it
to the repository (I don't sure).

[1]: https://tbrindus.ca/correct-ld-preload-hooking-libc/

WBR, Alexander Turenko.

On Wed, Jan 15, 2020 at 08:05:23PM +0300, Cyrill Gorcunov wrote:
> 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>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/lib/core/fiber.c | 70 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 53 insertions(+), 17 deletions(-)

----

diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 672da2119..10d761e69 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -135,6 +135,9 @@ 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_PROT_NONE, ERRINJ_BOOL, {.bparam = false}) \
+
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index fdad7607c..b698419cf 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() */
@@ -176,7 +177,12 @@ static int (*fiber_invoke)(fiber_func f, va_list ap);
 static inline int
 fiber_madvise(void *addr, size_t len, int advice)
 {
-	if (madvise(addr, len, advice) != 0) {
+	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 %p:%zu:%d failed",
 			 addr, len, advice);
 		return -1;
@@ -187,7 +193,14 @@ fiber_madvise(void *addr, size_t len, int advice)
 static inline int
 fiber_mprotect(void *addr, size_t len, int prot)
 {
-	if (mprotect(addr, len, prot) != 0) {
+	int rc = 0;
+	ERROR_INJECT(ERRINJ_FIBER_MPROTECT_PROT_NONE, {
+		if (prot == PROT_NONE) {
+			errno = ENOMEM;
+			rc = -1;
+		}
+	});
+	if (rc != 0 || mprotect(addr, len, prot) != 0) {
 		diag_set(SystemError, "fiber mprotect %p:%zu:%d failed",
 			 addr, len, prot);
 		return -1;
diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
index 91f7d43f9..d64c4f89d 100644
--- a/test/unit/fiber.cc
+++ b/test/unit/fiber.cc
@@ -2,6 +2,7 @@
 #include "fiber.h"
 #include "unit.h"
 #include "trivia/util.h"
+#include "errinj.h"
 
 static struct fiber_attr default_attr;
 
@@ -181,12 +182,43 @@ fiber_name_test()
 	footer();
 }
 
+static void
+fiber_mprotect_fail_test(void)
+{
+	header();
+
+	/*
+	 * 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());
+
+	struct errinj *errinj = &errinjs[ERRINJ_FIBER_MPROTECT_PROT_NONE];
+	errinj->bparam = true;
+	struct fiber *fiber = fiber_new_ex("test_mprotect", fiber_attr, noop_f);
+	errinj->bparam = false;
+
+	ok(fiber == NULL, "failed mprotect() causes fiber_new() fail");
+	struct error *error = diag_last_error(diag_get());
+	ok(error != NULL, "failed fiber_new() sets an error");
+
+	footer();
+}
+
 static int
 main_f(va_list ap)
 {
 	fiber_name_test();
 	fiber_join_test();
 	fiber_stack_test();
+	fiber_mprotect_fail_test();
 	ev_break(loop(), EVBREAK_ALL);
 	return 0;
 }

Re: [Tarantool-patches] [PATCH v2 2/2] fiber: exit with panic if we unable to revert guard page

[Alexander Turenko]

Your reasoning looks correct, but actually I don't have enough expertise
to understand all pros and cons.

So the patch LGTM, but I would ask you to acquire the second review from
Vlad Sh.

WBR, Alexander Turenko.

On Wed, Jan 15, 2020 at 08:05:24PM +0300, Cyrill Gorcunov wrote:
> At the moment we setup fiber's stack with a guard page
> which is used to detect stack overrun. This page is just
> a regular page taken from a slab with PROT_NONE attribute.
> 
> Once fiber is no longer needed we try to revert this
> attribute back to PROT_READ | PROT_WRITE. Still there
> is a pretty small chance that this attempt get failed.
> 
> Thus in such case we should not allow to proceed but rather
> lets panic, otherwise the slab won't longer be solid r/w memory
> area and attempt to write into this page will cause
> an unpredictable exception.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/lib/core/fiber.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index b51f46f2f..fdad7607c 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -1041,13 +1041,17 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
>  			 * to setup the original protection back in
>  			 * background.
>  			 *
> +			 * For now lets exit with panic: if mprotect
> +			 * failed we must not allow to reuse such slab
> +			 * with PROT_NONE'ed page somewhere inside.
> +			 *
>  			 * Note that in case if we're called from
>  			 * fiber_stack_create() the @mprotect_flags is
>  			 * the same as the slab been created with, so
>  			 * calling mprotect for VMA with same flags
>  			 * won't fail.
>  			 */
> -			diag_log();
> +			panic_syserror("fiber: Can't put guard page to slab");
>  		}
>  		slab_put(slabc, fiber->stack_slab);
>  	}
> -- 
> 2.20.1
> 

Re: [Tarantool-patches] [PATCH v2 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures

[Cyrill Gorcunov]

On Tue, Feb 04, 2020 at 12:56:27AM +0300, Alexander Turenko wrote:
> Everything looks correct.
> 
> However, I don't believe my eyes, so I wrote a test case (see below).
> 
> Don't sure whether it is good to have error injections for such
> functions. Maybe LD_PRELOAD way is better (see [1]). Anyway, I checked
> that the behaviour was incorrect using the test and that it becomes
> correct.
> 
> Please, consider the shared test and decide whether it worth to add it
> to the repository (I don't sure).

I think having errinj is better in a long term, it is controllable
by our code and doesn't depend on any other hooks. Except

 - I would call it just FIBER_MPROTECT (without prot-none suffix)
 - iirc there was some test in app/ errinj or similar which should
   be updated everytime new errinj introduced.

With this two things addressed feel free to put my Ack. And thanks
a huge, Sasha, for this test

Re: [Tarantool-patches] [PATCH v2 1/2] fiber: use diag_ logger in fiber_madvise/mprotect failures

[Alexander Turenko]

>  - I would call it just FIBER_MPROTECT (without prot-none suffix)

I don't mind.

>  - iirc there was some test in app/ errinj or similar which should
>    be updated everytime new errinj introduced.

Yep.

Re: [Tarantool-patches] [PATCH v2 2/2] fiber: exit with panic if we unable to revert guard page

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [20/01/15 20:07]:
> At the moment we setup fiber's stack with a guard page
> which is used to detect stack overrun. This page is just
> a regular page taken from a slab with PROT_NONE attribute.
> 
> Once fiber is no longer needed we try to revert this
> attribute back to PROT_READ | PROT_WRITE. Still there
> is a pretty small chance that this attempt get failed.
> 
> Thus in such case we should not allow to proceed but rather
> lets panic, otherwise the slab won't longer be solid r/w memory
> area and attempt to write into this page will cause
> an unpredictable exception.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/lib/core/fiber.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index b51f46f2f..fdad7607c 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -1041,13 +1041,17 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
>  			 * to setup the original protection back in
>  			 * background.
>  			 *
> +			 * For now lets exit with panic: if mprotect
> +			 * failed we must not allow to reuse such slab
> +			 * with PROT_NONE'ed page somewhere inside.
> +			 *

somewhere inside its stack area would be more clear.

>  			 * Note that in case if we're called from
>  			 * fiber_stack_create() the @mprotect_flags is
>  			 * the same as the slab been created with, so
>  			 * calling mprotect for VMA with same flags
>  			 * won't fail.
>  			 */
> -			diag_log();
> +			panic_syserror("fiber: Can't put guard page to slab");

While the patch itself is LGTM, we need to nail down the cause of
the failure even at the cost of crash, I suspect what we're getting
here is ENOMEM from the kernel. I suspect we have too many
mprotect regions and the kernel runs out of some internal
resources for them.

I think adding better diagnostics could help us identify the
issue: the failed address, its slab, the total number of fibers (and by
induction mprotected pages). 

I have also discussed the issue with @xemul, and he suggests that 
wrong slab alignment could be causing this.

Finally, we could try to clear mprotect() first, and if it fails,
avoid destroying the fiber and keep it cached for a while more.
We could retry destroying it when kernel has more memory.

These are of course ideas for follow ups.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 2/2] fiber: exit with panic if we unable to revert guard page

[Cyrill Gorcunov]

On Tue, Feb 04, 2020 at 06:47:42PM +0300, Konstantin Osipov wrote:
> > --- a/src/lib/core/fiber.c
> > +++ b/src/lib/core/fiber.c
> > @@ -1041,13 +1041,17 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
> >  			 * to setup the original protection back in
> >  			 * background.
> >  			 *
> > +			 * For now lets exit with panic: if mprotect
> > +			 * failed we must not allow to reuse such slab
> > +			 * with PROT_NONE'ed page somewhere inside.
> > +			 *
> 
> somewhere inside its stack area would be more clear.

Thanks!

> >  			 * Note that in case if we're called from
> >  			 * fiber_stack_create() the @mprotect_flags is
> >  			 * the same as the slab been created with, so
> >  			 * calling mprotect for VMA with same flags
> >  			 * won't fail.
> >  			 */
> > -			diag_log();
> > +			panic_syserror("fiber: Can't put guard page to slab");
> 
> While the patch itself is LGTM, we need to nail down the cause of
> the failure even at the cost of crash, I suspect what we're getting
> here is ENOMEM from the kernel. I suspect we have too many
> mprotect regions and the kernel runs out of some internal
> resources for them.

Yes, seems we might get out of VMA when there are too many fibers
and system doesn't have enough memory to continue splitting.

> I think adding better diagnostics could help us identify the
> issue: the failed address, its slab, the total number of fibers (and by
> induction mprotected pages). 
> 
> I have also discussed the issue with @xemul, and he suggests that 
> wrong slab alignment could be causing this.

True, and we need investigate it. Once we have the "fix" merged
in I'll file a bug to investigate this ideas. Thanks a huge, Kostya!

> 
> Finally, we could try to clear mprotect() first, and if it fails,
> avoid destroying the fiber and keep it cached for a while more.
> We could retry destroying it when kernel has more memory.

Yes. I put FIXME for such intelligent exit (I thought about
stack slabs only not the complete fibers though).

> 
> These are of course ideas for follow ups.