From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <4AEF0BCC.2020208@domain.hid> Date: Mon, 02 Nov 2009 17:41:48 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <20091020113724.9069.23594.stgit@domain.hid> <20091020113725.9069.28060.stgit@domain.hid> <1256404942.2862.498.camel@domain.hid> <1257177854.2065.616.camel@domain.hid> In-Reply-To: <1257177854.2065.616.camel@domain.hid> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped List-Id: Xenomai life and development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Philippe Gerum Cc: "xenomai@xenomai.org" Philippe Gerum wrote: > On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote: >> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote: >>> Allowing xnheap_delete_mapped to return an error and then attempting to >>> recover from it does not work out very well: Corner cases are racy, >>> intransparent to the user, and proper error handling imposes a lot of >>> complexity on the caller - if it actually bothers to check the return >>> value... >>> >>> Fortunately, there is no reason for this function to fail: If the heap >>> is still mapped, just install the provide cleanup handler and switch to >>> deferred removal. If the unmapping fails, we either raced with some >>> other caller of unmap or user space provided a bogus address, or >>> something else is wrong. In any case, leaving the cleanup callback >>> behind is the best we can do anyway. >>> >>> Removing the return value immediately allows to simplify the callers, >>> namemly rt_queue_delete and rt_heap_delete. >>> >>> Note: This is still not 100% waterproof. If we issue >>> xnheap_destroy_mapped from module cleanup passing a release handler >>> that belongs to the module text, deferred release will cause a crash. >>> But this corner case is no new regression, so let's keep the head in the >>> sand. >> I agree with this one, eventually. This does make things clearer, and >> removes some opportunities for the upper interfaces to shot themselves >> in the foot. Merged, thanks. > > Well, actually, it does make things clearer, but it is broken. Enabling > list debugging makes the nucleus pull the break after a double unlink in > vmclose(). > > Basically, the issue is that calling rt_queue/heap_delete() explicitly > from userland will break, due to the vmclose() handler being indirectly > called by do_munmap() for the last mapping. The nasty thing is that > without debugs on, kheapq is just silently trashed. > > Fix is on its way, along with nommu support for shared heaps as well. OK, I see. Just on minor add-on to your fix: diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c index ec14f73..1ae6af6 100644 --- a/ksrc/nucleus/heap.c +++ b/ksrc/nucleus/heap.c @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap, down_write(¤t->mm->mmap_sem); heap->archdep.release = NULL; do_munmap(current->mm, (unsigned long)mapaddr, len); + heap->archdep.release = release; up_write(¤t->mm->mmap_sem); } @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap, if (heap->archdep.numaps > 0) { /* The release handler is supposed to clean up the rest. */ XENO_ASSERT(NUCLEUS, release != NULL, /* nop */); - heap->archdep.release = release; return; } This is safer than leaving a potential race window open between dropping mmap_sem and fixing up archdep.release again. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux