Bug 138788 - [PATCH] pidof bug renders machine unbootable when NFS is stuck
[PATCH] pidof bug renders machine unbootable when NFS is stuck
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 3
Classification: Red Hat
Component: SysVinit (Show other bugs)
3.0
All Linux
medium Severity high
: ---
: ---
Assigned To: Bill Nottingham
:
Depends On: 85796 163623
Blocks: 156320 156322
  Show dependency treegraph
 
Reported: 2004-11-11 01:06 EST by Constantine Gavrilov
Modified: 2014-03-16 22:50 EDT (History)
4 users (show)

See Also:
Fixed In Version: RHBA-2005-511
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-10-05 10:29:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
use readlink instead of stat to solve blcking on NFS (1.84 KB, patch)
2004-11-11 01:12 EST, Constantine Gavrilov
no flags Details | Diff
instead of stat to solve blocking on NFS for AS3 SysVinit-2.85-4 (1.64 KB, patch)
2004-11-11 02:02 EST, Constantine Gavrilov
no flags Details | Diff
Updated patch for SysVinit-2.85-4 to support symlinks as prog args (1.64 KB, patch)
2004-12-14 11:56 EST, Constantine Gavrilov
no flags Details | Diff
Updated patch for SysVinit-2.85-4 to support symlinks as prog args (2.10 KB, patch)
2004-12-14 11:59 EST, Constantine Gavrilov
no flags Details | Diff
Updated patch for SysVinit-2.84 to support symlinks as prog args (2.30 KB, text/plain)
2004-12-14 12:00 EST, Constantine Gavrilov
no flags Details
new patch (2.13 KB, patch)
2005-04-25 17:29 EDT, Bill Nottingham
no flags Details | Diff

  None (edit)
Description Constantine Gavrilov 2004-11-11 01:06:45 EST
Description of problem:

If there is a stuck NFS mount, and there are processes running from
that mount point pidof utility may block forever. Since this utility
is used in most stop scripts, machine becomes unbootable.

This is expected to exist on all versions of Red Hat. (Tested 7.3, AS
2.1, AS 30, update 3).

Steps to Reproduce:
1. mount a NFS partition (can be from a local machine) with default
options.
2. put a sample prog on the mounted partition (the simples program
will suffice).
3. start the prog.
4. stop nfs on the nfs server.
5. run `pidof xxx` or pidof `some_real_name`, provided some_real_name
comes before your test prog in the /proc list.
6. Try `init 6` on the machine.

  
Actual results:
Since reboot uses pidof a lot, and since pidof scans the whole proc
tree, and since pidof blocks forever scanning the /proc entry for a
process which was run from NFS, the whole reboot blocks and the box is
stuck.

Expected results:
Machine should be able to reboot, even if there is a stuck NFS partition.

Additional info:

The problem is in a brain-damaged code in killall5.c that tries to
call stat() on /proc/pid/exe file, that will block for ever on a stuck
NFS. I provide a patch that uses readlink() instead. It also fixes
several memory leaks for races against exited processes.
Comment 1 Constantine Gavrilov 2004-11-11 01:12:20 EST
Created attachment 106475 [details]
use readlink instead of stat to solve blcking on NFS

Also fixes some memory leaks when cleaning up for processes that exited during
their proc scan.
Comment 2 Constantine Gavrilov 2004-11-11 01:18:41 EST
In steps to reporduce, the step 5 should read:

5. run `pidof xxx` or pidof `some_real_name`, provided some_real_name
comes AFTER your test prog in the /proc list. This when a real problem
shows up -- when you try to look up the pid of the process that is
listed in the proc tree after you.
Comment 3 Constantine Gavrilov 2004-11-11 02:02:21 EST
Created attachment 106478 [details]
instead of stat to solve blocking on NFS for AS3 SysVinit-2.85-4

Also fixes some memory leaks when cleaning up for processes that exited during
their proc scan.

Must be used as the last patch.
Comment 4 Constantine Gavrilov 2004-11-11 02:04:11 EST
The first patch is for SysVinit-2.84.
Comment 5 Bill Nottingham 2004-11-11 14:30:00 EST
The second patch obsoletes the first, correct?
Comment 6 Constantine Gavrilov 2004-11-14 10:13:32 EST
The first patch is against version 2.84 of SysVinit and was tested on
RedHat 7.3 platform. This is probably what will have to be used for
advanced server 2.1.

The second patch is against SysVinit-2.85-4 from RedHat advanced
server 3.0 update 3. So, the second patch does not obsolete the first
one, unless a new version of the package (2.85) will be provided for
both AS 2.1 and AS 3.0 to fix the problem.
Comment 7 Bill Nottingham 2004-11-15 00:37:45 EST
I'd be curious how this would work with namespaces and/or chroots(),
when the same 'path' would be different.
Comment 8 Constantine Gavrilov 2004-11-15 05:03:58 EST
When you perform pidof /sbin/my_prog or killall /sbin/my_prog, would
you not expect it to take into account all processes that run on the
machine, even if they run in the chrooted environment? Is it not
perfectly logical? So, the new code is more consitent than the old,
because with the old code you would have to do killall
/path_to_chroot/sbin/myprog to get the chrooted progs. I doubt that
this is sane for killall or compatible with other unixes for killall
behavior. Also, even the unpatched code began to do just command name
comparisons if it could not find any match based on the path inode and
device info.

So, with the old code, you would not get any chrooted programs when
there were progs running from /. To get the chrooted stuff, you would
have to call with /path_to_chroot/sbin/myprog as an argument. However,
if there were no instances of /sbin/myprog running form /, you would
get chrooted ones by simply using /sbin/myprog as an argument. Pretty
inconsistent, I think! What do you think?
Comment 9 Constantine Gavrilov 2004-11-15 06:52:03 EST
I think my previous comment is wrong, even though it would be nice to
get the behavior I describe as desirable. With my changes, the code
will behave exactly as before. Since /proc/pid/exe will show the
symlink to full path of the executable (with chroot constraints
decribed below), the readlink will return the full path of the
executable. So, with the patch,  the file locations comparisons will 
yield the same results as with the unpatched versions of the software.
Even for the chrooted environments, if one starts a process from
chroot, and does killall from chroot, then within the chroot, the exe
link in proc will not contain the path to chroot. So, running the
unitlity is OK from chroot and behaves exactly as with unpatched
version of the utility. However, when running the utility from /, the
comparison by file location will fail, since exe link will have the
chroot_path, and only the comparsion by prog name may succeed
(depending what is the command name that we get from proc). Thus, we
get the same behavior as with unpatched version.

For processes that are run from /, and we try to "get them" from
chroot, the situation is reversed -- comparison by file location will
succeed from /, and only comparison by name may succeed from chroot
(since chrooted proc will have exe entries that will point outside of
chroot, so we will get unresolved links). Again exactly the same
behavior as with unpatched software. Do you concur?
Comment 10 Bill Nottingham 2004-12-04 21:14:07 EST
It does not behave *exactly* as before.

pidof /some/file/that/is/a/symlink will fail,
as it relies on a dev/inode match to work. An example of this
currently in use is the postmaster symlink in postgresql.
Comment 11 Constantine Gavrilov 2004-12-06 10:58:48 EST
OK. Thank you for testing the patch. I have several comments:

1) In general, idea of pidof/killproc searching by filename (file
location) and not by command name is bizzare to me. I always thought
it searched by command name until I looked at the code. What it really
does, it tries to take the "program" argument as file location and
match the executables of running processes to that location. Only if
this procedure fails, it will compare the "short" command line of
running processes with the "program" argument passed to
pidof/killproc.  Such policy seems really strange to me, especially
since most init scripts use it by passing the basename of the
executable, and not its full path. However, at this point I am not
saying the utility must be changed.

2) When I said "exactly as before", I addressed concerns of running
pidof either form / or from chroot on the processes that have been
started either from / or chroot. I still claim it will produce the
same results as without the patch, provided the "program" argument
passed to pidof is not a symlink. 

3) You are correct that I have not thought about "program" arguments
in the form of "/some_path/symlink". (My first item may explain why.)
However, this can be very easily fixed. The fix is like this:

Call readlink() on the "program" argument itself. If the argument is a
valid symbolic link, then use the name returned by readlink() in
further processing instead of the "program" argument. We also will
have to use realpath() to further process the "program" name to
resolve other symbolic links or relative paths.  After we have the
full path to the "program" argument, we can continue matching as
before. (We need the "real" full path because exe symlink in
/proc/pid/exe points to "real" full path of the executable). I can
redo the patch if you wish or you can do it yourself.

If realpath() blocks on the "stuck" NFS system, we may need to use an
unblocking variant of it in order to allow calls like "pidof
/stucked_nfs/bin/my_prog".  BTW, AFAIK, such calls will still block
even with my patch, since I have not removed the stat() call on the
"program" argument. This still may need to be fixed. The main problem
that I was trying to solve is "pidof sendmail" (or some other service
name) blocks causing reboot to block because there is a process
running on a NFS stuck partition.
Comment 12 Constantine Gavrilov 2004-12-14 11:56:58 EST
Created attachment 108533 [details]
Updated patch for SysVinit-2.85-4 to support symlinks as prog args

This patch solves a problem with the previous patch -- symbolic links as
"program" arguments work now as expected.
Comment 13 Constantine Gavrilov 2004-12-14 11:59:32 EST
Created attachment 108534 [details]
Updated patch for SysVinit-2.85-4 to support symlinks as prog args

This patch solves a problem with the previous patch -- symbolic links as
"program" arguments work now as expected. Initial replacement used the "old"
patch. Sorry.
Comment 14 Constantine Gavrilov 2004-12-14 12:00:40 EST
Created attachment 108535 [details]
Updated patch for SysVinit-2.84 to support symlinks as prog args

This patch solves a problem with the previous patch -- symbolic links as
"program" arguments work now as expected.
Comment 15 Constantine Gavrilov 2004-12-14 12:15:57 EST
With latest variants of the patches, symlink "program" argument work
as expected. The change is trivial -- we call realpath() on the
program argument instead of stat. The full name returned by realpath()
is then used in comparisons against the proc entries.

In the original code, stat call was used to decide whether the file
was present and to get device and inode numbers. In the first version
of the patch, the stat call was left just to decide whether the file
is present. In this version of the patch, stat is replaced with
realpath() -- we both get the full file name and the error code if the
file is not present.

Like stat(), realpath() blocks on stuck NFS since realpath() calls
stat on each subdirectory entry. So, if there is a requirement to be
able to call 'pidof /stuck_nfs/my_prog' without blocking, a
non-blocking realpath() variant must be written.

At lest with these patches, the system is able to reboot if there are
processes running from a stuck nfs mount.

Do you think ability to run 'pidof /stuck_nfs/my_prog' without
blocking is important? I have realpath() like code that is well
debugged. It does not call readlink() but it strips all dots and
slashes from the path string correctly and it can be reused to write a
non-blocking variant of realpath().
Comment 20 Bill Nottingham 2005-04-25 16:22:44 EDT
The use of readlink effectively breaks it for non-root usage entirely.
Comment 21 Bill Nottingham 2005-04-25 17:29:49 EDT
Created attachment 113650 [details]
new patch

This patch (against the devel tree) fixes the previous problem.
Comment 22 Bill Nottingham 2005-04-25 18:10:14 EDT
FWIW, this is fixed in 2.85-38 in the devel tree.
Comment 28 Red Hat Bugzilla 2005-09-28 16:00:09 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2005-510.html
Comment 29 Red Hat Bugzilla 2005-10-05 10:29:51 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2005-511.html

Note You need to log in before you can comment on or make changes to this bug.