Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 07/11/2011 10:12 PM, Jan Kiszka wrote: > On 2011-07-11 22:09, Gilles Chanteperdrix wrote: >> On 07/11/2011 10:06 PM, Jan Kiszka wrote: >>> On 2011-07-11 22:02, Gilles Chanteperdrix wrote: On 07/11/2011 09:59 PM, Jan Kiszka wrote: > On 2011-07-11 21:51, Gilles Chanteperdrix wrote: >> On 07/11/2011 09:16 PM, Jan Kiszka wrote: >>> On 2011-07-11 21:10, Jan Kiszka wrote: On 2011-07-11 20:53, Gilles Chanteperdrix wrote: > On 07/08/2011 06:29 PM, GIT version control wrote: >> @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct >> task_struct *p) >> magic = xnthread_get_magic(thread); >> >> xnlock_get_irqsave(&nklock, s); >> + >> +gksched = thread->gksched; >> +if (gksched) { >> +xnlock_put_irqrestore(&nklock, s); > > Are we sure irqs are on here? Are you sure that what is needed is not > an > xnlock_clear_irqon? We are in the context of do_exit. Not only IRQs are on, also preemption. And surely no nklock is held. > Furthermore, I do not understand how we > "synchronize" with the gatekeeper, how is the gatekeeper garanteed to > wait for this assignment? The gatekeeper holds the gksync token while it's active. We request it, thus we wait for the gatekeeper to become idle again. While it is idle, we reset the queued reference - but I just realized that this may tramp on other tasks' values. I need to add a check that the value to be null'ified is actually still ours. >>> >>> Thinking again, that's actually not a problem: gktarget is only needed >>> while gksync is zero - but then we won't get hold of it anyway and, >>> thus, can't cause any damage. >> >> Well, you make it look like it does not work. From what I understand, >> what you want is to set gktarget to null if a task being hardened is >> destroyed. But by waiting for the semaphore, you actually wait for the >> harden to be complete, so setting to NULL is useless. Or am I missing >> something else? > > Setting to NULL is probably unneeded but still better than rely on the > gatekeeper never waking up spuriously and then dereferencing a stale > pointer. > > The key element of this fix is waitng on gksync, thus on the completion > of the non-RT part of the hardening. Actually, this part usually fails > as the target task received a termination signal at this point. Yes, but since you wait on the completion of the hardening, the test if (target &&...) in the gatekeeper code will always be true, because at this point the cleanup code will still be waiting for the semaphore. >>> >>> Yes, except we will ever wake up the gatekeeper later on without an >>> updated gktarget, ie. spuriously. Better safe than sorry, this is hairy >>> code anyway (hopefully obsolete one day). >> >> The gatekeeper is not woken up by posting the semaphore, the gatekeeper >> is woken up by the thread which is going to be hardened (and this thread >> is the one which waits for the semaphore). > > All true. And what is the point? The point being, would not something like this patch be sufficient? diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c index 01f4200..4742c02 100644 --- a/ksrc/nucleus/shadow.c +++ b/ksrc/nucleus/shadow.c @@ -2527,6 +2527,18 @@ static inline void do_taskexit_event(struct task_struct *p) magic = xnthread_get_magic(thread); xnlock_get_irqsave(&nklock, s); + if (xnthread_test_info(thread, XNATOMIC)) { + struct xnsched *gksched = xnpod_sched_slot(task_cpu(p)); + xnlock_put_irqrestore(&nklock, s); + + /* Thread is in flight to primary mode, wait for the + gatekeeper to be done with it. */ + down(&gksched->gksync); + up(&gksched->gksync); + + xnlock_get_irqsave(&nklock, s); + } + /* Prevent wakeup call from xnshadow_unmap(). */ xnshadow_thrptd(p) = NULL; xnthread_archtcb(thread)->user_task = NULL; -- Gilles. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 2011-07-11 22:09, Gilles Chanteperdrix wrote: > On 07/11/2011 10:06 PM, Jan Kiszka wrote: >> On 2011-07-11 22:02, Gilles Chanteperdrix wrote: >>> On 07/11/2011 09:59 PM, Jan Kiszka wrote: On 2011-07-11 21:51, Gilles Chanteperdrix wrote: > On 07/11/2011 09:16 PM, Jan Kiszka wrote: >> On 2011-07-11 21:10, Jan Kiszka wrote: >>> On 2011-07-11 20:53, Gilles Chanteperdrix wrote: On 07/08/2011 06:29 PM, GIT version control wrote: > @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct > task_struct *p) > magic = xnthread_get_magic(thread); > > xnlock_get_irqsave(&nklock, s); > + > + gksched = thread->gksched; > + if (gksched) { > + xnlock_put_irqrestore(&nklock, s); Are we sure irqs are on here? Are you sure that what is needed is not an xnlock_clear_irqon? >>> >>> We are in the context of do_exit. Not only IRQs are on, also preemption. >>> And surely no nklock is held. >>> Furthermore, I do not understand how we "synchronize" with the gatekeeper, how is the gatekeeper garanteed to wait for this assignment? >>> >>> The gatekeeper holds the gksync token while it's active. We request it, >>> thus we wait for the gatekeeper to become idle again. While it is idle, >>> we reset the queued reference - but I just realized that this may tramp >>> on other tasks' values. I need to add a check that the value to be >>> null'ified is actually still ours. >> >> Thinking again, that's actually not a problem: gktarget is only needed >> while gksync is zero - but then we won't get hold of it anyway and, >> thus, can't cause any damage. > > Well, you make it look like it does not work. From what I understand, > what you want is to set gktarget to null if a task being hardened is > destroyed. But by waiting for the semaphore, you actually wait for the > harden to be complete, so setting to NULL is useless. Or am I missing > something else? Setting to NULL is probably unneeded but still better than rely on the gatekeeper never waking up spuriously and then dereferencing a stale pointer. The key element of this fix is waitng on gksync, thus on the completion of the non-RT part of the hardening. Actually, this part usually fails as the target task received a termination signal at this point. >>> >>> Yes, but since you wait on the completion of the hardening, the test >>> if (target &&...) in the gatekeeper code will always be true, because at >>> this point the cleanup code will still be waiting for the semaphore. >> >> Yes, except we will ever wake up the gatekeeper later on without an >> updated gktarget, ie. spuriously. Better safe than sorry, this is hairy >> code anyway (hopefully obsolete one day). > > The gatekeeper is not woken up by posting the semaphore, the gatekeeper > is woken up by the thread which is going to be hardened (and this thread > is the one which waits for the semaphore). All true. And what is the point? Jan signature.asc Description: OpenPGP digital signature ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 07/11/2011 10:06 PM, Jan Kiszka wrote: > On 2011-07-11 22:02, Gilles Chanteperdrix wrote: >> On 07/11/2011 09:59 PM, Jan Kiszka wrote: >>> On 2011-07-11 21:51, Gilles Chanteperdrix wrote: On 07/11/2011 09:16 PM, Jan Kiszka wrote: > On 2011-07-11 21:10, Jan Kiszka wrote: >> On 2011-07-11 20:53, Gilles Chanteperdrix wrote: >>> On 07/08/2011 06:29 PM, GIT version control wrote: @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct task_struct *p) magic = xnthread_get_magic(thread); xnlock_get_irqsave(&nklock, s); + + gksched = thread->gksched; + if (gksched) { + xnlock_put_irqrestore(&nklock, s); >>> >>> Are we sure irqs are on here? Are you sure that what is needed is not an >>> xnlock_clear_irqon? >> >> We are in the context of do_exit. Not only IRQs are on, also preemption. >> And surely no nklock is held. >> >>> Furthermore, I do not understand how we >>> "synchronize" with the gatekeeper, how is the gatekeeper garanteed to >>> wait for this assignment? >> >> The gatekeeper holds the gksync token while it's active. We request it, >> thus we wait for the gatekeeper to become idle again. While it is idle, >> we reset the queued reference - but I just realized that this may tramp >> on other tasks' values. I need to add a check that the value to be >> null'ified is actually still ours. > > Thinking again, that's actually not a problem: gktarget is only needed > while gksync is zero - but then we won't get hold of it anyway and, > thus, can't cause any damage. Well, you make it look like it does not work. From what I understand, what you want is to set gktarget to null if a task being hardened is destroyed. But by waiting for the semaphore, you actually wait for the harden to be complete, so setting to NULL is useless. Or am I missing something else? >>> >>> Setting to NULL is probably unneeded but still better than rely on the >>> gatekeeper never waking up spuriously and then dereferencing a stale >>> pointer. >>> >>> The key element of this fix is waitng on gksync, thus on the completion >>> of the non-RT part of the hardening. Actually, this part usually fails >>> as the target task received a termination signal at this point. >> >> Yes, but since you wait on the completion of the hardening, the test >> if (target &&...) in the gatekeeper code will always be true, because at >> this point the cleanup code will still be waiting for the semaphore. > > Yes, except we will ever wake up the gatekeeper later on without an > updated gktarget, ie. spuriously. Better safe than sorry, this is hairy > code anyway (hopefully obsolete one day). The gatekeeper is not woken up by posting the semaphore, the gatekeeper is woken up by the thread which is going to be hardened (and this thread is the one which waits for the semaphore). > > Jan > -- Gilles. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 2011-07-11 22:02, Gilles Chanteperdrix wrote: > On 07/11/2011 09:59 PM, Jan Kiszka wrote: >> On 2011-07-11 21:51, Gilles Chanteperdrix wrote: >>> On 07/11/2011 09:16 PM, Jan Kiszka wrote: On 2011-07-11 21:10, Jan Kiszka wrote: > On 2011-07-11 20:53, Gilles Chanteperdrix wrote: >> On 07/08/2011 06:29 PM, GIT version control wrote: >>> @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct >>> task_struct *p) >>> magic = xnthread_get_magic(thread); >>> >>> xnlock_get_irqsave(&nklock, s); >>> + >>> + gksched = thread->gksched; >>> + if (gksched) { >>> + xnlock_put_irqrestore(&nklock, s); >> >> Are we sure irqs are on here? Are you sure that what is needed is not an >> xnlock_clear_irqon? > > We are in the context of do_exit. Not only IRQs are on, also preemption. > And surely no nklock is held. > >> Furthermore, I do not understand how we >> "synchronize" with the gatekeeper, how is the gatekeeper garanteed to >> wait for this assignment? > > The gatekeeper holds the gksync token while it's active. We request it, > thus we wait for the gatekeeper to become idle again. While it is idle, > we reset the queued reference - but I just realized that this may tramp > on other tasks' values. I need to add a check that the value to be > null'ified is actually still ours. Thinking again, that's actually not a problem: gktarget is only needed while gksync is zero - but then we won't get hold of it anyway and, thus, can't cause any damage. >>> >>> Well, you make it look like it does not work. From what I understand, >>> what you want is to set gktarget to null if a task being hardened is >>> destroyed. But by waiting for the semaphore, you actually wait for the >>> harden to be complete, so setting to NULL is useless. Or am I missing >>> something else? >> >> Setting to NULL is probably unneeded but still better than rely on the >> gatekeeper never waking up spuriously and then dereferencing a stale >> pointer. >> >> The key element of this fix is waitng on gksync, thus on the completion >> of the non-RT part of the hardening. Actually, this part usually fails >> as the target task received a termination signal at this point. > > Yes, but since you wait on the completion of the hardening, the test > if (target &&...) in the gatekeeper code will always be true, because at > this point the cleanup code will still be waiting for the semaphore. Yes, except we will ever wake up the gatekeeper later on without an updated gktarget, ie. spuriously. Better safe than sorry, this is hairy code anyway (hopefully obsolete one day). Jan signature.asc Description: OpenPGP digital signature ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 07/11/2011 09:59 PM, Jan Kiszka wrote: > On 2011-07-11 21:51, Gilles Chanteperdrix wrote: >> On 07/11/2011 09:16 PM, Jan Kiszka wrote: >>> On 2011-07-11 21:10, Jan Kiszka wrote: On 2011-07-11 20:53, Gilles Chanteperdrix wrote: > On 07/08/2011 06:29 PM, GIT version control wrote: >> @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct >> task_struct *p) >> magic = xnthread_get_magic(thread); >> >> xnlock_get_irqsave(&nklock, s); >> + >> +gksched = thread->gksched; >> +if (gksched) { >> +xnlock_put_irqrestore(&nklock, s); > > Are we sure irqs are on here? Are you sure that what is needed is not an > xnlock_clear_irqon? We are in the context of do_exit. Not only IRQs are on, also preemption. And surely no nklock is held. > Furthermore, I do not understand how we > "synchronize" with the gatekeeper, how is the gatekeeper garanteed to > wait for this assignment? The gatekeeper holds the gksync token while it's active. We request it, thus we wait for the gatekeeper to become idle again. While it is idle, we reset the queued reference - but I just realized that this may tramp on other tasks' values. I need to add a check that the value to be null'ified is actually still ours. >>> >>> Thinking again, that's actually not a problem: gktarget is only needed >>> while gksync is zero - but then we won't get hold of it anyway and, >>> thus, can't cause any damage. >> >> Well, you make it look like it does not work. From what I understand, >> what you want is to set gktarget to null if a task being hardened is >> destroyed. But by waiting for the semaphore, you actually wait for the >> harden to be complete, so setting to NULL is useless. Or am I missing >> something else? > > Setting to NULL is probably unneeded but still better than rely on the > gatekeeper never waking up spuriously and then dereferencing a stale > pointer. > > The key element of this fix is waitng on gksync, thus on the completion > of the non-RT part of the hardening. Actually, this part usually fails > as the target task received a termination signal at this point. Yes, but since you wait on the completion of the hardening, the test if (target &&...) in the gatekeeper code will always be true, because at this point the cleanup code will still be waiting for the semaphore. -- Gilles. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 2011-07-11 21:51, Gilles Chanteperdrix wrote: > On 07/11/2011 09:16 PM, Jan Kiszka wrote: >> On 2011-07-11 21:10, Jan Kiszka wrote: >>> On 2011-07-11 20:53, Gilles Chanteperdrix wrote: On 07/08/2011 06:29 PM, GIT version control wrote: > @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct > task_struct *p) > magic = xnthread_get_magic(thread); > > xnlock_get_irqsave(&nklock, s); > + > + gksched = thread->gksched; > + if (gksched) { > + xnlock_put_irqrestore(&nklock, s); Are we sure irqs are on here? Are you sure that what is needed is not an xnlock_clear_irqon? >>> >>> We are in the context of do_exit. Not only IRQs are on, also preemption. >>> And surely no nklock is held. >>> Furthermore, I do not understand how we "synchronize" with the gatekeeper, how is the gatekeeper garanteed to wait for this assignment? >>> >>> The gatekeeper holds the gksync token while it's active. We request it, >>> thus we wait for the gatekeeper to become idle again. While it is idle, >>> we reset the queued reference - but I just realized that this may tramp >>> on other tasks' values. I need to add a check that the value to be >>> null'ified is actually still ours. >> >> Thinking again, that's actually not a problem: gktarget is only needed >> while gksync is zero - but then we won't get hold of it anyway and, >> thus, can't cause any damage. > > Well, you make it look like it does not work. From what I understand, > what you want is to set gktarget to null if a task being hardened is > destroyed. But by waiting for the semaphore, you actually wait for the > harden to be complete, so setting to NULL is useless. Or am I missing > something else? Setting to NULL is probably unneeded but still better than rely on the gatekeeper never waking up spuriously and then dereferencing a stale pointer. The key element of this fix is waitng on gksync, thus on the completion of the non-RT part of the hardening. Actually, this part usually fails as the target task received a termination signal at this point. Jan signature.asc Description: OpenPGP digital signature ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 07/11/2011 09:16 PM, Jan Kiszka wrote: > On 2011-07-11 21:10, Jan Kiszka wrote: >> On 2011-07-11 20:53, Gilles Chanteperdrix wrote: >>> On 07/08/2011 06:29 PM, GIT version control wrote: @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct task_struct *p) magic = xnthread_get_magic(thread); xnlock_get_irqsave(&nklock, s); + + gksched = thread->gksched; + if (gksched) { + xnlock_put_irqrestore(&nklock, s); >>> >>> Are we sure irqs are on here? Are you sure that what is needed is not an >>> xnlock_clear_irqon? >> >> We are in the context of do_exit. Not only IRQs are on, also preemption. >> And surely no nklock is held. >> >>> Furthermore, I do not understand how we >>> "synchronize" with the gatekeeper, how is the gatekeeper garanteed to >>> wait for this assignment? >> >> The gatekeeper holds the gksync token while it's active. We request it, >> thus we wait for the gatekeeper to become idle again. While it is idle, >> we reset the queued reference - but I just realized that this may tramp >> on other tasks' values. I need to add a check that the value to be >> null'ified is actually still ours. > > Thinking again, that's actually not a problem: gktarget is only needed > while gksync is zero - but then we won't get hold of it anyway and, > thus, can't cause any damage. Well, you make it look like it does not work. From what I understand, what you want is to set gktarget to null if a task being hardened is destroyed. But by waiting for the semaphore, you actually wait for the harden to be complete, so setting to NULL is useless. Or am I missing something else? -- Gilles. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 2011-07-11 21:10, Jan Kiszka wrote: > On 2011-07-11 20:53, Gilles Chanteperdrix wrote: >> On 07/08/2011 06:29 PM, GIT version control wrote: >>> @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct >>> task_struct *p) >>> magic = xnthread_get_magic(thread); >>> >>> xnlock_get_irqsave(&nklock, s); >>> + >>> + gksched = thread->gksched; >>> + if (gksched) { >>> + xnlock_put_irqrestore(&nklock, s); >> >> Are we sure irqs are on here? Are you sure that what is needed is not an >> xnlock_clear_irqon? > > We are in the context of do_exit. Not only IRQs are on, also preemption. > And surely no nklock is held. > >> Furthermore, I do not understand how we >> "synchronize" with the gatekeeper, how is the gatekeeper garanteed to >> wait for this assignment? > > The gatekeeper holds the gksync token while it's active. We request it, > thus we wait for the gatekeeper to become idle again. While it is idle, > we reset the queued reference - but I just realized that this may tramp > on other tasks' values. I need to add a check that the value to be > null'ified is actually still ours. Thinking again, that's actually not a problem: gktarget is only needed while gksync is zero - but then we won't get hold of it anyway and, thus, can't cause any damage. Jan signature.asc Description: OpenPGP digital signature ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 2011-07-11 20:53, Gilles Chanteperdrix wrote: > On 07/08/2011 06:29 PM, GIT version control wrote: >> @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct >> task_struct *p) >> magic = xnthread_get_magic(thread); >> >> xnlock_get_irqsave(&nklock, s); >> + >> +gksched = thread->gksched; >> +if (gksched) { >> +xnlock_put_irqrestore(&nklock, s); > > Are we sure irqs are on here? Are you sure that what is needed is not an > xnlock_clear_irqon? We are in the context of do_exit. Not only IRQs are on, also preemption. And surely no nklock is held. > Furthermore, I do not understand how we > "synchronize" with the gatekeeper, how is the gatekeeper garanteed to > wait for this assignment? The gatekeeper holds the gksync token while it's active. We request it, thus we wait for the gatekeeper to become idle again. While it is idle, we reset the queued reference - but I just realized that this may tramp on other tasks' values. I need to add a check that the value to be null'ified is actually still ours. Jan signature.asc Description: OpenPGP digital signature ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion
On 07/08/2011 06:29 PM, GIT version control wrote: > @@ -2528,6 +2534,22 @@ static inline void do_taskexit_event(struct > task_struct *p) > magic = xnthread_get_magic(thread); > > xnlock_get_irqsave(&nklock, s); > + > + gksched = thread->gksched; > + if (gksched) { > + xnlock_put_irqrestore(&nklock, s); Are we sure irqs are on here? Are you sure that what is needed is not an xnlock_clear_irqon? Furthermore, I do not understand how we "synchronize" with the gatekeeper, how is the gatekeeper garanteed to wait for this assignment? -- Gilles. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] Problems with gcc 4.6.0 (rt_task_shadow fails with ENOSYS)
On 07/07/2011 11:47 PM, Anders Blomdell wrote: > When compiling kernel 2.6.37.3 and xenomai 2.5.6 with "gcc version 4.6.0 > 20110530 (Red Hat 4.6.0-9) (GCC)", programs fail with -ENOSYS in > rt_task_shadow. If compiled with "gcc version 4.5.1 20100924 (Red Hat > 4.5.1-4) (GCC)" everything works as expected. I think it is due to the modifications of syscall.h I made to implement pseudo-signals handling, which implementation was never finished, and removed in xenomai-head. Could you try xenomai-head to see if you get the failure? I tried backporting it, but the code evolved too much in both repositories, the backport needs to spend some time on it, so, I would like to know whether it is really needed. -- Gilles. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] Problems with gcc 4.6.0 (rt_task_shadow fails with ENOSYS)
On 07/07/11 22:47, Anders Blomdell wrote: > When compiling kernel 2.6.37.3 and xenomai 2.5.6 with "gcc version 4.6.0 > 20110530 (Red Hat 4.6.0-9) (GCC)", programs fail with -ENOSYS in > rt_task_shadow. If compiled with "gcc version 4.5.1 20100924 (Red Hat > 4.5.1-4) (GCC)" everything works as expected. I just want to note that I encountered the same problem on Debian testing. Unfortunately I can not provide more details right now (I'm at the Amaldi9 conference in Cardiff, any Xenomai dev or user around?) Cheers, -- Daniele ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core