| Message ID | 20190405165225.27216-2-pvorel@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LTP reproducer on broken IMA on overlayfs | expand |
On Fri, 2019-04-05 at 18:52 +0200, Petr Vorel wrote: > to work the same way as setup > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > testcases/kernel/security/integrity/ima/tests/ima_setup.sh | 6 +++++- > .../kernel/security/integrity/ima/tests/ima_violations.sh | 2 -- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > index 52551190a..cbded42c2 100644 > --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > @@ -20,7 +20,8 @@ > TST_TESTFUNC="test" > TST_SETUP_CALLER="$TST_SETUP" > TST_SETUP="ima_setup" > -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}" > +TST_CLEANUP_CALLER="$TST_CLEANUP" > +TST_CLEANUP="ima_cleanup" It seems to be working, but defining TST_SETUP and TST_CLEANUP after defining the respective _CALLER looks strange. The _CALLER's string must be empty. > TST_NEEDS_TMPDIR=1 > TST_NEEDS_ROOT=1 > > @@ -95,6 +96,9 @@ ima_setup() > ima_cleanup() > { > local dir > + > + [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER > + Is something else setting TST_CLEANUP_CALLER? > for dir in $UMOUNT; do > umount $dir > done > diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh > index 74223c221..a44bd1230 100755 > --- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh > +++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh > @@ -51,8 +51,6 @@ cleanup() > { > [ "$PRINTK_RATE_LIMIT" != "0" ] && \ > sysctl -wq kernel.printk_ratelimit=$PRINTK_RATE_LIMIT > - > - ima_cleanup > } > > open_file_read()
Hi Mimi, thanks for your comments. ... > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > > @@ -20,7 +20,8 @@ > > TST_TESTFUNC="test" > > TST_SETUP_CALLER="$TST_SETUP" > > TST_SETUP="ima_setup" > > -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}" > > +TST_CLEANUP_CALLER="$TST_CLEANUP" > > +TST_CLEANUP="ima_cleanup" > It seems to be working, but defining TST_SETUP and TST_CLEANUP after > defining the respective _CALLER looks strange. The _CALLER's string > must be empty. TST_{SETUP,CALLER}_CALLER takes setup from the test. It's IMHO cleaner way allowing tests to set their setup/cleanup functions and not care that there is also some library setup/cleanup (kind of encapsulation). We already used this for setup, I wanted to have a same approach for both setup and cleanup. Sure I can instead add ima_setup/ima_cleanup into tests' setup/cleanup functions, but both solutions are working and I consider encapsulation as a benefit. The only problematic thing would be if some test needed to run it's custom cleanup *before* library one while other tests *after*. But that's not a case here. We also use this approach in tst_net.sh [1]. > > TST_NEEDS_TMPDIR=1 > > TST_NEEDS_ROOT=1 > > @@ -95,6 +96,9 @@ ima_setup() > > ima_cleanup() > > { > > local dir > > + > > + [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER > > + > Is something else setting TST_CLEANUP_CALLER? Kind regards, Petr [1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_net.sh#L11
On Thu, 2019-04-11 at 07:51 +0200, Petr Vorel wrote: > Hi Mimi, > > thanks for your comments. > > ... > > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > > > @@ -20,7 +20,8 @@ > > > TST_TESTFUNC="test" > > > TST_SETUP_CALLER="$TST_SETUP" > > > TST_SETUP="ima_setup" > > > -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}" > > > +TST_CLEANUP_CALLER="$TST_CLEANUP" > > > +TST_CLEANUP="ima_cleanup" > > > It seems to be working, but defining TST_SETUP and TST_CLEANUP after > > defining the respective _CALLER looks strange. The _CALLER's string > > must be empty. > TST_{SETUP,CALLER}_CALLER takes setup from the test. > It's IMHO cleaner way allowing tests to set their setup/cleanup functions and > not care that there is also some library setup/cleanup (kind of encapsulation). I'm not questioning the method for initializing this test. I guess I'm asking why bother to set TST_{SETUP,CLEANUP}_CALLER this way, if we know that it isn't set. Why not just initialize it as ""? Mimi > > We already used this for setup, I wanted to have a same approach for both setup > and cleanup. Sure I can instead add ima_setup/ima_cleanup into tests' setup/cleanup > functions, but both solutions are working and I consider encapsulation as a benefit. > The only problematic thing would be if some test needed to run it's custom > cleanup *before* library one while other tests *after*. But that's not a case here. > We also use this approach in tst_net.sh [1]. > > > > TST_NEEDS_TMPDIR=1 > > > TST_NEEDS_ROOT=1 > > > > @@ -95,6 +96,9 @@ ima_setup() > > > ima_cleanup() > > > { > > > local dir > > > + > > > + [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER > > > + > > > Is something else setting TST_CLEANUP_CALLER? > > > Kind regards, > Petr > > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_net.sh#L11 >
Hi Mimi, > > > > +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh > > > > @@ -20,7 +20,8 @@ > > > > TST_TESTFUNC="test" > > > > TST_SETUP_CALLER="$TST_SETUP" > > > > TST_SETUP="ima_setup" > > > > -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}" > > > > +TST_CLEANUP_CALLER="$TST_CLEANUP" > > > > +TST_CLEANUP="ima_cleanup" > > > It seems to be working, but defining TST_SETUP and TST_CLEANUP after > > > defining the respective _CALLER looks strange. The _CALLER's string > > > must be empty. > > TST_{SETUP,CALLER}_CALLER takes setup from the test. > > It's IMHO cleaner way allowing tests to set their setup/cleanup functions and > > not care that there is also some library setup/cleanup (kind of encapsulation). > I'm not questioning the method for initializing this test. I guess > I'm asking why bother to set TST_{SETUP,CLEANUP}_CALLER this way, if > we know that it isn't set. Why not just initialize it as ""? Sorry, I wasn't clear, TST_{SETUP,CLEANUP}_CALLER are set by (some) tests (as TST_{SETUP,CLEANUP}): $ git grep TST_SETUP= testcases/kernel/security/integrity/ima/tests/*.sh |grep -v ima_setup.sh testcases/kernel/security/integrity/ima/tests/evm_overlay.sh:TST_SETUP="setup" testcases/kernel/security/integrity/ima/tests/ima_measurements.sh:TST_SETUP="setup" testcases/kernel/security/integrity/ima/tests/ima_policy.sh:TST_SETUP="setup" testcases/kernel/security/integrity/ima/tests/ima_violations.sh:TST_SETUP="setup" $ git grep TST_CLEANUP= testcases/kernel/security/integrity/ima/tests/*.sh |grep -v ima_setup.sh testcases/kernel/security/integrity/ima/tests/evm_overlay.sh:TST_CLEANUP="cleanup" testcases/kernel/security/integrity/ima/tests/ima_violations.sh:TST_CLEANUP="cleanup" And this variables are set before loading ima_setup.sh. So TST_{SETUP,CLEANUP}_CALLER get value from tests (if defined), overwrites it with it's own function for tst_test.sh, where it calls them (if defined). Or am I missing something? > Mimi Kind regards, Petr
diff --git a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh index 52551190a..cbded42c2 100644 --- a/testcases/kernel/security/integrity/ima/tests/ima_setup.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_setup.sh @@ -20,7 +20,8 @@ TST_TESTFUNC="test" TST_SETUP_CALLER="$TST_SETUP" TST_SETUP="ima_setup" -TST_CLEANUP="${TST_CLEANUP:-ima_cleanup}" +TST_CLEANUP_CALLER="$TST_CLEANUP" +TST_CLEANUP="ima_cleanup" TST_NEEDS_TMPDIR=1 TST_NEEDS_ROOT=1 @@ -95,6 +96,9 @@ ima_setup() ima_cleanup() { local dir + + [ -n "$TST_CLEANUP_CALLER" ] && $TST_CLEANUP_CALLER + for dir in $UMOUNT; do umount $dir done diff --git a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh index 74223c221..a44bd1230 100755 --- a/testcases/kernel/security/integrity/ima/tests/ima_violations.sh +++ b/testcases/kernel/security/integrity/ima/tests/ima_violations.sh @@ -51,8 +51,6 @@ cleanup() { [ "$PRINTK_RATE_LIMIT" != "0" ] && \ sysctl -wq kernel.printk_ratelimit=$PRINTK_RATE_LIMIT - - ima_cleanup } open_file_read()
to work the same way as setup Signed-off-by: Petr Vorel <pvorel@suse.cz> --- testcases/kernel/security/integrity/ima/tests/ima_setup.sh | 6 +++++- .../kernel/security/integrity/ima/tests/ima_violations.sh | 2 -- 2 files changed, 5 insertions(+), 3 deletions(-)