linux-kernel - Re: [PATCH v2 03/24] selftests/resctrl: Move resctrl FS mount/umount to higher level

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]

Date:   Fri, 21 Apr 2023 17:09:10 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        <linux-kselftest@...r.kernel.org>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "Shuah Khan" <shuah@...nel.org>,
        Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>,
        Babu Moger <babu.moger@....com>, <linux-kernel@...r.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v2 03/24] selftests/resctrl: Move resctrl FS mount/umount
 to higher level

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
> A few places currently lack umounting resctrl FS on error paths.

You mention "A few places" (plural). In the patch I do see that
cmt_resctrl_val() is missing an unmount. Where are the other places?

> Each and every test does require resctrl FS to be present already for
> feature check. Thus, it makes sense to just mount it on higher level in
> resctrl_tests.c.
> 
> Move resctrl FS mount/unmount into each test function in
> resctrl_tests.c. Make feature validation to simply check that resctrl
> FS is mounted.
> 

...

> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index af71b2141271..426d11189a99 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -86,10 +86,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  
>  	cache_size = 0;
>  
> -	ret = remount_resctrlfs(true);
> -	if (ret)
> -		return ret;
> -
>  	if (!validate_resctrl_feature_request(CMT_STR))
>  		return -1;
>  
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 9b9751206e1c..5c9ed52b69f2 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -77,9 +77,15 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
>  
>  	ksft_print_msg("Starting MBM BW change ...\n");
>  
> +	res = remount_resctrlfs(false);

I think that should be remount_resctrlfs(true). Please note that any of the tests could be
run separately from the command line and thus each test need to ensure a clean
environment, it cannot assume that (a) user space provided it with a clean and/or
unmounted resctrl or (b) that any test was run before it.

> +	if (res) {
> +		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> +		return;
> +	}
> +
>  	if (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) {
>  		ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n");
> -		return;
> +		goto umount;
>  	}
>  

Reinette