LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] x86 tls prevent_tail_call
@ 2008-02-26 21:00 Roland McGrath
  2008-02-27  7:26 ` Ingo Molnar
  0 siblings, 1 reply; 4+ messages in thread
From: Roland McGrath @ 2008-02-26 21:00 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner
  Cc: Andrew Morton, Linus Torvalds, linux-kernel, Tomasz Grobelny


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,

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86 tls prevent_tail_call
  2008-02-26 21:00 [PATCH] x86 tls prevent_tail_call Roland McGrath
@ 2008-02-27  7:26 ` Ingo Molnar
  2008-02-27  7:38   ` Roland McGrath
  0 siblings, 1 reply; 4+ messages in thread
From: Ingo Molnar @ 2008-02-27  7:26 UTC (permalink / raw)
  To: Roland McGrath
  Cc: Thomas Gleixner, Andrew Morton, Linus Torvalds, linux-kernel,
	Tomasz Grobelny


* 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86 tls prevent_tail_call
  2008-02-27  7:26 ` Ingo Molnar
@ 2008-02-27  7:38   ` Roland McGrath
  2008-02-27 19:59     ` Tomasz Grobelny
  0 siblings, 1 reply; 4+ messages in thread
From: Roland McGrath @ 2008-02-27  7:38 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Thomas Gleixner, Andrew Morton, Linus Torvalds, linux-kernel,
	Tomasz Grobelny

> 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86 tls prevent_tail_call
  2008-02-27  7:38   ` Roland McGrath
@ 2008-02-27 19:59     ` Tomasz Grobelny
  0 siblings, 0 replies; 4+ messages in thread
From: Tomasz Grobelny @ 2008-02-27 19:59 UTC (permalink / raw)
  To: Roland McGrath
  Cc: Ingo Molnar, Thomas Gleixner, Andrew Morton, Linus Torvalds,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2008-02-27 20:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-26 21:00 [PATCH] x86 tls prevent_tail_call Roland McGrath
2008-02-27  7:26 ` Ingo Molnar
2008-02-27  7:38   ` Roland McGrath
2008-02-27 19:59     ` Tomasz Grobelny

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).