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: <406496b7-877b-d9e7-10e-d7b31da51add@linux.intel.com>
Date:   Tue, 7 Feb 2023 10:50:23 +0200 (EET)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     "Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.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

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.

-- 
 i.

> To make the child process wait to be killed by parent process
> in any case(an error occurred or a signal was received),
> I fixed it like this.
> 
> This fix was discussed here.
> https://lore.kernel.org/lkml/2ab9ca20-c757-7dd8-b770-2b84d171cbfb@intel.com/