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] [day] [month] [year] [list]

Date:   Wed, 8 Feb 2023 09:54:19 +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 Wed, 8 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>
> > > ---
> > 
> > >  	if (bm_pid == 0) {
> > >  		/* Tell parent that child is ready */
> > >  		close(pipefd[0]);
> > >  		pipe_message = 1;
> > >  		if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
> > > -		    sizeof(pipe_message)) {
> > > -			close(pipefd[1]);
> > > +		    sizeof(pipe_message))
> > > +			/*
> > > +			 * Just print the error message.
> > > +			 * Let while(1) run and wait for itself to be killed.
> > > +			 */
> > >  			perror("# failed signaling parent process");
> > 
> > If the write error is ignored here, won't it just lead to parent hanging forever
> > waiting for the child to send the message through the pipe which will never
> > come?
> 
> If the write error is ignored here, the pipe will be closed by "close(pipefd[1]);" and child process will wait to be killed by "while(1)".
> ---
> -			return errno;
> -		}
> 
>  		close(pipefd[1]);
>  		while (1)
> ---
> 
> If all file descriptors referring to the write end of a pipe have been closed, 
> then an attempt to read(2) from the pipe will see end-of-file (read(2) will return 0).
> Then, "perror("# failed reading from child process");" occurs.
> ---
>         } else {
>                 /* Parent waits for child to be ready. */
>                 close(pipefd[1]);
>                 pipe_message = 0;
>                 while (pipe_message != 1) {
>                         if (read(pipefd[0], &pipe_message,
>                                  sizeof(pipe_message)) < sizeof(pipe_message)) {
>                                 perror("# failed reading from child process");
>                                 break;
>                         }
>                 }
>                 close(pipefd[0]);
>                 kill(bm_pid, SIGKILL);
>                 signal_handler_unregister();
>         }

Ah, indeed read() will pick up the close event. So your code seem fine 
after all.

-- 
 i.