Bug#391319: [Buildd-tools-devel] Bug#391319: schroot: leftover processes cause umount to fail
tags 391319 + fixed-upstream pending
thanks
Kees Cook [EMAIL PROTECTED] writes:
On Wed, May 23, 2007 at 08:38:15PM +0100, Roger Leigh wrote:
This is already true. If no proc items are found to kill, the function
immediately exits.
Great.
I think the while loop needs some optimisation, such as checking if
the process has terminated so that it can break out of the loop early.
I think simply repeating the readlink inside the loop would be
sufficient. So instead of while kill, so while readlink, and then
kill inside the loop.
I was trading off between a 100% CPU spin on readlink vs 1 second
granularity for each process that fails to immediately die. As it is
written, if the process dies immediately, the kill -0 will fail and no
waiting will be done.
Another nice addition would be to print out which processes are being
killed if in verbose mode.
Is this valuable? By the time the user sees the warning, the process is
already dead. Perhaps both the pid and the exe name?
Attached is an improved patch.
This is fantastic. I have separated it from 10mount, as 15killprocs
(it will always get run before 10mount on shutdown). This is because
you might want to run it even if there are no filesystems to unmount.
I have tweaked it slightly in a few places, but it's otherwise the
same as your patch. This is mainly using /proc/pid/root as well as
/proc/pid/exe (saves a grep invocation), and adding an additional log
message. I also used signal names rather than numbers with kill (for
portability). I also took the liberty of adding you to the AUTHORS
file for this contribution--I hope that's acceptable to you?
I have attached the patch I have just committed to SVN. Is this OK,
or would you like any further changes making?
Thanks,
Roger
--
.''`. Roger Leigh
: :' : Debian GNU/Linux http://people.debian.org/~rleigh/
`. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/
`-GPG Public Key: 0x25BFB848 Please GPG sign your mail.
Index: debian/changelog
===
--- debian/changelog (revision 1192)
+++ debian/changelog (working copy)
@@ -2,8 +2,11 @@
* New upstream development release.
* debian/control: Change section of libsbuild-doc to doc.
+ * Processes running in the chroot on stopping a session are now killed
+by the 15killprocs setup script (Closes: #391319). Many thanks to
+Kees Cook for implementing this.
- -- Roger Leigh [EMAIL PROTECTED] Sun, 20 May 2007 20:04:37 +0100
+ -- Roger Leigh [EMAIL PROTECTED] Mon, 28 May 2007 13:06:05 +0100
schroot (1.1.3-1) unstable; urgency=low
Index: AUTHORS
===
--- AUTHORS (revision 1192)
+++ AUTHORS (working copy)
@@ -4,6 +4,9 @@
Andreas Bombe [EMAIL PROTECTED]
Documentation improvements.
+Kees Cook [EMAIL PROTECTED]
+ 15killprocs script to kill processes.
+
Federico Di Gregorio [EMAIL PROTECTED]
Init script improvements.
Index: ChangeLog
===
--- ChangeLog (revision 1192)
+++ ChangeLog (working copy)
@@ -1,3 +1,16 @@
+2007-05-28 Roger Leigh [EMAIL PROTECTED]
+
+ * debian/changelog: Close #391319.
+
+ * AUTHORS: Add Kees Cook.
+
+ * bin/schroot/setup/15killprocs: New file. Kill processes in the
+ chroot before unmounting any filesystems. Many thanks to Kees
+ Cook for implementing this.
+
+ * bin/schroot/setup/Makefile.am
+ (setup_SCRIPTS): Add 15killprocs
+
2007-05-22 Roger Leigh [EMAIL PROTECTED]
* bin/schroot/setup/10mount
Index: bin/schroot/setup/15killprocs
===
--- bin/schroot/setup/15killprocs (revision 0)
+++ bin/schroot/setup/15killprocs (revision 0)
@@ -0,0 +1,64 @@
+#!/bin/sh
+# Copyright © 2007 Kees Cook [EMAIL PROTECTED]
+# Copyright © 2007 Roger Leigh [EMAIL PROTECTED]
+#
+# schroot is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# schroot is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+
+set -e
+
+# Kill all processes that were run from within the chroot environment
+# $1: mount base location
+do_kill_all()
+{
+if [ $AUTH_VERBOSITY = verbose ]; then
+echo Killing processes run inside $1
+fi
+ls /proc | egrep '^[[:digit:]]+$' |
+while read pid; do
+exe=$(readlink /proc/$pid/exe || true)
Bug#391319: [Buildd-tools-devel] Bug#391319: schroot: leftover processes cause umount to fail
Hi Roger,
On Mon, May 28, 2007 at 01:17:30PM +0100, Roger Leigh wrote:
This is fantastic. I have separated it from 10mount, as 15killprocs
(it will always get run before 10mount on shutdown). This is because
you might want to run it even if there are no filesystems to unmount.
Ah, yes. Great idea.
I have tweaked it slightly in a few places, but it's otherwise the
same as your patch. This is mainly using /proc/pid/root as well as
/proc/pid/exe (saves a grep invocation), and adding an additional log
message. I also used signal names rather than numbers with kill (for
portability). I also took the liberty of adding you to the AUTHORS
file for this contribution--I hope that's acceptable to you?
I'm honored! Thanks, that's terrific. I like your changes.
I have attached the patch I have just committed to SVN. Is this OK,
or would you like any further changes making?
The only thing I can see to change is just a very minor performance tweak,
which is to move the exe readlink into the verbose if, just to save one
exec unless we're in verbose:
#... was here ...
+root=$(readlink /proc/$pid/root || true)
+if [ $root = $1 ]; then
+if [ $AUTH_VERBOSITY = verbose ]; then
+exe=$(readlink /proc/$pid/exe || true) #- now here
+echo Killing left-over pid $pid (${exe##$1})
Thanks!
--
Kees Cook@outflux.net
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#391319: [Buildd-tools-devel] Bug#391319: schroot: leftover processes cause umount to fail
Kees Cook [EMAIL PROTECTED] writes: While using schroot on LVM snapshots with sbuild, I have sometimes run into situations where build deps start up services (like cron, dbus, etc). Once the build is finished, schroot attempts to umount and destroy the LVM snapshot, but since there are still processes running in the chroot, the umount fails. Since the chroot is over, it seems like it makes sense to kill all the processes left in the chroot, and then reattempt to umount. This patch implements that. Does this seem like a sensible change? Yes, it's a good idea. One concern I have is what will happen to bind mounted filesystems, or filesystems mounted multiple times? If there are open files, we don't want to kill anything /outside/ the chroot, which may also be using the filesystem. As an example, if I have a /srv mounted inside and outside the chroot, and a daemon outside is using it, and a daemon inside is using it, the umount will fail, but only the inside daemon should be killed. Another example is a bind mounted /home. If this fails to umount, we might blow away all the user's processes on the entire system. If this can be done safely, I'll be happy to apply the patch. Regards, Roger -- .''`. Roger Leigh : :' : Debian GNU/Linux http://people.debian.org/~rleigh/ `. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/ `-GPG Public Key: 0x25BFB848 Please GPG sign your mail. pgpswjcbgqv48.pgp Description: PGP signature
Bug#391319: [Buildd-tools-devel] Bug#391319: schroot: leftover processes cause umount to fail
On Sat, Oct 07, 2006 at 10:54:10AM +0100, Roger Leigh wrote:
One concern I have is what will happen to bind mounted filesystems, or
filesystems mounted multiple times? If there are open files, we don't
want to kill anything /outside/ the chroot, which may also be using
the filesystem.
Ah, yes. fuser isn't as smart as I was hoping. Here is a better patch,
which looks for processes that were run from the chroot base dir, which
will protect processes on mount points built with bind. This will
also not kill processes that are using the chroot area but were run from
outside the chroot. (Causing the umounts to correctly fail.)
How does this look?
--
Kees Cook@outflux.net
Index: schroot/setup/10mount
===
--- schroot/setup/10mount (revision 1032)
+++ schroot/setup/10mount (working copy)
@@ -23,10 +23,26 @@
mount $VERBOSE $1 $2 $3
}
+# Kill all processes that were run from within the chroot environment
+# $1: mount base location
+do_kill_all()
+{
+if [ $AUTH_VERBOSITY = verbose ]; then
+echo Killing processes run inside $1
+fi
+ls /proc | egrep '^[[:digit:]]+$' |
+while read pid; do
+if readlink /proc/$pid/exe | grep ^$1/ /dev/null; then
+kill $pid
+fi
+done
+}
+
# Unmount all filesystem under specified location
# $1: mount base location
do_umount_all()
{
+do_kill_all $1
$LIBEXEC_DIR/schroot-listmounts -m $1 |
while read mountloc; do
if [ $AUTH_VERBOSITY = verbose ]; then
