[Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Cyrill Gorcunov]

Currently we use say_x to log if error happened.
Strictly speaking when madvise is used to relax
pressue on mm subsystem since we started to use
bigger stacks (due to libreadline increased memory
usage).

Same time mprotect usage is more sensitive -- we
setup guard pages with its help and when failing
it is definitely an error which we should not only
mention in logger but provide a caller diag error.
Here is an example from memcached

 |	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 make fiber_madivse and fiber_mprotect being inline
functions instead of macros and setup diag error accordingly.

Fixes #4722

Reported-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue https://github.com/tarantool/tarantool/issues/4722
branch gorcunov/gh-4722-mprotect-diag-error

 src/lib/core/fiber.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 00ae8cded..724260bad 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)) {
+		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)) {
+		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;
-- 
2.20.1

Re: [Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Oleg Babin]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

Hello, thanks for your patch!

Our documentation doesn't contain any mentions that fiber.create can fail.

Could you please file an issue (or may be doc bot request) with 
description in what cases user

can expect that fiber.create will fail. Usually users don't use pcall 
for fiber.create and this fact can

be quite unexpected for them.


On 10/01/2020 17:27, Cyrill Gorcunov wrote:
> Currently we use say_x to log if error happened.
> Strictly speaking when madvise is used to relax
> pressue on mm subsystem since we started to use
> bigger stacks (due to libreadline increased memory
> usage).
>
> Same time mprotect usage is more sensitive -- we
> setup guard pages with its help and when failing
> it is definitely an error which we should not only
> mention in logger but provide a caller diag error.
> Here is an example from memcached
>
--
Oleg Babin


[-- Attachment #2: Type: text/html, Size: 3092 bytes --]

Re: [Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Cyrill Gorcunov]

On Sat, Jan 11, 2020 at 10:03:33AM +0300, Oleg Babin wrote:
>    Hello, thanks for your patch!
> 
>    Our documentation doesn't contain any mentions that fiber.create can fail.
> 
>    Could you please file an issue (or may be doc bot request) with
>    description in what cases user
> 
>    can expect that fiber.create will fail. Usually users don't use pcall for
>    fiber.create and this fact can
>    be quite unexpected for them.

Will do. Thanks!

Re: [Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Cyrill Gorcunov]

On Sat, Jan 11, 2020 at 10:03:33AM +0300, Oleg Babin wrote:
>    Hello, thanks for your patch!
> 
>    Our documentation doesn't contain any mentions that fiber.create can fail.
> 
>    Could you please file an issue (or may be doc bot request) with
>    description in what cases user
> 
>    can expect that fiber.create will fail. Usually users don't use pcall for
>    fiber.create and this fact can
> 
>    be quite unexpected for them.
> 

Actually we do have reference that fiber_new() can fail.
https://www.tarantool.io/ru/doc/2.3/dev_guide/reference_capi/fiber/#c.fiber_new

Re: [Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Alexander Turenko]

My understanding of error handling is so:

* Utility functions, simple wrappers and so just set a diag.
* A function that able to return an error to a user do it (set or pass
  through a diag).
* A background / delayed function logs an occured errors.

So I think that fiber_madvise() and fiber_mprotect() should set a diag:
this is correct. Usages of those functions should do one of the
following actions:

* return -1 (the diag remains set)
* ignore the error, but log it: call diag_log() and continue
* ignore w/o even logging, but this should be commented in the code

As I see, fiber_madvise() log an occured error before the patch, now the
error is ignored. I think that doing something like the following in
fiber_stack_recycle() and fiber_stack_watermark_create() would be good:

 | if (fiber_madvise(<...>) != 0)
 |     diag_log();

fiber_mprotect() error is handled in fiber_stack_create() (correctly
after the patch), but ignored in fiber_stack_destroy(). Let's log it in
the latter function.

This is how I understand a right way to handle exceptional situations:
don't sure it is right or very consistent with other code. So, please,
double-check my proposal.

Aside of that I googled a bit whether we can wrap madvise() / mprotect()
using LD_PRELOAD for unit testing and it seems it is doable:
https://tbrindus.ca/correct-ld-preload-hooking-libc/

I would not bother you with testing of errors in madvise() / mprotect()
right now, but it is good topic to experiment around it in a future.

See several minor comments below.

WBR, Alexander Turenko.

> Same time mprotect usage is more sensitive -- we
> setup guard pages with its help and when failing
> it is definitely an error which we should not only
> mention in logger but provide a caller diag error.
> Here is an example from memcached

Nit: I would say tarantool/memcached just to distinguish from the
original memcached software.

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

> -#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)) {

Nit: We usually check against zero explicitly.

> +		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)) {

Nit: Same here.

> +		diag_set(SystemError, "fiber mprotect %p:%zu:%d failed",
> +			 addr, len, prot);
> +		return -1;
> +	}
> +	return 0;
> +}

Re: [Tarantool-patches] [PATCH] fiber: set diag error on madvise, mprotect fails

[Cyrill Gorcunov]

On Wed, Jan 15, 2020 at 04:35:49PM +0300, Alexander Turenko wrote:
> My understanding of error handling is so:
> 
...
Thanks for review, Sasha! I'll try to address the comments today.