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
_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
