linux-kernel - Re: [PATCH v2 1/5] selftests/resctrl: Kill the child process created by fork() when the SIGTERM signal comes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e10e4e8f-97a0-3fba-bd51-a2b14f40499e@intel.com>
Date:   Thu, 6 Jan 2022 15:46:56 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Shaopeng Tan <tan.shaopeng@...fujitsu.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Shuah Khan <shuah@...nel.org>
CC:     <linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v2 1/5] selftests/resctrl: Kill the child process created
 by fork() when the SIGTERM signal comes

Hi Shaopeng Tan, 

On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> In kselftest framework there is a limited time for each sub test,
> when the time limit comes SIGTEM signal will be sent to sub test by

SIGTEM -> SIGTERM ?

> "timeout --foregroup <seconds>" command.

foregroup?

This is a bit confusing though since it mentions that the "timeout" utility
is called after the test times out. Perhaps you can just describe that, if
present, the test is run using the timeout utility and it will
send SIGTERM to the test upon timeout.

> In resctrl_tests, fork() is used to create a child process.
> This commit ensures child process to be killed before parent process

Especially since I know you are planning more changes in the x86 area
where this is enforced more strictly, please do get into the habit of
describing your changes in imperative mood. Please search for
"This patch" in Documentation/process/submitting-patches.rst (your
usage of "This commit" is equivalent to "This patch") for more details.

Please address this in all your patches where "This commit" is frequently
used. 

> exiting if SIGTERM signal comes.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@...fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 95224345c78e..b32b96356ec7 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -678,6 +678,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
>  	sigemptyset(&sigact.sa_mask);
>  	sigact.sa_flags = SA_SIGINFO;
>  	if (sigaction(SIGINT, &sigact, NULL) ||
> +	    sigaction(SIGTERM, &sigact, NULL) ||
>  	    sigaction(SIGHUP, &sigact, NULL)) {
>  		perror("# sigaction");
>  		ret = errno;

The change looks good. Since this snippet is preceded with a comment
that describes its usage you could also update it with the expanded
use of the kselftest framework.

Reinette