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

[Jan Kiszka <jan.kiszka@domain.hid>]

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