Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

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.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

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.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2159 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

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.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

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.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

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.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4645 bytes --]

On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
> 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));

That's not reliable, the task might have been migrated by Linux in the
meantime. We must use the stored gksched.

> +		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;
> 

Again, setting gktarget to NULL and testing for NULL is simply safer,
and I see no gain in skipping that. But if you prefer the
micro-optimization, I'll drop it.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 09:22 AM, Jan Kiszka wrote:
> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>> 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));
> 
> That's not reliable, the task might have been migrated by Linux in the
> meantime. We must use the stored gksched.
> 
>> +		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;
>>
> 
> Again, setting gktarget to NULL and testing for NULL is simply safer,

>From my point of view, testing for NULL is misleading dead code, since
it will never happen.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 09:22 AM, Jan Kiszka wrote:
> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>> 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));
> 
> That's not reliable, the task might have been migrated by Linux in the
> meantime. We must use the stored gksched.
> 
>> +		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;
>>
> 
> Again, setting gktarget to NULL and testing for NULL is simply safer,
> and I see no gain in skipping that. But if you prefer the
> micro-optimization, I'll drop it.

Could not we use an info bit instead of adding a pointer?

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5136 bytes --]

On 2011-07-12 12:59, Gilles Chanteperdrix wrote:
> On 07/12/2011 09:22 AM, Jan Kiszka wrote:
>> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>>> 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));
>>
>> That's not reliable, the task might have been migrated by Linux in the
>> meantime. We must use the stored gksched.
>>
>>> +		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;
>>>
>>
>> Again, setting gktarget to NULL and testing for NULL is simply safer,
>> and I see no gain in skipping that. But if you prefer the
>> micro-optimization, I'll drop it.
> 
> Could not we use an info bit instead of adding a pointer?
> 

"That's not reliable, the task might have been migrated by Linux in the
meantime. We must use the stored gksched."

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 01:00 PM, Jan Kiszka wrote:
> On 2011-07-12 12:59, Gilles Chanteperdrix wrote:
>> On 07/12/2011 09:22 AM, Jan Kiszka wrote:
>>> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>>>> 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));
>>>
>>> That's not reliable, the task might have been migrated by Linux in the
>>> meantime. We must use the stored gksched.
>>>
>>>> +		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;
>>>>
>>>
>>> Again, setting gktarget to NULL and testing for NULL is simply safer,
>>> and I see no gain in skipping that. But if you prefer the
>>> micro-optimization, I'll drop it.
>>
>> Could not we use an info bit instead of adding a pointer?
>>
> 
> "That's not reliable, the task might have been migrated by Linux in the
> meantime. We must use the stored gksched."

I mean add another info bit to mean that the task is queued for wakeup
by the gatekeeper.

XNGKQ, or something.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5723 bytes --]

On 2011-07-12 13:04, Gilles Chanteperdrix wrote:
> On 07/12/2011 01:00 PM, Jan Kiszka wrote:
>> On 2011-07-12 12:59, Gilles Chanteperdrix wrote:
>>> On 07/12/2011 09:22 AM, Jan Kiszka wrote:
>>>> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>>>>> 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));
>>>>
>>>> That's not reliable, the task might have been migrated by Linux in the
>>>> meantime. We must use the stored gksched.
>>>>
>>>>> +		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;
>>>>>
>>>>
>>>> Again, setting gktarget to NULL and testing for NULL is simply safer,
>>>> and I see no gain in skipping that. But if you prefer the
>>>> micro-optimization, I'll drop it.
>>>
>>> Could not we use an info bit instead of adding a pointer?
>>>
>>
>> "That's not reliable, the task might have been migrated by Linux in the
>> meantime. We must use the stored gksched."
> 
> I mean add another info bit to mean that the task is queued for wakeup
> by the gatekeeper.
> 
> XNGKQ, or something.

What additional value does it provide to gksched != NULL? We need that
pointer anyway to identify the gatekeeper that holds a reference.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 01:06 PM, Jan Kiszka wrote:
> On 2011-07-12 13:04, Gilles Chanteperdrix wrote:
>> On 07/12/2011 01:00 PM, Jan Kiszka wrote:
>>> On 2011-07-12 12:59, Gilles Chanteperdrix wrote:
>>>> On 07/12/2011 09:22 AM, Jan Kiszka wrote:
>>>>> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>>>>>> 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));
>>>>>
>>>>> That's not reliable, the task might have been migrated by Linux in the
>>>>> meantime. We must use the stored gksched.
>>>>>
>>>>>> +		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;
>>>>>>
>>>>>
>>>>> Again, setting gktarget to NULL and testing for NULL is simply safer,
>>>>> and I see no gain in skipping that. But if you prefer the
>>>>> micro-optimization, I'll drop it.
>>>>
>>>> Could not we use an info bit instead of adding a pointer?
>>>>
>>>
>>> "That's not reliable, the task might have been migrated by Linux in the
>>> meantime. We must use the stored gksched."
>>
>> I mean add another info bit to mean that the task is queued for wakeup
>> by the gatekeeper.
>>
>> XNGKQ, or something.
> 
> What additional value does it provide to gksched != NULL? We need that
> pointer anyway to identify the gatekeeper that holds a reference.

No, the scheduler which holds the reference is xnpod_sched_slot(task_cpu(p))

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6247 bytes --]

On 2011-07-12 13:08, Gilles Chanteperdrix wrote:
> On 07/12/2011 01:06 PM, Jan Kiszka wrote:
>> On 2011-07-12 13:04, Gilles Chanteperdrix wrote:
>>> On 07/12/2011 01:00 PM, Jan Kiszka wrote:
>>>> On 2011-07-12 12:59, Gilles Chanteperdrix wrote:
>>>>> On 07/12/2011 09:22 AM, Jan Kiszka wrote:
>>>>>> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>>>>>>> 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));
>>>>>>
>>>>>> That's not reliable, the task might have been migrated by Linux in the
>>>>>> meantime. We must use the stored gksched.
>>>>>>
>>>>>>> +		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;
>>>>>>>
>>>>>>
>>>>>> Again, setting gktarget to NULL and testing for NULL is simply safer,
>>>>>> and I see no gain in skipping that. But if you prefer the
>>>>>> micro-optimization, I'll drop it.
>>>>>
>>>>> Could not we use an info bit instead of adding a pointer?
>>>>>
>>>>
>>>> "That's not reliable, the task might have been migrated by Linux in the
>>>> meantime. We must use the stored gksched."
>>>
>>> I mean add another info bit to mean that the task is queued for wakeup
>>> by the gatekeeper.
>>>
>>> XNGKQ, or something.
>>
>> What additional value does it provide to gksched != NULL? We need that
>> pointer anyway to identify the gatekeeper that holds a reference.
> 
> No, the scheduler which holds the reference is xnpod_sched_slot(task_cpu(p))

And that is still not reliable, the task may have been pushed to a
different CPU in the meantime.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 01:10 PM, Jan Kiszka wrote:
> On 2011-07-12 13:08, Gilles Chanteperdrix wrote:
>> On 07/12/2011 01:06 PM, Jan Kiszka wrote:
>>> On 2011-07-12 13:04, Gilles Chanteperdrix wrote:
>>>> On 07/12/2011 01:00 PM, Jan Kiszka wrote:
>>>>> On 2011-07-12 12:59, Gilles Chanteperdrix wrote:
>>>>>> On 07/12/2011 09:22 AM, Jan Kiszka wrote:
>>>>>>> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>>>>>>>> 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));
>>>>>>>
>>>>>>> That's not reliable, the task might have been migrated by Linux in the
>>>>>>> meantime. We must use the stored gksched.
>>>>>>>
>>>>>>>> +		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;
>>>>>>>>
>>>>>>>
>>>>>>> Again, setting gktarget to NULL and testing for NULL is simply safer,
>>>>>>> and I see no gain in skipping that. But if you prefer the
>>>>>>> micro-optimization, I'll drop it.
>>>>>>
>>>>>> Could not we use an info bit instead of adding a pointer?
>>>>>>
>>>>>
>>>>> "That's not reliable, the task might have been migrated by Linux in the
>>>>> meantime. We must use the stored gksched."
>>>>
>>>> I mean add another info bit to mean that the task is queued for wakeup
>>>> by the gatekeeper.
>>>>
>>>> XNGKQ, or something.
>>>
>>> What additional value does it provide to gksched != NULL? We need that
>>> pointer anyway to identify the gatekeeper that holds a reference.
>>
>> No, the scheduler which holds the reference is xnpod_sched_slot(task_cpu(p))
> 
> And that is still not reliable, the task may have been pushed to a
> different CPU in the meantime.

I find all this complicated for a very small corner-case, so, I keep
looking for a simpler solution. Let us try something else.

If the thread is woken up for whatever reason before the gatekeeper,
then we will hit the following "if" in xnshadow_harden, can we set the
target to NULL at this point if it is the current thread? With a cmpxchg
perhaps?

	/*
	 * Rare case: we might have been awaken by a signal before the
	 * gatekeeper sent us to primary mode. Since
	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
	 * the runqueue's count of uniniterruptible tasks, we just
	 * notice the issue and gracefully fail; the caller will have
	 * to process this signal anyway.
	 */
	if (rthal_current_domain == rthal_root_domain) {
		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
		    || this_task->state != TASK_RUNNING))
			xnpod_fatal
			    ("xnshadow_harden() failed for thread %s[%d]",
			     thread->name, xnthread_user_pid(thread));
		return -ERESTARTSYS;
	}


> 
> Jan
> 


-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 7107 bytes --]

On 2011-07-12 13:26, Gilles Chanteperdrix wrote:
> On 07/12/2011 01:10 PM, Jan Kiszka wrote:
>> On 2011-07-12 13:08, Gilles Chanteperdrix wrote:
>>> On 07/12/2011 01:06 PM, Jan Kiszka wrote:
>>>> On 2011-07-12 13:04, Gilles Chanteperdrix wrote:
>>>>> On 07/12/2011 01:00 PM, Jan Kiszka wrote:
>>>>>> On 2011-07-12 12:59, Gilles Chanteperdrix wrote:
>>>>>>> On 07/12/2011 09:22 AM, Jan Kiszka wrote:
>>>>>>>> On 2011-07-12 08:41, Gilles Chanteperdrix wrote:
>>>>>>>>> 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));
>>>>>>>>
>>>>>>>> That's not reliable, the task might have been migrated by Linux in the
>>>>>>>> meantime. We must use the stored gksched.
>>>>>>>>
>>>>>>>>> +		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;
>>>>>>>>>
>>>>>>>>
>>>>>>>> Again, setting gktarget to NULL and testing for NULL is simply safer,
>>>>>>>> and I see no gain in skipping that. But if you prefer the
>>>>>>>> micro-optimization, I'll drop it.
>>>>>>>
>>>>>>> Could not we use an info bit instead of adding a pointer?
>>>>>>>
>>>>>>
>>>>>> "That's not reliable, the task might have been migrated by Linux in the
>>>>>> meantime. We must use the stored gksched."
>>>>>
>>>>> I mean add another info bit to mean that the task is queued for wakeup
>>>>> by the gatekeeper.
>>>>>
>>>>> XNGKQ, or something.
>>>>
>>>> What additional value does it provide to gksched != NULL? We need that
>>>> pointer anyway to identify the gatekeeper that holds a reference.
>>>
>>> No, the scheduler which holds the reference is xnpod_sched_slot(task_cpu(p))
>>
>> And that is still not reliable, the task may have been pushed to a
>> different CPU in the meantime.
> 
> I find all this complicated for a very small corner-case, so, I keep
> looking for a simpler solution. Let us try something else.

It's rather the contrary: this solution is straightforward IMHO.

> 
> If the thread is woken up for whatever reason before the gatekeeper,
> then we will hit the following "if" in xnshadow_harden, can we set the
> target to NULL at this point if it is the current thread? With a cmpxchg
> perhaps?

Gatekeeper and target task aren't synchronized, are they?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 01:29 PM, Jan Kiszka wrote:
>> I find all this complicated for a very small corner-case, so, I keep
>> looking for a simpler solution. Let us try something else.
> 
> It's rather the contrary: this solution is straightforward IMHO.
> 
>>
>> If the thread is woken up for whatever reason before the gatekeeper,
>> then we will hit the following "if" in xnshadow_harden, can we set the
>> target to NULL at this point if it is the current thread? With a cmpxchg
>> perhaps?
> 
> Gatekeeper and target task aren't synchronized, are they?

That is another reason why the solution to synchronize using the
semaphore may not work. If the thread hits the call to down before the
gatekeeper is woken up, then the gatekeeper test may see the target
thread as suspended, and we may return from the call to "down" in
xenomai domain.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

On 2011-07-12 13:41, Gilles Chanteperdrix wrote:
> On 07/12/2011 01:29 PM, Jan Kiszka wrote:
>>> I find all this complicated for a very small corner-case, so, I keep
>>> looking for a simpler solution. Let us try something else.
>>
>> It's rather the contrary: this solution is straightforward IMHO.
>>
>>>
>>> If the thread is woken up for whatever reason before the gatekeeper,
>>> then we will hit the following "if" in xnshadow_harden, can we set the
>>> target to NULL at this point if it is the current thread? With a cmpxchg
>>> perhaps?
>>
>> Gatekeeper and target task aren't synchronized, are they?
> 
> That is another reason why the solution to synchronize using the
> semaphore may not work. If the thread hits the call to down before the

"thread" means do_taskexit_event here?

> gatekeeper is woken up, then the gatekeeper test may see the target
> thread as suspended, and we may return from the call to "down" in
> xenomai domain.

Good point, but fortunately not possible: If we block on down() in
do_taskexit_event, we enter TASK_UNINTERRUPTIBLE state. But the
gatekeeper checks for TASK_INTERRUPTIBLE.

However, this parallel unsynchronized execution of the gatekeeper and
its target thread leaves an increasingly bad feeling on my side. Did we
really catch all corner cases now? I wouldn't guarantee that yet.
Specifically as I still have an obscure crash of a Xenomai thread on
Linux schedule() on my table.

What if the target thread woke up due to a signal, continued much
further on a different CPU, blocked in TASK_INTERRUPTIBLE, and then the
gatekeeper continued? I wish we could already eliminate this complexity
and do the migration directly inside schedule()...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

On 2011-07-12 13:56, Jan Kiszka wrote:
> However, this parallel unsynchronized execution of the gatekeeper and
> its target thread leaves an increasingly bad feeling on my side. Did we
> really catch all corner cases now? I wouldn't guarantee that yet.
> Specifically as I still have an obscure crash of a Xenomai thread on
> Linux schedule() on my table.
> 
> What if the target thread woke up due to a signal, continued much
> further on a different CPU, blocked in TASK_INTERRUPTIBLE, and then the
> gatekeeper continued? I wish we could already eliminate this complexity
> and do the migration directly inside schedule()...

BTW, we do we mask out TASK_ATOMICSWITCH when checking the task state in
the gatekeeper? What would happen if we included it (state ==
(TASK_ATOMICSWITCH | TASK_INTERRUPTIBLE))?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 01:58 PM, Jan Kiszka wrote:
> On 2011-07-12 13:56, Jan Kiszka wrote:
>> However, this parallel unsynchronized execution of the gatekeeper and
>> its target thread leaves an increasingly bad feeling on my side. Did we
>> really catch all corner cases now? I wouldn't guarantee that yet.
>> Specifically as I still have an obscure crash of a Xenomai thread on
>> Linux schedule() on my table.
>>
>> What if the target thread woke up due to a signal, continued much
>> further on a different CPU, blocked in TASK_INTERRUPTIBLE, and then the
>> gatekeeper continued? I wish we could already eliminate this complexity
>> and do the migration directly inside schedule()...
> 
> BTW, we do we mask out TASK_ATOMICSWITCH when checking the task state in
> the gatekeeper? What would happen if we included it (state ==
> (TASK_ATOMICSWITCH | TASK_INTERRUPTIBLE))?

I would tend to think that what we should check is
xnthread_test_info(XNATOMIC). Or maybe check both, the interruptible
state and the XNATOMIC info bit.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

On 2011-07-12 14:06, Gilles Chanteperdrix wrote:
> On 07/12/2011 01:58 PM, Jan Kiszka wrote:
>> On 2011-07-12 13:56, Jan Kiszka wrote:
>>> However, this parallel unsynchronized execution of the gatekeeper and
>>> its target thread leaves an increasingly bad feeling on my side. Did we
>>> really catch all corner cases now? I wouldn't guarantee that yet.
>>> Specifically as I still have an obscure crash of a Xenomai thread on
>>> Linux schedule() on my table.
>>>
>>> What if the target thread woke up due to a signal, continued much
>>> further on a different CPU, blocked in TASK_INTERRUPTIBLE, and then the
>>> gatekeeper continued? I wish we could already eliminate this complexity
>>> and do the migration directly inside schedule()...
>>
>> BTW, we do we mask out TASK_ATOMICSWITCH when checking the task state in
>> the gatekeeper? What would happen if we included it (state ==
>> (TASK_ATOMICSWITCH | TASK_INTERRUPTIBLE))?
> 
> I would tend to think that what we should check is
> xnthread_test_info(XNATOMIC). Or maybe check both, the interruptible
> state and the XNATOMIC info bit.

Actually, neither the info bits nor the task state is sufficiently
synchronized against the gatekeeper yet. We need to hold a shared lock
when testing and resetting the state. I'm not sure yet if that is
fixable given the gatekeeper architecture.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4141 bytes --]

On 2011-07-12 14:13, Jan Kiszka wrote:
> On 2011-07-12 14:06, Gilles Chanteperdrix wrote:
>> On 07/12/2011 01:58 PM, Jan Kiszka wrote:
>>> On 2011-07-12 13:56, Jan Kiszka wrote:
>>>> However, this parallel unsynchronized execution of the gatekeeper and
>>>> its target thread leaves an increasingly bad feeling on my side. Did we
>>>> really catch all corner cases now? I wouldn't guarantee that yet.
>>>> Specifically as I still have an obscure crash of a Xenomai thread on
>>>> Linux schedule() on my table.
>>>>
>>>> What if the target thread woke up due to a signal, continued much
>>>> further on a different CPU, blocked in TASK_INTERRUPTIBLE, and then the
>>>> gatekeeper continued? I wish we could already eliminate this complexity
>>>> and do the migration directly inside schedule()...
>>>
>>> BTW, we do we mask out TASK_ATOMICSWITCH when checking the task state in
>>> the gatekeeper? What would happen if we included it (state ==
>>> (TASK_ATOMICSWITCH | TASK_INTERRUPTIBLE))?
>>
>> I would tend to think that what we should check is
>> xnthread_test_info(XNATOMIC). Or maybe check both, the interruptible
>> state and the XNATOMIC info bit.
> 
> Actually, neither the info bits nor the task state is sufficiently
> synchronized against the gatekeeper yet. We need to hold a shared lock
> when testing and resetting the state. I'm not sure yet if that is
> fixable given the gatekeeper architecture.
> 

This may work (on top of the exit-race fix):

diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
index 50dcf43..90feb16 100644
--- a/ksrc/nucleus/shadow.c
+++ b/ksrc/nucleus/shadow.c
@@ -913,20 +913,27 @@ static int gatekeeper_thread(void *data)
 		if ((xnthread_user_task(target)->state & ~TASK_ATOMICSWITCH) == TASK_INTERRUPTIBLE) {
 			rpi_pop(target);
 			xnlock_get_irqsave(&nklock, s);
-#ifdef CONFIG_SMP
+
 			/*
-			 * If the task changed its CPU while in
-			 * secondary mode, change the CPU of the
-			 * underlying Xenomai shadow too. We do not
-			 * migrate the thread timers here, it would
-			 * not work. For a "full" migration comprising
-			 * timers, using xnpod_migrate_thread is
-			 * required.
+			 * Recheck XNATOMIC to avoid waking the shadow if the
+			 * Linux task received a signal meanwhile.
 			 */
-			if (target->sched != sched)
-				xnsched_migrate_passive(target, sched);
+			if (xnthread_test_info(target, XNATOMIC)) {
+#ifdef CONFIG_SMP
+				/*
+				 * If the task changed its CPU while in
+				 * secondary mode, change the CPU of the
+				 * underlying Xenomai shadow too. We do not
+				 * migrate the thread timers here, it would
+				 * not work. For a "full" migration comprising
+				 * timers, using xnpod_migrate_thread is
+				 * required.
+				 */
+				if (target->sched != sched)
+					xnsched_migrate_passive(target, sched);
 #endif /* CONFIG_SMP */
-			xnpod_resume_thread(target, XNRELAX);
+				xnpod_resume_thread(target, XNRELAX);
+			}
 			xnlock_put_irqrestore(&nklock, s);
 			xnpod_schedule();
 		}
@@ -1036,6 +1043,7 @@ redo:
 	 * to process this signal anyway.
 	 */
 	if (rthal_current_domain == rthal_root_domain) {
+		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
 		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
 		    || this_task->state != TASK_RUNNING))
 			xnpod_fatal
@@ -1044,6 +1052,8 @@ redo:
 		return -ERESTARTSYS;
 	}
 
+	xnthread_clear_info(thread, XNATOMIC);
+
 	/* "current" is now running into the Xenomai domain. */
 	thread->gksched = NULL;
 	sched = xnsched_finish_unlocked_switch(thread->sched);
@@ -2650,6 +2660,8 @@ static inline void do_sigwake_event(struct task_struct *p)
 
 	xnlock_get_irqsave(&nklock, s);
 
+	xnthread_clear_info(thread, XNATOMIC);
+
 	if ((p->ptrace & PT_PTRACED) && !xnthread_test_state(thread, XNDEBUG)) {
 		sigset_t pending;
 

It totally ignores RPI and PREEMPT_RT for now. RPI is broken anyway,
ripping it out would allow to use solely XNATOMIC as condition in the
gatekeeper.

/me is now looking to get this applied to a testbox that still crashes
suspiciously.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Philippe Gerum]

On Tue, 2011-07-12 at 14:57 +0200, Jan Kiszka wrote:
> On 2011-07-12 14:13, Jan Kiszka wrote:
> > On 2011-07-12 14:06, Gilles Chanteperdrix wrote:
> >> On 07/12/2011 01:58 PM, Jan Kiszka wrote:
> >>> On 2011-07-12 13:56, Jan Kiszka wrote:
> >>>> However, this parallel unsynchronized execution of the gatekeeper and
> >>>> its target thread leaves an increasingly bad feeling on my side. Did we
> >>>> really catch all corner cases now? I wouldn't guarantee that yet.
> >>>> Specifically as I still have an obscure crash of a Xenomai thread on
> >>>> Linux schedule() on my table.
> >>>>
> >>>> What if the target thread woke up due to a signal, continued much
> >>>> further on a different CPU, blocked in TASK_INTERRUPTIBLE, and then the
> >>>> gatekeeper continued? I wish we could already eliminate this complexity
> >>>> and do the migration directly inside schedule()...
> >>>
> >>> BTW, we do we mask out TASK_ATOMICSWITCH when checking the task state in
> >>> the gatekeeper? What would happen if we included it (state ==
> >>> (TASK_ATOMICSWITCH | TASK_INTERRUPTIBLE))?
> >>
> >> I would tend to think that what we should check is
> >> xnthread_test_info(XNATOMIC). Or maybe check both, the interruptible
> >> state and the XNATOMIC info bit.
> > 
> > Actually, neither the info bits nor the task state is sufficiently
> > synchronized against the gatekeeper yet. We need to hold a shared lock
> > when testing and resetting the state. I'm not sure yet if that is
> > fixable given the gatekeeper architecture.
> > 
> 
> This may work (on top of the exit-race fix):
> 
> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
> index 50dcf43..90feb16 100644
> --- a/ksrc/nucleus/shadow.c
> +++ b/ksrc/nucleus/shadow.c
> @@ -913,20 +913,27 @@ static int gatekeeper_thread(void *data)
>  		if ((xnthread_user_task(target)->state & ~TASK_ATOMICSWITCH) == TASK_INTERRUPTIBLE) {
>  			rpi_pop(target);
>  			xnlock_get_irqsave(&nklock, s);
> -#ifdef CONFIG_SMP
> +
>  			/*
> -			 * If the task changed its CPU while in
> -			 * secondary mode, change the CPU of the
> -			 * underlying Xenomai shadow too. We do not
> -			 * migrate the thread timers here, it would
> -			 * not work. For a "full" migration comprising
> -			 * timers, using xnpod_migrate_thread is
> -			 * required.
> +			 * Recheck XNATOMIC to avoid waking the shadow if the
> +			 * Linux task received a signal meanwhile.
>  			 */
> -			if (target->sched != sched)
> -				xnsched_migrate_passive(target, sched);
> +			if (xnthread_test_info(target, XNATOMIC)) {
> +#ifdef CONFIG_SMP
> +				/*
> +				 * If the task changed its CPU while in
> +				 * secondary mode, change the CPU of the
> +				 * underlying Xenomai shadow too. We do not
> +				 * migrate the thread timers here, it would
> +				 * not work. For a "full" migration comprising
> +				 * timers, using xnpod_migrate_thread is
> +				 * required.
> +				 */
> +				if (target->sched != sched)
> +					xnsched_migrate_passive(target, sched);
>  #endif /* CONFIG_SMP */
> -			xnpod_resume_thread(target, XNRELAX);
> +				xnpod_resume_thread(target, XNRELAX);
> +			}
>  			xnlock_put_irqrestore(&nklock, s);
>  			xnpod_schedule();
>  		}
> @@ -1036,6 +1043,7 @@ redo:
>  	 * to process this signal anyway.
>  	 */
>  	if (rthal_current_domain == rthal_root_domain) {
> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>  		    || this_task->state != TASK_RUNNING))
>  			xnpod_fatal
> @@ -1044,6 +1052,8 @@ redo:
>  		return -ERESTARTSYS;
>  	}
>  
> +	xnthread_clear_info(thread, XNATOMIC);
> +
>  	/* "current" is now running into the Xenomai domain. */
>  	thread->gksched = NULL;
>  	sched = xnsched_finish_unlocked_switch(thread->sched);
> @@ -2650,6 +2660,8 @@ static inline void do_sigwake_event(struct task_struct *p)
>  
>  	xnlock_get_irqsave(&nklock, s);
>  
> +	xnthread_clear_info(thread, XNATOMIC);
> +
>  	if ((p->ptrace & PT_PTRACED) && !xnthread_test_state(thread, XNDEBUG)) {
>  		sigset_t pending;
>  
> 
> It totally ignores RPI and PREEMPT_RT for now. RPI is broken anyway,

I want to drop RPI in v3 for sure because it is misleading people. I'm
still pondering whether we should do that earlier during the 2.6
timeframe.

> ripping it out would allow to use solely XNATOMIC as condition in the
> gatekeeper.
> 
> /me is now looking to get this applied to a testbox that still crashes
> suspiciously.
> 
> Jan
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core

-- 
Philippe.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4833 bytes --]

On 2011-07-12 17:48, Philippe Gerum wrote:
> On Tue, 2011-07-12 at 14:57 +0200, Jan Kiszka wrote:
>> On 2011-07-12 14:13, Jan Kiszka wrote:
>>> On 2011-07-12 14:06, Gilles Chanteperdrix wrote:
>>>> On 07/12/2011 01:58 PM, Jan Kiszka wrote:
>>>>> On 2011-07-12 13:56, Jan Kiszka wrote:
>>>>>> However, this parallel unsynchronized execution of the gatekeeper and
>>>>>> its target thread leaves an increasingly bad feeling on my side. Did we
>>>>>> really catch all corner cases now? I wouldn't guarantee that yet.
>>>>>> Specifically as I still have an obscure crash of a Xenomai thread on
>>>>>> Linux schedule() on my table.
>>>>>>
>>>>>> What if the target thread woke up due to a signal, continued much
>>>>>> further on a different CPU, blocked in TASK_INTERRUPTIBLE, and then the
>>>>>> gatekeeper continued? I wish we could already eliminate this complexity
>>>>>> and do the migration directly inside schedule()...
>>>>>
>>>>> BTW, we do we mask out TASK_ATOMICSWITCH when checking the task state in
>>>>> the gatekeeper? What would happen if we included it (state ==
>>>>> (TASK_ATOMICSWITCH | TASK_INTERRUPTIBLE))?
>>>>
>>>> I would tend to think that what we should check is
>>>> xnthread_test_info(XNATOMIC). Or maybe check both, the interruptible
>>>> state and the XNATOMIC info bit.
>>>
>>> Actually, neither the info bits nor the task state is sufficiently
>>> synchronized against the gatekeeper yet. We need to hold a shared lock
>>> when testing and resetting the state. I'm not sure yet if that is
>>> fixable given the gatekeeper architecture.
>>>
>>
>> This may work (on top of the exit-race fix):
>>
>> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
>> index 50dcf43..90feb16 100644
>> --- a/ksrc/nucleus/shadow.c
>> +++ b/ksrc/nucleus/shadow.c
>> @@ -913,20 +913,27 @@ static int gatekeeper_thread(void *data)
>>  		if ((xnthread_user_task(target)->state & ~TASK_ATOMICSWITCH) == TASK_INTERRUPTIBLE) {
>>  			rpi_pop(target);
>>  			xnlock_get_irqsave(&nklock, s);
>> -#ifdef CONFIG_SMP
>> +
>>  			/*
>> -			 * If the task changed its CPU while in
>> -			 * secondary mode, change the CPU of the
>> -			 * underlying Xenomai shadow too. We do not
>> -			 * migrate the thread timers here, it would
>> -			 * not work. For a "full" migration comprising
>> -			 * timers, using xnpod_migrate_thread is
>> -			 * required.
>> +			 * Recheck XNATOMIC to avoid waking the shadow if the
>> +			 * Linux task received a signal meanwhile.
>>  			 */
>> -			if (target->sched != sched)
>> -				xnsched_migrate_passive(target, sched);
>> +			if (xnthread_test_info(target, XNATOMIC)) {
>> +#ifdef CONFIG_SMP
>> +				/*
>> +				 * If the task changed its CPU while in
>> +				 * secondary mode, change the CPU of the
>> +				 * underlying Xenomai shadow too. We do not
>> +				 * migrate the thread timers here, it would
>> +				 * not work. For a "full" migration comprising
>> +				 * timers, using xnpod_migrate_thread is
>> +				 * required.
>> +				 */
>> +				if (target->sched != sched)
>> +					xnsched_migrate_passive(target, sched);
>>  #endif /* CONFIG_SMP */
>> -			xnpod_resume_thread(target, XNRELAX);
>> +				xnpod_resume_thread(target, XNRELAX);
>> +			}
>>  			xnlock_put_irqrestore(&nklock, s);
>>  			xnpod_schedule();
>>  		}
>> @@ -1036,6 +1043,7 @@ redo:
>>  	 * to process this signal anyway.
>>  	 */
>>  	if (rthal_current_domain == rthal_root_domain) {
>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>  		    || this_task->state != TASK_RUNNING))
>>  			xnpod_fatal
>> @@ -1044,6 +1052,8 @@ redo:
>>  		return -ERESTARTSYS;
>>  	}
>>  
>> +	xnthread_clear_info(thread, XNATOMIC);
>> +
>>  	/* "current" is now running into the Xenomai domain. */
>>  	thread->gksched = NULL;
>>  	sched = xnsched_finish_unlocked_switch(thread->sched);
>> @@ -2650,6 +2660,8 @@ static inline void do_sigwake_event(struct task_struct *p)
>>  
>>  	xnlock_get_irqsave(&nklock, s);
>>  
>> +	xnthread_clear_info(thread, XNATOMIC);
>> +
>>  	if ((p->ptrace & PT_PTRACED) && !xnthread_test_state(thread, XNDEBUG)) {
>>  		sigset_t pending;
>>  
>>
>> It totally ignores RPI and PREEMPT_RT for now. RPI is broken anyway,
> 
> I want to drop RPI in v3 for sure because it is misleading people. I'm
> still pondering whether we should do that earlier during the 2.6
> timeframe.

That would only leave us with XNATOMIC being used under PREEMPT-RT for
signaling LO_GKWAKE_REQ on schedule out while my patch may clear it on
signal arrival. That would prevent a gatekeeper wakeup and lead to a
deadlock. I guess we need some separate flag.

Anyway, this patch was no magic bullet yet. Our crashes persist.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>  			xnlock_put_irqrestore(&nklock, s);
>  			xnpod_schedule();
>  		}
> @@ -1036,6 +1043,7 @@ redo:
>  	 * to process this signal anyway.
>  	 */
>  	if (rthal_current_domain == rthal_root_domain) {
> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));

Misleading dead code again, XNATOMIC is cleared not ten lines above.

>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>  		    || this_task->state != TASK_RUNNING))
>  			xnpod_fatal
> @@ -1044,6 +1052,8 @@ redo:
>  		return -ERESTARTSYS;
>  	}
>  
> +	xnthread_clear_info(thread, XNATOMIC);

Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
place at the point it currently is.

> +
>  	/* "current" is now running into the Xenomai domain. */
>  	thread->gksched = NULL;
>  	sched = xnsched_finish_unlocked_switch(thread->sched);
> @@ -2650,6 +2660,8 @@ static inline void do_sigwake_event(struct task_struct *p)
>  
>  	xnlock_get_irqsave(&nklock, s);
>  
> +	xnthread_clear_info(thread, XNATOMIC);
> +

Ditto.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>  			xnlock_put_irqrestore(&nklock, s);
>>  			xnpod_schedule();
>>  		}
>> @@ -1036,6 +1043,7 @@ redo:
>>  	 * to process this signal anyway.
>>  	 */
>>  	if (rthal_current_domain == rthal_root_domain) {
>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
> 
> Misleading dead code again, XNATOMIC is cleared not ten lines above.

Nope, I forgot to remove that line.

> 
>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>  		    || this_task->state != TASK_RUNNING))
>>  			xnpod_fatal
>> @@ -1044,6 +1052,8 @@ redo:
>>  		return -ERESTARTSYS;
>>  	}
>>  
>> +	xnthread_clear_info(thread, XNATOMIC);
> 
> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
> place at the point it currently is.

Nope. Now we either clear XNATOMIC after successful migration or when
the signal is about to be sent (ie. in the hook). That way we can test
more reliably (TM) in the gatekeeper if the thread can be migrated.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 07:34 PM, Jan Kiszka wrote:
> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>  			xnlock_put_irqrestore(&nklock, s);
>>>  			xnpod_schedule();
>>>  		}
>>> @@ -1036,6 +1043,7 @@ redo:
>>>  	 * to process this signal anyway.
>>>  	 */
>>>  	if (rthal_current_domain == rthal_root_domain) {
>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>
>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
> 
> Nope, I forgot to remove that line.
> 
>>
>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>  		    || this_task->state != TASK_RUNNING))
>>>  			xnpod_fatal
>>> @@ -1044,6 +1052,8 @@ redo:
>>>  		return -ERESTARTSYS;
>>>  	}
>>>  
>>> +	xnthread_clear_info(thread, XNATOMIC);
>>
>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>> place at the point it currently is.
> 
> Nope. Now we either clear XNATOMIC after successful migration or when
> the signal is about to be sent (ie. in the hook). That way we can test
> more reliably (TM) in the gatekeeper if the thread can be migrated.

Ok for adding the XNATOMIC test, because it improves the robustness, but
why changing the way XNATOMIC is set and clear? Chances of breaking
thing while changing code in this area are really high...

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>  			xnlock_put_irqrestore(&nklock, s);
>>>>  			xnpod_schedule();
>>>>  		}
>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>  	 * to process this signal anyway.
>>>>  	 */
>>>>  	if (rthal_current_domain == rthal_root_domain) {
>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>>
>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>
>> Nope, I forgot to remove that line.
>>
>>>
>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>  		    || this_task->state != TASK_RUNNING))
>>>>  			xnpod_fatal
>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>  		return -ERESTARTSYS;
>>>>  	}
>>>>  
>>>> +	xnthread_clear_info(thread, XNATOMIC);
>>>
>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>> place at the point it currently is.
>>
>> Nope. Now we either clear XNATOMIC after successful migration or when
>> the signal is about to be sent (ie. in the hook). That way we can test
>> more reliably (TM) in the gatekeeper if the thread can be migrated.
> 
> Ok for adding the XNATOMIC test, because it improves the robustness, but
> why changing the way XNATOMIC is set and clear? Chances of breaking
> thing while changing code in this area are really high...

The current code is (most probably) broken as it does not properly
synchronizes the gatekeeper against a signaled and "runaway" target
Linux task.

We need an indication if a Linux signal will (or already has) woken up
the to-be-migrated task. That task may have continued over its context,
potentially on a different CPU. Providing this indication is the purpose
of changing where XNATOMIC is cleared.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/12/2011 07:43 PM, Jan Kiszka wrote:
> On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
>> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>>  			xnlock_put_irqrestore(&nklock, s);
>>>>>  			xnpod_schedule();
>>>>>  		}
>>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>>  	 * to process this signal anyway.
>>>>>  	 */
>>>>>  	if (rthal_current_domain == rthal_root_domain) {
>>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>>>
>>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>>
>>> Nope, I forgot to remove that line.
>>>
>>>>
>>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>>  		    || this_task->state != TASK_RUNNING))
>>>>>  			xnpod_fatal
>>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>>  		return -ERESTARTSYS;
>>>>>  	}
>>>>>  
>>>>> +	xnthread_clear_info(thread, XNATOMIC);
>>>>
>>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>>> place at the point it currently is.
>>>
>>> Nope. Now we either clear XNATOMIC after successful migration or when
>>> the signal is about to be sent (ie. in the hook). That way we can test
>>> more reliably (TM) in the gatekeeper if the thread can be migrated.
>>
>> Ok for adding the XNATOMIC test, because it improves the robustness, but
>> why changing the way XNATOMIC is set and clear? Chances of breaking
>> thing while changing code in this area are really high...
> 
> The current code is (most probably) broken as it does not properly
> synchronizes the gatekeeper against a signaled and "runaway" target
> Linux task.
> 
> We need an indication if a Linux signal will (or already has) woken up
> the to-be-migrated task. That task may have continued over its context,
> potentially on a different CPU. Providing this indication is the purpose
> of changing where XNATOMIC is cleared.

What about synchronizing with the gatekeeper with a semaphore, as done
in the first patch you sent, but doing it in xnshadow_harden, as soon as
we detect that we are not back from schedule in primary mode? It seems
it would avoid any further issue, as we would then be guaranteed that
the thread could not switch to TASK_INTERRUPTIBLE again before the
gatekeeper is finished.

What worries me is the comment in xnshadow_harden:

	 * gatekeeper sent us to primary mode. Since
	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
	 * the runqueue's count of uniniterruptible tasks, we just
	 * notice the issue and gracefully fail; the caller will have
	 * to process this signal anyway.
	 */

Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
business of xnshadow_harden?

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3929 bytes --]

On 2011-07-13 20:39, Gilles Chanteperdrix wrote:
> On 07/12/2011 07:43 PM, Jan Kiszka wrote:
>> On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
>>> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>>>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>>>  			xnlock_put_irqrestore(&nklock, s);
>>>>>>  			xnpod_schedule();
>>>>>>  		}
>>>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>>>  	 * to process this signal anyway.
>>>>>>  	 */
>>>>>>  	if (rthal_current_domain == rthal_root_domain) {
>>>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>>>>
>>>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>>>
>>>> Nope, I forgot to remove that line.
>>>>
>>>>>
>>>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>>>  		    || this_task->state != TASK_RUNNING))
>>>>>>  			xnpod_fatal
>>>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>>>  		return -ERESTARTSYS;
>>>>>>  	}
>>>>>>  
>>>>>> +	xnthread_clear_info(thread, XNATOMIC);
>>>>>
>>>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>>>> place at the point it currently is.
>>>>
>>>> Nope. Now we either clear XNATOMIC after successful migration or when
>>>> the signal is about to be sent (ie. in the hook). That way we can test
>>>> more reliably (TM) in the gatekeeper if the thread can be migrated.
>>>
>>> Ok for adding the XNATOMIC test, because it improves the robustness, but
>>> why changing the way XNATOMIC is set and clear? Chances of breaking
>>> thing while changing code in this area are really high...
>>
>> The current code is (most probably) broken as it does not properly
>> synchronizes the gatekeeper against a signaled and "runaway" target
>> Linux task.
>>
>> We need an indication if a Linux signal will (or already has) woken up
>> the to-be-migrated task. That task may have continued over its context,
>> potentially on a different CPU. Providing this indication is the purpose
>> of changing where XNATOMIC is cleared.
> 
> What about synchronizing with the gatekeeper with a semaphore, as done
> in the first patch you sent, but doing it in xnshadow_harden, as soon as
> we detect that we are not back from schedule in primary mode? It seems
> it would avoid any further issue, as we would then be guaranteed that
> the thread could not switch to TASK_INTERRUPTIBLE again before the
> gatekeeper is finished.

The problem is that the gatekeeper tests the task state without holding
the task's rq lock (which is not available to us without a kernel
patch). That cannot work reliably as long as we accept signals. That's
why I'm trying to move state change and test under nklock.

> 
> What worries me is the comment in xnshadow_harden:
> 
> 	 * gatekeeper sent us to primary mode. Since
> 	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
> 	 * the runqueue's count of uniniterruptible tasks, we just
> 	 * notice the issue and gracefully fail; the caller will have
> 	 * to process this signal anyway.
> 	 */
> 
> Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
> point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
> business of xnshadow_harden?
> 

TASK_UNINTERRUPTIBLE is not available without patching the kernel's
scheduler for the reason mentioned in the comment (the scheduler becomes
confused and may pick the wrong tasks, IIRC).

But I would refrain from trying to "improve" the gatekeeper design. I've
recently mentioned this to Philippe offlist: For Xenomai 3 with some
ipipe v3, we must rather patch schedule() to enable zero-switch domain
migration. Means: enter the scheduler, let it suspend current and pick
another task, but then simply escalate to the RT domain before doing any
context switch. That's much cheaper than the current design and
hopefully also less error-prone.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/13/2011 09:04 PM, Jan Kiszka wrote:
> On 2011-07-13 20:39, Gilles Chanteperdrix wrote:
>> On 07/12/2011 07:43 PM, Jan Kiszka wrote:
>>> On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
>>>> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>>>>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>>>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>>>>  			xnlock_put_irqrestore(&nklock, s);
>>>>>>>  			xnpod_schedule();
>>>>>>>  		}
>>>>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>>>>  	 * to process this signal anyway.
>>>>>>>  	 */
>>>>>>>  	if (rthal_current_domain == rthal_root_domain) {
>>>>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>>>>>
>>>>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>>>>
>>>>> Nope, I forgot to remove that line.
>>>>>
>>>>>>
>>>>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>>>>  		    || this_task->state != TASK_RUNNING))
>>>>>>>  			xnpod_fatal
>>>>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>>>>  		return -ERESTARTSYS;
>>>>>>>  	}
>>>>>>>  
>>>>>>> +	xnthread_clear_info(thread, XNATOMIC);
>>>>>>
>>>>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>>>>> place at the point it currently is.
>>>>>
>>>>> Nope. Now we either clear XNATOMIC after successful migration or when
>>>>> the signal is about to be sent (ie. in the hook). That way we can test
>>>>> more reliably (TM) in the gatekeeper if the thread can be migrated.
>>>>
>>>> Ok for adding the XNATOMIC test, because it improves the robustness, but
>>>> why changing the way XNATOMIC is set and clear? Chances of breaking
>>>> thing while changing code in this area are really high...
>>>
>>> The current code is (most probably) broken as it does not properly
>>> synchronizes the gatekeeper against a signaled and "runaway" target
>>> Linux task.
>>>
>>> We need an indication if a Linux signal will (or already has) woken up
>>> the to-be-migrated task. That task may have continued over its context,
>>> potentially on a different CPU. Providing this indication is the purpose
>>> of changing where XNATOMIC is cleared.
>>
>> What about synchronizing with the gatekeeper with a semaphore, as done
>> in the first patch you sent, but doing it in xnshadow_harden, as soon as
>> we detect that we are not back from schedule in primary mode? It seems
>> it would avoid any further issue, as we would then be guaranteed that
>> the thread could not switch to TASK_INTERRUPTIBLE again before the
>> gatekeeper is finished.
> 
> The problem is that the gatekeeper tests the task state without holding
> the task's rq lock (which is not available to us without a kernel
> patch). That cannot work reliably as long as we accept signals. That's
> why I'm trying to move state change and test under nklock.
> 
>>
>> What worries me is the comment in xnshadow_harden:
>>
>> 	 * gatekeeper sent us to primary mode. Since
>> 	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
>> 	 * the runqueue's count of uniniterruptible tasks, we just
>> 	 * notice the issue and gracefully fail; the caller will have
>> 	 * to process this signal anyway.
>> 	 */
>>
>> Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
>> point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
>> business of xnshadow_harden?
>>
> 
> TASK_UNINTERRUPTIBLE is not available without patching the kernel's
> scheduler for the reason mentioned in the comment (the scheduler becomes
> confused and may pick the wrong tasks, IIRC).

Does not using down/up in the taskexit event handler risk to cause the
same issue?

> 
> But I would refrain from trying to "improve" the gatekeeper design. I've
> recently mentioned this to Philippe offlist: For Xenomai 3 with some
> ipipe v3, we must rather patch schedule() to enable zero-switch domain
> migration. Means: enter the scheduler, let it suspend current and pick
> another task, but then simply escalate to the RT domain before doing any
> context switch. That's much cheaper than the current design and
> hopefully also less error-prone.

So, do you want me to merge your for-upstream branch?
-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Philippe Gerum]

On Wed, 2011-07-13 at 20:39 +0200, Gilles Chanteperdrix wrote:
> On 07/12/2011 07:43 PM, Jan Kiszka wrote:
> > On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
> >> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
> >>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
> >>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
> >>>>>  			xnlock_put_irqrestore(&nklock, s);
> >>>>>  			xnpod_schedule();
> >>>>>  		}
> >>>>> @@ -1036,6 +1043,7 @@ redo:
> >>>>>  	 * to process this signal anyway.
> >>>>>  	 */
> >>>>>  	if (rthal_current_domain == rthal_root_domain) {
> >>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
> >>>>
> >>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
> >>>
> >>> Nope, I forgot to remove that line.
> >>>
> >>>>
> >>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
> >>>>>  		    || this_task->state != TASK_RUNNING))
> >>>>>  			xnpod_fatal
> >>>>> @@ -1044,6 +1052,8 @@ redo:
> >>>>>  		return -ERESTARTSYS;
> >>>>>  	}
> >>>>>  
> >>>>> +	xnthread_clear_info(thread, XNATOMIC);
> >>>>
> >>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
> >>>> place at the point it currently is.
> >>>
> >>> Nope. Now we either clear XNATOMIC after successful migration or when
> >>> the signal is about to be sent (ie. in the hook). That way we can test
> >>> more reliably (TM) in the gatekeeper if the thread can be migrated.
> >>
> >> Ok for adding the XNATOMIC test, because it improves the robustness, but
> >> why changing the way XNATOMIC is set and clear? Chances of breaking
> >> thing while changing code in this area are really high...
> > 
> > The current code is (most probably) broken as it does not properly
> > synchronizes the gatekeeper against a signaled and "runaway" target
> > Linux task.
> > 
> > We need an indication if a Linux signal will (or already has) woken up
> > the to-be-migrated task. That task may have continued over its context,
> > potentially on a different CPU. Providing this indication is the purpose
> > of changing where XNATOMIC is cleared.
> 
> What about synchronizing with the gatekeeper with a semaphore, as done
> in the first patch you sent, but doing it in xnshadow_harden, as soon as
> we detect that we are not back from schedule in primary mode? It seems
> it would avoid any further issue, as we would then be guaranteed that
> the thread could not switch to TASK_INTERRUPTIBLE again before the
> gatekeeper is finished.
> 
> What worries me is the comment in xnshadow_harden:
> 
> 	 * gatekeeper sent us to primary mode. Since
> 	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
> 	 * the runqueue's count of uniniterruptible tasks, we just
> 	 * notice the issue and gracefully fail; the caller will have
> 	 * to process this signal anyway.
> 	 */
> 
> Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
> point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
> business of xnshadow_harden?

Second interpretation is correct.

> 

-- 
Philippe.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4747 bytes --]

On 2011-07-13 21:12, Gilles Chanteperdrix wrote:
> On 07/13/2011 09:04 PM, Jan Kiszka wrote:
>> On 2011-07-13 20:39, Gilles Chanteperdrix wrote:
>>> On 07/12/2011 07:43 PM, Jan Kiszka wrote:
>>>> On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
>>>>> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>>>>>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>>>>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>>>>>  			xnlock_put_irqrestore(&nklock, s);
>>>>>>>>  			xnpod_schedule();
>>>>>>>>  		}
>>>>>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>>>>>  	 * to process this signal anyway.
>>>>>>>>  	 */
>>>>>>>>  	if (rthal_current_domain == rthal_root_domain) {
>>>>>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>>>>>>
>>>>>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>>>>>
>>>>>> Nope, I forgot to remove that line.
>>>>>>
>>>>>>>
>>>>>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>>>>>  		    || this_task->state != TASK_RUNNING))
>>>>>>>>  			xnpod_fatal
>>>>>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>>>>>  		return -ERESTARTSYS;
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>> +	xnthread_clear_info(thread, XNATOMIC);
>>>>>>>
>>>>>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>>>>>> place at the point it currently is.
>>>>>>
>>>>>> Nope. Now we either clear XNATOMIC after successful migration or when
>>>>>> the signal is about to be sent (ie. in the hook). That way we can test
>>>>>> more reliably (TM) in the gatekeeper if the thread can be migrated.
>>>>>
>>>>> Ok for adding the XNATOMIC test, because it improves the robustness, but
>>>>> why changing the way XNATOMIC is set and clear? Chances of breaking
>>>>> thing while changing code in this area are really high...
>>>>
>>>> The current code is (most probably) broken as it does not properly
>>>> synchronizes the gatekeeper against a signaled and "runaway" target
>>>> Linux task.
>>>>
>>>> We need an indication if a Linux signal will (or already has) woken up
>>>> the to-be-migrated task. That task may have continued over its context,
>>>> potentially on a different CPU. Providing this indication is the purpose
>>>> of changing where XNATOMIC is cleared.
>>>
>>> What about synchronizing with the gatekeeper with a semaphore, as done
>>> in the first patch you sent, but doing it in xnshadow_harden, as soon as
>>> we detect that we are not back from schedule in primary mode? It seems
>>> it would avoid any further issue, as we would then be guaranteed that
>>> the thread could not switch to TASK_INTERRUPTIBLE again before the
>>> gatekeeper is finished.
>>
>> The problem is that the gatekeeper tests the task state without holding
>> the task's rq lock (which is not available to us without a kernel
>> patch). That cannot work reliably as long as we accept signals. That's
>> why I'm trying to move state change and test under nklock.
>>
>>>
>>> What worries me is the comment in xnshadow_harden:
>>>
>>> 	 * gatekeeper sent us to primary mode. Since
>>> 	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
>>> 	 * the runqueue's count of uniniterruptible tasks, we just
>>> 	 * notice the issue and gracefully fail; the caller will have
>>> 	 * to process this signal anyway.
>>> 	 */
>>>
>>> Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
>>> point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
>>> business of xnshadow_harden?
>>>
>>
>> TASK_UNINTERRUPTIBLE is not available without patching the kernel's
>> scheduler for the reason mentioned in the comment (the scheduler becomes
>> confused and may pick the wrong tasks, IIRC).
> 
> Does not using down/up in the taskexit event handler risk to cause the
> same issue?

Yes, and that means the first patch is incomplete without something like
the second.

> 
>>
>> But I would refrain from trying to "improve" the gatekeeper design. I've
>> recently mentioned this to Philippe offlist: For Xenomai 3 with some
>> ipipe v3, we must rather patch schedule() to enable zero-switch domain
>> migration. Means: enter the scheduler, let it suspend current and pick
>> another task, but then simply escalate to the RT domain before doing any
>> context switch. That's much cheaper than the current design and
>> hopefully also less error-prone.
> 
> So, do you want me to merge your for-upstream branch?

You may merge up to for-upstream^, ie. without any gatekeeper fixes.

I strongly suspect that there are still more races in the migration
path. The crashes we face even with all patches applied may be related
to a shadow task being executed under Linux and Xenomai at the same time.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Gilles Chanteperdrix]

On 07/14/2011 10:57 PM, Jan Kiszka wrote:
> On 2011-07-13 21:12, Gilles Chanteperdrix wrote:
>> On 07/13/2011 09:04 PM, Jan Kiszka wrote:
>>> On 2011-07-13 20:39, Gilles Chanteperdrix wrote:
>>>> On 07/12/2011 07:43 PM, Jan Kiszka wrote:
>>>>> On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
>>>>>> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>>>>>>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>>>>>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>>>>>>  			xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>  			xnpod_schedule();
>>>>>>>>>  		}
>>>>>>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>>>>>>  	 * to process this signal anyway.
>>>>>>>>>  	 */
>>>>>>>>>  	if (rthal_current_domain == rthal_root_domain) {
>>>>>>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>>>>>>>
>>>>>>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>>>>>>
>>>>>>> Nope, I forgot to remove that line.
>>>>>>>
>>>>>>>>
>>>>>>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>>>>>>  		    || this_task->state != TASK_RUNNING))
>>>>>>>>>  			xnpod_fatal
>>>>>>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>>>>>>  		return -ERESTARTSYS;
>>>>>>>>>  	}
>>>>>>>>>  
>>>>>>>>> +	xnthread_clear_info(thread, XNATOMIC);
>>>>>>>>
>>>>>>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>>>>>>> place at the point it currently is.
>>>>>>>
>>>>>>> Nope. Now we either clear XNATOMIC after successful migration or when
>>>>>>> the signal is about to be sent (ie. in the hook). That way we can test
>>>>>>> more reliably (TM) in the gatekeeper if the thread can be migrated.
>>>>>>
>>>>>> Ok for adding the XNATOMIC test, because it improves the robustness, but
>>>>>> why changing the way XNATOMIC is set and clear? Chances of breaking
>>>>>> thing while changing code in this area are really high...
>>>>>
>>>>> The current code is (most probably) broken as it does not properly
>>>>> synchronizes the gatekeeper against a signaled and "runaway" target
>>>>> Linux task.
>>>>>
>>>>> We need an indication if a Linux signal will (or already has) woken up
>>>>> the to-be-migrated task. That task may have continued over its context,
>>>>> potentially on a different CPU. Providing this indication is the purpose
>>>>> of changing where XNATOMIC is cleared.
>>>>
>>>> What about synchronizing with the gatekeeper with a semaphore, as done
>>>> in the first patch you sent, but doing it in xnshadow_harden, as soon as
>>>> we detect that we are not back from schedule in primary mode? It seems
>>>> it would avoid any further issue, as we would then be guaranteed that
>>>> the thread could not switch to TASK_INTERRUPTIBLE again before the
>>>> gatekeeper is finished.
>>>
>>> The problem is that the gatekeeper tests the task state without holding
>>> the task's rq lock (which is not available to us without a kernel
>>> patch). That cannot work reliably as long as we accept signals. That's
>>> why I'm trying to move state change and test under nklock.
>>>
>>>>
>>>> What worries me is the comment in xnshadow_harden:
>>>>
>>>> 	 * gatekeeper sent us to primary mode. Since
>>>> 	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
>>>> 	 * the runqueue's count of uniniterruptible tasks, we just
>>>> 	 * notice the issue and gracefully fail; the caller will have
>>>> 	 * to process this signal anyway.
>>>> 	 */
>>>>
>>>> Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
>>>> point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
>>>> business of xnshadow_harden?
>>>>
>>>
>>> TASK_UNINTERRUPTIBLE is not available without patching the kernel's
>>> scheduler for the reason mentioned in the comment (the scheduler becomes
>>> confused and may pick the wrong tasks, IIRC).
>>
>> Does not using down/up in the taskexit event handler risk to cause the
>> same issue?
> 
> Yes, and that means the first patch is incomplete without something like
> the second.
> 
>>
>>>
>>> But I would refrain from trying to "improve" the gatekeeper design. I've
>>> recently mentioned this to Philippe offlist: For Xenomai 3 with some
>>> ipipe v3, we must rather patch schedule() to enable zero-switch domain
>>> migration. Means: enter the scheduler, let it suspend current and pick
>>> another task, but then simply escalate to the RT domain before doing any
>>> context switch. That's much cheaper than the current design and
>>> hopefully also less error-prone.
>>
>> So, do you want me to merge your for-upstream branch?
> 
> You may merge up to for-upstream^, ie. without any gatekeeper fixes.
> 
> I strongly suspect that there are still more races in the migration
> path. The crashes we face even with all patches applied may be related
> to a shadow task being executed under Linux and Xenomai at the same time.

Maybe we could try the following patch instead?

diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
index 01f4200..deb7620 100644
--- a/ksrc/nucleus/shadow.c
+++ b/ksrc/nucleus/shadow.c
@@ -1033,6 +1033,8 @@ redo:
 			xnpod_fatal
 			    ("xnshadow_harden() failed for thread %s[%d]",
 			     thread->name, xnthread_user_pid(thread));
+		down(&sched->gksync);
+		up(&sched->gksync);
 		return -ERESTARTSYS;
 	}
 

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6062 bytes --]

On 2011-07-15 14:30, Gilles Chanteperdrix wrote:
> On 07/14/2011 10:57 PM, Jan Kiszka wrote:
>> On 2011-07-13 21:12, Gilles Chanteperdrix wrote:
>>> On 07/13/2011 09:04 PM, Jan Kiszka wrote:
>>>> On 2011-07-13 20:39, Gilles Chanteperdrix wrote:
>>>>> On 07/12/2011 07:43 PM, Jan Kiszka wrote:
>>>>>> On 2011-07-12 19:38, Gilles Chanteperdrix wrote:
>>>>>>> On 07/12/2011 07:34 PM, Jan Kiszka wrote:
>>>>>>>> On 2011-07-12 19:31, Gilles Chanteperdrix wrote:
>>>>>>>>> On 07/12/2011 02:57 PM, Jan Kiszka wrote:
>>>>>>>>>>  			xnlock_put_irqrestore(&nklock, s);
>>>>>>>>>>  			xnpod_schedule();
>>>>>>>>>>  		}
>>>>>>>>>> @@ -1036,6 +1043,7 @@ redo:
>>>>>>>>>>  	 * to process this signal anyway.
>>>>>>>>>>  	 */
>>>>>>>>>>  	if (rthal_current_domain == rthal_root_domain) {
>>>>>>>>>> +		XENO_BUGON(NUCLEUS, xnthread_test_info(thread, XNATOMIC));
>>>>>>>>>
>>>>>>>>> Misleading dead code again, XNATOMIC is cleared not ten lines above.
>>>>>>>>
>>>>>>>> Nope, I forgot to remove that line.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>  		if (XENO_DEBUG(NUCLEUS) && (!signal_pending(this_task)
>>>>>>>>>>  		    || this_task->state != TASK_RUNNING))
>>>>>>>>>>  			xnpod_fatal
>>>>>>>>>> @@ -1044,6 +1052,8 @@ redo:
>>>>>>>>>>  		return -ERESTARTSYS;
>>>>>>>>>>  	}
>>>>>>>>>>  
>>>>>>>>>> +	xnthread_clear_info(thread, XNATOMIC);
>>>>>>>>>
>>>>>>>>> Why this? I find the xnthread_clear_info(XNATOMIC) right at the right
>>>>>>>>> place at the point it currently is.
>>>>>>>>
>>>>>>>> Nope. Now we either clear XNATOMIC after successful migration or when
>>>>>>>> the signal is about to be sent (ie. in the hook). That way we can test
>>>>>>>> more reliably (TM) in the gatekeeper if the thread can be migrated.
>>>>>>>
>>>>>>> Ok for adding the XNATOMIC test, because it improves the robustness, but
>>>>>>> why changing the way XNATOMIC is set and clear? Chances of breaking
>>>>>>> thing while changing code in this area are really high...
>>>>>>
>>>>>> The current code is (most probably) broken as it does not properly
>>>>>> synchronizes the gatekeeper against a signaled and "runaway" target
>>>>>> Linux task.
>>>>>>
>>>>>> We need an indication if a Linux signal will (or already has) woken up
>>>>>> the to-be-migrated task. That task may have continued over its context,
>>>>>> potentially on a different CPU. Providing this indication is the purpose
>>>>>> of changing where XNATOMIC is cleared.
>>>>>
>>>>> What about synchronizing with the gatekeeper with a semaphore, as done
>>>>> in the first patch you sent, but doing it in xnshadow_harden, as soon as
>>>>> we detect that we are not back from schedule in primary mode? It seems
>>>>> it would avoid any further issue, as we would then be guaranteed that
>>>>> the thread could not switch to TASK_INTERRUPTIBLE again before the
>>>>> gatekeeper is finished.
>>>>
>>>> The problem is that the gatekeeper tests the task state without holding
>>>> the task's rq lock (which is not available to us without a kernel
>>>> patch). That cannot work reliably as long as we accept signals. That's
>>>> why I'm trying to move state change and test under nklock.
>>>>
>>>>>
>>>>> What worries me is the comment in xnshadow_harden:
>>>>>
>>>>> 	 * gatekeeper sent us to primary mode. Since
>>>>> 	 * TASK_UNINTERRUPTIBLE is unavailable to us without wrecking
>>>>> 	 * the runqueue's count of uniniterruptible tasks, we just
>>>>> 	 * notice the issue and gracefully fail; the caller will have
>>>>> 	 * to process this signal anyway.
>>>>> 	 */
>>>>>
>>>>> Does this mean that we can not switch to TASK_UNINTERRUPTIBLE at this
>>>>> point? Or simply that TASK_UNINTERRUPTIBLE is not available for the
>>>>> business of xnshadow_harden?
>>>>>
>>>>
>>>> TASK_UNINTERRUPTIBLE is not available without patching the kernel's
>>>> scheduler for the reason mentioned in the comment (the scheduler becomes
>>>> confused and may pick the wrong tasks, IIRC).
>>>
>>> Does not using down/up in the taskexit event handler risk to cause the
>>> same issue?
>>
>> Yes, and that means the first patch is incomplete without something like
>> the second.
>>
>>>
>>>>
>>>> But I would refrain from trying to "improve" the gatekeeper design. I've
>>>> recently mentioned this to Philippe offlist: For Xenomai 3 with some
>>>> ipipe v3, we must rather patch schedule() to enable zero-switch domain
>>>> migration. Means: enter the scheduler, let it suspend current and pick
>>>> another task, but then simply escalate to the RT domain before doing any
>>>> context switch. That's much cheaper than the current design and
>>>> hopefully also less error-prone.
>>>
>>> So, do you want me to merge your for-upstream branch?
>>
>> You may merge up to for-upstream^, ie. without any gatekeeper fixes.
>>
>> I strongly suspect that there are still more races in the migration
>> path. The crashes we face even with all patches applied may be related
>> to a shadow task being executed under Linux and Xenomai at the same time.
> 
> Maybe we could try the following patch instead?
> 
> diff --git a/ksrc/nucleus/shadow.c b/ksrc/nucleus/shadow.c
> index 01f4200..deb7620 100644
> --- a/ksrc/nucleus/shadow.c
> +++ b/ksrc/nucleus/shadow.c
> @@ -1033,6 +1033,8 @@ redo:
>  			xnpod_fatal
>  			    ("xnshadow_harden() failed for thread %s[%d]",
>  			     thread->name, xnthread_user_pid(thread));
> +		down(&sched->gksync);
> +		up(&sched->gksync);
>  		return -ERESTARTSYS;
>  	}
> 

I don't think we need this. But we may need locking around
clear_task_nowakeup in xnshadow_relax, it could race with state
manipulation done over the task context on a different CPU.

But... right now it looks like we found our primary regression:
"nucleus/shadow: shorten the uninterruptible path to secondary mode". It
opens a short windows during relax where the migrated task may be active
under both schedulers. We are currently evaluating a revert (looks good
so far), and I need to work out my theory in more details.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2011-07-15 15:10, Jan Kiszka wrote:
> But... right now it looks like we found our primary regression: 
> "nucleus/shadow: shorten the uninterruptible path to secondary mode".
> It opens a short windows during relax where the migrated task may be
> active under both schedulers. We are currently evaluating a revert
> (looks good so far), and I need to work out my theory in more
> details.

Looks like this commit just made a long-standing flaw in Xenomai's
interrupt handling more visible: We reschedule over the interrupt stack
in the Xenomai interrupt handler tails, at least on x86-64. Not sure if
other archs have interrupt stacks, the point is Xenomai's design wrongly
assumes there are no such things. We were lucky so far that the values
saved on this shared stack were apparently "compatible", means we were
overwriting them with identical or harmless values. But that's no longer
true when interrupts are hitting us in the xnpod_suspend_thread path of
a relaxing shadow.

Likely the only possible fix is establishing a reschedule hook for
Xenomai in the interrupt exit path after the original stack is restored
- - just like Linux works. Requires changes to both ipipe and Xenomai
unfortunately.

Jan

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.16 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk4hSDsACgkQitSsb3rl5xSmOACfbZfcNKyO9YDvPE+R5H75d0ky
DX0An32BrZW+lpEnxnLLCHSQ5r8itnE9
=n6u8
-----END PGP SIGNATURE-----

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Philippe Gerum]

On Sat, 2011-07-16 at 10:13 +0200, Jan Kiszka wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 2011-07-15 15:10, Jan Kiszka wrote:
> > But... right now it looks like we found our primary regression: 
> > "nucleus/shadow: shorten the uninterruptible path to secondary mode".
> > It opens a short windows during relax where the migrated task may be
> > active under both schedulers. We are currently evaluating a revert
> > (looks good so far), and I need to work out my theory in more
> > details.
> 
> Looks like this commit just made a long-standing flaw in Xenomai's
> interrupt handling more visible: We reschedule over the interrupt stack
> in the Xenomai interrupt handler tails, at least on x86-64. Not sure if
> other archs have interrupt stacks, the point is Xenomai's design wrongly
> assumes there are no such things.

Fortunately, no, this is not a design issue, no such assumption was ever
made, but the Xenomai core expects this to be handled on a per-arch
basis with the interrupt pipeline. As you pointed out, there is no way
to handle this via some generic Xenomai-only support.

ppc64 now has separate interrupt stacks, which is why I disabled
IRQSTACKS which became the builtin default at some point. Blackfin goes
through a Xenomai-defined irq tail handler as well, because it may not
reschedule over nested interrupt stacks. Fact is that such pending
problem with x86_64 was overlooked since day #1 by /me.

>  We were lucky so far that the values
> saved on this shared stack were apparently "compatible", means we were
> overwriting them with identical or harmless values. But that's no longer
> true when interrupts are hitting us in the xnpod_suspend_thread path of
> a relaxing shadow.
> 

Makes sense. It would be better to find a solution that does not make
the relax path uninterruptible again for a significant amount of time.
On low end platforms we support (i.e. non-x86* mainly), this causes
obvious latency spots.

> Likely the only possible fix is establishing a reschedule hook for
> Xenomai in the interrupt exit path after the original stack is restored
> - - just like Linux works. Requires changes to both ipipe and Xenomai
> unfortunately.

__ipipe_run_irqtail() is in the I-pipe core for such purpose. If
instantiated properly for x86_64, and paired with xnarch_escalate() for
that arch as well, it could be an option for running the rescheduling
procedure when safe.

> 
> Jan
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.16 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAk4hSDsACgkQitSsb3rl5xSmOACfbZfcNKyO9YDvPE+R5H75d0ky
> DX0An32BrZW+lpEnxnLLCHSQ5r8itnE9
> =n6u8
> -----END PGP SIGNATURE-----

-- 
Philippe.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

On 2011-07-16 10:52, Philippe Gerum wrote:
> On Sat, 2011-07-16 at 10:13 +0200, Jan Kiszka wrote:
>> On 2011-07-15 15:10, Jan Kiszka wrote:
>>> But... right now it looks like we found our primary regression:
>>> "nucleus/shadow: shorten the uninterruptible path to secondary mode".
>>> It opens a short windows during relax where the migrated task may be
>>> active under both schedulers. We are currently evaluating a revert
>>> (looks good so far), and I need to work out my theory in more
>>> details.
>>
>> Looks like this commit just made a long-standing flaw in Xenomai's
>> interrupt handling more visible: We reschedule over the interrupt stack
>> in the Xenomai interrupt handler tails, at least on x86-64. Not sure if
>> other archs have interrupt stacks, the point is Xenomai's design wrongly
>> assumes there are no such things.
>
> Fortunately, no, this is not a design issue, no such assumption was ever
> made, but the Xenomai core expects this to be handled on a per-arch
> basis with the interrupt pipeline.

And that's already the problem: If Linux uses interrupt stacks, relying 
on ipipe to disable this during Xenomai interrupt handler execution is 
at best a workaround. A fragile one unless you increase the pre-thread 
stack size by the size of the interrupt stack. Lacking support for a 
generic rescheduling hook became a problem by the time Linux introduced 
interrupt threads.

> As you pointed out, there is no way
> to handle this via some generic Xenomai-only support.
>
> ppc64 now has separate interrupt stacks, which is why I disabled
> IRQSTACKS which became the builtin default at some point. Blackfin goes
> through a Xenomai-defined irq tail handler as well, because it may not
> reschedule over nested interrupt stacks.

How does this arch prevent that xnpod_schedule in the generic interrupt 
handler tail does its normal work?

> Fact is that such pending
> problem with x86_64 was overlooked since day #1 by /me.
>
>>   We were lucky so far that the values
>> saved on this shared stack were apparently "compatible", means we were
>> overwriting them with identical or harmless values. But that's no longer
>> true when interrupts are hitting us in the xnpod_suspend_thread path of
>> a relaxing shadow.
>>
>
> Makes sense. It would be better to find a solution that does not make
> the relax path uninterruptible again for a significant amount of time.
> On low end platforms we support (i.e. non-x86* mainly), this causes
> obvious latency spots.

I agree. Conceptually, the interruptible relaxation should be safe now 
after recent fixes.

>
>> Likely the only possible fix is establishing a reschedule hook for
>> Xenomai in the interrupt exit path after the original stack is restored
>> - - just like Linux works. Requires changes to both ipipe and Xenomai
>> unfortunately.
>
> __ipipe_run_irqtail() is in the I-pipe core for such purpose. If
> instantiated properly for x86_64, and paired with xnarch_escalate() for
> that arch as well, it could be an option for running the rescheduling
> procedure when safe.

Nope, that doesn't work. The stack is switched later in the return path 
in entry_64.S. We need a hook there, ideally a conditional one, 
controlled by some per-cpu variable that is set by Xenomai on return 
from its interrupt handlers to signal the rescheduling need.

Jan

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Philippe Gerum]

On Sat, 2011-07-16 at 11:15 +0200, Jan Kiszka wrote:
> On 2011-07-16 10:52, Philippe Gerum wrote:
> > On Sat, 2011-07-16 at 10:13 +0200, Jan Kiszka wrote:
> >> On 2011-07-15 15:10, Jan Kiszka wrote:
> >>> But... right now it looks like we found our primary regression:
> >>> "nucleus/shadow: shorten the uninterruptible path to secondary mode".
> >>> It opens a short windows during relax where the migrated task may be
> >>> active under both schedulers. We are currently evaluating a revert
> >>> (looks good so far), and I need to work out my theory in more
> >>> details.
> >>
> >> Looks like this commit just made a long-standing flaw in Xenomai's
> >> interrupt handling more visible: We reschedule over the interrupt stack
> >> in the Xenomai interrupt handler tails, at least on x86-64. Not sure if
> >> other archs have interrupt stacks, the point is Xenomai's design wrongly
> >> assumes there are no such things.
> >
> > Fortunately, no, this is not a design issue, no such assumption was ever
> > made, but the Xenomai core expects this to be handled on a per-arch
> > basis with the interrupt pipeline.
> 
> And that's already the problem: If Linux uses interrupt stacks, relying 
> on ipipe to disable this during Xenomai interrupt handler execution is 
> at best a workaround. A fragile one unless you increase the pre-thread 
> stack size by the size of the interrupt stack. Lacking support for a 
> generic rescheduling hook became a problem by the time Linux introduced 
> interrupt threads.

Don't assume too much. What was done for ppc64 was not meant as a
general policy. Again, this is a per-arch decision.

> 
> > As you pointed out, there is no way
> > to handle this via some generic Xenomai-only support.
> >
> > ppc64 now has separate interrupt stacks, which is why I disabled
> > IRQSTACKS which became the builtin default at some point. Blackfin goes
> > through a Xenomai-defined irq tail handler as well, because it may not
> > reschedule over nested interrupt stacks.
> 
> How does this arch prevent that xnpod_schedule in the generic interrupt 
> handler tail does its normal work?

It polls some hw status to know whether a rescheduling would be safe.
See xnarch_escalate().

> 
> > Fact is that such pending
> > problem with x86_64 was overlooked since day #1 by /me.
> >
> >>   We were lucky so far that the values
> >> saved on this shared stack were apparently "compatible", means we were
> >> overwriting them with identical or harmless values. But that's no longer
> >> true when interrupts are hitting us in the xnpod_suspend_thread path of
> >> a relaxing shadow.
> >>
> >
> > Makes sense. It would be better to find a solution that does not make
> > the relax path uninterruptible again for a significant amount of time.
> > On low end platforms we support (i.e. non-x86* mainly), this causes
> > obvious latency spots.
> 
> I agree. Conceptually, the interruptible relaxation should be safe now 
> after recent fixes.
> 
> >
> >> Likely the only possible fix is establishing a reschedule hook for
> >> Xenomai in the interrupt exit path after the original stack is restored
> >> - - just like Linux works. Requires changes to both ipipe and Xenomai
> >> unfortunately.
> >
> > __ipipe_run_irqtail() is in the I-pipe core for such purpose. If
> > instantiated properly for x86_64, and paired with xnarch_escalate() for
> > that arch as well, it could be an option for running the rescheduling
> > procedure when safe.
> 
> Nope, that doesn't work. The stack is switched later in the return path 
> in entry_64.S. We need a hook there, ideally a conditional one, 
> controlled by some per-cpu variable that is set by Xenomai on return 
> from its interrupt handlers to signal the rescheduling need.
> 

Yes, makes sense. The way to make it conditional without dragging bits
of Xenomai logic into the kernel innards is not obvious though.

It is probably time to officially introduce "exo-kernel" oriented bits
into the Linux thread info. PTDs have too lose semantics to be practical
if we want to avoid trashing the I-cache by calling probe hooks within
the dual kernel, each time we want to check some basic condition (e.g.
resched needed). A backlink to a foreign TCB there would help too.

Which leads us to killing the ad hoc kernel threads (and stacks) at some
point, which are an absolute pain.

> Jan

-- 
Philippe.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix race between gatekeeper and thread deletion

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2442 bytes --]

On 2011-07-16 11:56, Philippe Gerum wrote:
> On Sat, 2011-07-16 at 11:15 +0200, Jan Kiszka wrote:
>> On 2011-07-16 10:52, Philippe Gerum wrote:
>>> On Sat, 2011-07-16 at 10:13 +0200, Jan Kiszka wrote:
>>>> On 2011-07-15 15:10, Jan Kiszka wrote:
>>>>> But... right now it looks like we found our primary regression:
>>>>> "nucleus/shadow: shorten the uninterruptible path to secondary mode".
>>>>> It opens a short windows during relax where the migrated task may be
>>>>> active under both schedulers. We are currently evaluating a revert
>>>>> (looks good so far), and I need to work out my theory in more
>>>>> details.
>>>>
>>>> Looks like this commit just made a long-standing flaw in Xenomai's
>>>> interrupt handling more visible: We reschedule over the interrupt stack
>>>> in the Xenomai interrupt handler tails, at least on x86-64. Not sure if
>>>> other archs have interrupt stacks, the point is Xenomai's design wrongly
>>>> assumes there are no such things.
>>>
>>> Fortunately, no, this is not a design issue, no such assumption was ever
>>> made, but the Xenomai core expects this to be handled on a per-arch
>>> basis with the interrupt pipeline.
>>
>> And that's already the problem: If Linux uses interrupt stacks, relying 
>> on ipipe to disable this during Xenomai interrupt handler execution is 
>> at best a workaround. A fragile one unless you increase the pre-thread 
>> stack size by the size of the interrupt stack. Lacking support for a 
>> generic rescheduling hook became a problem by the time Linux introduced 
>> interrupt threads.
> 
> Don't assume too much. What was done for ppc64 was not meant as a
> general policy. Again, this is a per-arch decision.

Actually, it was the right decision, not only for ppc64: Reusing Linux
interrupt stacks for Xenomai does not work. If we interrupt Linux while
it is already running over the interrupt stack, the stack becomes taboo
on that CPU. From that point on, no RT IRQ must run over the Linux
interrupt stack as it would smash it.

But then the question is why we should try to use the interrupt stacks
for Xenomai at all. It's better to increase the task kernel stacks and
disable interrupt stacks when ipipe is enabled. That's what I'm heading
for with x86-64 now (THREAD_ORDER 2, no stack switching).

What we may do is introducing per-domain interrupt stacks. But that's at
best Xenomai 3 / I-pipe 3 stuff.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]