[Pkg-sysvinit-devel] Bug#476695: Bug#476695: Needless stats in readproc()

[Goswin von Brederlow]

Petter Reinholdtsen <pere at hungry.com> writes:

> [Goswin von Brederlow]
>> The attached patch adds a new parameter to readproc() telling it to
>> stat or not to stat and sets this flag for the killa5 and pidof
>> case.
>
> I suspect this is the wrong approach.  The sendsigs omitpid feature is
> created for this use case, and it can be used to make sure killall5 do
> not kill the fuse processes.  I recommend symlinking from a file in
> /lib/init/rw/sendsigs.omit.d/ to the pidfile for the fuse process, to
> make sure the pid is ignored only when it is listed in the pid file.
>
> Happy hacking,
> -- 
> Petter Reinholdtsen

Sorry, nope. That doesn't help prevent the deadlock and seems to be
buggy as well. Read on.


Point 1: sendsigs.omit.d does not prevent killall5 from sending
SIGSTOP. Only the term/kill signal (if it would work at all). This
prevents race condition with new threads being created while killall5
runs.

----------------------------------------------------------------------
	/* Now stop all processes. */
	kill(-1, SIGSTOP);
	sent_sigstop = 1;

	/* Read /proc filesystem */
	if (readproc() < 0) {
		kill(-1, SIGCONT);
		exit(1);
	}
----------------------------------------------------------------------

At the readproc() time all processes will be stoped. And that has to
stay. Only way out would be to SIGCONT all ommited pids rights there
before the readproc() call. But luckily  the stat() is unneeded for
killall5 so why waste time doing it at all and complicate things?


Point 2: I believe putting anything in /lib/init/rw/sendsigs.omit.d/
will break killall5, it doesn't accept extra args beside the signal.
Should I open a new bug for that?

----------------------------------------------------------------------
/* Give usage message and exit. */
void usage(void)
{
	nsyslog(LOG_ERR, "only one argument, a signal number, allowed");
	closelog();
	exit(1);
}
----------------------------------------------------------------------
	/* Right, so we are "killall". */
	if (argc > 1) {
		if (argc != 2) usage();
		if (argv[1][0] == '-') (argv[1])++;
		if ((sig = atoi(argv[1])) <= 0 || sig > 31) usage();
	}
----------------------------------------------------------------------


Point 3: Even if you could put any pids in argv[2], argv[3],
... killall5 doesn't look at them.

----------------------------------------------------------------------
	for (p = plist; p; p = p->next)
		if (p->pid != pid && p->sid != sid && !p->kernel)
			kill(p->pid, sig);
----------------------------------------------------------------------


Point 4: Fuse forks threads to handle requests so the pids can change
all the time. Even while killall5 runs the stat() could create new
threads.

I guess something fixed up using the session ID instead of pids to be
omitted when killing processes. But that is only relevant to the other
patch I send in. The stat() deadlock remains.

MfG
        Goswin

PS: splash should use the sendsigs.omit.d dir to keep itself from
getting killed if you get it fixed instead of the "splash_back()"
function in /etc/init.d/sendsigs.