Re: [LTP] [PATCH] syscalls/fanotify: Kill the child process before exit

[Matthew Bobrowski via ltp <ltp@lists.linux.it>]

On Fri, Nov 05, 2021 at 02:22:51AM +0000, zhaogongyi wrote:
> Hi,
> 
> > 
> > On Thu, Nov 04, 2021 at 07:37:35PM +0100, Petr Vorel wrote:
> > > Hi all,
> > >
> > > > Before the main process exit abnormally, we need to kill the child
> > > > process.
> > >
> > > Amir, Matthew, could you please have a look?
> > 
> > If anything, I feel as though stop_children() should probably be called from
> > cleanup() as that callback will be invoked if any of the
> > SAFE_FANOTIFY_* macros fail anyway, right?
> 
> It seems that calling before run_children, we need not cleanup though the SAFE_FANOTIFY_* macros fail.

Even if stop_children() is called prior to run_children(), maybe it would
be OK given the fact that child_pid[] is a global and pre-intialized with
zeros. Meaning, that if SAFE_KILL() was called passing a PID value of 0,
then all processes part of the calling process' process group would be sent
a SIGKILL signal, which is kind of what we want anyway when taking a
teardown path. This is just a quick thought.

Nonetheless, you could also introduce barriers into stop_children()
i.e. checking whether PID values in child_pid[] are all zero and if so
return early. That'd prevent you from taking any unnecessary cleanup path
in the event that something fails before any child processes are spawned?

Amir likely has an alternate suggestion, or he may be happy with your
initial proposal.

> > I don't feel like there's a need to introduce another helper here to
> > explicitly handle the cleanup case, but I could also be wrong.
> 
> When we run the testcase simultaneously, and the ulimit of open files is small, the testcase will fail and remain
> many while(1) tasks in system, it makes the system very stuck.
> 
> Shall we need to cleanup it?

I'm not saying that this cleanup isn't necessary, just that it could be
placed in a more appropriate location.

> > > > Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> > > > ---
> > > >  .../kernel/syscalls/fanotify/fanotify07.c     | 35
> > ++++++++++++++++++-
> > > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > >
> > > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify07.c
> > > > b/testcases/kernel/syscalls/fanotify/fanotify07.c
> > > > index cc56d9019..0a0b4f1e4 100644
> > > > --- a/testcases/kernel/syscalls/fanotify/fanotify07.c
> > > > +++ b/testcases/kernel/syscalls/fanotify/fanotify07.c
> > > > @@ -108,6 +108,39 @@ static int setup_instance(void)
> > > >  	return fd;
> > > >  }
> > >
> > > > +static int setup_another_instance(void) {
> > > > +	int rval;
> > > > +	int fd = fanotify_init(FAN_CLASS_CONTENT, O_RDONLY);
> > > > +
> > > > +	if (fd == -1) {
> > > > +		stop_children();
> > > > +		tst_brk(TBROK | TERRNO, "fanotify_init() failed");
> > > > +	}
> > > > +
> > > > +	if (fd < -1) {
> > > > +		stop_children();
> > > > +		tst_brk(TBROK | TERRNO,
> > > > +			"invalid fanotify_init() return %d", fd);
> > > > +	}
> > > > +
> > > > +	rval = fanotify_mark(fd,
> > > > +		FAN_MARK_ADD, FAN_ACCESS_PERM, AT_FDCWD, fname);
> > > > +
> > > > +	if (rval == -1) {
> > > > +		stop_children();
> > > > +		tst_brk(TBROK | TERRNO, "fanotify_mark() failed");
> > > > +	}
> > > > +
> > > > +	if (rval < -1) {
> > > > +		stop_children();
> > > > +		tst_brk(TBROK | TERRNO,
> > > > +			"invalid fanotify_mark() return %d", rval);
> > > > +	}
> > > > +
> > > > +	return fd;
> > > > +}
> > > > +
> > > >  static void loose_fanotify_events(void)  {
> > > >  	int not_responded = 0;
> > > > @@ -160,7 +193,7 @@ static void test_fanotify(void)
> > > >  	 * Create and destroy another instance. This may hang if
> > > >  	 * unanswered fanotify events block notification subsystem.
> > > >  	 */
> > > > -	newfd = setup_instance();
> > > > +	newfd = setup_another_instance();
> > >
> > > >  	SAFE_CLOSE(newfd);
> > 
> > /M
/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp