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

PATCH In review Oleg Nesterov 4 participants 7 messages Sep 27, 2011 – Oct 7, 2011

Subsystems (3)

x86 Scheduler x86 MM

lore

Share & tools

lore

Oleg Nesterov

author

2011-09-27 17:08 UTC #0 lore raw

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.

include/linux/sched.h +2 -0

@@ -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 {

arch/x86/include/asm/mmu.h +0 -5

@@ -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;

arch/x86/kernel/process_64.c +1 -6

@@ -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;

arch/x86/mm/init_64.c +1 -1

@@ -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;

arch/x86/ia32/ia32_aout.c +1 -1

@@ -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);

Al Viro

2011-09-27 18:32 UTC #1 lore raw

arch/x86/kernel/process_64.c void set_personality_64bit(void) · line 507

Quoted 9 lines · Oleg Nesterov

arch/x86/kernel/process_64.c +0 -3

@@ -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?

arch/x86/kernel/process_64.c void set_personality_ia32(void) · line 521

Quoted 3 lines

arch/x86/kernel/process_64.c +1 -2

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

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

Stephen Wilson

2011-09-28 02:07 UTC #3 lore raw

In reply to Oleg Nesterov

Quoted 5 lines · Oleg Nesterov

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,

Signature · 1 line

steve

Oleg Nesterov

author

2011-09-28 15:59 UTC #2 lore raw

In reply to Al Viro

Quoted 8 lines · Al Viro

Quoted 6 lines · Oleg Nesterov
arch/x86/kernel/process_64.c +0 -0
@@ -502,10 +502,6 @@ void set_personality_64bit(void)
 	/* Make sure to be in 64bit mode */
 	clear_thread_flag(TIF_IA32);
 

Quoted 5 lines

arch/x86/kernel/process_64.c void set_personality_64bit(void) · line 507
Quoted 3 lines
arch/x86/kernel/process_64.c +0 -3
-	/* 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!

Quoted 5 lines

arch/x86/kernel/process_64.c void set_personality_ia32(void) · line 521
Quoted 3 lines
arch/x86/kernel/process_64.c +1 -2
-	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.

Oleg Nesterov

author

2011-09-28 16:00 UTC #4 lore raw

In reply to Stephen Wilson

Quoted 9 lines · Stephen Wilson

Quoted 5 lines · Oleg Nesterov
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.

Oleg Nesterov

author

2011-09-28 18:45 UTC #5 lore raw

PATCH 1/1 x86: replace mm_context_t.ia32_compat by MMF_COMPAT

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 <[email protected]>

---

 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(-)

include/linux/sched.h renamed from include/linux/sched.h~MMF_C +4 -1

@@ -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;

arch/x86/include/asm/mmu.h renamed from arch/x86/include/asm/mmu.h~MMF_C +0 -5

@@ -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;

arch/x86/kernel/process_64.c renamed from arch/x86/kernel/process_64.c~MMF_C +2 -6

@@ -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;

arch/x86/mm/init_64.c renamed from arch/x86/mm/init_64.c~MMF_C +1 -1

@@ -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;

arch/x86/ia32/ia32_aout.c renamed from arch/x86/ia32/ia32_aout.c~MMF_C +1 -1

@@ -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);

Johannes Weiner

Acked-by on v1 1/1

2011-10-07 09:31 UTC #6 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Acked-by Johannes Weiner <[email protected]>