linux-kernel - Re: [PATCH V2 09/13] selftests/resctrl: Make benchmark parameter passing robust

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: <6ae8be57-be40-4b85-a171-ce535bb5b5a4@intel.com>
Date: Fri, 4 Oct 2024 15:21:31 -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 V2 09/13] selftests/resctrl: Make benchmark parameter
 passing robust

Hi Ilpo,

On 10/4/24 7:05 AM, Ilpo Järvinen wrote:
> On Thu, 12 Sep 2024, Reinette Chatre wrote:

>>  tools/testing/selftests/resctrl/cmt_test.c    |  32 ++----
>>  tools/testing/selftests/resctrl/mba_test.c    |  13 ++-
>>  tools/testing/selftests/resctrl/mbm_test.c    |  22 ++--
>>  tools/testing/selftests/resctrl/resctrl.h     |  54 ++++++---
>>  .../testing/selftests/resctrl/resctrl_tests.c | 104 +++++++++++++-----
>>  tools/testing/selftests/resctrl/resctrl_val.c |  43 ++++----
>>  6 files changed, 176 insertions(+), 92 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> index 0c045080d808..a7effe76b419 100644
>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>> @@ -116,15 +116,13 @@ static void cmt_test_cleanup(void)
>>  
>>  static int cmt_run_test(const struct resctrl_test *test, const struct user_params *uparams)
>>  {
>> -	const char * const *cmd = uparams->benchmark_cmd;
>> -	const char *new_cmd[BENCHMARK_ARGS];
>> +	struct fill_buf_param fill_buf = {0};
> 
> Empty initializer is enough to have the fields initialized to zero.

Sure.

...

>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 82801245e4c1..98462752cb46 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -43,16 +43,35 @@
>>  
>>  #define DEFAULT_SPAN		(250 * MB)
>>  
>> +/*
>> + * fill_buf_param:	"fill_buf" benchmark parameters
>> + * @buf_size:		Size (in bytes) of buffer used in benchmark.
>> + *			"fill_buf" allocates and initializes buffer of
>> + *			@buf_size. User can change value via command line.
>> + * @memflush:		If 0 then the buffer will not be flushed after
>> + *			allocation and initialization, otherwise the
>> + *			buffer will be flushed. User can change value via
>> + *			command line.
>> + */
>> +struct fill_buf_param {
>> +	size_t		buf_size;
>> +	int		memflush;
> 
> bool?
> 
> Or is there a plan to use other values than 0 and 1?

Sure. There is no need to keep the original user provided values. I am not aware of
any other planned values. 

>> +static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
>> +{
>> +	struct fill_buf_param *fill_param = NULL;
>> +	char *endptr = NULL;
>> +
>> +	if (!uparams->benchmark_cmd[0] || strcmp(uparams->benchmark_cmd[0], "fill_buf"))
>> +		return NULL;
>> +
>> +	fill_param = malloc(sizeof(*fill_param));
>> +	if (!fill_param)
>> +		ksft_exit_skip("Unable to allocate memory for fill_buf parameters.\n");
>> +
>> +	if (uparams->benchmark_cmd[1]) {
>> +		errno = 0;
>> +		fill_param->buf_size = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
>> +		if (errno || uparams->benchmark_cmd[1] == endptr) {
> 
> Should this also check that there is no extra garbage?

Good point. Instead of adding another check I plan to replace existing endptr check
with a different test that ensures that "*endptr == '\0'".


>>  static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>>  {
>> -	char **benchmark_cmd;
>> -	int ret, memflush;
>> -	size_t span;
>> +	struct benchmark_info *benchmark_info;
>> +	const struct user_params *uparams;
>> +	struct resctrl_val_param *param;
>>  	FILE *fp;
>> +	int ret;
>>  
>> -	benchmark_cmd = info->si_ptr;
>> +	benchmark_info = info->si_ptr;
> 
> I'd just assign this directly while defining the local variable.
> 

Sure.

Thank you.

Reinette