test_kmod: fixes for v4.13-final
Subsystems (3)
Luis R. Rodriguez
maintainercover test_kmod: fixes for v4.13-final
Andrew,
Here are a few fixes I've queued up so far for test_kmod for the v4.13-final kernel release. These patches are also present on my kernel.org linux git tree on the 20170801-kmod-for-v4.13-final branch [0].
Please let me know if there any questions or issues.
Colin Ian King (1):
test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY"
Dan Carpenter (2):
test_kmod: fix the lock in register_test_dev_kmod()
test_kmod: fix small memory leak on filesystem tests
Luis R. Rodriguez (2):
test_kmod: make selftest executable
test_kmod: fix bug which allows negative values on two config options
lib/test_kmod.c | 16 ++++++++-------- tools/testing/selftests/kmod/kmod.sh | 0 2 files changed, 8 insertions(+), 8 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
Signature · 1 line
2.11.0
Luis R. Rodriguez
maintainerPATCH 1/5 test_kmod: make selftest executable
We had just forgotten to do this.
--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
tools/testing/selftests/kmod/kmod.sh +0 -0
Signature · 1 line
2.11.0
Andrew Morton
maintainerRe: [PATCH 1/5] test_kmod: make selftest executable
tools/testing/selftests/kmod/kmod.sh
Quoted 12 lines · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.shtools/testing/selftests/kmod/kmod.sh +0
-0
This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
It's more robust to not care about the x bit at all. Something like this?
tools/testing/selftests/lib.mk renamed from tools/testing/selftests/lib.mk~a +1 -1
@@ -14,7 +14,7 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_ define RUN_TESTS @for TEST in $(TEST_GEN_PROGS) $(TEST_PROGS); do \ BASENAME_TEST=`basename $$TEST`; \ - cd `dirname $$TEST`; (./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ + cd `dirname $$TEST`; (/bin/sh ./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ done; endef
(probably incomplete, should presumably use $SHELL or something)
Luis R. Rodriguez
maintainerRe: [PATCH 1/5] test_kmod: make selftest executable
Quoted 11 lines · Andrew Morton
Quoted 9 lines · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
Quoted 7 lines
tools/testing/selftests/kmod/kmod.sh
Quoted 3 lines
tools/testing/selftests/kmod/kmod.sh +0
-0This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
True if using regular diff, if using git, it will catch it though.
Quoted 15 lines
It's more robust to not care about the x bit at all. Something like this?
tools/testing/selftests/lib.mk renamed from tools/testing/selftests/lib.mk~a +1
-1@@ -14,7 +14,7 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_ define RUN_TESTS @for TEST in $(TEST_GEN_PROGS) $(TEST_PROGS); do \ BASENAME_TEST=`basename $$TEST`; \ - cd `dirname $$TEST`; (./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ + cd `dirname $$TEST`; (/bin/sh ./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ done; endef
Quoted 1 line
(probably incomplete, should presumably use $SHELL or something)
Probably a good idea indeed, Shuah ?
Although I'll note I also do like to run selftest scripts on my own too, so the chmod is still desirable.
Luis
Andrew Morton
maintainerRe: [PATCH 1/5] test_kmod: make selftest executable
Quoted 20 lines · On Wed, 2 Aug 2017 15:55:50 -0700 "Luis R. Rodriguez" <[email protected]> wrote:
Quoted 11 lines · Andrew Morton
Quoted 9 lines · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.shQuoted 7 lines
tools/testing/selftests/kmod/kmod.sh
Quoted 3 lines
tools/testing/selftests/kmod/kmod.sh +0
-0This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
Quoted 1 line
True if using regular diff, if using git, it will catch it though.
Yeah. I don't think this change will make it through my maze-of-scripts. Perhaps Shuah can pick it up.
Also...
# find tools/testing/selftests -name "*.sh" -a ! -executable tools/testing/selftests/rcutorture/configs/rcuperf/ver_functions.sh tools/testing/selftests/rcutorture/configs/lock/ver_functions.sh tools/testing/selftests/rcutorture/configs/rcu/ver_functions.sh tools/testing/selftests/rcutorture/bin/functions.sh tools/testing/selftests/locking/ww_mutex.sh tools/testing/selftests/kmod/kmod.sh tools/testing/selftests/sysctl/sysctl.sh
Luis R. Rodriguez
maintainerRe: [PATCH 1/5] test_kmod: make selftest executable
Quoted 22 lines · Andrew Morton
Quoted 20 lines · On Wed, 2 Aug 2017 15:55:50 -0700 "Luis R. Rodriguez" <[email protected]> wrote:
Quoted 18 lines · Andrew Morton
tools/testing/selftests/kmod/kmod.sh
Quoted 12 lines · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.shtools/testing/selftests/kmod/kmod.sh +0
-0This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
Quoted 4 lines
Quoted 1 line
True if using regular diff, if using git, it will catch it though.
Yeah. I don't think this change will make it through my maze-of-scripts. Perhaps Shuah can pick it up.
Ah sure, Shuah, can this go through your tree?
Quoted 8 lines
Also...
# find tools/testing/selftests -name "*.sh" -a ! -executable tools/testing/selftests/rcutorture/configs/rcuperf/ver_functions.sh tools/testing/selftests/rcutorture/configs/lock/ver_functions.sh tools/testing/selftests/rcutorture/configs/rcu/ver_functions.sh tools/testing/selftests/rcutorture/bin/functions.sh tools/testing/selftests/locking/ww_mutex.sh
Best is we embrace your suggested change indeed.
Quoted 1 line
tools/testing/selftests/kmod/kmod.sh
That's this one.
Quoted 1 line
tools/testing/selftests/sysctl/sysctl.sh
and I can follow up with Shuah on this one too.
Luis
Shuah Khan
maintainerRe: [PATCH 1/5] test_kmod: make selftest executable
In reply to Andrew MortonQuoted 22 lines · Andrew Morton
Quoted 20 lines · On Wed, 2 Aug 2017 15:55:50 -0700 "Luis R. Rodriguez" <[email protected]> wrote:
Quoted 18 lines · Andrew Morton
tools/testing/selftests/kmod/kmod.sh
Quoted 12 lines · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.shtools/testing/selftests/kmod/kmod.sh +0
-0This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
Quoted 4 lines
Quoted 1 line
True if using regular diff, if using git, it will catch it though.
Yeah. I don't think this change will make it through my maze-of-scripts. Perhaps Shuah can pick it up.
Thanks Andrew. I will pick this up.
Quoted 9 lines
Also...
# find tools/testing/selftests -name "*.sh" -a ! -executable tools/testing/selftests/rcutorture/configs/rcuperf/ver_functions.sh tools/testing/selftests/rcutorture/configs/lock/ver_functions.sh tools/testing/selftests/rcutorture/configs/rcu/ver_functions.sh tools/testing/selftests/rcutorture/bin/functions.sh tools/testing/selftests/locking/ww_mutex.sh
The above aren't an issue. They aren't included in the selftests/Makefile
Quoted 2 lines
tools/testing/selftests/kmod/kmod.shtools/testing/selftests/sysctl/sysctl.sh
Yeah. sysctl needs to be fixed. Thanks for pointing this out.
thanks,
-- Shuah
Shuah Khan
maintainerRe: [PATCH 1/5] test_kmod: make selftest executable
In reply to Luis R. RodriguezHi Luis,
Quoted 4 lines · Luis R. Rodriguez
Quoted 3 lines · Andrew Morton
Quoted 1 line · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Could you please include the real reason you need this change. My guess is, you are seeing this failure.
make -C kmod/ run_tests
make: Entering directory '/lkml/linux_4.13/tools/testing/selftests/kmod' /bin/sh: 1: ./kmod.sh: Permission denied
selftests: kmod.sh [FAIL]
Quoted 33 lines
Quoted 8 lines
Quoted 8 lines
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.shQuoted 7 lines
tools/testing/selftests/kmod/kmod.sh
Quoted 3 lines
tools/testing/selftests/kmod/kmod.sh +0
-0This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
True if using regular diff, if using git, it will catch it though.
Quoted 15 lines
It's more robust to not care about the x bit at all. Something like this?
tools/testing/selftests/lib.mk renamed from tools/testing/selftests/lib.mk~a +1
-1@@ -14,7 +14,7 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_ define RUN_TESTS @for TEST in $(TEST_GEN_PROGS) $(TEST_PROGS); do \ BASENAME_TEST=`basename $$TEST`; \ - cd `dirname $$TEST`; (./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ + cd `dirname $$TEST`; (/bin/sh ./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ done; endef
Quoted 3 lines
Quoted 1 line
(probably incomplete, should presumably use $SHELL or something)
Probably a good idea indeed, Shuah ?
I think this is a good idea. At the moment any script without +x fails. It would be good change to make in the generic layer.
Quoted 3 lines
Although I'll note I also do like to run selftest scripts on my own too, so the chmod is still desirable.
You will have to run it under bash kmod/kmod.sh - It can be painful.
This is one of the reasons a lot of the scripts are executable. I am curious how you didn't catch this before.
I can take this one. Please fix the change log though. That will explain why this change is made.
thanks,
-- Shuah
Re: [PATCH 1/5] test_kmod: make selftest executable
In reply to Andrew MortonQuoted 25 lines · Andrew Morton
tools/testing/selftests/kmod/kmod.sh
Quoted 12 lines · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.shtools/testing/selftests/kmod/kmod.sh +0
-0This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
It's more robust to not care about the x bit at all. Something like this?
tools/testing/selftests/lib.mk all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_ · line 19
Quoted 7 lines
tools/testing/selftests/lib.mk renamed from tools/testing/selftests/lib.mk~a +1
-1@@ -14,7 +14,7 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_ define RUN_TESTS @for TEST in $(TEST_GEN_PROGS) $(TEST_PROGS); do \ BASENAME_TEST=`basename $$TEST`; \ - cd `dirname $$TEST`; (./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ + cd `dirname $$TEST`; (/bin/sh ./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ done; endef
Most (many?) of the tests aren't shell scripts, so that doesn't work, eg:
$ make -C tools/testing/selftests/ TARGETS=kcmp run_tests
...
make[1]: Entering directory '/home/michael/linus/tools/testing/selftests/kcmp'
./kcmp_test: 1: ./kcmp_test: ELF...0.@�.@@..@@.@.�.�..........����.��.�.�...�.�..�.�.�.....DD.Q�td.R�td.����.��../lib64/ld64.so.2..GNU...GNUC{: not found
./kcmp_test: 13: ./kcmp_test: Syntax error: Unterminated quoted string
selftests: kcmp_test [FAIL]Also some of the ones which are shell scripts require bash, and /bin/sh may not be bash, eg:
$ make -C tools/testing/selftests/ TARGETS=cpu-hotplug run_tests ... make[1]: Entering directory '/home/michael/linus/tools/testing/selftests/cpu-hotplug' ./cpu-on-off-test.sh: 9: [: !=: unexpected operator
cheers
Luis R. Rodriguez
maintainerRe: [PATCH 1/5] test_kmod: make selftest executable
Quoted 44 lines · Michael Ellerman
Quoted 25 lines · Andrew Morton
tools/testing/selftests/kmod/kmod.sh
Quoted 12 lines · On Wed, 2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <[email protected]>…
We had just forgotten to do this.
Fixes 39258f448d71 ("kmod: add test driver to stress test the module loader")Signed-off-by Luis R. Rodriguez <[email protected]>--- tools/testing/selftests/kmod/kmod.sh | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.shtools/testing/selftests/kmod/kmod.sh +0
-0This is pretty fragile - I'm not sure that patch/diff are capable of communicating a bare chmod. If someone does a "patch -p1 < patch-4.14" or whatever, this change is likely to get lost.
It's more robust to not care about the x bit at all. Something like this?
tools/testing/selftests/lib.mk all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_ · line 19
Quoted 7 lines
tools/testing/selftests/lib.mk renamed from tools/testing/selftests/lib.mk~a +1
-1@@ -14,7 +14,7 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_ define RUN_TESTS @for TEST in $(TEST_GEN_PROGS) $(TEST_PROGS); do \ BASENAME_TEST=`basename $$TEST`; \ - cd `dirname $$TEST`; (./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ + cd `dirname $$TEST`; (/bin/sh ./$$BASENAME_TEST && echo "selftests: $$BASENAME_TEST [PASS]") || echo "selftests: $$BASENAME_TEST [FAIL]"; cd -;\ done; endefMost (many?) of the tests aren't shell scripts, so that doesn't work, eg:
$ make -C tools/testing/selftests/ TARGETS=kcmp run_tests ... make[1]: Entering directory '/home/michael/linus/tools/testing/selftests/kcmp' ./kcmp_test: 1: ./kcmp_test: ELF...0.@�.@@..@@.@.�.�..........����.��.�.�...�.�..�.�.�.....DD.Q�td.R�td.����.��../lib64/ld64.so.2..GNU...GNUC{: not found ./kcmp_test: 13: ./kcmp_test: Syntax error: Unterminated quoted string selftests: kcmp_test [FAIL]
Quoted 7 lines
Also some of the ones which are shell scripts require bash, and
/bin/shmay not be bash, eg:$ make -C tools/testing/selftests/ TARGETS=cpu-hotplug run_tests ... make[1]: Entering directory '/home/michael/linus/tools/testing/selftests/cpu-hotplug' ./cpu-on-off-test.sh: 9: [: !=: unexpected operator
Which is precisely why I decided to just issue a warning instead:
https://lkml.kernel.org/r/[email protected]
Luis
Luis R. Rodriguez
maintainerPATCH 2/5 test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY"
In reply to Luis R. RodriguezFrom: Colin Ian King <[email protected]>
Trivial fix to spelling mistake in snprintf text
--- lib/test_kmod.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
lib/test_kmod.c +2 -2
@@ -485,7 +485,7 @@ static ssize_t config_show(struct device *dev, config->test_driver); else len += snprintf(buf+len, PAGE_SIZE - len, - "driver:\tEMTPY\n"); + "driver:\tEMPTY\n");if (config->test_fs) len += snprintf(buf+len, PAGE_SIZE - len,
@@ -493,7 +493,7 @@ static ssize_t config_show(struct device *dev, config->test_fs); else len += snprintf(buf+len, PAGE_SIZE - len, - "fs:\tEMTPY\n"); + "fs:\tEMPTY\n");mutex_unlock (&test_dev->config_mutex);
Signature · 1 line
2.11.0
Luis R. Rodriguez
maintainerPATCH 3/5 test_kmod: fix bug which allows negative values on two config options
In reply to Luis R. RodriguezParsing with kstrtol() enables values to be negative, and we failed to check for negative values when parsing with either test_dev_config_update_uint_sync() or test_dev_config_update_uint_range().
test_dev_config_update_uint_range() has a minimum check though so an issue is not present there. test_dev_config_update_uint_sync() is only used for the number of threads to use (config_num_threads_store()), and indeed this would fail with an attempt for a large allocation.
Although the issue is only present in practice with the first fix both by using kstrtoul() instead of kstrtol().
--- lib/test_kmod.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
lib/test_kmod.c +4 -4
@@ -880,10 +880,10 @@ static int test_dev_config_update_uint_sync(struct kmod_test_device *test_dev, int (*test_sync)(struct kmod_test_device *test_dev)) {int ret; -long new; +unsigned long new;unsigned int old_val; - ret =kstrtol (buf,10 , &new); + ret =kstrtoul (buf,10 , &new);if (ret)return ret;
@@ -918,9 +918,9 @@ static int test_dev_config_update_uint_range(struct kmod_test_device *test_dev, unsigned int max) {int ret; -long new; +unsigned long new; - ret =kstrtol (buf,10 , &new); + ret =kstrtoul (buf,10 , &new);if (ret)return ret;
Signature · 1 line
2.11.0
Luis R. Rodriguez
maintainerPATCH 4/5 test_kmod: fix the lock in register_test_dev_kmod()
In reply to Luis R. RodriguezFrom: Dan Carpenter <[email protected]>
We accidentally just drop the lock twice instead of taking it and then releasing it. This isn't a big issue unless you are adding more than one device to test on, and the kmod.sh doesn't do that yet, however this obviously is the correct thing to do.
--- lib/test_kmod.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/test_kmod.c +1 -1
@@ -1146,7 +1146,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)struct kmod_test_device *test_dev =NULL ;int ret; -mutex_unlock (®_dev_mutex); +mutex_lock (®_dev_mutex);/* int should suffice for number of devices, test for wrap */ if (unlikely (num_test_devs +1 ) <0 ) {
Signature · 1 line
2.11.0
Luis R. Rodriguez
maintainerPATCH 5/5 test_kmod: fix small memory leak on filesystem tests
In reply to Luis R. RodriguezFrom: Dan Carpenter <[email protected]>
The break was in the wrong place so file system tests don't work as intended, leaking memory at each test switch.
--- lib/test_kmod.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/test_kmod.c +1 -1
@@ -746,11 +746,11 @@ static int trigger_config_run_type(struct kmod_test_device *test_dev, strlen(test_str));break ;case TEST_KMOD_FS_TYPE : -break ;kfree_const (config->test_fs); config->test_driver =NULL ; copied =config_copy_test_fs (config, test_str,strlen (test_str)); +break ;default :mutex_unlock (&test_dev->config_mutex);return -EINVAL ;
Signature · 1 line
2.11.0