[PATCH rc1-mm 2/3] coredump: shutdown current process first

[PATCH rc1-mm 2/3] coredump: shutdown current process first

[Oleg Nesterov]

This patch optimize zap_threads() for the case when there are
no ->mm users except the current's thread group. In that case
we can avoid 'for_each_process()' loop.

It also adds a useful invariant: SIGNAL_GROUP_EXIT (if checked
under ->siglock) always implies that all threads (except may be
current) have pending SIGKILL.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- MM/fs/exec.c~2_optmz	2006-04-09 02:33:28.000000000 +0400
+++ MM/fs/exec.c	2006-04-09 03:06:43.000000000 +0400
@@ -1383,13 +1383,7 @@ static void format_corename(char *corena
 static void zap_process(struct task_struct *start)
 {
 	struct task_struct *t;
-	unsigned long flags;
 
-	/*
-	 * start->sighand can't disappear, but may be
-	 * changed by de_thread()
-	 */
-	lock_task_sighand(start, &flags);
 	start->signal->flags = SIGNAL_GROUP_EXIT;
 	start->signal->group_stop_count = 0;
 
@@ -1401,40 +1395,51 @@ static void zap_process(struct task_stru
 			signal_wake_up(t, 1);
 		}
 	} while ((t = next_thread(t)) != start);
-
-	unlock_task_sighand(start, &flags);
 }
 
 static inline int zap_threads(struct task_struct *tsk, struct mm_struct *mm,
 				int exit_code)
 {
 	struct task_struct *g, *p;
+	unsigned long flags;
 	int err = -EAGAIN;
 
 	spin_lock_irq(&tsk->sighand->siglock);
 	if (!(tsk->signal->flags & SIGNAL_GROUP_EXIT)) {
-		tsk->signal->flags = SIGNAL_GROUP_EXIT;
 		tsk->signal->group_exit_code = exit_code;
-		tsk->signal->group_stop_count = 0;
+		zap_process(tsk);
 		err = 0;
 	}
 	spin_unlock_irq(&tsk->sighand->siglock);
 	if (err)
 		return err;
 
+	if (atomic_read(&mm->mm_users) == mm->core_waiters + 1)
+		goto done;
+
 	rcu_read_lock();
 	for_each_process(g) {
+		if (g == tsk->group_leader)
+			continue;
+
 		p = g;
 		do {
 			if (p->mm) {
-				if (p->mm == mm)
+				if (p->mm == mm) {
+					/*
+					 * p->sighand can't disappear, but
+					 * may be changed by de_thread()
+					 */
+					lock_task_sighand(p, &flags);
 					zap_process(p);
+					unlock_task_sighand(p, &flags);
+				}
 				break;
 			}
 		} while ((p = next_thread(p)) != g);
 	}
 	rcu_read_unlock();
-
+done:
 	return mm->core_waiters;
 }
 

Re: [PATCH rc1-mm 2/3] coredump: shutdown current process first

[Roland McGrath]

> This patch optimize zap_threads() for the case when there are
> no ->mm users except the current's thread group. In that case
> we can avoid 'for_each_process()' loop.

This is a very good optimization.  Please don't use a goto here when a
simple if block and some reindenting works just fine.

I would be inclined to restructure the inner loop something like this:

		p = g;
		while (unlikely(p->mm == NULL)) {
			p = next_thread(p);
			if (p == g)
				break;
		}
		if (p->mm == mm) {
			/*
			 * p->sighand can't disappear, but
			 * may be changed by de_thread()
			 */
			lock_task_sighand(p, &flags);
			zap_process(p);
			unlock_task_sighand(p, &flags);
		}

But that is just taste.

> It also adds a useful invariant: SIGNAL_GROUP_EXIT (if checked
> under ->siglock) always implies that all threads (except may be
> current) have pending SIGKILL.

I agree that's a sensible thing to be able to rely on (though I don't know
of a practical difference it makes atm).  If this is merged with by
SIGNAL_GROUP_EXEC change, then the invariant is that SIGNAL_GROUP_EXIT
always means that all threads (including current) either have pending
SIGKILL or are already calling do_group_exit/do_exit.


Thanks,
Roland

Re: [PATCH rc1-mm 2/3] coredump: shutdown current process first

[Oleg Nesterov]

On 04/10, Roland McGrath wrote:
>
> I would be inclined to restructure the inner loop something like this:
> 
> 		p = g;
> 		while (unlikely(p->mm == NULL)) {
> 			p = next_thread(p);
> 			if (p == g)
> 				break;
> 		}
> 		if (p->mm == mm) {
> 			/*
> 			 * p->sighand can't disappear, but
> 			 * may be changed by de_thread()
> 			 */
> 			lock_task_sighand(p, &flags);
> 			zap_process(p);
> 			unlock_task_sighand(p, &flags);
> 		}

Yes, I agree, this is much more understandable.

Oleg.

Re: [PATCH rc1-mm 2/3] coredump: shutdown current process first

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/10, Roland McGrath wrote:
>>
>> I would be inclined to restructure the inner loop something like this:
>> 
>> 		p = g;
>> 		while (unlikely(p->mm == NULL)) {
>> 			p = next_thread(p);
>> 			if (p == g)
>> 				break;
>> 		}
>> 		if (p->mm == mm) {
>> 			/*
>> 			 * p->sighand can't disappear, but
>> 			 * may be changed by de_thread()
>> 			 */
>> 			lock_task_sighand(p, &flags);
>> 			zap_process(p);
>> 			unlock_task_sighand(p, &flags);
>> 		}
>
> Yes, I agree, this is much more understandable.

There is one piece of zap_threads that still makes me uncomfortable.

task_lock is used to protect p->mm.
Therefore killing a process based upon p->mm == mm is racy
with respect to sys_unshare I believe if we don't take
task_lock.

Eric

Re: [PATCH rc1-mm 2/3] coredump: shutdown current process first

[Oleg Nesterov]

On 04/14, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > On 04/10, Roland McGrath wrote:
> >>
> >> I would be inclined to restructure the inner loop something like this:
> >> 
> >> 		p = g;
> >> 		while (unlikely(p->mm == NULL)) {
> >> 			p = next_thread(p);
> >> 			if (p == g)
> >> 				break;
> >> 		}
> >> 		if (p->mm == mm) {
> >> 			/*
> >> 			 * p->sighand can't disappear, but
> >> 			 * may be changed by de_thread()
> >> 			 */
> >> 			lock_task_sighand(p, &flags);
> >> 			zap_process(p);
> >> 			unlock_task_sighand(p, &flags);
> >> 		}
> >
> > Yes, I agree, this is much more understandable.
> 
> There is one piece of zap_threads that still makes me uncomfortable.
> 
> task_lock is used to protect p->mm.
> Therefore killing a process based upon p->mm == mm is racy
> with respect to sys_unshare I believe if we don't take
> task_lock.

Well, unshare(CLONE_VM) is not yet supported. Currently (as I see
it) mm->mmap_sem is enough to protect against changing ->mm. Yes,
exit_mm/exec_mmap take task_lock too, so it can be used as well.
Please correct my understanding.

I think it is better to take ->mmap_sem in sys_unshare, this path
is rare.

Oleg.

Re: [PATCH rc1-mm 2/3] coredump: shutdown current process first

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/14, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@tv-sign.ru> writes:
>> 
>> > On 04/10, Roland McGrath wrote:
>> >>
>> >> I would be inclined to restructure the inner loop something like this:
>> >> 
>> >> 		p = g;
>> >> 		while (unlikely(p->mm == NULL)) {
>> >> 			p = next_thread(p);
>> >> 			if (p == g)
>> >> 				break;
>> >> 		}
>> >> 		if (p->mm == mm) {
>> >> 			/*
>> >> 			 * p->sighand can't disappear, but
>> >> 			 * may be changed by de_thread()
>> >> 			 */
>> >> 			lock_task_sighand(p, &flags);
>> >> 			zap_process(p);
>> >> 			unlock_task_sighand(p, &flags);
>> >> 		}
>> >
>> > Yes, I agree, this is much more understandable.
>> 
>> There is one piece of zap_threads that still makes me uncomfortable.
>> 
>> task_lock is used to protect p->mm.
>> Therefore killing a process based upon p->mm == mm is racy
>> with respect to sys_unshare I believe if we don't take
>> task_lock.
>
> Well, unshare(CLONE_VM) is not yet supported. Currently (as I see
> it) mm->mmap_sem is enough to protect against changing ->mm. Yes,
> exit_mm/exec_mmap take task_lock too, so it can be used as well.
> Please correct my understanding.

So what has me unsettled is that task_lock is used to
protect p->mm.  The other place this could be a problem
is exit_mm.  But it does appear that deliberately takes the mm_sem
to prevent this problem.  So it looks like I was just missed
that trick.

> I think it is better to take ->mmap_sem in sys_unshare, this path
> is rare.

Agreed.

Eric

[PATCH] 2.6.17-rc4 bugfix with initramfs

[Nickolay]

This patch fix double inclusion of ramfs-input.

Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>

--- /linux-2.6.17-rc4.orig.old/usr/Makefile 2006-06-08 
21:41:08.000000000 +0400
+++ linux-2.6.17/usr/Makefile 2006-06-09 21:16:53.000000000 +0400
@@ -21,8 +21,7 @@
$(CONFIG_INITRAMFS_SOURCE),-d)
ramfs-args := \
$(if $(CONFIG_INITRAMFS_ROOT_UID), -u $(CONFIG_INITRAMFS_ROOT_UID)) \
- $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID)) \
- $(ramfs-input)
+ $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID))

# .initramfs_data.cpio.gz.d is used to identify all files included
# in initramfs and to detect if any files are added/removed.


-- 
Nickolay Vinogradov
Russia, Saint Petersburg

Re: [PATCH] 2.6.17-rc4 bugfix with initramfs

[Sam Ravnborg]

On Fri, Jun 09, 2006 at 09:24:14PM +0400, Nickolay wrote:
> This patch fix double inclusion of ramfs-input.
> 
> Signed-off-by: Nickolay Vinogradov <nickolay@protei.ru>
Applied, thanks.
[Patch was malformed so applied by hand]

	Sam