linux-kernel - Re: [PATCH v4 05/19] selftests/resctrl: Unmount resctrl FS before starting the first 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <406820f8-1d28-cdde-9494-20231fec0468@intel.com>
Date:   Thu, 13 Jul 2023 15:53:22 -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>,
        "Shaopeng Tan" <tan.shaopeng@...fujitsu.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 05/19] selftests/resctrl: Unmount resctrl FS before
 starting the first test

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Resctrl FS mount/remount/umount code is hard to track. Better approach
> is to use mount/umount pair for each test but that assumes resctrl FS
> is not mounted beforehand.
> 
> Change umount_resctrlfs() so that it can unmount resctrl FS from any
> path, and enable further simplifications into mount/remount/umount
> logic by unmounting resctrl FS at the start if a pre-existing
> mountpoint is found.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@...el.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
>  tools/testing/selftests/resctrl/resctrlfs.c     | 6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 9b9751206e1c..b1b2d28b52f7 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -250,6 +250,8 @@ int main(int argc, char **argv)
>  	if (!check_resctrlfs_support())
>  		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
>  
> +	umount_resctrlfs();
> +

umount_resctrlfs() can fail. I think there should be error checking
here and no tests should be run if resctrl cannot be unmounted since
the hardware and resctrl state is not known (without more effort).

>  	filter_dmesg();
>  
>  	ksft_set_plan(tests ? : 4);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index fb00245dee92..23f75aeaa198 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -82,10 +82,12 @@ int remount_resctrlfs(bool mum_resctrlfs)
>  
>  int umount_resctrlfs(void)
>  {
> -	if (find_resctrl_mount(NULL))
> +	char mountpoint[256];
> +
> +	if (find_resctrl_mount(mountpoint))
>  		return 0;

It looks like the intent is to return 0 if find_resctrl_mount()
returns -ENOENT. It is not clear to me that it should also
return 0 if find_resctrl_mount() returns -ENXIO.

>  
> -	if (umount(RESCTRL_PATH)) {
> +	if (umount(mountpoint)) {
>  		perror("# Unable to umount resctrl");
>  
>  		return errno;

Reinette