From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 8E87646970E for ; Thu, 6 Feb 2020 14:10:28 +0300 (MSK) Date: Thu, 6 Feb 2020 14:10:42 +0300 From: Alexander Turenko Message-ID: <20200206111042.hubzqcmmhkncqvrh@tkn_work_nb> References: <20200205220624.8764-1-gorcunov@gmail.com> <20200205220624.8764-3-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200205220624.8764-3-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH v5 2/2] fiber: leak slab if unable to bring prots back List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml Okay except one style comment. WBR, Alexander TUrenko. On Thu, Feb 06, 2020 at 01:06:24AM +0300, 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 I agree. > 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. > > Signed-off-by: Cyrill Gorcunov > --- > src/lib/core/fiber.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c > index 57e4cb6ef..5298541d4 100644 > --- a/src/lib/core/fiber.c > +++ b/src/lib/core/fiber.c > @@ -1055,15 +1055,21 @@ 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 @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); > + 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); Usual way (AFAIK) is to use braces around both branches or neither of them. Our code style mentions it, see [1] (from words 'Do not unnecessarily use braces'). AFAIS there are exceptions across our code, but rules are rules :) [1]: https://www.tarantool.io/en/doc/1.10/dev_guide/c_style_guide/ > } > } > > -- > 2.20.1 >