linux-kernel - Re: [PATCH v5 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

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] [day] [month] [year] [list]

Message-ID: <d1e0c7b7-4555-1afb-b33a-4390857395c6@intel.com>
Date:   Tue, 24 Jan 2023 09:19:35 -0800
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     "Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Shuah Khan <shuah@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v5 4/5] selftests/resctrl: Cleanup properly when an error
 occurs in CAT test

Hi Shaopeng,

On 1/23/2023 6:16 PM, Shaopeng Tan (Fujitsu) wrote:
>> On 1/22/2023 8:22 PM, Shaopeng Tan (Fujitsu) wrote:
>>>> On 1/10/2023 11:58 PM, Shaopeng Tan wrote:
>>>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c
>>>>> b/tools/testing/selftests/resctrl/cat_test.c
>>>>> index 6a8306b0a109..87302b882929 100644
>>>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>>>> @@ -103,7 +103,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
>>>> *cache_type)
>>>>>  	unsigned long l_mask, l_mask_1;
>>>>>  	int ret, pipefd[2], sibling_cpu_no;
>>>>>  	char pipe_message;
>>>>> -	pid_t bm_pid;
>>>>>
>>>>>  	cache_size = 0;
>>>>>
>>>>> @@ -181,28 +180,29 @@ int cat_perf_miss_val(int cpu_no, int n, char
>>>> *cache_type)
>>>>>  		strcpy(param.filename, RESULT_FILE_NAME1);
>>>>>  		param.num_of_runs = 0;
>>>>>  		param.cpu_no = sibling_cpu_no;
>>>>> +	} else {
>>>>> +		ret = signal_handler_register();
>>>>> +		if (ret)
>>>>> +			goto out;
>>>>
>>>> The "goto" will unregister the signal handler. Is that necessary if
>>>> the registration failed?
>>>>
>>>> Also, if signal_handler_register() fails then the child will keep
>>>> running and run its test ... would child not then run forever?
>>>
>>> A signal handler is needed here, but it is rarely used.
>>> Also, the registration rarely fails.
>>> Therefore, if registration failed,
>>> just print a warning/info message as follow.
>>> how about this idea?
>>>
>>> ksft_print_msg("Failed to register signal handler, signals
>>> SIGINT/SIGTERM/SIGHUP will not be handled as expected");
>>>
>>
>> I do not think this is necessary considering that signal_handler_register()
>> already prints an error on failure.
>>
>> Adding an error message does not address the two issues I raised.
> 
> The previous idea was just to print a message instead of "goto".
> How about the idea to keep the parent&child process running forever even if the signal handler registration fails.
> 
> +	} else {
> +		signal_handler_register();
> +	}
> 
> I don't think the test need stop when the signal handler registration fails,
> since this signal handler is rarely used and registration of signal handlers rarely fails.

Please always handle errors properly. If the registration fails the test should be
stopped and proper cleanup should be done. 

Reinette