linux-kernel - Re: [PATCH V3 02/15] selftests/resctrl: Print accurate buffer size as part of MBM results

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: <42afcd9f-48a3-4d31-a39b-df8c42f2e5ff@intel.com>
Date: Fri, 18 Oct 2024 10:34:13 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <fenghua.yu@...el.com>, <shuah@...nel.org>, <tony.luck@...el.com>,
	<peternewman@...gle.com>, <babu.moger@....com>,
	Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>,
	<linux-kselftest@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 02/15] selftests/resctrl: Print accurate buffer size as
 part of MBM results

Hi Ilpo,

On 10/18/24 1:46 AM, Ilpo Järvinen wrote:
> On Thu, 17 Oct 2024, Reinette Chatre wrote:

>> @@ -138,15 +139,26 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
>>  		.setup		= mbm_setup,
>>  		.measure	= mbm_measure,
>>  	};
>> +	char *endptr = NULL;
>> +	size_t span = 0;
>>  	int ret;
>>  
>>  	remove(RESULT_FILE_NAME);
>>  
>> +	if (uparams->benchmark_cmd[0] && strcmp(uparams->benchmark_cmd[0], "fill_buf") == 0) {
>> +		if (uparams->benchmark_cmd[1]) {
>> +			errno = 0;
>> +			span = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
>> +			if (errno || *endptr != '\0')
> 
> This no longer catches "" string as error. I tested strtoul() with an 
> empty string and errno remains at 0.
> 
>> +				return -errno;
> 
> Another issue is that in cases where errno=0 (both *endptr != '\0' and 
> endptr == uparams->benchmark_cmd[1]), this function doesn't return 
> a proper error code but -0.
> 

Thank you for catching this. I addressed it with the fixup below:

@@ -146,11 +146,11 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 	remove(RESULT_FILE_NAME);
 
 	if (uparams->benchmark_cmd[0] && strcmp(uparams->benchmark_cmd[0], "fill_buf") == 0) {
-		if (uparams->benchmark_cmd[1]) {
+		if (uparams->benchmark_cmd[1] && *uparams->benchmark_cmd[1] != '\0') {
 			errno = 0;
 			span = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
 			if (errno || *endptr != '\0')
-				return -errno;
+				return -EINVAL;
 		}
 	}

The above does not address all that can go wrong when user, for example, runs:
		resctrl_tests -t mbm fill_buf ""
The above snippet will ignore the invalid value and focus on the issue addressed in
this patch, which is to not print DEFAULT_SPAN when that is not the size used.

At this point of the series run_benchmark() still uses strtoul() to parse the
empty string. This is fixed in the later patch "selftests/resctrl: Make benchmark
parameter passing robust" that will also be fixed to handle the empty string.

Reinette