From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 B398E469719 for ; Fri, 14 Feb 2020 02:26:06 +0300 (MSK) References: <20200213205618.7982-1-gorcunov@gmail.com> <20200213205618.7982-3-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: <2291dd87-04c7-dd92-d8aa-e56ad46e4681@tarantool.org> Date: Fri, 14 Feb 2020 00:26:04 +0100 MIME-Version: 1.0 In-Reply-To: <20200213205618.7982-3-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v7 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 , tml 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 > Signed-off-by: Cyrill Gorcunov > --- > 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 *** >