[PATCH 1/2] ima_violations.sh: Wait for ima_mmap to exit

[PATCH 1/2] ima_violations.sh: Wait for ima_mmap to exit

[Petr Vorel]

This fixes a race when even 2 sec sleep is not enough (f26399583e was
a wrong approach).

While at it, check for ima_mmap exit code (missing since ever).

Fixes: f26399583e ("ima/{ima_measurements,ima_violations}.sh: Avoid running on tmpfs")
Fixes: 3a8efbcc46 ("This patch adds Integrity Measurement Architecture(IMA) testing support ...")
Reported-by: Martin Doucha <mdoucha@suse.cz>
Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/security/integrity/ima/tests/ima_violations.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
index 13cc9d804d..0c03c30786 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
@@ -157,6 +157,8 @@ test2()
 
 test3()
 {
+	local pid
+
 	tst_res TINFO "verify open_writers using mmapped files"
 
 	local search="open_writers"
@@ -168,6 +170,7 @@ test3()
 	echo 'testing testing' > $FILE
 
 	ima_mmap -f $FILE &
+	pid=$!
 	# wait for violations appear in logs
 	tst_sleep 1s
 
@@ -177,7 +180,10 @@ test3()
 	validate $num_violations $count $search
 
 	# wait for ima_mmap to exit, so we can umount
-	tst_sleep 2s
+	wait $pid
+	if [ $? -ne 0 ]; then
+		tst_brk TBROK "failed to execute ima_mmap"
+	fi
 }
 
 test4()
-- 
2.54.0

[PATCH 2/2] ima_violations.sh: ima_mmap.c: Replace sleep with checkpoints

[Petr Vorel]

Using checkpoints is a proper way in LTP new API [1] to avoid races and
waste of time.  It reduces 3 sec sleep in ima_mmap.c and 1 sec sleep in
ima_violations.sh with just checkpoints.

NOTE: tst_reinit() is really needed instead of .needs_checkpoints = 1
as documented in Shell-Test-API.asciidoc.

[1] https://people.kernel.org/metan/why-sleep-is-almost-never-acceptable-in-tests

Fixes: 0e4cbf753f ("security/ima: Rewrite tests into new API + fixes")
Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/kernel/security/integrity/ima/src/ima_mmap.c     | 7 ++++---
 .../kernel/security/integrity/ima/tests/ima_violations.sh  | 6 +++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/security/integrity/ima/src/ima_mmap.c b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
index 8596809ef4..09b22fd4f4 100644
--- a/testcases/kernel/security/integrity/ima/src/ima_mmap.c
+++ b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
@@ -9,7 +9,6 @@
 
 #include "tst_test.h"
 
-#define SLEEP_AFTER_CLOSE 3
 #define MMAPSIZE 1024
 
 static char *filename;
@@ -35,8 +34,10 @@ static void run(void)
 	file = SAFE_MMAP(NULL, MMAPSIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
 	SAFE_CLOSE(fd);
 
-	tst_res(TINFO, "sleep %ds", SLEEP_AFTER_CLOSE);
-	sleep(SLEEP_AFTER_CLOSE);
+	tst_reinit();
+	TST_CHECKPOINT_WAIT(0);
+	/* keep running until ima_violations.sh open and close file */
+	TST_CHECKPOINT_WAKE_AND_WAIT(0);
 
 	tst_res(TPASS, "test completed");
 }
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
index 0c03c30786..d7dcd077b4 100755
--- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
+++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
@@ -8,6 +8,7 @@
 # test[4-6] test 6.15 commit 5b3cd801155f ("ima: limit the number of open-writers integrity violations")
 # test[7-8] test 6.15 commit a414016218ca ("ima: limit the number of ToMToU integrity violations")
 
+TST_NEEDS_CHECKPOINTS=1
 TST_SETUP="setup"
 TST_CLEANUP="cleanup"
 TST_CNT=8
@@ -171,12 +172,15 @@ test3()
 
 	ima_mmap -f $FILE &
 	pid=$!
+
 	# wait for violations appear in logs
-	tst_sleep 1s
+	TST_CHECKPOINT_WAKE_AND_WAIT 0
 
 	open_file_read
 	close_file_read
 
+	TST_CHECKPOINT_WAKE 0
+
 	validate $num_violations $count $search
 
 	# wait for ima_mmap to exit, so we can umount
-- 
2.54.0

Re: [LTP] [PATCH 2/2] ima_violations.sh: ima_mmap.c: Replace sleep with checkpoints

[Martin Doucha]

Hi,
for both patches:

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 4/28/26 18:10, Petr Vorel wrote:
> Using checkpoints is a proper way in LTP new API [1] to avoid races and
> waste of time.  It reduces 3 sec sleep in ima_mmap.c and 1 sec sleep in
> ima_violations.sh with just checkpoints.
> 
> NOTE: tst_reinit() is really needed instead of .needs_checkpoints = 1
> as documented in Shell-Test-API.asciidoc.
> 
> [1] https://people.kernel.org/metan/why-sleep-is-almost-never-acceptable-in-tests
> 
> Fixes: 0e4cbf753f ("security/ima: Rewrite tests into new API + fixes")
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>   testcases/kernel/security/integrity/ima/src/ima_mmap.c     | 7 ++++---
>   .../kernel/security/integrity/ima/tests/ima_violations.sh  | 6 +++++-
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/security/integrity/ima/src/ima_mmap.c b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> index 8596809ef4..09b22fd4f4 100644
> --- a/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> +++ b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> @@ -9,7 +9,6 @@
>   
>   #include "tst_test.h"
>   
> -#define SLEEP_AFTER_CLOSE 3
>   #define MMAPSIZE 1024
>   
>   static char *filename;
> @@ -35,8 +34,10 @@ static void run(void)
>   	file = SAFE_MMAP(NULL, MMAPSIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>   	SAFE_CLOSE(fd);
>   
> -	tst_res(TINFO, "sleep %ds", SLEEP_AFTER_CLOSE);
> -	sleep(SLEEP_AFTER_CLOSE);
> +	tst_reinit();
> +	TST_CHECKPOINT_WAIT(0);
> +	/* keep running until ima_violations.sh open and close file */
> +	TST_CHECKPOINT_WAKE_AND_WAIT(0);
>   
>   	tst_res(TPASS, "test completed");
>   }
> diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> index 0c03c30786..d7dcd077b4 100755
> --- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> +++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> @@ -8,6 +8,7 @@
>   # test[4-6] test 6.15 commit 5b3cd801155f ("ima: limit the number of open-writers integrity violations")
>   # test[7-8] test 6.15 commit a414016218ca ("ima: limit the number of ToMToU integrity violations")
>   
> +TST_NEEDS_CHECKPOINTS=1
>   TST_SETUP="setup"
>   TST_CLEANUP="cleanup"
>   TST_CNT=8
> @@ -171,12 +172,15 @@ test3()
>   
>   	ima_mmap -f $FILE &
>   	pid=$!
> +
>   	# wait for violations appear in logs
> -	tst_sleep 1s
> +	TST_CHECKPOINT_WAKE_AND_WAIT 0
>   
>   	open_file_read
>   	close_file_read
>   
> +	TST_CHECKPOINT_WAKE 0
> +
>   	validate $num_violations $count $search
>   
>   	# wait for ima_mmap to exit, so we can umount


-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

Re: [LTP] [PATCH 2/2] ima_violations.sh: ima_mmap.c: Replace sleep with checkpoints

[Petr Vorel]

Hi Martin, all,

> Hi,
> for both patches:

> Reviewed-by: Martin Doucha <mdoucha@suse.cz>

Thanks for review, patchset merged.

Kind regards,
Petr

> On 4/28/26 18:10, Petr Vorel wrote:
> > Using checkpoints is a proper way in LTP new API [1] to avoid races and
> > waste of time.  It reduces 3 sec sleep in ima_mmap.c and 1 sec sleep in
> > ima_violations.sh with just checkpoints.

> > NOTE: tst_reinit() is really needed instead of .needs_checkpoints = 1
> > as documented in Shell-Test-API.asciidoc.

> > [1] https://people.kernel.org/metan/why-sleep-is-almost-never-acceptable-in-tests

> > Fixes: 0e4cbf753f ("security/ima: Rewrite tests into new API + fixes")
> > Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >   testcases/kernel/security/integrity/ima/src/ima_mmap.c     | 7 ++++---
> >   .../kernel/security/integrity/ima/tests/ima_violations.sh  | 6 +++++-
> >   2 files changed, 9 insertions(+), 4 deletions(-)

> > diff --git a/testcases/kernel/security/integrity/ima/src/ima_mmap.c b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> > index 8596809ef4..09b22fd4f4 100644
> > --- a/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> > +++ b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> > @@ -9,7 +9,6 @@
> >   #include "tst_test.h"
> > -#define SLEEP_AFTER_CLOSE 3
> >   #define MMAPSIZE 1024
> >   static char *filename;
> > @@ -35,8 +34,10 @@ static void run(void)
> >   	file = SAFE_MMAP(NULL, MMAPSIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> >   	SAFE_CLOSE(fd);
> > -	tst_res(TINFO, "sleep %ds", SLEEP_AFTER_CLOSE);
> > -	sleep(SLEEP_AFTER_CLOSE);
> > +	tst_reinit();
> > +	TST_CHECKPOINT_WAIT(0);
> > +	/* keep running until ima_violations.sh open and close file */
> > +	TST_CHECKPOINT_WAKE_AND_WAIT(0);
> >   	tst_res(TPASS, "test completed");
> >   }
> > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> > index 0c03c30786..d7dcd077b4 100755
> > --- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> > +++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh
> > @@ -8,6 +8,7 @@
> >   # test[4-6] test 6.15 commit 5b3cd801155f ("ima: limit the number of open-writers integrity violations")
> >   # test[7-8] test 6.15 commit a414016218ca ("ima: limit the number of ToMToU integrity violations")
> > +TST_NEEDS_CHECKPOINTS=1
> >   TST_SETUP="setup"
> >   TST_CLEANUP="cleanup"
> >   TST_CNT=8
> > @@ -171,12 +172,15 @@ test3()
> >   	ima_mmap -f $FILE &
> >   	pid=$!
> > +
> >   	# wait for violations appear in logs
> > -	tst_sleep 1s
> > +	TST_CHECKPOINT_WAKE_AND_WAIT 0
> >   	open_file_read
> >   	close_file_read
> > +	TST_CHECKPOINT_WAKE 0
> > +
> >   	validate $num_violations $count $search
> >   	# wait for ima_mmap to exit, so we can umount

Re: [PATCH 1/2] ima_violations.sh: Wait for ima_mmap to exit

[Cyril Hrubis]

Hi!
>  test3()
>  {
> +	local pid
> +
>  	tst_res TINFO "verify open_writers using mmapped files"
>  
>  	local search="open_writers"
> @@ -168,6 +170,7 @@ test3()
>  	echo 'testing testing' > $FILE
>  
>  	ima_mmap -f $FILE &
> +	pid=$!
>  	# wait for violations appear in logs
>  	tst_sleep 1s
>  
> @@ -177,7 +180,10 @@ test3()
>  	validate $num_violations $count $search
>  
>  	# wait for ima_mmap to exit, so we can umount
> -	tst_sleep 2s
> +	wait $pid

You can simplify this just by doing 'wait' that will wait for all
background jobs (we have only one here).

Either way:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

> +	if [ $? -ne 0 ]; then
> +		tst_brk TBROK "failed to execute ima_mmap"
> +	fi
>  }
>  
>  test4()
> -- 
> 2.54.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

Re: [PATCH 2/2] ima_violations.sh: ima_mmap.c: Replace sleep with checkpoints

[Cyril Hrubis]

Hi!
> Using checkpoints is a proper way in LTP new API [1] to avoid races and
> waste of time.  It reduces 3 sec sleep in ima_mmap.c and 1 sec sleep in
> ima_violations.sh with just checkpoints.
> 
> NOTE: tst_reinit() is really needed instead of .needs_checkpoints = 1
> as documented in Shell-Test-API.asciidoc.
> 
> [1] https://people.kernel.org/metan/why-sleep-is-almost-never-acceptable-in-tests
> 
> Fixes: 0e4cbf753f ("security/ima: Rewrite tests into new API + fixes")
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testcases/kernel/security/integrity/ima/src/ima_mmap.c     | 7 ++++---
>  .../kernel/security/integrity/ima/tests/ima_violations.sh  | 6 +++++-
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/security/integrity/ima/src/ima_mmap.c b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> index 8596809ef4..09b22fd4f4 100644
> --- a/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> +++ b/testcases/kernel/security/integrity/ima/src/ima_mmap.c
> @@ -9,7 +9,6 @@
>  
>  #include "tst_test.h"
>  
> -#define SLEEP_AFTER_CLOSE 3
>  #define MMAPSIZE 1024
>  
>  static char *filename;
> @@ -35,8 +34,10 @@ static void run(void)
>  	file = SAFE_MMAP(NULL, MMAPSIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>  	SAFE_CLOSE(fd);
>  
> -	tst_res(TINFO, "sleep %ds", SLEEP_AFTER_CLOSE);
> -	sleep(SLEEP_AFTER_CLOSE);
> +	tst_reinit();
> +	TST_CHECKPOINT_WAIT(0);
> +	/* keep running until ima_violations.sh open and close file */
> +	TST_CHECKPOINT_WAKE_AND_WAIT(0);
>  
>  	tst_res(TPASS, "test completed");
>  }

C helpers that call tst_reinit() should implement main(). These are not
complete tests, just helpers. The main problem is that if you add
tst_reinit() to a source that defines tst_test structure you are
initializing the test library for a second time, which overwrites the
some of the already initialized variables.

We probably want:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index 26f6510a0..971a184f2 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -196,6 +196,9 @@ void tst_reinit(void)
        size_t size = getpagesize();
        int fd;

+       if (ipc)
+               tst_brk(TBROK, "Test library already initialized!");
+
        if (!path)
                tst_brk(TBROK, IPC_ENV_VAR" is not defined");


So that this kind of mistake is caught early.

-- 
Cyril Hrubis
chrubis@suse.cz