recent change to exit_mmap

Hi,

On ppc64 a 32bit task has 4GB and a 64bit task has 2TB of address space.

We use a bit in the thread struct to decide which limit to apply against TASK_SIZE:

#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \
                TASK_SIZE_USER32 : TASK_SIZE_USER64)

The TIF_32BIT flag gets set in the arch specific SET_PERSONALITY hook in load_elf_binary.

After the recent changes in mm/mmap.c, the following sequence of events happens:

1. a 64bit task tries to exec a 32bit one
2. we reach load_elf_binary
3. call SET_PERSONALITY which sets TIF_32BIT to true
4. call flush_old_exec->exec_mmap->mmput->exit_mmap
5. call unmap_vmas(,,,,TASK_SIZE,) which only flushes mappings below 4GB
6. BUG_ON in exit_mmap

It seems with the TIF_32BIT scheme we have a window between SET_PERSONALITY until exec returns where TASK_SIZE might be considered incorrect (although at which exact point to you decide that, yes we are now in the new context).

Any ideas on how to fix this? Maybe we can move SET_PERSONALITY down after flush_old_exec.

Anton

(Does this only apply to elf?)

If the old process has 64-bit virtual address space and the new process has 32-bit, you want to unmap the 64-bit space, yes?

And if the old process is 32-bit and the new process is 64-bit (possible?) you want to unmap the 32-bit space. But unmapping 64-bit space here is harmless, yes?

Yes, it would be more logical to still be running with the personality of the old process when we try to expunge all its mappings. However I suspect we need all those mappings to grab the exec arguments.

How about we make exit_mmap() be independent of the personality again by doing something like this?

Looks like ia64 needs work, too...

 asm-ppc64/processor.h |    1 +
 mmap.c                |   18 +++++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff -puN mm/mmap.c~exit_mmap-fix mm/mmap.c
@@ -1379,6 +1379,22 @@ void build_mmap_rb(struct mm_struct * mm
 	}
 }
 
+/*
+ * During exit_mmap, TASK_SIZE is not a reliable indication of the virtual
+ * size of the mm which is being torn down.  Because on the exec() path, this
+ * process may have switched its personality from 64-bit to 32-bit prior to
+ * calling exit_mmap().  So TASK_SIZE returns a value suitable for a 32-bit
+ * process, and not the 64-bit process whose mm we need to invalidate.
+ *
+ * So what we do is to always unmap the largest virtual address space which
+ * the architecture supports.  unmap_vmas() will then unmap every VMA in the
+ * mm, which is what we want to happen here.
+ */
+
+#ifndef TASK_SIZE_MAX
+#define TASK_SIZE_MAX TASK_SIZE
+#endif
+
 /* Release all mmaps. */
 void exit_mmap(struct mm_struct *mm)
 {

@@ -1395,7 +1411,7 @@ void exit_mmap(struct mm_struct *mm)
 	tlb = tlb_gather_mmu(mm, 1);
 	flush_cache_mm(mm);
 	mm->map_count -= unmap_vmas(&tlb, mm, mm->mmap, 0,
-					TASK_SIZE, &nr_accounted);
+					TASK_SIZE_MAX, &nr_accounted);
 	vm_unacct_memory(nr_accounted);
 	BUG_ON(mm->map_count);	/* This is just debugging */
 	clear_page_tables(tlb, FIRST_USER_PGD_NR, USER_PTRS_PER_PGD);

diff -puN include/asm-ppc64/processor.h~exit_mmap-fix include/asm-ppc64/processor.h
@@ -630,6 +630,7 @@ extern struct task_struct *last_task_use
 
 #define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \
 		TASK_SIZE_USER32 : TASK_SIZE_USER64)
+#define TASK_SIZE_MAX	TASK_SIZE_USER64
 #endif /* __KERNEL__ */
 
 

_

Andrew> Looks like ia64 needs work, too...

Yes, should be the same problem there. The fix looks fine to me. (Let's just hope I remember it when Linus puts it in his tree...).

Thanks,

	--david

I've updated that patch to cover ia64, but I think we'll run with the other approach - just remove those calls to SET_PERSONALITY().

They just seem illogical anyway - why are we switching into the new image's personality prior to unmapping the old image's memory?

Andrew> Looks like ia64 needs work, too...

Andrew> I've updated that patch to cover ia64, but I think we'll run Andrew> with the other approach - just remove those calls to Andrew> SET_PERSONALITY().

Andrew> They just seem illogical anyway - why are we switching into Andrew> the new image's personality prior to unmapping the old Andrew> image's memory?

I don't know why SET_PERSONALITY() came to be where it is now, but it does make some sense to me. One thing that comes to mind: on ia64, we normally don't map data segments with execute permission but for backwards-compatibility, we need to do that for x86 binaries. I think there might be a problem with that if SET_PERSONALITY() was done too late. Certainly something that could be fixed, but I suspect a similar ordering issue (perhaps on SPARC?) might have triggered the current placement of SET_PERSONALITY().

	--david

hmm. Seems that all the activities between the two first SET_PERSONALITY() calls and the flush_old_exec() are pretty innocuous. And no mappings could be set up there, because flush_old_exec() would remove them again.

I'll ask Dave about it.

Andrew> hmm. Seems that all the activities between the two first Andrew> SET_PERSONALITY() calls and the flush_old_exec() are pretty Andrew> innocuous. And no mappings could be set up there, because Andrew> flush_old_exec() would remove them again.

That's true. Perhaps it is just cruft.

	--david

On seconds thoughts...

Are the first two SET_PERSONALITY() calls in there actually doing anything? Perhaps you're right, and we only need the one at line 615, whcih is in the right place?

Sounds good. The following patch tests out OK.

Anton

===== fs/binfmt_elf.c 1.34 vs edited =====

@@ -536,8 +536,6 @@
 			    strcmp(elf_interpreter,"/usr/lib/ld.so.1") == 0)
 				ibcs2_interpreter = 1;
 
-			SET_PERSONALITY(elf_ex, ibcs2_interpreter);
-
 			interpreter = open_exec(elf_interpreter);
 			retval = PTR_ERR(interpreter);
 			if (IS_ERR(interpreter))

@@ -578,9 +576,6 @@
 			// printk(KERN_WARNING "ELF: Ambiguous type, using ELF\n");
 			interpreter_type = INTERPRETER_ELF;
 		}
-	} else {
-		/* Executables without an interpreter also need a personality  */
-		SET_PERSONALITY(elf_ex, ibcs2_interpreter);
 	}
 
 	/* OK, we are done with that, now set up the arg stuff,