Q: x86: add context tag to mark mm when running a task in 32-bit compatibility mode

Q: x86: add context tag to mark mm when running a task in 32-bit compatibility mode

[Oleg Nesterov]

commit c2ef45df3b98a027ec8f9081bd2a19dff520ef9d

    This tag is intended to mirror the thread info TIF_IA32 flag.

OK, but

	+#ifdef CONFIG_X86_64
	+	/* True if mm supports a task running in 32 bit compatibility mode. */
	+	unsigned short ia32_compat;
	+#endif

Stupid question, why we can't add a simple arch-independent MMF_COMPAT
flag instead?

I am asking because we probably want to backport this change and this
ia32_compat looks a bit annoying, why should we waste a word?

Sure, this is very minor, but perhaps the trivial patch below makes
sense?

Oleg.


--- x/include/linux/sched.h
+++ x/include/linux/sched.h
@@ -436,6 +436,8 @@ extern int get_dumpable(struct mm_struct
 #define MMF_VM_MERGEABLE	16	/* KSM may merge identical pages */
 #define MMF_VM_HUGEPAGE		17	/* set when VM_HUGEPAGE is set on vma */
 
+#define MMF_COMPAT		18	/* this task runs in compat mode. */
+
 #define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 
 struct sighand_struct {
--- x/arch/x86/include/asm/mmu.h
+++ x/arch/x86/include/asm/mmu.h
@@ -12,11 +12,6 @@ typedef struct {
 	void *ldt;
 	int size;
 
-#ifdef CONFIG_X86_64
-	/* True if mm supports a task running in 32 bit compatibility mode. */
-	unsigned short ia32_compat;
-#endif
-
 	struct mutex lock;
 	void *vdso;
 } mm_context_t;
--- x/arch/x86/kernel/process_64.c
+++ x/arch/x86/kernel/process_64.c
@@ -502,10 +502,6 @@ void set_personality_64bit(void)
 	/* Make sure to be in 64bit mode */
 	clear_thread_flag(TIF_IA32);
 
-	/* Ensure the corresponding mm is not marked. */
-	if (current->mm)
-		current->mm->context.ia32_compat = 0;
-
 	/* TBD: overwrites user setup. Should have two bits.
 	   But 64bit processes have always behaved this way,
 	   so it's not too bad. The main problem is just that
@@ -522,8 +518,7 @@ void set_personality_ia32(void)
 	current->personality |= force_personality32;
 
 	/* Mark the associated mm as containing 32-bit tasks. */
-	if (current->mm)
-		current->mm->context.ia32_compat = 1;
+	set_bit(MMF_COMPAT, &current->mm->flags);
 
 	/* Prepare the first "return" to user space */
 	current_thread_info()->status |= TS_COMPAT;
--- x/arch/x86/mm/init_64.c
+++ x/arch/x86/mm/init_64.c
@@ -860,7 +860,7 @@ static struct vm_area_struct gate_vma = 
 struct vm_area_struct *get_gate_vma(struct mm_struct *mm)
 {
 #ifdef CONFIG_IA32_EMULATION
-	if (!mm || mm->context.ia32_compat)
+	if (!mm || test_bit(MMF_COMPAT, &mm->flags))
 		return NULL;
 #endif
 	return &gate_vma;
--- x/arch/x86/ia32/ia32_aout.c
+++ x/arch/x86/ia32/ia32_aout.c
@@ -298,7 +298,7 @@ static int load_aout_binary(struct linux
 	/* OK, This is the point of no return */
 	set_personality(PER_LINUX);
 	set_thread_flag(TIF_IA32);
-	current->mm->context.ia32_compat = 1;
+	set_bit(MMF_COMPAT, &current->mm->flags);
 
 	setup_new_exec(bprm);
 

Re: Q: x86: add context tag to mark mm when running a task in 32-bit compatibility mode

[Al Viro]

On Tue, Sep 27, 2011 at 07:04:48PM +0200, Oleg Nesterov wrote:
> --- x/arch/x86/kernel/process_64.c
> +++ x/arch/x86/kernel/process_64.c
> @@ -502,10 +502,6 @@ void set_personality_64bit(void)
>  	/* Make sure to be in 64bit mode */
>  	clear_thread_flag(TIF_IA32);
>  
> -	/* Ensure the corresponding mm is not marked. */
> -	if (current->mm)
> -		current->mm->context.ia32_compat = 0;

What happens when 32bit task does exec on 64bit binary?

> -	if (current->mm)
> -		current->mm->context.ia32_compat = 1;
> +	set_bit(MMF_COMPAT, &current->mm->flags);

... assuming current->mm is never NULL here.

Re: Q: x86: add context tag to mark mm when running a task in 32-bit compatibility mode

[Stephen Wilson]

On Tue, Sep 27, 2011 at 07:04:48PM +0200, Oleg Nesterov wrote:
> I am asking because we probably want to backport this change and this
> ia32_compat looks a bit annoying, why should we waste a word?
> 
> Sure, this is very minor, but perhaps the trivial patch below makes
> sense?

Definitely makes sense (modulo Al's remarks).


Thanks,

-- 
steve

Re: Q: x86: add context tag to mark mm when running a task in 32-bit compatibility mode

[Oleg Nesterov]

On 09/27, Al Viro wrote:
>
> On Tue, Sep 27, 2011 at 07:04:48PM +0200, Oleg Nesterov wrote:
> > --- x/arch/x86/kernel/process_64.c
> > +++ x/arch/x86/kernel/process_64.c
> > @@ -502,10 +502,6 @@ void set_personality_64bit(void)
> >  	/* Make sure to be in 64bit mode */
> >  	clear_thread_flag(TIF_IA32);
> >
> > -	/* Ensure the corresponding mm is not marked. */
> > -	if (current->mm)
> > -		current->mm->context.ia32_compat = 0;
>
> What happens when 32bit task does exec on 64bit binary?

Nothing. This bit is always zero after init_mm(), it can't copy
MMF_COMPAT.

But. This is wrong anyway. I forgot about fork(). We do not want to
uglify copy_mm(), MMF_INIT_MASK should include MMF_COMPAT and then
set_personality_64bit() needs to clear this bit.

Thanks!

> > -	if (current->mm)
> > -		current->mm->context.ia32_compat = 1;
> > +	set_bit(MMF_COMPAT, &current->mm->flags);
>
> ... assuming current->mm is never NULL here.

Yes, but it can't be NULL?

SET_PERSONALITY() can only be called during exec, and it must be called
after exec_mmap() has already installed the new mm != NULL, otherwise
the current code is buggy anyway.

Oleg.

Re: Q: x86: add context tag to mark mm when running a task in 32-bit compatibility mode

[Oleg Nesterov]

On 09/27, Stephen Wilson wrote:
>
> On Tue, Sep 27, 2011 at 07:04:48PM +0200, Oleg Nesterov wrote:
> > I am asking because we probably want to backport this change and this
> > ia32_compat looks a bit annoying, why should we waste a word?
> >
> > Sure, this is very minor, but perhaps the trivial patch below makes
> > sense?
>
> Definitely makes sense (modulo Al's remarks).

OK, good.

I'll try to test (and double check) this patch, then I'll resend it
"officially".

Oleg.

[PATCH] x86: replace mm_context_t.ia32_compat by MMF_COMPAT

[Oleg Nesterov]

Kill mm_context_t.ia32_compat in favour of arch-independent MMF_COMPAT
flag. This saves a word in mm_struct, and the new flag can be probably
use outside of arch/x86/.

Also, remove the "if (current->mm)" check from set_personality_*().
This can only be called after exec_mmap() installs the new mm != NULL.

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

 include/linux/sched.h        |    5 ++++-
 arch/x86/include/asm/mmu.h   |    5 -----
 arch/x86/kernel/process_64.c |    8 ++------
 arch/x86/mm/init_64.c        |    2 +-
 arch/x86/ia32/ia32_aout.c    |    2 +-
 5 files changed, 8 insertions(+), 14 deletions(-)

--- 3.1/include/linux/sched.h~MMF_C	2011-09-28 19:53:26.000000000 +0200
+++ 3.1/include/linux/sched.h	2011-09-28 19:57:06.000000000 +0200
@@ -436,7 +436,10 @@ extern int get_dumpable(struct mm_struct
 #define MMF_VM_MERGEABLE	16	/* KSM may merge identical pages */
 #define MMF_VM_HUGEPAGE		17	/* set when VM_HUGEPAGE is set on vma */
 
-#define MMF_INIT_MASK		(MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
+#define MMF_COMPAT		18	/* this task runs in compat mode. */
+
+#define MMF_INIT_MASK	\
+	((1 << MMF_COMPAT) | MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
 
 struct sighand_struct {
 	atomic_t		count;
--- 3.1/arch/x86/include/asm/mmu.h~MMF_C	2011-09-28 19:53:26.000000000 +0200
+++ 3.1/arch/x86/include/asm/mmu.h	2011-09-28 19:53:49.000000000 +0200
@@ -12,11 +12,6 @@ typedef struct {
 	void *ldt;
 	int size;
 
-#ifdef CONFIG_X86_64
-	/* True if mm supports a task running in 32 bit compatibility mode. */
-	unsigned short ia32_compat;
-#endif
-
 	struct mutex lock;
 	void *vdso;
 } mm_context_t;
--- 3.1/arch/x86/kernel/process_64.c~MMF_C	2011-09-28 19:53:26.000000000 +0200
+++ 3.1/arch/x86/kernel/process_64.c	2011-09-28 19:59:39.000000000 +0200
@@ -501,10 +501,7 @@ void set_personality_64bit(void)
 
 	/* Make sure to be in 64bit mode */
 	clear_thread_flag(TIF_IA32);
-
-	/* Ensure the corresponding mm is not marked. */
-	if (current->mm)
-		current->mm->context.ia32_compat = 0;
+	clear_bit(MMF_COMPAT, &current->mm->flags);
 
 	/* TBD: overwrites user setup. Should have two bits.
 	   But 64bit processes have always behaved this way,
@@ -522,8 +519,7 @@ void set_personality_ia32(void)
 	current->personality |= force_personality32;
 
 	/* Mark the associated mm as containing 32-bit tasks. */
-	if (current->mm)
-		current->mm->context.ia32_compat = 1;
+	set_bit(MMF_COMPAT, &current->mm->flags);
 
 	/* Prepare the first "return" to user space */
 	current_thread_info()->status |= TS_COMPAT;
--- 3.1/arch/x86/mm/init_64.c~MMF_C	2011-09-28 19:53:26.000000000 +0200
+++ 3.1/arch/x86/mm/init_64.c	2011-09-28 19:53:49.000000000 +0200
@@ -860,7 +860,7 @@ static struct vm_area_struct gate_vma = 
 struct vm_area_struct *get_gate_vma(struct mm_struct *mm)
 {
 #ifdef CONFIG_IA32_EMULATION
-	if (!mm || mm->context.ia32_compat)
+	if (!mm || test_bit(MMF_COMPAT, &mm->flags))
 		return NULL;
 #endif
 	return &gate_vma;
--- 3.1/arch/x86/ia32/ia32_aout.c~MMF_C	2011-09-28 19:53:26.000000000 +0200
+++ 3.1/arch/x86/ia32/ia32_aout.c	2011-09-28 19:53:49.000000000 +0200
@@ -298,7 +298,7 @@ static int load_aout_binary(struct linux
 	/* OK, This is the point of no return */
 	set_personality(PER_LINUX);
 	set_thread_flag(TIF_IA32);
-	current->mm->context.ia32_compat = 1;
+	set_bit(MMF_COMPAT, &current->mm->flags);
 
 	setup_new_exec(bprm);
 

Re: [PATCH] x86: replace mm_context_t.ia32_compat by MMF_COMPAT

[Johannes Weiner]

On Wed, Sep 28, 2011 at 08:41:49PM +0200, Oleg Nesterov wrote:
> Kill mm_context_t.ia32_compat in favour of arch-independent MMF_COMPAT
> flag. This saves a word in mm_struct, and the new flag can be probably
> use outside of arch/x86/.
> 
> Also, remove the "if (current->mm)" check from set_personality_*().
> This can only be called after exec_mmap() installs the new mm != NULL.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Johannes Weiner <jweiner@redhat.com>