Re: SIGCHLD set to SIG_DFL on exec(3)
Date:Sat, 8 Feb 2020 16:47:42 +0100 From:Kamil Rytarowski Message-ID: | We are allowed to fix this in the kernel for everybody: Indeed we are. And if you want to change things that way, that's fine. It turns out this one wasn't the actual problem in this instance, so doing that would have made no difference... The issue with was with SIGCHLD blocked, and that one POSIX doesn't allow us to undo. Certainly, as the extract you quoted from the standard says, exec()ing a new process with "unusual" signals blocked can lead to weird behaviour, as almost nothing races around unblocking (etc) signals as part of the startup code. It doesn't help that SIGCHLD is decidely weird (thanks to Sys V) with its bizarre side effects. It seems likely that our shell actually never had a problem (though would have, had SIGCHLD been ignored) with this, as we don't actually use SIGCHLD for anything at all.Why some other (well, apparently all other) ash derived shells do is not clear to me - though over the years our internal sh wait(1) code has been rewritten quite frequently it seems. I would however keep the new code that sets SIGCHLD to SIG_DFL, and unblocks it (always, regardless of its state at entry) even if the kernel were changed as you suggest above, as one day I'd like to make the netbsd sh (like several other programs we have) exportable to others to use, and we cannot control what their kernels do. We did have (well, I introduced) a problem when the shell inherited children from a previous executable to occupy the process, if that child completed at the "wrong" time we would dump core.. (which was also an aspect of this issue) - which was trivial to fix once the possibility even occurred to me. It isn't something that happens often. Finally note that it took a particularly bizarre piece of bash code to make bash exec another shell (or for that matter, any other process) in this way, and that it did is most likely a bug in it (ie: not intententional) and will probably get fixed, this is a very rare problem, with a very simple fix, which really affects almost nothing, so I wouldn't spend a lot of time worrying about other possible solutions. kre
Re: fault(4)
> Date: Sat, 8 Feb 2020 06:19:43 -0800 (PST) > From: Paul Goyette > > If this is a device on which you can use ioctl() to configure, why is > it not stored under sys/dev and why is it not included in kernel config > with pseudo-device directive (and corresponding files.kern changes)? There's no need for an _autoconf_ device (usually representing a physical device in the hardware's device tree) in order to have a userland /dev node (representing an interface between userland and kernel). See kern_cpu.c, kern_drvctl.c, , for other examples of /dev nodes without autoconf devices. One could use a pseudo-device just to get an initialization routine so that it's self-contained; maxv chose to use a module instead for that. We could have a fault injection hook that an unloadable module would set up, but I see no need to block adoption of a very good thing that already catches bugs until it is generalized to perfection!
SIGCHLD set to SIG_DFL on exec(3)
On 06.02.2020 20:51, Robert Elz wrote: > Module Name: src > Committed By: kre > Date: Thu Feb 6 19:51:59 UTC 2020 > > Modified Files: > src/bin/sh: main.c > > Log Message: > If we are invoked with SIGCHLD ignored, we fail badly, as we assume > that we can always wait(2) for our children, and an ignored SIGCHLD > prevents that. Recent versions of bash can be convinced (due to a > bug most likely) to invoke us that way. Always return SIGCHLD to > SIG_DFL during init - we already prevent scripts from fiddling it. > > All ash derived shells apparently have this problem (observed by > Martijn Dekker, and notified on the bash-bug list). Actual issue > diagnosed by Harald van Dijk (same list). > > + (void)signal(SIGCHLD, SIG_DFL); We are allowed to fix this in the kernel for everybody: "If the SIGCHLD signal is set to be ignored by the calling process image, it is unspecified whether the SIGCHLD signal is set to be ignored or to the default action in the new process image." "This volume of POSIX.1-2017 specifies that signals set to SIG_IGN remain set to SIG_IGN, and that the new process image inherits the signal mask of the thread that called exec in the old process image. This is consistent with historical implementations, and it permits some useful functionality, such as the nohup command. However, it should be noted that many existing applications wrongly assume that they start with certain signals set to the default action and/or unblocked. In particular, applications written with a simpler signal model that does not include blocking of signals, such as the one in the ISO C standard, may not behave properly if invoked with some signals blocked. Therefore, it is best not to block or ignore signals across execs without explicit reason to do so, and especially not to block signals across execs of arbitrary (not closely cooperating) programs." https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html signature.asc Description: OpenPGP digital signature
Re: fault(4)
On Sat, 8 Feb 2020, Martin Husemann wrote:
On Sat, Feb 08, 2020 at 06:19:43AM -0800, Paul Goyette wrote:
The module should be MODULE_CLASS_DRIVER. And there should be a
sys/module/fault/Makefile to build the module, along with changes to
sys/module/Makefile (to descend into the fault directory) and to
src/distrib/sets/lists/modules/mi (for new files). The module init
code should also have appropriate config_{init,fini}_component()
calls for inserting the device into the autoconfig database.
Since all probes have to be hard-coded this is very intrusive and I am
not sure a module makes sense.
True. But in that case it should not be a module at all.
But shouldn't it still be a pseudo-device and not just an option?
++--+---+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
++--+---+
Re: fault(4)
On Sat, Feb 08, 2020 at 06:19:43AM -0800, Paul Goyette wrote:
> The module should be MODULE_CLASS_DRIVER. And there should be a
> sys/module/fault/Makefile to build the module, along with changes to
> sys/module/Makefile (to descend into the fault directory) and to
> src/distrib/sets/lists/modules/mi (for new files). The module init
> code should also have appropriate config_{init,fini}_component()
> calls for inserting the device into the autoconfig database.
Since all probes have to be hard-coded this is very intrusive and I am
not sure a module makes sense.
Martin
Re: fault(4)
If this is a device on which you can use ioctl() to configure, why is
it not stored under sys/dev and why is it not included in kernel config
with pseudo-device directive (and corresponding files.kern changes)?
The module should be MODULE_CLASS_DRIVER. And there should be a
sys/module/fault/Makefile to build the module, along with changes to
sys/module/Makefile (to descend into the fault directory) and to
src/distrib/sets/lists/modules/mi (for new files). The module init
code should also have appropriate config_{init,fini}_component()
calls for inserting the device into the autoconfig database.
Finally, it would be nice if it were possible to unload the module,
although I understand why it might be difficult to ensure that it is
safe to remove.
On Sat, 8 Feb 2020, Maxime Villard wrote:
[I am not subscribed to this list, so if you want to answer, make sure to CC
me]
In order to explore error branches, and to test the kernel's ability to cope
with failures, it is often necessary to hard-trigger such failures.
Here is an implementation [1] for fault(4), a driver that allows to trigger
failures in the kernel. A similar driver exists in Linux.
The fault_inject() function decides whether to return true or false,
depending
on parameters configurable by userland via ioctls on /dev/fault. The caller
of
this function should then error out depending on the return value. Typically:
whatever_subsystem()
{
...
if (fault_inject())
return NULL; // means failure
...
return non_null; // means success
}
Several modes can be available, I have implemented one for now, the N-th
mode:
every N-th call to fault_inject (N being configurable) will make it return
true.
Several scopes are available: global (ie system-wide), or calling LWP.
Examples:
- mode=NTH scope=GLOBAL: every N-th call to fault_inject() in the whole
kernel
will return true, regardless of the LWP.
- mode=NTH scope=LWP: every N-th call to fault_inject() made by the LWP that
enabled the mode will return true. For the other LWPs, fault_inject()
always
returns false.
fault_inject() can be introduced in any place of interest. For now I added it
in pool_cache_get():
if (flags & PR_NOWAIT) {
if (fault_inject())
return NULL;
}
Running ATF with kASan+LOCKDEBUG+fault with {N=32 scope=GLOBAL} already gives
an instant crash:
kernel diagnostic assertion
"radix_tree_empty_tree_p(>pm_pvtree)"
failed: file ".../sys/arch/x86/x86/pmap.c"
Looks like radixtree.c doesn't handle allocation failures very well
somewhere.
fault(4) seems like the kind of feature that would be useful for
stress-testing
and fuzzing. As you can see in the diff, its code is extremely simple.
Maxime
[1] https://m00nbsd.net/garbage/fault/fault.diff
!DSPAM:5e3e9212102679617345149!
++--+---+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
++--+---+
fault(4)
[I am not subscribed to this list, so if you want to answer, make sure to CC me]
In order to explore error branches, and to test the kernel's ability to cope
with failures, it is often necessary to hard-trigger such failures.
Here is an implementation [1] for fault(4), a driver that allows to trigger
failures in the kernel. A similar driver exists in Linux.
The fault_inject() function decides whether to return true or false, depending
on parameters configurable by userland via ioctls on /dev/fault. The caller of
this function should then error out depending on the return value. Typically:
whatever_subsystem()
{
...
if (fault_inject())
return NULL; // means failure
...
return non_null; // means success
}
Several modes can be available, I have implemented one for now, the N-th mode:
every N-th call to fault_inject (N being configurable) will make it return
true.
Several scopes are available: global (ie system-wide), or calling LWP.
Examples:
- mode=NTH scope=GLOBAL: every N-th call to fault_inject() in the whole kernel
will return true, regardless of the LWP.
- mode=NTH scope=LWP: every N-th call to fault_inject() made by the LWP that
enabled the mode will return true. For the other LWPs, fault_inject() always
returns false.
fault_inject() can be introduced in any place of interest. For now I added it
in pool_cache_get():
if (flags & PR_NOWAIT) {
if (fault_inject())
return NULL;
}
Running ATF with kASan+LOCKDEBUG+fault with {N=32 scope=GLOBAL} already gives
an instant crash:
kernel diagnostic assertion "radix_tree_empty_tree_p(>pm_pvtree)"
failed: file ".../sys/arch/x86/x86/pmap.c"
Looks like radixtree.c doesn't handle allocation failures very well somewhere.
fault(4) seems like the kind of feature that would be useful for stress-testing
and fuzzing. As you can see in the diff, its code is extremely simple.
Maxime
[1] https://m00nbsd.net/garbage/fault/fault.diff
