[Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Jan Kiszka]

My customer managed to find another hidden door to Xenomai's hell:
Trying to create a Xenomai POSIX thread from within a native thread. A
think the other way around would "work" as well. The precise scenario
was native thread -> dlopen(libs that's linked against libpthread_rt) ->
__init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
scenario revealed two more issues in fact.

Here is a patch to remove this hell gate by catching xnshadow_map
invocations from withing already mapped shadow thread.

Jan

---
 ChangeLog             |    5 +++++
 ksrc/nucleus/shadow.c |    5 +++++
 2 files changed, 10 insertions(+)

Index: xenomai/ksrc/nucleus/shadow.c
===================================================================
--- xenomai.orig/ksrc/nucleus/shadow.c
+++ xenomai/ksrc/nucleus/shadow.c
@@ -1334,6 +1334,8 @@ void xnshadow_exit(void)
  * - -EINVAL is returned if the thread control block does not bear the
  * XNSHADOW bit, or if the thread has already been mapped.
  *
+ * - -EBUSY is returned if the current Linux task is already mapped.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -1351,6 +1353,9 @@ int xnshadow_map(xnthread_t *thread, xnc
 	unsigned muxid, magic;
 	int err;

+	if (xnshadow_thread(current))
+		return -EBUSY;
+
 	if (!xnthread_test_state(thread, XNSHADOW))
 		return -EINVAL;

Index: xenomai/ChangeLog
===================================================================
--- xenomai.orig/ChangeLog
+++ xenomai/ChangeLog
@@ -1,3 +1,8 @@
+2008-11-21  Jan Kiszka  <jan.kiszka@domain.hid>
+
+	* ksrc/nucleus/shadow.c (xnshadow_map): Reject already mapped
+	tasks with -EBUSY.
+
 2008-11-16  Philippe Gerum  <rpm@xenomai.org>

 	* ksrc/arch/blackfin/patches: Upgrade to to
-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Philippe Gerum]

Jan Kiszka wrote:
> My customer managed to find another hidden door to Xenomai's hell:
> Trying to create a Xenomai POSIX thread from within a native thread. A
> think the other way around would "work" as well. The precise scenario
> was native thread -> dlopen(libs that's linked against libpthread_rt) ->
> __init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
> scenario revealed two more issues in fact.
> 
> Here is a patch to remove this hell gate by catching xnshadow_map
> invocations from withing already mapped shadow thread.
>

Any reason why the XNMAPPED predicate did not work in your case?

> Jan
> 
> ---
>  ChangeLog             |    5 +++++
>  ksrc/nucleus/shadow.c |    5 +++++
>  2 files changed, 10 insertions(+)
> 
> Index: xenomai/ksrc/nucleus/shadow.c
> ===================================================================
> --- xenomai.orig/ksrc/nucleus/shadow.c
> +++ xenomai/ksrc/nucleus/shadow.c
> @@ -1334,6 +1334,8 @@ void xnshadow_exit(void)
>   * - -EINVAL is returned if the thread control block does not bear the
>   * XNSHADOW bit, or if the thread has already been mapped.
>   *
> + * - -EBUSY is returned if the current Linux task is already mapped.
> + *
>   * Environments:
>   *
>   * This service can be called from:
> @@ -1351,6 +1353,9 @@ int xnshadow_map(xnthread_t *thread, xnc
>  	unsigned muxid, magic;
>  	int err;
> 
> +	if (xnshadow_thread(current))
> +		return -EBUSY;
> +
>  	if (!xnthread_test_state(thread, XNSHADOW))
>  		return -EINVAL;
> 
> Index: xenomai/ChangeLog
> ===================================================================
> --- xenomai.orig/ChangeLog
> +++ xenomai/ChangeLog
> @@ -1,3 +1,8 @@
> +2008-11-21  Jan Kiszka  <jan.kiszka@domain.hid>
> +
> +	* ksrc/nucleus/shadow.c (xnshadow_map): Reject already mapped
> +	tasks with -EBUSY.
> +
>  2008-11-16  Philippe Gerum  <rpm@xenomai.org>
> 
>  	* ksrc/arch/blackfin/patches: Upgrade to to


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Jan Kiszka]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> My customer managed to find another hidden door to Xenomai's hell:
>> Trying to create a Xenomai POSIX thread from within a native thread. A
>> think the other way around would "work" as well. The precise scenario
>> was native thread -> dlopen(libs that's linked against libpthread_rt) ->
>> __init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
>> scenario revealed two more issues in fact.
>>
>> Here is a patch to remove this hell gate by catching xnshadow_map
>> invocations from withing already mapped shadow thread.
>>
> 
> Any reason why the XNMAPPED predicate did not work in your case?

Sorry, which one? Keep in mind that the thread thrown at the second
xnshadow_map is a virgin one, just allocated from scratch.

Jan

> 
>> Jan
>>
>> ---
>>  ChangeLog             |    5 +++++
>>  ksrc/nucleus/shadow.c |    5 +++++
>>  2 files changed, 10 insertions(+)
>>
>> Index: xenomai/ksrc/nucleus/shadow.c
>> ===================================================================
>> --- xenomai.orig/ksrc/nucleus/shadow.c
>> +++ xenomai/ksrc/nucleus/shadow.c
>> @@ -1334,6 +1334,8 @@ void xnshadow_exit(void)
>>   * - -EINVAL is returned if the thread control block does not bear the
>>   * XNSHADOW bit, or if the thread has already been mapped.
>>   *
>> + * - -EBUSY is returned if the current Linux task is already mapped.
>> + *
>>   * Environments:
>>   *
>>   * This service can be called from:
>> @@ -1351,6 +1353,9 @@ int xnshadow_map(xnthread_t *thread, xnc
>>  	unsigned muxid, magic;
>>  	int err;
>>
>> +	if (xnshadow_thread(current))
>> +		return -EBUSY;
>> +
>>  	if (!xnthread_test_state(thread, XNSHADOW))
>>  		return -EINVAL;
>>
>> Index: xenomai/ChangeLog
>> ===================================================================
>> --- xenomai.orig/ChangeLog
>> +++ xenomai/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2008-11-21  Jan Kiszka  <jan.kiszka@domain.hid>
>> +
>> +	* ksrc/nucleus/shadow.c (xnshadow_map): Reject already mapped
>> +	tasks with -EBUSY.
>> +
>>  2008-11-16  Philippe Gerum  <rpm@xenomai.org>
>>
>>  	* ksrc/arch/blackfin/patches: Upgrade to to
> 
> 


-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> My customer managed to find another hidden door to Xenomai's hell:
>>> Trying to create a Xenomai POSIX thread from within a native thread. A
>>> think the other way around would "work" as well. The precise scenario
>>> was native thread -> dlopen(libs that's linked against libpthread_rt) ->
>>> __init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
>>> scenario revealed two more issues in fact.
>>>
>>> Here is a patch to remove this hell gate by catching xnshadow_map
>>> invocations from withing already mapped shadow thread.
>>>
>> Any reason why the XNMAPPED predicate did not work in your case?
> 
> Sorry, which one? Keep in mind that the thread thrown at the second
> xnshadow_map is a virgin one, just allocated from scratch.
>

Ok, got it, it's the converse case, so let's deal for that:

--- ksrc/nucleus/shadow.c	(revision 4411)
+++ ksrc/nucleus/shadow.c	(working copy)
@@ -1331,7 +1331,7 @@
  * case, the real-time mapping operation has failed globally, and no
  * Xenomai resource remains attached to it.
  *
- * - -EINVAL is returned if the thread control block does not bear the
+ * - -EBUSY is returned if the thread control block does not bear the
  * XNSHADOW bit, or if the thread has already been mapped.
  *
  * Environments:
@@ -1354,8 +1354,8 @@
 	if (!xnthread_test_state(thread, XNSHADOW))
 		return -EINVAL;

-	if (xnthread_test_state(thread, XNMAPPED))
-		return -EINVAL;
+	if (xnshadow_thread(current) || xnthread_test_state(thread, XNMAPPED))
+		return -EBUSY;

 	if (!access_wok(u_mode, sizeof(*u_mode)))
 		return -EFAULT;

> Jan
> 
>>> Jan
>>>
>>> ---
>>>  ChangeLog             |    5 +++++
>>>  ksrc/nucleus/shadow.c |    5 +++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> Index: xenomai/ksrc/nucleus/shadow.c
>>> ===================================================================
>>> --- xenomai.orig/ksrc/nucleus/shadow.c
>>> +++ xenomai/ksrc/nucleus/shadow.c
>>> @@ -1334,6 +1334,8 @@ void xnshadow_exit(void)
>>>   * - -EINVAL is returned if the thread control block does not bear the
>>>   * XNSHADOW bit, or if the thread has already been mapped.
>>>   *
>>> + * - -EBUSY is returned if the current Linux task is already mapped.
>>> + *
>>>   * Environments:
>>>   *
>>>   * This service can be called from:
>>> @@ -1351,6 +1353,9 @@ int xnshadow_map(xnthread_t *thread, xnc
>>>  	unsigned muxid, magic;
>>>  	int err;
>>>
>>> +	if (xnshadow_thread(current))
>>> +		return -EBUSY;
>>> +
>>>  	if (!xnthread_test_state(thread, XNSHADOW))
>>>  		return -EINVAL;
>>>
>>> Index: xenomai/ChangeLog
>>> ===================================================================
>>> --- xenomai.orig/ChangeLog
>>> +++ xenomai/ChangeLog
>>> @@ -1,3 +1,8 @@
>>> +2008-11-21  Jan Kiszka  <jan.kiszka@domain.hid>
>>> +
>>> +	* ksrc/nucleus/shadow.c (xnshadow_map): Reject already mapped
>>> +	tasks with -EBUSY.
>>> +
>>>  2008-11-16  Philippe Gerum  <rpm@xenomai.org>
>>>
>>>  	* ksrc/arch/blackfin/patches: Upgrade to to
>>
> 
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Philippe Gerum]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> My customer managed to find another hidden door to Xenomai's hell:
>>>> Trying to create a Xenomai POSIX thread from within a native thread. A
>>>> think the other way around would "work" as well. The precise scenario
>>>> was native thread -> dlopen(libs that's linked against libpthread_rt) ->
>>>> __init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
>>>> scenario revealed two more issues in fact.
>>>>
>>>> Here is a patch to remove this hell gate by catching xnshadow_map
>>>> invocations from withing already mapped shadow thread.
>>>>
>>> Any reason why the XNMAPPED predicate did not work in your case?
>> Sorry, which one? Keep in mind that the thread thrown at the second
>> xnshadow_map is a virgin one, just allocated from scratch.
>>
> 
> Ok, got it, it's the converse case, so let's deal for that:
> 
> --- ksrc/nucleus/shadow.c	(revision 4411)
> +++ ksrc/nucleus/shadow.c	(working copy)
> @@ -1331,7 +1331,7 @@
>   * case, the real-time mapping operation has failed globally, and no
>   * Xenomai resource remains attached to it.
>   *
> - * - -EINVAL is returned if the thread control block does not bear the
> + * - -EBUSY is returned if the thread control block does not bear the
>   * XNSHADOW bit, or if the thread has already been mapped.
>   *
>   * Environments:
> @@ -1354,8 +1354,8 @@
>  	if (!xnthread_test_state(thread, XNSHADOW))
>  		return -EINVAL;
> 
> -	if (xnthread_test_state(thread, XNMAPPED))
> -		return -EINVAL;
> +	if (xnshadow_thread(current) || xnthread_test_state(thread, XNMAPPED))
> +		return -EBUSY;
> 
>  	if (!access_wok(u_mode, sizeof(*u_mode)))
>  		return -EFAULT;
> 

--- ksrc/nucleus/shadow.c	(revision 4411)
+++ ksrc/nucleus/shadow.c	(working copy)
@@ -1332,8 +1332,10 @@
  * Xenomai resource remains attached to it.
  *
  * - -EINVAL is returned if the thread control block does not bear the
- * XNSHADOW bit, or if the thread has already been mapped.
+ * XNSHADOW bit.
  *
+ * - -EBUSY is returned if the thread has already been mapped.
+ *
  * Environments:

-- 
Philippe.

Re: [Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Jan Kiszka]

Philippe Gerum wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> My customer managed to find another hidden door to Xenomai's hell:
>>>>> Trying to create a Xenomai POSIX thread from within a native thread. A
>>>>> think the other way around would "work" as well. The precise scenario
>>>>> was native thread -> dlopen(libs that's linked against libpthread_rt) ->
>>>>> __init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
>>>>> scenario revealed two more issues in fact.
>>>>>
>>>>> Here is a patch to remove this hell gate by catching xnshadow_map
>>>>> invocations from withing already mapped shadow thread.
>>>>>
>>>> Any reason why the XNMAPPED predicate did not work in your case?
>>> Sorry, which one? Keep in mind that the thread thrown at the second
>>> xnshadow_map is a virgin one, just allocated from scratch.
>>>
>> Ok, got it, it's the converse case, so let's deal for that:
>>
>> --- ksrc/nucleus/shadow.c	(revision 4411)
>> +++ ksrc/nucleus/shadow.c	(working copy)
>> @@ -1331,7 +1331,7 @@
>>   * case, the real-time mapping operation has failed globally, and no
>>   * Xenomai resource remains attached to it.
>>   *
>> - * - -EINVAL is returned if the thread control block does not bear the
>> + * - -EBUSY is returned if the thread control block does not bear the
>>   * XNSHADOW bit, or if the thread has already been mapped.
>>   *
>>   * Environments:
>> @@ -1354,8 +1354,8 @@
>>  	if (!xnthread_test_state(thread, XNSHADOW))
>>  		return -EINVAL;
>>
>> -	if (xnthread_test_state(thread, XNMAPPED))
>> -		return -EINVAL;
>> +	if (xnshadow_thread(current) || xnthread_test_state(thread, XNMAPPED))
>> +		return -EBUSY;
>>
>>  	if (!access_wok(u_mode, sizeof(*u_mode)))
>>  		return -EFAULT;
>>
> 
> --- ksrc/nucleus/shadow.c	(revision 4411)
> +++ ksrc/nucleus/shadow.c	(working copy)
> @@ -1332,8 +1332,10 @@
>   * Xenomai resource remains attached to it.
>   *
>   * - -EINVAL is returned if the thread control block does not bear the
> - * XNSHADOW bit, or if the thread has already been mapped.
> + * XNSHADOW bit.
>   *
> + * - -EBUSY is returned if the thread has already been mapped.
> + *
>   * Environments:

The code now returns -EBUSY if someone passes down an already touched
xnthread object - for me a clear case of EINVAL (the object should be a
virgin one). EBUSY should be dedicated to the case that the current
_Linux_task_ is already mapped (to some other xnthread object). I think
my patch is clearer in this respect.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> Jan Kiszka wrote:
>>>>>> My customer managed to find another hidden door to Xenomai's hell:
>>>>>> Trying to create a Xenomai POSIX thread from within a native thread. A
>>>>>> think the other way around would "work" as well. The precise scenario
>>>>>> was native thread -> dlopen(libs that's linked against libpthread_rt) ->
>>>>>> __init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
>>>>>> scenario revealed two more issues in fact.
>>>>>>
>>>>>> Here is a patch to remove this hell gate by catching xnshadow_map
>>>>>> invocations from withing already mapped shadow thread.
>>>>>>
>>>>> Any reason why the XNMAPPED predicate did not work in your case?
>>>> Sorry, which one? Keep in mind that the thread thrown at the second
>>>> xnshadow_map is a virgin one, just allocated from scratch.
>>>>
>>> Ok, got it, it's the converse case, so let's deal for that:
>>>
>>> --- ksrc/nucleus/shadow.c	(revision 4411)
>>> +++ ksrc/nucleus/shadow.c	(working copy)
>>> @@ -1331,7 +1331,7 @@
>>>   * case, the real-time mapping operation has failed globally, and no
>>>   * Xenomai resource remains attached to it.
>>>   *
>>> - * - -EINVAL is returned if the thread control block does not bear the
>>> + * - -EBUSY is returned if the thread control block does not bear the
>>>   * XNSHADOW bit, or if the thread has already been mapped.
>>>   *
>>>   * Environments:
>>> @@ -1354,8 +1354,8 @@
>>>  	if (!xnthread_test_state(thread, XNSHADOW))
>>>  		return -EINVAL;
>>>
>>> -	if (xnthread_test_state(thread, XNMAPPED))
>>> -		return -EINVAL;
>>> +	if (xnshadow_thread(current) || xnthread_test_state(thread, XNMAPPED))
>>> +		return -EBUSY;
>>>
>>>  	if (!access_wok(u_mode, sizeof(*u_mode)))
>>>  		return -EFAULT;
>>>
>> --- ksrc/nucleus/shadow.c	(revision 4411)
>> +++ ksrc/nucleus/shadow.c	(working copy)
>> @@ -1332,8 +1332,10 @@
>>   * Xenomai resource remains attached to it.
>>   *
>>   * - -EINVAL is returned if the thread control block does not bear the
>> - * XNSHADOW bit, or if the thread has already been mapped.
>> + * XNSHADOW bit.
>>   *
>> + * - -EBUSY is returned if the thread has already been mapped.
>> + *
>>   * Environments:
> 
> The code now returns -EBUSY if someone passes down an already touched
> xnthread object - for me a clear case of EINVAL (the object should be a
> virgin one).

That's precisely a case for EBUSY; someone is attempting to reuse a TCB.

 EBUSY should be dedicated to the case that the current
> _Linux_task_ is already mapped (to some other xnthread object). I think
> my patch is clearer in this respect.
>

My intent regarding XNMAPPED was precisely to catch that case on the xnthread
struct, returning -EINVAL was a bad idea: this did not convey the right
information. Your addition completes that test by checking the task_struct side
as well. EINVAL should be kept for out-of-bound / badly specified input param;
in the XNMAPPED case, the xnthread struct is ok, but in the wrong state. This is
what EBUSY is supposed to say.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH] Catch multiple xnshadow_map attempts

[Jan Kiszka]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> Philippe Gerum wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> My customer managed to find another hidden door to Xenomai's hell:
>>>>>>> Trying to create a Xenomai POSIX thread from within a native thread. A
>>>>>>> think the other way around would "work" as well. The precise scenario
>>>>>>> was native thread -> dlopen(libs that's linked against libpthread_rt) ->
>>>>>>> __init_posix_interface -> __wrap_pthread_setschedparam -> oops. That
>>>>>>> scenario revealed two more issues in fact.
>>>>>>>
>>>>>>> Here is a patch to remove this hell gate by catching xnshadow_map
>>>>>>> invocations from withing already mapped shadow thread.
>>>>>>>
>>>>>> Any reason why the XNMAPPED predicate did not work in your case?
>>>>> Sorry, which one? Keep in mind that the thread thrown at the second
>>>>> xnshadow_map is a virgin one, just allocated from scratch.
>>>>>
>>>> Ok, got it, it's the converse case, so let's deal for that:
>>>>
>>>> --- ksrc/nucleus/shadow.c	(revision 4411)
>>>> +++ ksrc/nucleus/shadow.c	(working copy)
>>>> @@ -1331,7 +1331,7 @@
>>>>   * case, the real-time mapping operation has failed globally, and no
>>>>   * Xenomai resource remains attached to it.
>>>>   *
>>>> - * - -EINVAL is returned if the thread control block does not bear the
>>>> + * - -EBUSY is returned if the thread control block does not bear the
>>>>   * XNSHADOW bit, or if the thread has already been mapped.
>>>>   *
>>>>   * Environments:
>>>> @@ -1354,8 +1354,8 @@
>>>>  	if (!xnthread_test_state(thread, XNSHADOW))
>>>>  		return -EINVAL;
>>>>
>>>> -	if (xnthread_test_state(thread, XNMAPPED))
>>>> -		return -EINVAL;
>>>> +	if (xnshadow_thread(current) || xnthread_test_state(thread, XNMAPPED))
>>>> +		return -EBUSY;
>>>>
>>>>  	if (!access_wok(u_mode, sizeof(*u_mode)))
>>>>  		return -EFAULT;
>>>>
>>> --- ksrc/nucleus/shadow.c	(revision 4411)
>>> +++ ksrc/nucleus/shadow.c	(working copy)
>>> @@ -1332,8 +1332,10 @@
>>>   * Xenomai resource remains attached to it.
>>>   *
>>>   * - -EINVAL is returned if the thread control block does not bear the
>>> - * XNSHADOW bit, or if the thread has already been mapped.
>>> + * XNSHADOW bit.
>>>   *
>>> + * - -EBUSY is returned if the thread has already been mapped.
>>> + *
>>>   * Environments:
>> The code now returns -EBUSY if someone passes down an already touched
>> xnthread object - for me a clear case of EINVAL (the object should be a
>> virgin one).
> 
> That's precisely a case for EBUSY; someone is attempting to reuse a TCB.
> 
>  EBUSY should be dedicated to the case that the current
>> _Linux_task_ is already mapped (to some other xnthread object). I think
>> my patch is clearer in this respect.
>>
> 
> My intent regarding XNMAPPED was precisely to catch that case on the xnthread
> struct, returning -EINVAL was a bad idea: this did not convey the right
> information. Your addition completes that test by checking the task_struct side
> as well. EINVAL should be kept for out-of-bound / badly specified input param;
> in the XNMAPPED case, the xnthread struct is ok, but in the wrong state. This is
> what EBUSY is supposed to say.

Well, OK. But then clarify in the doc that EBUSY is about both the
thread structure and the current Linux task.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux