linux-kernel - RE: [PATCH v6 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT 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: <OSZPR01MB63287E0DB7DEF1029305D7D98BD89@OSZPR01MB6328.jpnprd01.prod.outlook.com>
Date:   Wed, 8 Feb 2023 02:42:56 +0000
From:   "Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>
To:     'Ilpo Järvinen' 
        <ilpo.jarvinen@...ux.intel.com>
CC:     Fenghua Yu <fenghua.yu@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Shuah Khan <shuah@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: RE: [PATCH v6 4/5] selftests/resctrl: Cleanup properly when an error
 occurs in CAT test

Hi Ilpo,

> On Tue, 7 Feb 2023, Shaopeng Tan (Fujitsu) wrote:
> 
> > > On Tue, 31 Jan 2023, Shaopeng Tan wrote:
> > >
> > > > After creating a child process with fork() in CAT test, if an
> > > > error occurs or a signal such as SIGINT is received, the parent
> > > > process will be terminated immediately, and therefor the child
> > > > process will not be killed and also resctrlfs is not unmounted.
> > > >
> > > > There is a signal handler registered in CMT/MBM/MBA tests, which
> > > > kills child process, unmount resctrlfs, cleanups result files,
> > > > etc., if a signal such as SIGINT is received.
> > > >
> > > > Commonize the signal handler registered for CMT/MBM/MBA tests and
> > > > reuse it in CAT too.
> > > >
> > > > To reuse the signal handler, make the child process in CAT wait to
> > > > be killed by parent process in any case (an error occurred or a
> > > > signal was received), and when killing child process use global
> > > > bm_pid instead of local bm_pid.
> > > >
> > > > Also, since the MBA/MBA/CMT/CAT are run in order, unregister the
> > > > signal handler at the end of each test so that the signal handler
> > > > cannot be inherited by other tests.
> > > >
> > > > Signed-off-by: Shaopeng Tan <tan.shaopeng@...fujitsu.com>
> > > > ---
> 
> > > >  	ret = cat_val(&param);
> > > > -	if (ret)
> > > > -		return ret;
> > > > -
> > > > -	ret = check_results(&param);
> > > > -	if (ret)
> > > > -		return ret;
> > > > +	if (ret == 0)
> > > > +		ret = check_results(&param);
> > >
> > > It would be take this program flow fix out of the signal handler
> > > change into a separate change.
> >
> > Do you mean this fix should be separated into two patches?
> 
> Yes.
> 
> Currently, I see your patch doing (mainly) two things:
> 1) cleaning up the messy signal handler logic
> 2) fixing the early return in case of error from cat_val() or
>    check_results()
> 
> Both are good changes and both are needed to fully fix things. But (IMHO)
> those are indepedent enough that it would warrant to split this change into two.

Thanks for your advice, I will split it in next version


Best regards,
Shaopeng TAN