From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id BB0634696C3
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 14 Feb 2020 02:26:06 +0300 (MSK)
References: <20200213205618.7982-1-gorcunov@gmail.com>
 <20200213205618.7982-2-gorcunov@gmail.com>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <46c82f5e-5e4c-ec4a-b8ce-b999e6a6230c@tarantool.org>
Date: Fri, 14 Feb 2020 00:26:04 +0100
MIME-Version: 1.0
In-Reply-To: <20200213205618.7982-2-gorcunov@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v7 1/2] fiber: set diagnostics at
	madvise/mprotect failure
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org>

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;
>  	}