[PATCH] x86 tls prevent_tail_call

[PATCH] x86 tls prevent_tail_call

[Roland McGrath]

The x86 TLS cleanup in commit efd1ca52d04d2f6df337a3332cee56cd60e6d4c4
made the sys_set_thread_area and sys_get_thread_area functions ripe for
tail call optimization.  If the compiler chooses to use it for them, it
can clobber the user trap frame because these are asmlinkage functions.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/kernel/tls.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/tls.c b/arch/x86/kernel/tls.c
index 6dfd4e7..022bcaa 100644
--- a/arch/x86/kernel/tls.c
+++ b/arch/x86/kernel/tls.c
@@ -91,7 +91,9 @@ int do_set_thread_area(struct task_struct *p, int idx,
 
 asmlinkage int sys_set_thread_area(struct user_desc __user *u_info)
 {
-	return do_set_thread_area(current, -1, u_info, 1);
+	int ret = do_set_thread_area(current, -1, u_info, 1);
+	prevent_tail_call(ret);
+	return ret;
 }
 
 
@@ -139,7 +141,9 @@ int do_get_thread_area(struct task_struct *p, int idx,
 
 asmlinkage int sys_get_thread_area(struct user_desc __user *u_info)
 {
-	return do_get_thread_area(current, -1, u_info);
+	int ret = do_get_thread_area(current, -1, u_info);
+	prevent_tail_call(ret);
+	return ret;
 }
 
 int regset_tls_active(struct task_struct *target,

Re: [PATCH] x86 tls prevent_tail_call

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> The x86 TLS cleanup in commit efd1ca52d04d2f6df337a3332cee56cd60e6d4c4 
> made the sys_set_thread_area and sys_get_thread_area functions ripe 
> for tail call optimization.  If the compiler chooses to use it for 
> them, it can clobber the user trap frame because these are asmlinkage 
> functions.

thanks, applied.

>  asmlinkage int sys_set_thread_area(struct user_desc __user *u_info)
>  {
> -	return do_set_thread_area(current, -1, u_info, 1);
> +	int ret = do_set_thread_area(current, -1, u_info, 1);
> +	prevent_tail_call(ret);
> +	return ret;

i'm wondering, have you seen this happen in practice? We use 
sys_set_thread_area() for every new task started up. I guess we havent 
seen problems in the field yet because this early during startup tasks 
do not normally receive signals? (or if they do they are fatal and no 
user signal context is used.)

btw., gcc 4.2.3 doesnt do it due to CONFIG_FRAME_POINTERS=y here, and 
most distros turn on frame pointers.

btw., this whole thing of us having to notice such tail-optimization 
incidents is totally fragile and unreliable. Shouldnt there be a "dont 
tail-optimize me" attribute which we could stick into asmlinkage? 
Perhaps sparse could detect asmlinkage functions that do not do 
prevent_tail_call()s?

	Ingo

Re: [PATCH] x86 tls prevent_tail_call

[Roland McGrath]

> i'm wondering, have you seen this happen in practice? We use 
> sys_set_thread_area() for every new task started up. I guess we havent 
> seen problems in the field yet because this early during startup tasks 
> do not normally receive signals? (or if they do they are fatal and no 
> user signal context is used.)

Tomasz saw it.  I don't know what compiler or exact options to it he used.

> btw., this whole thing of us having to notice such tail-optimization 
> incidents is totally fragile and unreliable. Shouldnt there be a "dont 
> tail-optimize me" attribute which we could stick into asmlinkage? 

I agree.  It's come up before.  I'll talk to compiler folks about it again.

> Perhaps sparse could detect asmlinkage functions that do not do 
> prevent_tail_call()s?

That sounds like a good idea to me.


Thanks,
Roland

Re: [PATCH] x86 tls prevent_tail_call

[Tomasz Grobelny]

Dnia Wednesday 27 of February 2008, Roland McGrath napisał:
> > i'm wondering, have you seen this happen in practice? We use
> > sys_set_thread_area() for every new task started up. I guess we havent
> > seen problems in the field yet because this early during startup tasks
> > do not normally receive signals? (or if they do they are fatal and no
> > user signal context is used.)
>
> Tomasz saw it.  I don't know what compiler or exact options to it he used.
>
VMware image from http://student.agh.edu.pl/~grobelny/linux/BUG.tar 
demonstrates the problem. I didn't play with compiler options myself but used
http://cvs.pld-linux.org/cgi-bin/cvsweb.cgi/SPECS/kernel-vanilla.spec (rev. 
1.129) as basis for my build.
-- 
Regards,
Tomasz Grobelny