[LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Li Wang via ltp]

To get rid of failure from github CI.

CI Test Job: https://github.com/wangli5665/ltp/actions/runs/14988530205

Follow-up-fix: commit b987b8ac5 ("lib: child process exit with error due to uninitialized lib_pid")
Signed-off-by: Li Wang <liwang@redhat.com>
---
 lib/tst_test.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/tst_test.c b/lib/tst_test.c
index b666715b9..9f11c1c47 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -59,7 +59,6 @@ static const char *tid;
 static int iterations = 1;
 static float duration = -1;
 static float timeout_mul = -1;
-static pid_t lib_pid;
 static int mntpoint_mounted;
 static int ovl_mounted;
 static struct timespec tst_start_time; /* valid only for test pid */
@@ -143,7 +142,9 @@ static void setup_ipc(void)
 		tst_futexes = (char *)results + sizeof(struct results);
 		tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
 	}
-	results->lib_pid = lib_pid;
+
+	results->lib_pid = 0;
+	results->main_pid = 0;
 }
 
 static void cleanup_ipc(void)
@@ -394,7 +395,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
 	 * If tst_brk() is called from some of the C helpers even before the
 	 * library was initialized, just exit.
 	 */
-	if (!results->lib_pid)
+	if (!results || !results->lib_pid)
 		exit(TTYPE_RESULT(ttype));
 
 	update_results(TTYPE_RESULT(ttype));
@@ -1334,6 +1335,8 @@ static void do_setup(int argc, char *argv[])
 		tst_test->forks_child = 1;
 	}
 
+	setup_ipc();
+
 	if (tst_test->needs_kconfigs && tst_kconfig_check(tst_test->needs_kconfigs))
 		tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!");
 
@@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->hugepages.number)
 		tst_reserve_hugepages(&tst_test->hugepages);
 
-	setup_ipc();
-
 	if (tst_test->bufs)
 		tst_buffers_alloc(tst_test->bufs);
 
@@ -1929,10 +1930,11 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 	unsigned int test_variants = 1;
 	struct utsname uval;
 
-	lib_pid = getpid();
 	tst_test = self;
 
 	do_setup(argc, argv);
+
+	results->lib_pid = getpid();
 	tst_enable_oom_protection(results->lib_pid);
 
 	SAFE_SIGNAL(SIGALRM, alarm_handler);
@@ -1940,7 +1942,6 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
 
 	tst_res(TINFO, "LTP version: "LTP_VERSION);
 
-
 	uname(&uval);
 	tst_res(TINFO, "Tested kernel: %s %s %s", uval.release, uval.version, uval.machine);
 
-- 
2.49.0


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

[LTP] [PATCH 2/2] dirtyc0w_shmem: set child_needs_reinit

[Li Wang via ltp]

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
index 64b187b35..4b31da831 100644
--- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
+++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
@@ -104,6 +104,7 @@ static void cleanup(void)
 
 static struct tst_test test = {
 	.needs_checkpoints = 1,
+	.child_needs_reinit =1 ,
 	.forks_child = 1,
 	.needs_root = 1,
 	.runtime = 120,
-- 
2.49.0


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

Re: [LTP] [PATCH 2/2] dirtyc0w_shmem: set child_needs_reinit

[Li Wang via ltp]

On Tue, May 13, 2025 at 4:32 PM Wei Gao <wegao@suse.com> wrote:

> On Tue, May 13, 2025 at 01:05:30PM +0800, Li Wang wrote:
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > ---
> >  testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
> b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
> > index 64b187b35..4b31da831 100644
> > --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
> > +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
> > @@ -104,6 +104,7 @@ static void cleanup(void)
> >
> >  static struct tst_test test = {
> >       .needs_checkpoints = 1,
> > +     .child_needs_reinit =1 ,
> s/.child_needs_reinit =1 ,/.child_needs_reinit = 1,/
>

Good catch. That blank before the comma should be removed.


> Do we still need this? Since .needs_checkpoints is already setted.
>

I think yes!

While some fields may have overlapping effects, explicitly setting this
field
highlights its intended function and enhances the clarity of the code.


-- 
Regards,
Li Wang

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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Cyril Hrubis]

Hi!
> To get rid of failure from github CI.
> 
> CI Test Job: https://github.com/wangli5665/ltp/actions/runs/14988530205
> 
> Follow-up-fix: commit b987b8ac5 ("lib: child process exit with error due to uninitialized lib_pid")
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  lib/tst_test.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index b666715b9..9f11c1c47 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -59,7 +59,6 @@ static const char *tid;
>  static int iterations = 1;
>  static float duration = -1;
>  static float timeout_mul = -1;
> -static pid_t lib_pid;
>  static int mntpoint_mounted;
>  static int ovl_mounted;
>  static struct timespec tst_start_time; /* valid only for test pid */
> @@ -143,7 +142,9 @@ static void setup_ipc(void)
>  		tst_futexes = (char *)results + sizeof(struct results);
>  		tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
>  	}
> -	results->lib_pid = lib_pid;
> +
> +	results->lib_pid = 0;
> +	results->main_pid = 0;
>  }
>  
>  static void cleanup_ipc(void)
> @@ -394,7 +395,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
>  	 * If tst_brk() is called from some of the C helpers even before the
>  	 * library was initialized, just exit.
>  	 */
> -	if (!results->lib_pid)
> +	if (!results || !results->lib_pid)
>  		exit(TTYPE_RESULT(ttype));
>  
>  	update_results(TTYPE_RESULT(ttype));
> @@ -1334,6 +1335,8 @@ static void do_setup(int argc, char *argv[])
>  		tst_test->forks_child = 1;
>  	}
>  
> +	setup_ipc();
> +
>  	if (tst_test->needs_kconfigs && tst_kconfig_check(tst_test->needs_kconfigs))
>  		tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!");
>  
> @@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
>  	if (tst_test->hugepages.number)
>  		tst_reserve_hugepages(&tst_test->hugepages);
>  
> -	setup_ipc();
> -

I suppose that this has to go before the tst_reserve_hugepages() so that
we have results->lib_pid defined and properly clean up after the
hugepages. However for that to work we have to set the
results->lib_pid directly in the setup_ipc().

>  	if (tst_test->bufs)
>  		tst_buffers_alloc(tst_test->bufs);
>  
> @@ -1929,10 +1930,11 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
>  	unsigned int test_variants = 1;
>  	struct utsname uval;
>  
> -	lib_pid = getpid();
>  	tst_test = self;
>  
>  	do_setup(argc, argv);
> +
> +	results->lib_pid = getpid();

Setting it here is too late.

Other than that the patch looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 2/2] dirtyc0w_shmem: set child_needs_reinit

[Cyril Hrubis]

Hi!
> > Do we still need this? Since .needs_checkpoints is already setted.
> >
> 
> I think yes!
> 
> While some fields may have overlapping effects, explicitly setting this
> field
> highlights its intended function and enhances the clarity of the code.

Exactly.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Li Wang via ltp]

On Tue, May 13, 2025 at 8:08 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > To get rid of failure from github CI.
> >
> > CI Test Job: https://github.com/wangli5665/ltp/actions/runs/14988530205
> >
> > Follow-up-fix: commit b987b8ac5 ("lib: child process exit with error due to uninitialized lib_pid")
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > ---
> >  lib/tst_test.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > index b666715b9..9f11c1c47 100644
> > --- a/lib/tst_test.c
> > +++ b/lib/tst_test.c
> > @@ -59,7 +59,6 @@ static const char *tid;
> >  static int iterations = 1;
> >  static float duration = -1;
> >  static float timeout_mul = -1;
> > -static pid_t lib_pid;
> >  static int mntpoint_mounted;
> >  static int ovl_mounted;
> >  static struct timespec tst_start_time; /* valid only for test pid */
> > @@ -143,7 +142,9 @@ static void setup_ipc(void)
> >               tst_futexes = (char *)results + sizeof(struct results);
> >               tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
> >       }
> > -     results->lib_pid = lib_pid;
> > +
> > +     results->lib_pid = 0;
> > +     results->main_pid = 0;
> >  }
> >
> >  static void cleanup_ipc(void)
> > @@ -394,7 +395,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> >        * If tst_brk() is called from some of the C helpers even before the
> >        * library was initialized, just exit.
> >        */
> > -     if (!results->lib_pid)
> > +     if (!results || !results->lib_pid)
> >               exit(TTYPE_RESULT(ttype));
> >
> >       update_results(TTYPE_RESULT(ttype));
> > @@ -1334,6 +1335,8 @@ static void do_setup(int argc, char *argv[])
> >               tst_test->forks_child = 1;
> >       }
> >
> > +     setup_ipc();
> > +
> >       if (tst_test->needs_kconfigs && tst_kconfig_check(tst_test->needs_kconfigs))
> >               tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!");
> >
> > @@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
> >       if (tst_test->hugepages.number)
> >               tst_reserve_hugepages(&tst_test->hugepages);
> >
> > -     setup_ipc();
> > -
>
> I suppose that this has to go before the tst_reserve_hugepages() so that
> we have results->lib_pid defined and properly clean up after the
> hugepages. However for that to work we have to set the
> results->lib_pid directly in the setup_ipc().

In this patch, setup_ipc() is already moved up before the
tst_reserve_hugpages(). I particularly put it after tst_test->runs_scripts,
which is mandatory to have .child_needs_reinit initialized first.

And the hugpages cleanup work is completed in tst_sys_conf_restore(0)
in the do_cleanup, so that might not be a problem.
I have validated that all hugetlb tests work well.

Also,

Or, did I miss anything?

>
> >       if (tst_test->bufs)
> >               tst_buffers_alloc(tst_test->bufs);
> >
> > @@ -1929,10 +1930,11 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> >       unsigned int test_variants = 1;
> >       struct utsname uval;
> >
> > -     lib_pid = getpid();
> >       tst_test = self;
> >
> >       do_setup(argc, argv);
> > +
> > +     results->lib_pid = getpid();
>
> Setting it here is too late.

Since I added results exists check in tst_vbrk_(), so it should be safe
enough to set lib_pid just after setup_ipc() whatever the cleanup work
happen early or later in test lib.

if (!results || !results->lib_pid)
    exit(TTYPE_RESULT(ttype));


>
> Other than that the patch looks good to me.
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Petr Vorel]

Hi all,

...
> > +++ b/lib/tst_test.c
> > @@ -59,7 +59,6 @@ static const char *tid;
> >  static int iterations = 1;
> >  static float duration = -1;
> >  static float timeout_mul = -1;
> > -static pid_t lib_pid;
> >  static int mntpoint_mounted;
> >  static int ovl_mounted;
> >  static struct timespec tst_start_time; /* valid only for test pid */
> > @@ -143,7 +142,9 @@ static void setup_ipc(void)
> >  		tst_futexes = (char *)results + sizeof(struct results);
> >  		tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
> >  	}
> > -	results->lib_pid = lib_pid;
> > +
> > +	results->lib_pid = 0;
> > +	results->main_pid = 0;

nit: Is it really needed to int them to 0? Because they should be 0 due the
default struct value, right?

> >  }

> >  static void cleanup_ipc(void)
> > @@ -394,7 +395,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> >  	 * If tst_brk() is called from some of the C helpers even before the
> >  	 * library was initialized, just exit.
> >  	 */
> > -	if (!results->lib_pid)
> > +	if (!results || !results->lib_pid)
> >  		exit(TTYPE_RESULT(ttype));

> >  	update_results(TTYPE_RESULT(ttype));
> > @@ -1334,6 +1335,8 @@ static void do_setup(int argc, char *argv[])
> >  		tst_test->forks_child = 1;
> >  	}

> > +	setup_ipc();
> > +
> >  	if (tst_test->needs_kconfigs && tst_kconfig_check(tst_test->needs_kconfigs))
> >  		tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!");

> > @@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
> >  	if (tst_test->hugepages.number)
> >  		tst_reserve_hugepages(&tst_test->hugepages);

> > -	setup_ipc();
> > -

> I suppose that this has to go before the tst_reserve_hugepages() so that
> we have results->lib_pid defined and properly clean up after the
> hugepages.

Why? Is that due possible tst_brk() calls in tst_reserve_hugepages()?
(Which uses SAFE_* macros.) That would trigger Because there are tst_brk() calls before.

Also why not assign results->lib_pid = getpid() at the beginning of
tst_run_tcases() ?

Kind regards,
Petr

> However for that to work we have to set the
> results->lib_pid directly in the setup_ipc().

> >  	if (tst_test->bufs)
> >  		tst_buffers_alloc(tst_test->bufs);

> > @@ -1929,10 +1930,11 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> >  	unsigned int test_variants = 1;
> >  	struct utsname uval;

> > -	lib_pid = getpid();
> >  	tst_test = self;

> >  	do_setup(argc, argv);
> > +
> > +	results->lib_pid = getpid();

> Setting it here is too late.

> Other than that the patch looks good to me.

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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Li Wang via ltp]

On Tue, May 13, 2025 at 8:52 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi all,
>
> ...
> > > +++ b/lib/tst_test.c
> > > @@ -59,7 +59,6 @@ static const char *tid;
> > >  static int iterations = 1;
> > >  static float duration = -1;
> > >  static float timeout_mul = -1;
> > > -static pid_t lib_pid;
> > >  static int mntpoint_mounted;
> > >  static int ovl_mounted;
> > >  static struct timespec tst_start_time; /* valid only for test pid */
> > > @@ -143,7 +142,9 @@ static void setup_ipc(void)
> > >             tst_futexes = (char *)results + sizeof(struct results);
> > >             tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
> > >     }
> > > -   results->lib_pid = lib_pid;
> > > +
> > > +   results->lib_pid = 0;
> > > +   results->main_pid = 0;
>
> nit: Is it really needed to int them to 0? Because they should be 0 due the
> default struct value, right?

Right!

I think we need to initialize them, because the test may fail/break
just before creating main_pid, then test cleanup work needs to
invoke tst_vbrk_ and tst_res_ somewhere that compares pid to
invoke the corresponding cleanup work.

>
> > >  }
>
> > >  static void cleanup_ipc(void)
> > > @@ -394,7 +395,7 @@ void tst_vbrk_(const char *file, const int lineno, int ttype, const char *fmt,
> > >      * If tst_brk() is called from some of the C helpers even before the
> > >      * library was initialized, just exit.
> > >      */
> > > -   if (!results->lib_pid)
> > > +   if (!results || !results->lib_pid)
> > >             exit(TTYPE_RESULT(ttype));
>
> > >     update_results(TTYPE_RESULT(ttype));
> > > @@ -1334,6 +1335,8 @@ static void do_setup(int argc, char *argv[])
> > >             tst_test->forks_child = 1;
> > >     }
>
> > > +   setup_ipc();
> > > +
> > >     if (tst_test->needs_kconfigs && tst_kconfig_check(tst_test->needs_kconfigs))
> > >             tst_brk(TCONF, "Aborting due to unsuitable kernel config, see above!");
>
> > > @@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
> > >     if (tst_test->hugepages.number)
> > >             tst_reserve_hugepages(&tst_test->hugepages);
>
> > > -   setup_ipc();
> > > -
>
> > I suppose that this has to go before the tst_reserve_hugepages() so that
> > we have results->lib_pid defined and properly clean up after the
> > hugepages.
>
> Why? Is that due possible tst_brk() calls in tst_reserve_hugepages()?
> (Which uses SAFE_* macros.) That would trigger Because there are tst_brk() calls before.
>
> Also why not assign results->lib_pid = getpid() at the beginning of
> tst_run_tcases() ?

No, we can't.

Because we moved the lib_pid in the IPC region, which allocated memory
in the setup_ipc() function.

>
> Kind regards,
> Petr
>
> > However for that to work we have to set the
> > results->lib_pid directly in the setup_ipc().
>
> > >     if (tst_test->bufs)
> > >             tst_buffers_alloc(tst_test->bufs);
>
> > > @@ -1929,10 +1930,11 @@ void tst_run_tcases(int argc, char *argv[], struct tst_test *self)
> > >     unsigned int test_variants = 1;
> > >     struct utsname uval;
>
> > > -   lib_pid = getpid();
> > >     tst_test = self;
>
> > >     do_setup(argc, argv);
> > > +
> > > +   results->lib_pid = getpid();
>
> > Setting it here is too late.
>
> > Other than that the patch looks good to me.
>


-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Li Wang via ltp]

On Tue, May 13, 2025 at 9:02 PM Li Wang <liwang@redhat.com> wrote:
>
> On Tue, May 13, 2025 at 8:52 PM Petr Vorel <pvorel@suse.cz> wrote:
> >
> > Hi all,
> >
> > ...
> > > > +++ b/lib/tst_test.c
> > > > @@ -59,7 +59,6 @@ static const char *tid;
> > > >  static int iterations = 1;
> > > >  static float duration = -1;
> > > >  static float timeout_mul = -1;
> > > > -static pid_t lib_pid;
> > > >  static int mntpoint_mounted;
> > > >  static int ovl_mounted;
> > > >  static struct timespec tst_start_time; /* valid only for test pid */
> > > > @@ -143,7 +142,9 @@ static void setup_ipc(void)
> > > >             tst_futexes = (char *)results + sizeof(struct results);
> > > >             tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
> > > >     }
> > > > -   results->lib_pid = lib_pid;
> > > > +
> > > > +   results->lib_pid = 0;
> > > > +   results->main_pid = 0;
> >
> > nit: Is it really needed to int them to 0? Because they should be 0 due the
> > default struct value, right?

The results structure memory was dynamically allocated via
mmap so the default value might not be zero. Unless we do
explicitly call memset(, 0, ...).


-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Petr Vorel]

> On Tue, May 13, 2025 at 9:02 PM Li Wang <liwang@redhat.com> wrote:

> > On Tue, May 13, 2025 at 8:52 PM Petr Vorel <pvorel@suse.cz> wrote:

> > > Hi all,

> > > ...
> > > > > +++ b/lib/tst_test.c
> > > > > @@ -59,7 +59,6 @@ static const char *tid;
> > > > >  static int iterations = 1;
> > > > >  static float duration = -1;
> > > > >  static float timeout_mul = -1;
> > > > > -static pid_t lib_pid;
> > > > >  static int mntpoint_mounted;
> > > > >  static int ovl_mounted;
> > > > >  static struct timespec tst_start_time; /* valid only for test pid */
> > > > > @@ -143,7 +142,9 @@ static void setup_ipc(void)
> > > > >             tst_futexes = (char *)results + sizeof(struct results);
> > > > >             tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
> > > > >     }
> > > > > -   results->lib_pid = lib_pid;
> > > > > +
> > > > > +   results->lib_pid = 0;
> > > > > +   results->main_pid = 0;

> > > nit: Is it really needed to int them to 0? Because they should be 0 due the
> > > default struct value, right?

> The results structure memory was dynamically allocated via
> mmap so the default value might not be zero. Unless we do
> explicitly call memset(, 0, ...).

Thanks for info. Or just use {} instead of memset?

struct results saved_results = {};

Tiny detail, just it's more readable to me.

Kind regards,
Petr

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

Re: [LTP] [PATCH 2/2] dirtyc0w_shmem: set child_needs_reinit

[Wei Gao via ltp]

On Tue, May 13, 2025 at 01:05:30PM +0800, Li Wang wrote:
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
> index 64b187b35..4b31da831 100644
> --- a/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
> +++ b/testcases/kernel/security/dirtyc0w_shmem/dirtyc0w_shmem.c
> @@ -104,6 +104,7 @@ static void cleanup(void)
>  
>  static struct tst_test test = {
>  	.needs_checkpoints = 1,
> +	.child_needs_reinit =1 ,
s/.child_needs_reinit =1 ,/.child_needs_reinit = 1,/

Do we still need this? Since .needs_checkpoints is already setted.
>  	.forks_child = 1,
>  	.needs_root = 1,
>  	.runtime = 120,
> -- 
> 2.49.0
> 
Thanks for your quick patchset :)

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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Li Wang via ltp]

On Tue, May 13, 2025 at 11:42 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > On Tue, May 13, 2025 at 9:02 PM Li Wang <liwang@redhat.com> wrote:
>
> > > On Tue, May 13, 2025 at 8:52 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > > Hi all,
>
> > > > ...
> > > > > > +++ b/lib/tst_test.c
> > > > > > @@ -59,7 +59,6 @@ static const char *tid;
> > > > > >  static int iterations = 1;
> > > > > >  static float duration = -1;
> > > > > >  static float timeout_mul = -1;
> > > > > > -static pid_t lib_pid;
> > > > > >  static int mntpoint_mounted;
> > > > > >  static int ovl_mounted;
> > > > > >  static struct timespec tst_start_time; /* valid only for test pid */
> > > > > > @@ -143,7 +142,9 @@ static void setup_ipc(void)
> > > > > >             tst_futexes = (char *)results + sizeof(struct results);
> > > > > >             tst_max_futexes = (size - sizeof(struct results))/sizeof(futex_t);
> > > > > >     }
> > > > > > -   results->lib_pid = lib_pid;
> > > > > > +
> > > > > > +   results->lib_pid = 0;
> > > > > > +   results->main_pid = 0;
>
> > > > nit: Is it really needed to int them to 0? Because they should be 0 due the
> > > > default struct value, right?
>
> > The results structure memory was dynamically allocated via
> > mmap so the default value might not be zero. Unless we do
> > explicitly call memset(, 0, ...).
>
> Thanks for info. Or just use {} instead of memset?

This only applies to automatic (stack) or static allocation.
As we're dealing with dynamically allocated memory via mmap(),
we still need memset() or manually initialize it.

In setup_ipc():

results = SAFE_MMAP(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, ipc_fd, 0);

>
> struct results saved_results = {};
>
> Tiny detail, just it's more readable to me.
>
> Kind regards,
> Petr
>


-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Cyril Hrubis]

Hi!
> > > @@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
> > >       if (tst_test->hugepages.number)
> > >               tst_reserve_hugepages(&tst_test->hugepages);
> > >
> > > -     setup_ipc();
> > > -
> >
> > I suppose that this has to go before the tst_reserve_hugepages() so that
> > we have results->lib_pid defined and properly clean up after the
> > hugepages. However for that to work we have to set the
> > results->lib_pid directly in the setup_ipc().
> 
> In this patch, setup_ipc() is already moved up before the
> tst_reserve_hugpages(). I particularly put it after tst_test->runs_scripts,
> which is mandatory to have .child_needs_reinit initialized first.
> 
> And the hugpages cleanup work is completed in tst_sys_conf_restore(0)
> in the do_cleanup, so that might not be a problem.
> I have validated that all hugetlb tests work well.
> 
> Also,
> 
> Or, did I miss anything?

The problem is that no library cleanup will happen unless lib_pid is
set, that means that tst_brk() will be triggered anywhere in do_setup()
the clanup will not be run because we set the lib_pid after the
do_setup() in the patch.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Li Wang via ltp]

On Wed, May 14, 2025 at 4:00 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > > > @@ -1393,8 +1396,6 @@ static void do_setup(int argc, char *argv[])
> > > >       if (tst_test->hugepages.number)
> > > >               tst_reserve_hugepages(&tst_test->hugepages);
> > > >
> > > > -     setup_ipc();
> > > > -
> > >
> > > I suppose that this has to go before the tst_reserve_hugepages() so that
> > > we have results->lib_pid defined and properly clean up after the
> > > hugepages. However for that to work we have to set the
> > > results->lib_pid directly in the setup_ipc().
> >
> > In this patch, setup_ipc() is already moved up before the
> > tst_reserve_hugpages(). I particularly put it after tst_test->runs_scripts,
> > which is mandatory to have .child_needs_reinit initialized first.
> >
> > And the hugpages cleanup work is completed in tst_sys_conf_restore(0)
> > in the do_cleanup, so that might not be a problem.
> > I have validated that all hugetlb tests work well.
> >
> > Also,
> >
> > Or, did I miss anything?
>
> The problem is that no library cleanup will happen unless lib_pid is
> set, that means that tst_brk() will be triggered anywhere in do_setup()
> the clanup will not be run because we set the lib_pid after the
> do_setup() in the patch.

I see, and you're right we have to set lib_pid before any real setup.

Does this change below make sense (based on the v2 I just sent)?

--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -144,6 +144,8 @@ static void setup_ipc(void)
        }

        memset(results, 0 , size);
+
+       results->lib_pid = getpid();
 }

 static void cleanup_ipc(void)
@@ -1933,7 +1935,6 @@ void tst_run_tcases(int argc, char *argv[],
struct tst_test *self)

        do_setup(argc, argv);

-       results->lib_pid = getpid();
        tst_enable_oom_protection(results->lib_pid);

        SAFE_SIGNAL(SIGALRM, alarm_handler);



-- 
Regards,
Li Wang


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

Re: [LTP] [PATCH 1/2] lib: initialize lib|main_pid to zero in the setup_ipc

[Cyril Hrubis]

Hi!
> I see, and you're right we have to set lib_pid before any real setup.
> 
> Does this change below make sense (based on the v2 I just sent)?
> 
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -144,6 +144,8 @@ static void setup_ipc(void)
>         }
> 
>         memset(results, 0 , size);
> +
> +       results->lib_pid = getpid();
>  }
> 
>  static void cleanup_ipc(void)
> @@ -1933,7 +1935,6 @@ void tst_run_tcases(int argc, char *argv[],
> struct tst_test *self)
> 
>         do_setup(argc, argv);
> 
> -       results->lib_pid = getpid();
>         tst_enable_oom_protection(results->lib_pid);
> 
>         SAFE_SIGNAL(SIGALRM, alarm_handler);

Yes, this should fix it.

-- 
Cyril Hrubis
chrubis@suse.cz

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