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

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

[zhaogongyi]

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.

> 
> 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?

> 
> > > 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

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

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

[Matthew Bobrowski via ltp]

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

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

[Amir Goldstein]

On Fri, Nov 5, 2021 at 5:01 AM Matthew Bobrowski <repnop@google.com> wrote:
>
> 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.
>

This hack is not necessary.

> 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?
>

This would be the obvious solution.
But simpler to skip zero values then stop the iterator.
This makes the cleanup helper reenetrant:

static int stop_children(void)
{
        int child_ret;
        int i, ret = 0;

        for (i = 0; i < MAX_CHILDREN; i++) {
                if (!child_pid[i]) continue;
                SAFE_KILL(child_pid[i], SIGKILL);
        }
        for (i = 0; i < MAX_CHILDREN; i++) {
                if (!child_pid[i]) continue;
                SAFE_WAITPID(child_pid[i], &child_ret, 0);
                if (!WIFSIGNALED(child_ret))
                        ret = 1;
                child_pid[i] = 0;
       }

        return ret;
}

Thanks,
Amir.

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

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

[Petr Vorel]

Hi Zhao,

...
> > 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.

This kind of info is worth of adding into git commit message.

Kind regards,
Petr

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

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

[Petr Vorel]

Hi all,

...
> This would be the obvious solution.
> But simpler to skip zero values then stop the iterator.
+1
> This makes the cleanup helper reenetrant:

> static int stop_children(void)
> {
>         int child_ret;
>         int i, ret = 0;

>         for (i = 0; i < MAX_CHILDREN; i++) {
>                 if (!child_pid[i]) continue;
>                 SAFE_KILL(child_pid[i], SIGKILL);
>         }
>         for (i = 0; i < MAX_CHILDREN; i++) {
>                 if (!child_pid[i]) continue;
>                 SAFE_WAITPID(child_pid[i], &child_ret, 0);
>                 if (!WIFSIGNALED(child_ret))
>                         ret = 1;
>                 child_pid[i] = 0;
>        }

>         return ret;
> }

Amir, thanks! Zhao, could you please test this in your setup?
(+ please be verbose about your setup, it helps to decide in the future that fix
is still needed).

Kind regards,
Petr

> Thanks,
> Amir.

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

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

[zhaogongyi]

Hi,

I have resubmit a patch as your review. Please see: https://patchwork.ozlabs.org/project/ltp/patch/20211105090200.103485-1-zhaogongyi@huawei.com/

Thanks so much!
Gongyi

> 
> Hi all,
> 
> ...
> > This would be the obvious solution.
> > But simpler to skip zero values then stop the iterator.
> +1
> > This makes the cleanup helper reenetrant:
> 
> > static int stop_children(void)
> > {
> >         int child_ret;
> >         int i, ret = 0;
> 
> >         for (i = 0; i < MAX_CHILDREN; i++) {
> >                 if (!child_pid[i]) continue;
> >                 SAFE_KILL(child_pid[i], SIGKILL);
> >         }
> >         for (i = 0; i < MAX_CHILDREN; i++) {
> >                 if (!child_pid[i]) continue;
> >                 SAFE_WAITPID(child_pid[i], &child_ret, 0);
> >                 if (!WIFSIGNALED(child_ret))
> >                         ret = 1;
> >                 child_pid[i] = 0;
> >        }
> 
> >         return ret;
> > }
> 
> Amir, thanks! Zhao, could you please test this in your setup?
> (+ please be verbose about your setup, it helps to decide in the future that
> fix is still needed).
> 
> Kind regards,
> Petr
> 
> > Thanks,
> > Amir.

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

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

[Matthew Bobrowski via ltp]

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?

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.

> > 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

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

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

[Petr Vorel]

Hi all,

> Before the main process exit abnormally, we need to kill
> the child process.

Amir, Matthew, could you please have a look?

Kind regards,
Petr

> 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);

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

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

[Zhao Gongyi]

Before the main process exit abnormally, we need to kill
the child process.

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);

--
2.17.1


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