linux-kernel - Re: [PATCH v3 01/16] selftests/resctrl: Open get_mem_bw

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: <a35a663a-090c-45f8-84cc-1f575ef107c5@intel.com>
Date: Wed, 24 Apr 2024 21:37:02 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	<linux-kselftest@...r.kernel.org>, Shuah Khan <shuah@...nel.org>, Babu Moger
	<babu.moger@....com>, Maciej Wieczór-Retman
	<maciej.wieczor-retman@...el.com>
CC: Fenghua Yu <fenghua.yu@...el.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 01/16] selftests/resctrl: Open get_mem_bw_imc() fd for
 loops

Hi Ilpo,

On 4/8/2024 9:32 AM, Ilpo Järvinen wrote:
> get_mem_bw_imc() handles fds in a for loop but close() is based on two
> fixed indexes READ and WRITE.
> 
> Open code all for loops to READ+WRITE entries for clarity.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@...el.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
> 
> v3:
> - Rework entirely, use open coding instead of for loops for clarity
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++---------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 445f306d4c2f..456cf0d0b8ca 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -306,26 +306,28 @@ static int initialize_mem_bw_imc(void)
>  static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
>  {
>  	float reads, writes, of_mul_read, of_mul_write;
> -	int imc, j, ret;
> +	int imc, ret;
>  
>  	/* Start all iMC counters to log values (both read and write) */
>  	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
>  	for (imc = 0; imc < imcs; imc++) {
> -		for (j = 0; j < 2; j++) {
> -			ret = open_perf_event(imc, cpu_no, j);
> -			if (ret)
> -				return -1;
> -		}
> -		for (j = 0; j < 2; j++)
> -			membw_ioctl_perf_event_ioc_reset_enable(imc, j);
> +		ret = open_perf_event(imc, cpu_no, READ);
> +		if (ret)
> +			return -1;
> +		ret = open_perf_event(imc, cpu_no, WRITE);
> +		if (ret)
> +			return -1;
> +
> +		membw_ioctl_perf_event_ioc_reset_enable(imc, READ);
> +		membw_ioctl_perf_event_ioc_reset_enable(imc, WRITE);
>  	}

The above highlights how the error checking is broken here and
leaving this code like this until the later fix arrives is confusing (to me).
Could you please squash "selftests/resctrl: Fix closing IMC fds on error" into this
patch?

Even so, I do not think that this addresses all the issues with error handling
surrounding these fds.
If I understand correctly these fds are currently (and remains to be after this
series) closed within get_mem_bw_imc(). In this series there seems to be
many occasions where open_perf_event() succeeds but later failures encountered
before get_mem_bw_imc() results in these fds not being cleaned up. What do you think of
having a perf_close_imc_mem_bw() to match with perf_open_imc_mem_bw() that
closes the fds and can be called from within get_mem_bw_imc() as well as
earlier if a failure is encountered between perf_open_imc_mem_bw()
and get_mem_bw_imc()?

Reinette