[Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Gilles Chanteperdrix]

In order to be able to handle FPU faults in the Xenomai domain without
switching to the root domain, the FPU backup area must be systematically
allocated for all Linux tasks. Currently, the backup area is systematically
allocated in arch_dup_task_struct(), but gets freed upon exec by
flush_thread(). Stop freeing it in flush_thread(), there does not seem
to be any risk of leaking information, as the "used_math" flag is cleared
by drop_init_fpui() so that upon next fault, the context is reinitialized.
---
 arch/x86/kernel/process.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index a00bed1..4716eb2 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -73,14 +73,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 		if (ret)
 			return ret;
 		fpu_copy(dst, src);
-	} else {
-#ifdef CONFIG_IPIPE
-		/* unconditionally allocate, RT domain may need it */
-		memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
-		ret = fpu_alloc(&dst->thread.fpu);
-		if (ret)
-			return ret;
-#endif
 	}
 	return 0;
 }
@@ -170,8 +162,8 @@ void flush_thread(void)
 	 * lazily at the first use.
 	 */
 	if (!use_eager_fpu())
-#endif
 		free_thread_xstate(tsk);
+#endif
 }
 
 static void hard_disable_TSC(void)
-- 
1.7.10.4

Re: [Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Jan Kiszka]

On 2013-09-30 21:21, Gilles Chanteperdrix wrote:
> In order to be able to handle FPU faults in the Xenomai domain without
> switching to the root domain, the FPU backup area must be systematically
> allocated for all Linux tasks. Currently, the backup area is systematically
> allocated in arch_dup_task_struct(), but gets freed upon exec by
> flush_thread(). Stop freeing it in flush_thread(), there does not seem
> to be any risk of leaking information, as the "used_math" flag is cleared
> by drop_init_fpui() so that upon next fault, the context is reinitialized.

How to reproduce the scenario? Would be good to document it in order to
check for regressions in future kernels.

> ---
>  arch/x86/kernel/process.c |   10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index a00bed1..4716eb2 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -73,14 +73,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
>  		if (ret)
>  			return ret;
>  		fpu_copy(dst, src);
> -	} else {
> -#ifdef CONFIG_IPIPE
> -		/* unconditionally allocate, RT domain may need it */
> -		memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
> -		ret = fpu_alloc(&dst->thread.fpu);
> -		if (ret)
> -			return ret;
> -#endif

Why removing the systematic allocation? This is not explained above and
seems counterproductive.

>  	}
>  	return 0;
>  }
> @@ -170,8 +162,8 @@ void flush_thread(void)
>  	 * lazily at the first use.
>  	 */
>  	if (!use_eager_fpu())
> -#endif
>  		free_thread_xstate(tsk);
> +#endif

Yes, this makes sense. It was wrong in multiple ways.

>  }
>  
>  static void hard_disable_TSC(void)
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> On 2013-09-30 21:21, Gilles Chanteperdrix wrote:
>> In order to be able to handle FPU faults in the Xenomai domain without
>> switching to the root domain, the FPU backup area must be systematically
>> allocated for all Linux tasks. Currently, the backup area is
>> systematically
>> allocated in arch_dup_task_struct(), but gets freed upon exec by
>> flush_thread(). Stop freeing it in flush_thread(), there does not seem
>> to be any risk of leaking information, as the "used_math" flag is
>> cleared
>> by drop_init_fpui() so that upon next fault, the context is
>> reinitialized.
>
> How to reproduce the scenario? Would be good to document it in order to
> check for regressions in future kernels.

Simply shadow the main thread, and use FPU in primary mode, without having
first used it in secondary mode. You should get a kernel oops.

We can add a test to switchtest, the problem is that switchtest purposedly
avoided to use FPU in the main thread before creating the other threads
to test threads which are not created with the "used_math" flag. But we can
use FPU after the other threads creation, right before the call to sigwait.

-- 
                                              Gilles

Re: [Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Jan Kiszka]

On 2013-10-04 10:50, Gilles Chanteperdrix wrote:
> 
> Jan Kiszka wrote:
>> On 2013-09-30 21:21, Gilles Chanteperdrix wrote:
>>> In order to be able to handle FPU faults in the Xenomai domain without
>>> switching to the root domain, the FPU backup area must be systematically
>>> allocated for all Linux tasks. Currently, the backup area is
>>> systematically
>>> allocated in arch_dup_task_struct(), but gets freed upon exec by
>>> flush_thread(). Stop freeing it in flush_thread(), there does not seem
>>> to be any risk of leaking information, as the "used_math" flag is
>>> cleared
>>> by drop_init_fpui() so that upon next fault, the context is
>>> reinitialized.
>>
>> How to reproduce the scenario? Would be good to document it in order to
>> check for regressions in future kernels.
> 
> Simply shadow the main thread, and use FPU in primary mode, without having
> first used it in secondary mode. You should get a kernel oops.
> 
> We can add a test to switchtest, the problem is that switchtest purposedly
> avoided to use FPU in the main thread before creating the other threads
> to test threads which are not created with the "used_math" flag. But we can
> use FPU after the other threads creation, right before the call to sigwait.

Yes, having an explicit test case would be great. Also important is to
avoid using a modern CPU with XSAVE support. Most of our target have
this feature now.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> On 2013-10-04 10:50, Gilles Chanteperdrix wrote:
>>
>> Jan Kiszka wrote:
>>> On 2013-09-30 21:21, Gilles Chanteperdrix wrote:
>>>> In order to be able to handle FPU faults in the Xenomai domain without
>>>> switching to the root domain, the FPU backup area must be
>>>> systematically
>>>> allocated for all Linux tasks. Currently, the backup area is
>>>> systematically
>>>> allocated in arch_dup_task_struct(), but gets freed upon exec by
>>>> flush_thread(). Stop freeing it in flush_thread(), there does not seem
>>>> to be any risk of leaking information, as the "used_math" flag is
>>>> cleared
>>>> by drop_init_fpui() so that upon next fault, the context is
>>>> reinitialized.
>>>
>>> How to reproduce the scenario? Would be good to document it in order to
>>> check for regressions in future kernels.
>>
>> Simply shadow the main thread, and use FPU in primary mode, without
>> having
>> first used it in secondary mode. You should get a kernel oops.
>>
>> We can add a test to switchtest, the problem is that switchtest
>> purposedly
>> avoided to use FPU in the main thread before creating the other threads
>> to test threads which are not created with the "used_math" flag. But we
>> can
>> use FPU after the other threads creation, right before the call to
>> sigwait.
>
> Yes, having an explicit test case would be great. Also important is to
> avoid using a modern CPU with XSAVE support. Most of our target have
> this feature now.

My tests machines do not have xsave.

-- 
                                              Gilles

Re: [Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Gilles Chanteperdrix]

Jan Kiszka wrote:
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index a00bed1..4716eb2 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -73,14 +73,6 @@ int arch_dup_task_struct(struct task_struct *dst,
>> struct task_struct *src)
>>  		if (ret)
>>  			return ret;
>>  		fpu_copy(dst, src);
>> -	} else {
>> -#ifdef CONFIG_IPIPE
>> -		/* unconditionally allocate, RT domain may need it */
>> -		memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
>> -		ret = fpu_alloc(&dst->thread.fpu);
>> -		if (ret)
>> -			return ret;
>> -#endif
>
> Why removing the systematic allocation? This is not explained above and
> seems counterproductive.

Because when we stop deleting the fpu backup area upon exec, no fork ever
takes place for source thread without an fpu backup area. So, the "else"
clause never happens, and we can remove it entirely.


-- 
                                              Gilles

Re: [Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Jan Kiszka]

On 2013-10-04 10:53, Gilles Chanteperdrix wrote:
> 
> Jan Kiszka wrote:
>>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>>> index a00bed1..4716eb2 100644
>>> --- a/arch/x86/kernel/process.c
>>> +++ b/arch/x86/kernel/process.c
>>> @@ -73,14 +73,6 @@ int arch_dup_task_struct(struct task_struct *dst,
>>> struct task_struct *src)
>>>  		if (ret)
>>>  			return ret;
>>>  		fpu_copy(dst, src);
>>> -	} else {
>>> -#ifdef CONFIG_IPIPE
>>> -		/* unconditionally allocate, RT domain may need it */
>>> -		memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
>>> -		ret = fpu_alloc(&dst->thread.fpu);
>>> -		if (ret)
>>> -			return ret;
>>> -#endif
>>
>> Why removing the systematic allocation? This is not explained above and
>> seems counterproductive.
> 
> Because when we stop deleting the fpu backup area upon exec, no fork ever
> takes place for source thread without an fpu backup area. So, the "else"
> clause never happens, and we can remove it entirely.

OK, please add this to the commit log.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Xenomai] [PATCH] x86/ipipe: get FPU backup area allocated for all threads

[Gilles Chanteperdrix]

In order to be able to handle FPU faults in the Xenomai domain without
switching to the root domain, an FPU context must be systematically
allocated for all Linux tasks. Currently, the FPU context is systematically
allocated in arch_dup_task_struct(), but gets freed upon exec by
flush_thread(). Stop freeing it in flush_thread(), there does not seem
to be any risk of leaking information, as the "used_math" flag is cleared
by drop_init_fpu() so that upon next fault, the context is reinitialized.
Also stop creating an FPU context in arch_dup_task_struct() if the source
taks does not have one, as there are no longer any task without an FPU context.
---
 arch/x86/kernel/process.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index a00bed1..4716eb2 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -73,14 +73,6 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 		if (ret)
 			return ret;
 		fpu_copy(dst, src);
-	} else {
-#ifdef CONFIG_IPIPE
-		/* unconditionally allocate, RT domain may need it */
-		memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
-		ret = fpu_alloc(&dst->thread.fpu);
-		if (ret)
-			return ret;
-#endif
 	}
 	return 0;
 }
@@ -170,8 +162,8 @@ void flush_thread(void)
 	 * lazily at the first use.
 	 */
 	if (!use_eager_fpu())
-#endif
 		free_thread_xstate(tsk);
+#endif
 }
 
 static void hard_disable_TSC(void)
-- 
1.7.10.4