Skip to contentSkip to conversation

vfork & co bugfix

PATCH Eric W. Biederman 2 participants 7 messages Jan 10, 1999 – Jan 11, 1999

Subsystems (5)
Memory management VFS MM core Scheduler +1 more subsystems
lore
Share & tools
lore

Eric W. Biederman

author
#0 lore raw

Looking at vfork in 2.2.0-pre6 I was struck by how badly looking at current, (to release a process) hacks up mmput.

It took a little while but eventually a case where this really goes wrong.

In:
kernel/fork.c:copy_mm
fs/exec.c/exec_mmap

If the we can't allocate page tables, we currently erroneously wake up a vfork waiter, and clear our segments, and set the ldt to that of init.

To correct this I've added a companion function mm_release that we call to remove any dangling references to the old mm_struct of the current process.

This also ensures that segment and ldt clearing happen for every x86 process. Not just when the previous memory area was totally freed.

Eric

arch/i386/kernel/process.c +14 -11
diff -uNrX linux-ignore-files linux-2.2.0-pre6/arch/i386/kernel/process.c linux-2.2.0-pre6.eb1/arch/i386/kernel/process.c
@@ -475,22 +475,25 @@
 
 void release_segments(struct mm_struct *mm)
 {
-	/* forget local segments */
-	__asm__ __volatile__("movl %w0,%%fs ; movl %w0,%%gs"
-		: /* no outputs */
-		: "r" (0));
 	if (mm->segments) {
 		void * ldt = mm->segments;
-
-		/*
-		 * Get the LDT entry from init_task.
-		 */
-		current->tss.ldt = _LDT(0);
-		load_ldt(0);
-
 		mm->segments = NULL;
 		vfree(ldt);
 	}
+}
+
+void forget_segments(struct task_struct *tsk)
+{
+	/* forget local segments */
+	__asm__ __volatile__("movl %w0,%%fs ; movl %w0,%%gs"
+		: /* no outputs */
+		: "r" (0));
+
+	/*
+	 * Get the LDT entry from init_task.
+	 */
+	tsk->tss.ldt = _LDT(0);
+	load_ldt(0);
 }
 
 /*
fs/exec.c +2 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/fs/exec.c linux-2.2.0-pre6.eb1/fs/exec.c
@@ -383,6 +383,7 @@
 		exit_mmap(current->mm);
 		clear_page_tables(current);
 		flush_tlb_mm(current->mm);
+		mm_release(current);
 		return 0;
 	}
 

@@ -413,6 +414,7 @@
 	activate_context(current);
 	up(&mm->mmap_sem);
 	mmput(old_mm);
+	mm_release(current);
 	return 0;
 
 	/*
include/asm-alpha/processor.h +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-alpha/processor.h linux-2.2.0-pre6.eb1/include/asm-alpha/processor.h
@@ -113,6 +113,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 /* NOTE: The task struct and the stack go together!  */
 #define alloc_task_struct() \
include/asm-arm/processor.h +1 -3
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-arm/processor.h linux-2.2.0-pre6.eb1/include/asm-arm/processor.h
@@ -53,11 +53,9 @@
 extern void release_thread(struct task_struct *);
 
 /* Copy and release all segment info associated with a VM */
-extern void copy_segments(int nr, struct task_struct *p, struct mm_struct * mm);
-extern void release_segments(struct mm_struct * mm);
-
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 #define init_task	(init_task_union.task)
 #define init_stack	(init_task_union.stack)
include/asm-i386/processor.h +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-i386/processor.h linux-2.2.0-pre6.eb1/include/asm-i386/processor.h
@@ -281,6 +281,7 @@
 /* Copy and release all segment info associated with a VM */
 extern void copy_segments(int nr, struct task_struct *p, struct mm_struct * mm);
 extern void release_segments(struct mm_struct * mm);
+extern void forget_segments(struct task_struct * tsk);
 
 /*
  * FPU lazy state save handling..
include/asm-m68k/processor.h +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-m68k/processor.h linux-2.2.0-pre6.eb1/include/asm-m68k/processor.h
@@ -74,6 +74,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 /*
  * Free current thread data structures etc..
include/asm-mips/processor.h +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-mips/processor.h linux-2.2.0-pre6.eb1/include/asm-mips/processor.h
@@ -178,6 +178,7 @@
 /* Copy and release all segment info associated with a VM */
 #define copy_segments(nr, p, mm) do { } while(0)
 #define release_segments(mm) do { } while(0)
+#define forget_segments(tsk)		do { } while (0)
 
 /*
  * Return saved PC of a blocked thread.
include/asm-ppc/processor.h +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-ppc/processor.h linux-2.2.0-pre6.eb1/include/asm-ppc/processor.h
@@ -299,6 +299,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 /*
  * NOTE! The task struct and the stack go together
include/asm-sparc/processor.h +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-sparc/processor.h linux-2.2.0-pre6.eb1/include/asm-sparc/processor.h
@@ -148,6 +148,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 #ifdef __KERNEL__
include/asm-sparc64/processor.h +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/asm-sparc64/processor.h linux-2.2.0-pre6.eb1/include/asm-sparc64/processor.h
@@ -208,6 +208,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 #ifdef __KERNEL__
 /* Allocation and freeing of task_struct and kernel stack. */
include/linux/sched.h +2 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/include/linux/sched.h linux-2.2.0-pre6.eb1/include/linux/sched.h
@@ -609,6 +609,8 @@
 	atomic_inc(&mm->count);
 }
 extern void mmput(struct mm_struct *);
+/* Remove the current tasks stale references to the old mm_struct */
+extern void mm_release(struct task_struct *);
 
 extern int  copy_thread(int, unsigned long, unsigned long, struct task_struct *, struct pt_regs *);
 extern void flush_thread(void);
kernel/exit.c +1 -0
diff -uNrX linux-ignore-files linux-2.2.0-pre6/kernel/exit.c linux-2.2.0-pre6.eb1/kernel/exit.c
@@ -259,6 +259,7 @@
 		tsk->swappable = 0;
 		SET_PAGE_DIR(tsk, swapper_pg_dir);
 		mmput(mm);
+		mm_release(tsk);
 	}
 }
kernel/fork.c +8 -3
diff -uNrX linux-ignore-files linux-2.2.0-pre6/kernel/fork.c linux-2.2.0-pre6.eb1/kernel/fork.c
@@ -278,14 +278,19 @@
 	return mm;
 }
 
+void mm_release(struct task_struct *tsk)
+{
+	forget_segments(tsk);
+	/* Notify parent sleeping on vfork().
+	 */
+	wake_up(&tsk->p_opptr->vfork_sleep);
+}
+
 /*
  * Decrement the use count and release all resources for an mm.
  */
 void mmput(struct mm_struct *mm)
 {
-	/* notify parent sleeping on vfork() */
-	wake_up(&current->p_opptr->vfork_sleep);
-
 	if (atomic_dec_and_test(&mm->count)) {
 		release_segments(mm);
 		exit_mmap(mm);

--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]

#1 lore raw
Quoted 6 lines · Eric W. Biederman

Looking at vfork in 2.2.0-pre6 I was struck by how badly looking at current, (to release a process) hacks up mmput.

It took a little while but eventually a case where this really goes wrong.

Note that the pre6 version has a number of cases where it can really screw up, don't even look at them too closely. I think I've fixed them all - it was really simple, but there was a few things I hadn't originally thought of.

I'll make a pre7 once I've verified that the recursive semaphores work at least to some degree, could you please take a look at that one?

Linus

--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]

author
#2 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"LT" == Linus Torvalds <[email protected]> writes:

Quoted 6 lines · Eric W. Biederman
Quoted 6 lines

Looking at vfork in 2.2.0-pre6 I was struck by how badly looking at current, (to release a process) hacks up mmput.

It took a little while but eventually a case where this really goes wrong.

LT> Note that the pre6 version has a number of cases where it can really screw LT> up, don't even look at them too closely. I think I've fixed them all - it LT> was really simple, but there was a few things I hadn't originally thought LT> of.

LT> I'll make a pre7 once I've verified that the recursive semaphores work at LT> least to some degree, could you please take a look at that one?

Will do.

The messing up of segments, and the ldt is much older than pre6 though. The vfork release semaphore code just happened to make about the same assumption, so I fixed them both at the same time.

Eric
--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]

author
#3 lore raw
In reply to Linus Torvalds
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"LT" == Linus Torvalds <[email protected]> writes:

Quoted 6 lines · Eric W. Biederman
Quoted 6 lines

Looking at vfork in 2.2.0-pre6 I was struck by how badly looking at current, (to release a process) hacks up mmput.

It took a little while but eventually a case where this really goes wrong.

LT> Note that the pre6 version has a number of cases where it can really screw LT> up, don't even look at them too closely. I think I've fixed them all - it LT> was really simple, but there was a few things I hadn't originally thought LT> of.

Darn it I missed the easy bug (All children waking up the parent), and found the hard one (waking up the parent if fork fails).

The problem in summary is that mmput gets called on error where we keep the old mm, but get rid of the new one.

This introduces a second function that only gets called upon success.

Question. Why don't we let CLONE_VFORK be a standard clone flag?

Eric

arch/i386/kernel/process.c +14 -11
diff -uNrX linux-ignore-files linux-pre-7/arch/i386/kernel/process.c linux-pre-7.eb1/arch/i386/kernel/process.c
@@ -475,22 +475,25 @@
 
 void release_segments(struct mm_struct *mm)
 {
-	/* forget local segments */
-	__asm__ __volatile__("movl %w0,%%fs ; movl %w0,%%gs"
-		: /* no outputs */
-		: "r" (0));
 	if (mm->segments) {
 		void * ldt = mm->segments;
-
-		/*
-		 * Get the LDT entry from init_task.
-		 */
-		current->tss.ldt = _LDT(0);
-		load_ldt(0);
-
 		mm->segments = NULL;
 		vfree(ldt);
 	}
+}
+
+void forget_segments(struct task_struct *tsk)
+{
+	/* forget local segments */
+	__asm__ __volatile__("movl %w0,%%fs ; movl %w0,%%gs"
+		: /* no outputs */
+		: "r" (0));
+
+	/*
+	 * Get the LDT entry from init_task.
+	 */
+	tsk->tss.ldt = _LDT(0);
+	load_ldt(0);
 }
 
 /*
fs/exec.c +2 -0
diff -uNrX linux-ignore-files linux-pre-7/fs/exec.c linux-pre-7.eb1/fs/exec.c
@@ -383,6 +383,7 @@
 		exit_mmap(current->mm);
 		clear_page_tables(current);
 		flush_tlb_mm(current->mm);
+		mm_release(current);
 		return 0;
 	}
 

@@ -413,6 +414,7 @@
 	activate_context(current);
 	up(&mm->mmap_sem);
 	mmput(old_mm);
+	mm_release(current);
 	return 0;
 
 	/*
include/asm-alpha/processor.h +1 -0
diff -uNrX linux-ignore-files linux-pre-7/include/asm-alpha/processor.h linux-pre-7.eb1/include/asm-alpha/processor.h
@@ -113,6 +113,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 /* NOTE: The task struct and the stack go together!  */
 #define alloc_task_struct() \
include/asm-arm/processor.h +1 -3
diff -uNrX linux-ignore-files linux-pre-7/include/asm-arm/processor.h linux-pre-7.eb1/include/asm-arm/processor.h
@@ -53,11 +53,9 @@
 extern void release_thread(struct task_struct *);
 
 /* Copy and release all segment info associated with a VM */
-extern void copy_segments(int nr, struct task_struct *p, struct mm_struct * mm);
-extern void release_segments(struct mm_struct * mm);
-
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 #define init_task	(init_task_union.task)
 #define init_stack	(init_task_union.stack)
include/asm-i386/processor.h +1 -0
diff -uNrX linux-ignore-files linux-pre-7/include/asm-i386/processor.h linux-pre-7.eb1/include/asm-i386/processor.h
@@ -281,6 +281,7 @@
 /* Copy and release all segment info associated with a VM */
 extern void copy_segments(int nr, struct task_struct *p, struct mm_struct * mm);
 extern void release_segments(struct mm_struct * mm);
+extern void forget_segments(struct task_struct * tsk);
 
 /*
  * FPU lazy state save handling..
include/asm-m68k/processor.h +1 -0
diff -uNrX linux-ignore-files linux-pre-7/include/asm-m68k/processor.h linux-pre-7.eb1/include/asm-m68k/processor.h
@@ -74,6 +74,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 /*
  * Free current thread data structures etc..
include/asm-mips/processor.h +1 -0
diff -uNrX linux-ignore-files linux-pre-7/include/asm-mips/processor.h linux-pre-7.eb1/include/asm-mips/processor.h
@@ -178,6 +178,7 @@
 /* Copy and release all segment info associated with a VM */
 #define copy_segments(nr, p, mm) do { } while(0)
 #define release_segments(mm) do { } while(0)
+#define forget_segments(tsk)		do { } while (0)
 
 /*
  * Return saved PC of a blocked thread.
include/asm-ppc/processor.h +1 -0
diff -uNrX linux-ignore-files linux-pre-7/include/asm-ppc/processor.h linux-pre-7.eb1/include/asm-ppc/processor.h
@@ -299,6 +299,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 /*
  * NOTE! The task struct and the stack go together
include/asm-sparc/processor.h +1 -0
diff -uNrX linux-ignore-files linux-pre-7/include/asm-sparc/processor.h linux-pre-7.eb1/include/asm-sparc/processor.h
@@ -148,6 +148,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 #ifdef __KERNEL__
include/asm-sparc64/processor.h +1 -0
diff -uNrX linux-ignore-files linux-pre-7/include/asm-sparc64/processor.h linux-pre-7.eb1/include/asm-sparc64/processor.h
@@ -208,6 +208,7 @@
 
 #define copy_segments(nr, tsk, mm)	do { } while (0)
 #define release_segments(mm)		do { } while (0)
+#define forget_segments(tsk)		do { } while (0)
 
 #ifdef __KERNEL__
 /* Allocation and freeing of task_struct and kernel stack. */
include/linux/sched.h +4 -2
diff -uNrX linux-ignore-files linux-pre-7/include/linux/sched.h linux-pre-7.eb1/include/linux/sched.h
@@ -33,7 +33,7 @@
 #define CLONE_SIGHAND	0x00000800	/* set if signal handlers shared */
 #define CLONE_PID	0x00001000	/* set if pid shared */
 #define CLONE_PTRACE	0x00002000	/* set if we want to let tracing continue on the child too */
-#define CLONE_VFORK	0x00004000	/* set if the parent wants the child to wake it up on mmput */
+#define CLONE_VFORK	0x00004000	/* set if the parent wants the child to wake it up on mm_release */
 
 /*
  * These are the constant used to fake the fixed-point load-average

@@ -321,7 +321,7 @@
 #define PF_DUMPCORE	0x00000200	/* dumped core */
 #define PF_SIGNALED	0x00000400	/* killed by a signal */
 #define PF_MEMALLOC	0x00000800	/* Allocating memory */
-#define PF_VFORK	0x00001000	/* Wake up parent in mmput */
+#define PF_VFORK	0x00001000	/* Wake up parent in mm_release */
 
 #define PF_USEDFPU	0x00100000	/* task used FPU this quantum (SMP) */
 #define PF_DTRACE	0x00200000	/* delayed trace (used on m68k, i386) */

@@ -608,6 +608,8 @@
 	atomic_inc(&mm->count);
 }
 extern void mmput(struct mm_struct *);
+/* Remove the current tasks stale references to the old mm_struct */
+extern void mm_release(struct task_struct *);
 
 extern int  copy_thread(int, unsigned long, unsigned long, struct task_struct *, struct pt_regs *);
 extern void flush_thread(void);
kernel/exit.c +1 -0
diff -uNrX linux-ignore-files linux-pre-7/kernel/exit.c linux-pre-7.eb1/kernel/exit.c
@@ -259,6 +259,7 @@
 		tsk->swappable = 0;
 		SET_PAGE_DIR(tsk, swapper_pg_dir);
 		mmput(mm);
+		mm_release(tsk);
 	}
 }
kernel/fork.c +21 -4
diff -uNrX linux-ignore-files linux-pre-7/kernel/fork.c linux-pre-7.eb1/kernel/fork.c
@@ -278,17 +278,34 @@
 	return mm;
 }
 
-/*
- * Decrement the use count and release all resources for an mm.
+/* Please note the differences between mmput and mm_release.
+ * mmput is called whenever we stop holding onto a mm_struct,
+ * error success whatever.
+ *
+ * mm_release is called after a mm_struct has been removed
+ * from the current process.
+ *
+ * This difference is important for error handling, when we
+ * only half set up a mm_struct for a new process and need to restore
+ * the old one.  Because we mmput the new mm_struct before
+ * restoring the old one. . .
+ * Eric Biederman 10 January 1998
  */
-void mmput(struct mm_struct *mm)
+void mm_release(struct task_struct *tsk)
 {
+	forget_segments(tsk);
 	/* notify parent sleeping on vfork() */
-	if (current->flags & PF_VFORK) {
+	if (tsk->flags & PF_VFORK) {
 		current->flags &= ~PF_VFORK;
 		up(current->p_opptr->vfork_sem);
 	}
+}
 
+/*
+ * Decrement the use count and release all resources for an mm.
+ */
+void mmput(struct mm_struct *mm)
+{
 	if (atomic_dec_and_test(&mm->count)) {
 		release_segments(mm);
 		exit_mmap(mm);

--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]

#4 lore raw
Quoted 3 lines · Eric W. Biederman

Darn it I missed the easy bug (All children waking up the parent), and found the hard one (waking up the parent if fork fails).

I missed it too, until I started thinking about all the possible combinations.

Quoted 1 line

Question. Why don't we let CLONE_VFORK be a standard clone flag?

Because then we're back to the old problem: before doing a vfork(), somebody could do a "clone(CLONE_VFORK)" (which would _not_ wait on the semaphore like a real vfork() would), and now the wrong child can wake up the parent and mess up the real vfork().

Linus

--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]

author
#5 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"LT" == Linus Torvalds <[email protected]> writes:

LT> I missed it too, until I started thinking about all the possible LT> combinations.

Quoted 1 line
Quoted 1 line

Question. Why don't we let CLONE_VFORK be a standard clone flag?

LT> Because then we're back to the old problem: before doing a vfork(), LT> somebody could do a "clone(CLONE_VFORK)" (which would _not_ wait on the LT> semaphore like a real vfork() would), and now the wrong child can wake up LT> the parent and mess up the real vfork().

Sorry. I had the implicit assumption that if CLONE_VFORK was a standard clone flag, do_fork would include the five lines of semaphore code.

Eric

--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]

#6 lore raw
Quoted 11 lines · Eric W. Biederman
Quoted 2 lines
Quoted 2 lines

Question. Why don't we let CLONE_VFORK be a standard clone flag?

LT> Because then we're back to the old problem: before doing a vfork(), LT> somebody could do a "clone(CLONE_VFORK)" (which would _not_ wait on the LT> semaphore like a real vfork() would), and now the wrong child can wake up LT> the parent and mess up the real vfork().

Sorry. I had the implicit assumption that if CLONE_VFORK was a standard clone flag, do_fork would include the five lines of semaphore code.

Oh, ok.

Sure, makes sense, and is probably the right thing to do - that way you can (if you really want to) do some strange half-way vfork(), half-way clone() thing where you share your file descriptors in a vfork().

I don't know how useful it would be, but it would be no uglier than doing it any other way, and I see some advantages (no need for a separate vfork() system call - clone() can do it directly).

I thus remove all my objections,

Linus

--
This is a majordomo managed list. To unsubscribe, send a message with the body 'unsubscribe linux-mm me@address' to: [email protected]