Re: x86: get rid of the insane TIF_ABI_PENDING bit

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Oleg Nesterov]

commit 05d43ed8a89c159ff641d472f970e3f1baa66318
Author: H. Peter Anvin <hpa@zytor.com>
Date:   Thu Jan 28 22:14:43 2010 -0800

> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -181,14 +181,8 @@ do {							\
>  void start_thread_ia32(struct pt_regs *regs, u32 new_ip, u32 new_sp);
>  #define compat_start_thread start_thread_ia32
>
> -#define COMPAT_SET_PERSONALITY(ex)			\
> -do {							\
> -	if (test_thread_flag(TIF_IA32))			\
> -		clear_thread_flag(TIF_ABI_PENDING);	\
> -	else						\
> -		set_thread_flag(TIF_ABI_PENDING);	\
> -	current->personality |= force_personality32;	\
> -} while (0)
> +void set_personality_ia32(void);
> +#define COMPAT_SET_PERSONALITY(ex) set_personality_ia32()

OK, but what about force_personality32? With this patch it becomes
unused?

> +void set_personality_ia32(void)
> +{
> +	/* inherit personality from parent */
> +
> +	/* Make sure to be in 32bit mode */
> +	set_thread_flag(TIF_IA32);
> +
> +	/* Prepare the first "return" to user space */
> +	current_thread_info()->status |= TS_COMPAT;

Can't understand why we need TS_COMPAT. I assume this is correct,
this was copied from flush_thread().

What TS_COMPAT actually means? I thought it just means "the task
is inside 32-bit syscall".

If a 64bit task execs a 32bit app, can't this TS_COMPAT break, say,
syscall_get_arguments() ?

Just curious, I don't really understand COMPAT issues anyway.

Oleg.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[H. Peter Anvin]

On 02/15/2010 08:17 AM, Oleg Nesterov wrote:
> 
>> +void set_personality_ia32(void)
>> +{
>> +	/* inherit personality from parent */
>> +
>> +	/* Make sure to be in 32bit mode */
>> +	set_thread_flag(TIF_IA32);
>> +
>> +	/* Prepare the first "return" to user space */
>> +	current_thread_info()->status |= TS_COMPAT;
> 
> Can't understand why we need TS_COMPAT. I assume this is correct,
> this was copied from flush_thread().
> 
> What TS_COMPAT actually means? I thought it just means "the task
> is inside 32-bit syscall".

Yes.  In this case, though, it was a 64-bit syscall when the process did
the exec, but it needs to "return" as if it came from a 32-bit syscall;
that's why we set the TS_COMPAT bit.

> If a 64bit task execs a 32bit app, can't this TS_COMPAT break, say,
> syscall_get_arguments() ?
> 

At that point (this is after the exec!) we don't get arguments anyway.

> Just curious, I don't really understand COMPAT issues anyway.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Oleg Nesterov]

On 02/15, H. Peter Anvin wrote:
>
> On 02/15/2010 08:17 AM, Oleg Nesterov wrote:
> >
> >> +void set_personality_ia32(void)
> >> +{
> >> +	/* inherit personality from parent */
> >> +
> >> +	/* Make sure to be in 32bit mode */
> >> +	set_thread_flag(TIF_IA32);
> >> +
> >> +	/* Prepare the first "return" to user space */
> >> +	current_thread_info()->status |= TS_COMPAT;
> >
> > Can't understand why we need TS_COMPAT. I assume this is correct,
> > this was copied from flush_thread().
> >
> > What TS_COMPAT actually means? I thought it just means "the task
> > is inside 32-bit syscall".
>
> Yes.  In this case, though, it was a 64-bit syscall when the process did
> the exec, but it needs to "return" as if it came from a 32-bit syscall;

Could you please point me where do we check TS_COMPAT during return
to user-mode?

> > If a 64bit task execs a 32bit app, can't this TS_COMPAT break, say,
> > syscall_get_arguments() ?
> >
>
> At that point (this is after the exec!) we don't get arguments anyway.

I meant /proc/pid/syscall, but even if I am right this probably
doesn't matter.

Oleg.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[H. Peter Anvin]

On 02/15/2010 08:50 AM, Oleg Nesterov wrote:
> 
> Could you please point me where do we check TS_COMPAT during return
> to user-mode?
> 

Sorry, I was thinking about TIF_IA32, which is examined on line 415 of
entry_64.S.

>>> If a 64bit task execs a 32bit app, can't this TS_COMPAT break, say,
>>> syscall_get_arguments() ?
>>
>> At that point (this is after the exec!) we don't get arguments anyway.
> 
> I meant /proc/pid/syscall, but even if I am right this probably
> doesn't matter.

What does getting the arguments from a process which has never done a
system call yet even mean?  Presumably we get some kind of "null answer"
which depends on the default register set; in that case the compat null
answer is the correct one.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Oleg Nesterov]

On 02/15, H. Peter Anvin wrote:
>
> On 02/15/2010 08:50 AM, Oleg Nesterov wrote:
> >
> > Could you please point me where do we check TS_COMPAT during return
> > to user-mode?
> >
>
> Sorry, I was thinking about TIF_IA32, which is examined on line 415 of
> entry_64.S.

Yes, I understand we should set TIF_IA32, but my question was about
TS_COMPAT.

Oh. And ELF_PLAT_INIT() clears TIF_IA32... at this point it should
be already cleared by SET_PERSONALITY() ?

OK. If this is all really needed, please ignore me, I don't really
understand these details anyway.

> >> At that point (this is after the exec!) we don't get arguments anyway.
> >
> > I meant /proc/pid/syscall, but even if I am right this probably
> > doesn't matter.
>
> What does getting the arguments from a process which has never done a
> system call yet even mean?  Presumably we get some kind of "null answer"
> which depends on the default register set; in that case the compat null
> answer is the correct one.

Indeed, not sure what I was thinking about, thanks.

Oleg.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[H. Peter Anvin]

On 02/15/2010 08:17 AM, Oleg Nesterov wrote:
>> +void set_personality_ia32(void);
>> +#define COMPAT_SET_PERSONALITY(ex) set_personality_ia32()
> 
> OK, but what about force_personality32? With this patch it becomes
> unused?

This is a bug... I happened not to notice it before on my system
force_personality32 is zero.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[H. Peter Anvin]

On 02/15/2010 08:17 AM, Oleg Nesterov wrote:
>> +
>> +	/* Prepare the first "return" to user space */
>> +	current_thread_info()->status |= TS_COMPAT;
> 
> Can't understand why we need TS_COMPAT. I assume this is correct,
> this was copied from flush_thread().
> 
> What TS_COMPAT actually means? I thought it just means "the task
> is inside 32-bit syscall".
> 
> If a 64bit task execs a 32bit app, can't this TS_COMPAT break, say,
> syscall_get_arguments() ?
> 
> Just curious, I don't really understand COMPAT issues anyway.
> 

I suspect the purpose of TS_COMPAT is actually so you can ptrace() the
newly exec'd process (and see it as a 32-bit process!) before it returns
to userspace.  The comment, obviously, is wrong -- that again refers to
TIF_IA32.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Roland McGrath]

> I suspect the purpose of TS_COMPAT is actually so you can ptrace() the
> newly exec'd process (and see it as a 32-bit process!) before it returns
> to userspace.  The comment, obviously, is wrong -- that again refers to
> TIF_IA32.

I don't follow you.  TS_COMPAT does not affect ptrace.
It affects syscall-audit, but only at syscall entry time.
It affects asm/syscall.h accessors, but ptrace does not use those.
It affects whatever uses is_compat_task(), but I can't see anything
where that matters except inside some particular syscall or for
syscall restart after signals.

TS_COMPAT does not affect the syscall return machinery itself.
The return path just follows from the particular entry path.


Thanks,
Roland

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[H. Peter Anvin]

On 02/15/2010 11:41 AM, Roland McGrath wrote:
>> I suspect the purpose of TS_COMPAT is actually so you can ptrace() the
>> newly exec'd process (and see it as a 32-bit process!) before it returns
>> to userspace.  The comment, obviously, is wrong -- that again refers to
>> TIF_IA32.
> 
> I don't follow you.  TS_COMPAT does not affect ptrace.
> It affects syscall-audit, but only at syscall entry time.
> It affects asm/syscall.h accessors, but ptrace does not use those.
> It affects whatever uses is_compat_task(), but I can't see anything
> where that matters except inside some particular syscall or for
> syscall restart after signals.
> 
> TS_COMPAT does not affect the syscall return machinery itself.
> The return path just follows from the particular entry path.

It's entirely possible it is completely superfluous.  The patch in
question only moves the setting.  I don't know what gdb or strace use to
distinguish a 32-bit and a 64-bit process, which is why I thought it
might have something to do with ptrace.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Roland McGrath]

> It's entirely possible it is completely superfluous.  The patch in
> question only moves the setting.  I don't know what gdb or strace use to
> distinguish a 32-bit and a 64-bit process, which is why I thought it
> might have something to do with ptrace.

Well, whatever it that might be, they have no way to know about the
TS_COMPAT setting.  TS_COMPAT affects three things: audit_syscall_entry()
parameters, asm/syscall.h stuff, and is_compat_task()--nothing that is
directly visible from userland unless you count /proc/pid/syscall values.
Notably, that includes syscall_get_error() that is used in signal recovery.

AFAICT the only interesting use of is_compat_task() is the drivers/input stuff.


Thanks,
Roland

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[H. Peter Anvin]

On 02/15/2010 11:41 AM, Roland McGrath wrote:
> It affects whatever uses is_compat_task(), but I can't see anything
> where that matters except inside some particular syscall or for
> syscall restart after signals.

FWIW, the origin of this is checkin
4d9bc79cd28b779610d9590b3a96a28a0f64a25a (2.6.18-rc1), which somewhat
unhelpfully states "Make sure is_compat_task works early".  It doesn't
specify what the failure is if is_compat_task doesn't work early.  On
the other hand, it sure as heck seems better to set it and not need it
than the other way around.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Roland McGrath]

> FWIW, the origin of this is checkin
> 4d9bc79cd28b779610d9590b3a96a28a0f64a25a (2.6.18-rc1), which somewhat
> unhelpfully states "Make sure is_compat_task works early".  It doesn't
> specify what the failure is if is_compat_task doesn't work early.  

I can't figure out what could care.  Maybe at the time something else
tested it that doesn't do so now.

> On the other hand, it sure as heck seems better to set it and not need it
> than the other way around.

Agreed.


Thanks,
Roland

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Oleg Nesterov]

On 02/15, H. Peter Anvin wrote:
>
> On 02/15/2010 11:41 AM, Roland McGrath wrote:
> > It affects whatever uses is_compat_task(), but I can't see anything
> > where that matters except inside some particular syscall or for
> > syscall restart after signals.
>
> FWIW, the origin of this is checkin
> 4d9bc79cd28b779610d9590b3a96a28a0f64a25a (2.6.18-rc1), which somewhat
> unhelpfully states "Make sure is_compat_task works early".  It doesn't
> specify what the failure is if is_compat_task doesn't work early.

Perhaps Andi could explain us why this is needed,

> On
> the other hand, it sure as heck seems better to set it and not need it
> than the other way around.

Agreed, but otoh it is always good to understand the code. If we
really have a reason for TS_COMPAT, a small comment can help other
readers.

Oleg.

Re: x86: get rid of the insane TIF_ABI_PENDING bit

[Andi Kleen]

On Tue, Feb 16, 2010 at 11:19:03AM +0100, Oleg Nesterov wrote:
> On 02/15, H. Peter Anvin wrote:
> >
> > On 02/15/2010 11:41 AM, Roland McGrath wrote:
> > > It affects whatever uses is_compat_task(), but I can't see anything
> > > where that matters except inside some particular syscall or for
> > > syscall restart after signals.
> >
> > FWIW, the origin of this is checkin
> > 4d9bc79cd28b779610d9590b3a96a28a0f64a25a (2.6.18-rc1), which somewhat
> > unhelpfully states "Make sure is_compat_task works early".  It doesn't
> > specify what the failure is if is_compat_task doesn't work early.
> 
> Perhaps Andi could explain us why this is needed,
> 
> > On
> > the other hand, it sure as heck seems better to set it and not need it
> > than the other way around.
> 
> Agreed, but otoh it is always good to understand the code. If we
> really have a reason for TS_COMPAT, a small comment can help other
> readers.

My memory is somewhat fuzzy on this one, but I think it was related
to VMA placement (probably for stack randomization or something like that)
This happens before the first call. I might be wrong on that.

There might also have been other is_compat_task checks in the exec init
path, so partly it was defensive programming.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

[PATCH 0/3] Was: x86: get rid of the insane TIF_ABI_PENDING bit

[Oleg Nesterov]

On 02/16, Andi Kleen wrote:
>
> On Tue, Feb 16, 2010 at 11:19:03AM +0100, Oleg Nesterov wrote:
> >
> > Agreed, but otoh it is always good to understand the code. If we
> > really have a reason for TS_COMPAT, a small comment can help other
> > readers.
>
> My memory is somewhat fuzzy on this one, but I think it was related
> to VMA placement (probably for stack randomization or something like that)
> This happens before the first call. I might be wrong on that.

Afaics we never check TS_COMPAT/is_compat_task for this...

> There might also have been other is_compat_task checks in the exec init
> path, so partly it was defensive programming.

Understand, but it looks so confusing...

OK. Please feel free to ignore, but I am sending the trivial, but only
compile-tested patches. My main motivation is to simplify the reading
and understanding of this code.

The first patch looks like an "obvious" bugfix for 2.6.33 though, but
still untested.

Oleg.

[PATCH 1/3] x86: set_personality_ia32() misses force_personality32

[Oleg Nesterov]

05d43ed8a "x86: get rid of the insane TIF_ABI_PENDING bit" forgot
about force_personality32, fix.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 arch/x86/kernel/process_64.c |    1 +
 1 file changed, 1 insertion(+)

--- exec/arch/x86/kernel/process_64.c~1_force_personality32	2010-01-31 13:57:24.000000000 +0100
+++ exec/arch/x86/kernel/process_64.c	2010-02-16 13:26:34.000000000 +0100
@@ -527,6 +527,7 @@ void set_personality_ia32(void)
 
 	/* Make sure to be in 32bit mode */
 	set_thread_flag(TIF_IA32);
+	current->personality |= force_personality32;
 
 	/* Prepare the first "return" to user space */
 	current_thread_info()->status |= TS_COMPAT;

[tip:x86/urgent] x86: set_personality_ia32() misses force_personality32

[tip-bot for Oleg Nesterov]

Commit-ID:  1dfc76ec37a59018d8ed39152695ff27a434415b
Gitweb:     http://git.kernel.org/tip/1dfc76ec37a59018d8ed39152695ff27a434415b
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Tue, 16 Feb 2010 15:02:13 +0100
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Tue, 16 Feb 2010 09:44:30 -0800

x86: set_personality_ia32() misses force_personality32

05d43ed8a "x86: get rid of the insane TIF_ABI_PENDING bit" forgot
about force_personality32, fix.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
LKML-Reference: <20100216140213.GB16448@redhat.com>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/process_64.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 41a26a8..126f0b4 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -527,6 +527,7 @@ void set_personality_ia32(void)
 
 	/* Make sure to be in 32bit mode */
 	set_thread_flag(TIF_IA32);
+	current->personality |= force_personality32;
 
 	/* Prepare the first "return" to user space */
 	current_thread_info()->status |= TS_COMPAT;

[PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[Oleg Nesterov]

set_personality_ia32() sets TS_COMPAT for unknown reason.

This doesn't hurt but this is unneeded and confusing, TS_COMPAT
means we are inside the 32bit syscall.

In fact I'd say this is not right, but fortunetely do_execve() can
never return something which could confuse syscall_get_error().
And apart from do_signal() we never check TS_COMPAT during return
to user-mode.

Another reason why I think this is not right. I am not sure I fully
understand this asm, but it seems to me that system_call_fastpath
can "leak" TS_COMPAT. While probably this doesn't really matter, we
can return to user-mode with this bit set.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 arch/x86/kernel/process_64.c |    3 ---
 1 file changed, 3 deletions(-)

--- exec/arch/x86/kernel/process_64.c~2_dont_set_compat	2010-02-16 13:26:34.000000000 +0100
+++ exec/arch/x86/kernel/process_64.c	2010-02-16 13:46:25.000000000 +0100
@@ -528,9 +528,6 @@ void set_personality_ia32(void)
 	/* Make sure to be in 32bit mode */
 	set_thread_flag(TIF_IA32);
 	current->personality |= force_personality32;
-
-	/* Prepare the first "return" to user space */
-	current_thread_info()->status |= TS_COMPAT;
 }
 
 unsigned long get_wchan(struct task_struct *p)

Re: [PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[Linus Torvalds]

On Tue, 16 Feb 2010, Oleg Nesterov wrote:
> 
> In fact I'd say this is not right, but fortunetely do_execve() can
> never return something which could confuse syscall_get_error().
> And apart from do_signal() we never check TS_COMPAT during return
> to user-mode.

But 'do_signal()' _can_ happen the first thing after an execve(), no?

And after we have switched to 32-bit mode, we _are_ inside a 32-bit system 
call: the execve has "changed" from a 64-bit one to a 32-bit one.

So I really don't understand why you dislike TS_COMPAT here.

I understand not liking TS_COMPAT in the first place (it would be nice to 
not have that flag at all), but considering that it exists, and it is 
supposed to be set while in 32-bit system calls, setting it on a 32-bit 
execve() seems to be the RightThing(tm) to do.

		Linus

Re: [PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[Oleg Nesterov]

On 02/16, Linus Torvalds wrote:
>
> On Tue, 16 Feb 2010, Oleg Nesterov wrote:
> >
> > In fact I'd say this is not right, but fortunetely do_execve() can
> > never return something which could confuse syscall_get_error().
> > And apart from do_signal() we never check TS_COMPAT during return
> > to user-mode.
>
> But 'do_signal()' _can_ happen the first thing after an execve(), no?

this is what I meant,

> And after we have switched to 32-bit mode, we _are_ inside a 32-bit system
> call: the execve has "changed" from a 64-bit one to a 32-bit one.

And this is what I do not understand, we are still in 64-bit execve.

With ot without TS_COMPAT we take the same return path and I can't
see why should we sign-extend ->ax. But I didn't claim this is wrong,
and it is possible I missed something.

> So I really don't understand why you dislike TS_COMPAT here.

The only reason I dislike TS_COMPAT is that I spent a lot of time
trying to understand the necessity to set it here when I tried to
understand the basics of compat issues.

> I understand not liking TS_COMPAT in the first place (it would be nice to
> not have that flag at all), but considering that it exists, and it is
> supposed to be set while in 32-bit system calls, setting it on a 32-bit
> execve() seems to be the RightThing(tm) to do.

OK, please ignore the patch then.

As I said, only the first patch probably makes sense, and even it
was not tested.



Cough. And since I already made a lot of noise...

Now that we always call setup_new_exec() which does
arch_pick_mmap_layout(), what is the point of
exec_mmap()->arch_pick_mmap_layout() ?

Oleg.

Re: [PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[H. Peter Anvin]

On 02/16/2010 09:16 AM, Oleg Nesterov wrote:
> 
> The only reason I dislike TS_COMPAT is that I spent a lot of time
> trying to understand the necessity to set it here when I tried to
> understand the basics of compat issues.
> 

Just to throw something else into here...

please also keep in mind that there is value to the notion of keeping a
software invariant true.  In this case, even if setting TS_COMPAT
doesn't do anything *right now*, it helps maintain an invariant which
might avoid bugs in the future.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[Oleg Nesterov]

On 02/16, Linus Torvalds wrote:
>
> And after we have switched to 32-bit mode, we _are_ inside a 32-bit system
> call: the execve has "changed" from a 64-bit one to a 32-bit one.
>
> So I really don't understand why you dislike TS_COMPAT here.

and, following this logic, shouldn't set_personality_64bit() clear
TS_COMPAT ?

OK, in any case I do not claim we need fixes. Just I am confused.

Oleg.

Re: [PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[H. Peter Anvin]

On 02/16/2010 09:44 AM, Oleg Nesterov wrote:
> On 02/16, Linus Torvalds wrote:
>>
>> And after we have switched to 32-bit mode, we _are_ inside a 32-bit system
>> call: the execve has "changed" from a 64-bit one to a 32-bit one.
>>
>> So I really don't understand why you dislike TS_COMPAT here.
> 
> and, following this logic, shouldn't set_personality_64bit() clear
> TS_COMPAT ?
> 

It's quite possible it should... I haven't dug into if that isn't either
done elsewhere or isn't done for some other reason.  This would be worth
looking into.

> OK, in any case I do not claim we need fixes. Just I am confused.

Trying to understand the code is good.  However, you seem to have
started out with a point of view that we should have the minimal set of
state changes possible instead of keeping state as self-consistent as
possible.  Invariants are a Very Good Thing.  Documented invariants are
even better ;)

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[Oleg Nesterov]

On 02/16, H. Peter Anvin wrote:
>
> On 02/16/2010 09:44 AM, Oleg Nesterov wrote:
> >
> > and, following this logic, shouldn't set_personality_64bit() clear
> > TS_COMPAT ?
>
> It's quite possible it should... I haven't dug into if that isn't either
> done elsewhere or isn't done for some other reason.  This would be worth
> looking into.

OK. This was another source of confusion for me...

> > OK, in any case I do not claim we need fixes. Just I am confused.
>
> Trying to understand the code is good.  However, you seem to have
> started out with a point of view that we should have the minimal set of
> state changes possible

Well, I must admit... the only point of this patch was "please change
your code so that I could convince myself I understand what it does" ;)

> instead of keeping state as self-consistent as
> possible.  Invariants are a Very Good Thing.  Documented invariants are
> even better ;)

Agreed! But to me it looks as if TS_COMPAT breaks invariants.
In particular, because set_personality_64bit() didn't clear this flag.


Anyway. At least I can assume there is no "hard" reason to set this
bit currently, and this was my main question.

Thanks to all for your explanations!

Oleg.

Re: [PATCH 2/3] x86: set_personality_ia32() abuses TS_COMPAT

[Linus Torvalds]

On Tue, 16 Feb 2010, Oleg Nesterov wrote:
> 
> and, following this logic, shouldn't set_personality_64bit() clear
> TS_COMPAT ?

Yes, I think it should. If we come from a 32-bit environment, and execute 
a 64-bit binary, I think the execve() environment of the new 64-bit binary 
should be _exactly_ the same as if it was executed from a 64-bit one.

Of course, it's entirely possible that TS_COMPAT simply doesn't matter at 
all in either context, but I do think that we should strive for "the 
binary does exactly the same regardless of whether it was started from 
a 32-bit or a 64-bit binary".

And the best way to guarantee that is to make sure that we really do set 
the environment to the same thing. So yes, set_personality_64bit() should 
clear all the 32-bit compat bits.

		Linus

[PATCH 3/3] x86: ELF_PLAT_INIT() shouldn't worry about TIF_IA32

[Oleg Nesterov]

64bit vesrion of ELF_PLAT_INIT() clears TIF_IA32, but at this point it
must be already cleared by SET_PERSONALITY == set_personality_64bit.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 arch/x86/include/asm/elf.h |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- exec/arch/x86/include/asm/elf.h~3_ELF_PLAT_INIT_IA32	2010-01-31 13:57:24.000000000 +0100
+++ exec/arch/x86/include/asm/elf.h	2010-02-16 14:44:31.000000000 +0100
@@ -170,10 +170,7 @@ static inline void elf_common_init(struc
 }
 
 #define ELF_PLAT_INIT(_r, load_addr)			\
-do {							\
-	elf_common_init(&current->thread, _r, 0);	\
-	clear_thread_flag(TIF_IA32);			\
-} while (0)
+	elf_common_init(&current->thread, _r, 0);
 
 #define	COMPAT_ELF_PLAT_INIT(regs, load_addr)		\
 	elf_common_init(&current->thread, regs, __USER_DS)

Re: [PATCH 3/3] x86: ELF_PLAT_INIT() shouldn't worry about TIF_IA32

[Oleg Nesterov]

On 02/16, Oleg Nesterov wrote:
>
>  #define ELF_PLAT_INIT(_r, load_addr)			\
> -do {							\
> -	elf_common_init(&current->thread, _r, 0);	\
> -	clear_thread_flag(TIF_IA32);			\
> -} while (0)
> +	elf_common_init(&current->thread, _r, 0);
                                                ^
unneeded semicolon, sorry...

------------------------------------------------------------------------------
[PATCH 3/3] x86: ELF_PLAT_INIT() shouldn't worry about TIF_IA32

64bit vesrion of ELF_PLAT_INIT() clears TIF_IA32, but at this point it
must be already cleared by SET_PERSONALITY == set_personality_64bit.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 arch/x86/include/asm/elf.h |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- exec/arch/x86/include/asm/elf.h~3_ELF_PLAT_INIT_IA32	2010-01-31 13:57:24.000000000 +0100
+++ exec/arch/x86/include/asm/elf.h	2010-02-16 15:19:43.000000000 +0100
@@ -170,10 +170,7 @@ static inline void elf_common_init(struc
 }
 
 #define ELF_PLAT_INIT(_r, load_addr)			\
-do {							\
-	elf_common_init(&current->thread, _r, 0);	\
-	clear_thread_flag(TIF_IA32);			\
-} while (0)
+	elf_common_init(&current->thread, _r, 0)
 
 #define	COMPAT_ELF_PLAT_INIT(regs, load_addr)		\
 	elf_common_init(&current->thread, regs, __USER_DS)