All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] test_kmod: fixes for v4.13-final
@ 2017-08-02 21:14 Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 1/5] test_kmod: make selftest executable Luis R. Rodriguez
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 21:14 UTC (permalink / raw)
  To: akpm
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel, Luis R. Rodriguez

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.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20170801-kmod-for-v4.13-final

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

-- 
2.11.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 21:14 [PATCH 0/5] test_kmod: fixes for v4.13-final Luis R. Rodriguez
@ 2017-08-02 21:14 ` Luis R. Rodriguez
  2017-08-02 22:43   ` Andrew Morton
  2017-08-02 21:14 ` [PATCH 2/5] test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY" Luis R. Rodriguez
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 21:14 UTC (permalink / raw)
  To: akpm
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel, Luis R. Rodriguez

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 <mcgrof@kernel.org>
---
 tools/testing/selftests/kmod/kmod.sh | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh

diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
old mode 100644
new mode 100755
-- 
2.11.0

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/5] test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY"
  2017-08-02 21:14 [PATCH 0/5] test_kmod: fixes for v4.13-final Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 1/5] test_kmod: make selftest executable Luis R. Rodriguez
@ 2017-08-02 21:14 ` Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 3/5] test_kmod: fix bug which allows negative values on two config options Luis R. Rodriguez
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 21:14 UTC (permalink / raw)
  To: akpm
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel, Luis R . Rodriguez

From: Colin Ian King <colin.king@canonical.com>

Trivial fix to spelling mistake in snprintf text

Signed-off-by: Colin Ian King <colin.king@canonical.com>
[mcgrof: massaged commit message]
Fixes: 39258f448d71 ("kmod: add test driver to stress test the module loader")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/test_kmod.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index 6c1d678bcf8b..90c91541fc16 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -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);
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] test_kmod: fix bug which allows negative values on two config options
  2017-08-02 21:14 [PATCH 0/5] test_kmod: fixes for v4.13-final Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 1/5] test_kmod: make selftest executable Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 2/5] test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY" Luis R. Rodriguez
@ 2017-08-02 21:14 ` Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 4/5] test_kmod: fix the lock in register_test_dev_kmod() Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 5/5] test_kmod: fix small memory leak on filesystem tests Luis R. Rodriguez
  4 siblings, 0 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 21:14 UTC (permalink / raw)
  To: akpm
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel, Luis R. Rodriguez

Parsing 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().

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Fixes: 39258f448d71 ("kmod: add test driver to stress test the module loader")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/test_kmod.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index 90c91541fc16..8fc0a7a19c83 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -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;
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] test_kmod: fix the lock in register_test_dev_kmod()
  2017-08-02 21:14 [PATCH 0/5] test_kmod: fixes for v4.13-final Luis R. Rodriguez
                   ` (2 preceding siblings ...)
  2017-08-02 21:14 ` [PATCH 3/5] test_kmod: fix bug which allows negative values on two config options Luis R. Rodriguez
@ 2017-08-02 21:14 ` Luis R. Rodriguez
  2017-08-02 21:14 ` [PATCH 5/5] test_kmod: fix small memory leak on filesystem tests Luis R. Rodriguez
  4 siblings, 0 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 21:14 UTC (permalink / raw)
  To: akpm
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel, Luis R . Rodriguez

From: Dan Carpenter <dan.carpenter@oracle.com>

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.

Fixes: 39258f448d71 ("kmod: add test driver to stress test the module loader")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
[mcgrof: massaged subject, explain what happens]
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/test_kmod.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index 8fc0a7a19c83..1bc06bbfc97a 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -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(&reg_dev_mutex);
+	mutex_lock(&reg_dev_mutex);
 
 	/* int should suffice for number of devices, test for wrap */
 	if (unlikely(num_test_devs + 1) < 0) {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] test_kmod: fix small memory leak on filesystem tests
  2017-08-02 21:14 [PATCH 0/5] test_kmod: fixes for v4.13-final Luis R. Rodriguez
                   ` (3 preceding siblings ...)
  2017-08-02 21:14 ` [PATCH 4/5] test_kmod: fix the lock in register_test_dev_kmod() Luis R. Rodriguez
@ 2017-08-02 21:14 ` Luis R. Rodriguez
  4 siblings, 0 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 21:14 UTC (permalink / raw)
  To: akpm
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel, Luis R . Rodriguez

From: Dan Carpenter <dan.carpenter@oracle.com>

The break was in the wrong place so file system tests don't work as
intended, leaking memory at each test switch.

Fixes: 39258f448d71 ("kmod: add test driver to stress test the module loader")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reported-by: David Binderman <dcb314@hotmail.com>
[mcgrof: massaged commit subject, noted memory leak issue without the fix]
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 lib/test_kmod.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index 1bc06bbfc97a..ff9148969b92 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -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;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 21:14 ` [PATCH 1/5] test_kmod: make selftest executable Luis R. Rodriguez
@ 2017-08-02 22:43   ` Andrew Morton
  2017-08-02 22:55     ` Luis R. Rodriguez
  2017-08-08  9:50     ` Michael Ellerman
  0 siblings, 2 replies; 14+ messages in thread
From: Andrew Morton @ 2017-08-02 22:43 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel

On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:

> 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 <mcgrof@kernel.org>
> ---
>  tools/testing/selftests/kmod/kmod.sh | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
> 
> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
> old mode 100644
> new mode 100755

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?

--- a/tools/testing/selftests/lib.mk~a
+++ a/tools/testing/selftests/lib.mk
@@ -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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 22:43   ` Andrew Morton
@ 2017-08-02 22:55     ` Luis R. Rodriguez
  2017-08-02 23:42       ` Andrew Morton
  2017-08-02 23:57       ` Shuah Khan
  2017-08-08  9:50     ` Michael Ellerman
  1 sibling, 2 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 22:55 UTC (permalink / raw)
  To: Andrew Morton, shuah
  Cc: Dmitry Torokhov, Kees Cook, Jessica Yu, Rusty Russell,
	Michal Marek, Petr Mladek, Miroslav Benes, Josh Poimboeuf,
	Eric W. Biederman, Dan Carpenter, Colin Ian King, dcb314,
	linux-kselftest, linux-kernel

On Wed, Aug 2, 2017 at 3:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>
>> 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 <mcgrof@kernel.org>
>> ---
>>  tools/testing/selftests/kmod/kmod.sh | 0
>>  1 file changed, 0 insertions(+), 0 deletions(-)
>>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
>>
>> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
>> old mode 100644
>> new mode 100755
>
> 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.

True if using regular diff, if using git, it will catch it though.

> It's more robust to not care about the x bit at all.  Something like
> this?
>
> --- a/tools/testing/selftests/lib.mk~a
> +++ a/tools/testing/selftests/lib.mk
> @@ -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)

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 22:55     ` Luis R. Rodriguez
@ 2017-08-02 23:42       ` Andrew Morton
  2017-08-02 23:46         ` Luis R. Rodriguez
  2017-08-03  0:01         ` Shuah Khan
  2017-08-02 23:57       ` Shuah Khan
  1 sibling, 2 replies; 14+ messages in thread
From: Andrew Morton @ 2017-08-02 23:42 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: shuah, Dmitry Torokhov, Kees Cook, Jessica Yu, Rusty Russell,
	Michal Marek, Petr Mladek, Miroslav Benes, Josh Poimboeuf,
	Eric W. Biederman, Dan Carpenter, Colin Ian King, dcb314,
	linux-kselftest, linux-kernel

On Wed, 2 Aug 2017 15:55:50 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:

> On Wed, Aug 2, 2017 at 3:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> >
> >> 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 <mcgrof@kernel.org>
> >> ---
> >>  tools/testing/selftests/kmod/kmod.sh | 0
> >>  1 file changed, 0 insertions(+), 0 deletions(-)
> >>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
> >>
> >> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
> >> old mode 100644
> >> new mode 100755
> >
> > 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.
> 
> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 23:42       ` Andrew Morton
@ 2017-08-02 23:46         ` Luis R. Rodriguez
  2017-08-03  0:01         ` Shuah Khan
  1 sibling, 0 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 23:46 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Luis R. Rodriguez, shuah, Dmitry Torokhov, Kees Cook, Jessica Yu,
	Rusty Russell, Michal Marek, Petr Mladek, Miroslav Benes,
	Josh Poimboeuf, Eric W. Biederman, Dan Carpenter, Colin Ian King,
	dcb314, linux-kselftest, linux-kernel

On Wed, Aug 02, 2017 at 04:42:50PM -0700, Andrew Morton wrote:
> On Wed, 2 Aug 2017 15:55:50 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> 
> > On Wed, Aug 2, 2017 at 3:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > > On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> > >
> > >> 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 <mcgrof@kernel.org>
> > >> ---
> > >>  tools/testing/selftests/kmod/kmod.sh | 0
> > >>  1 file changed, 0 insertions(+), 0 deletions(-)
> > >>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
> > >>
> > >> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
> > >> old mode 100644
> > >> new mode 100755
> > >
> > > 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.
> > 
> > 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?

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

> tools/testing/selftests/kmod/kmod.sh

That's this one.

> tools/testing/selftests/sysctl/sysctl.sh

and I can follow up with Shuah on this one too.

  Luis

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 22:55     ` Luis R. Rodriguez
  2017-08-02 23:42       ` Andrew Morton
@ 2017-08-02 23:57       ` Shuah Khan
  1 sibling, 0 replies; 14+ messages in thread
From: Shuah Khan @ 2017-08-02 23:57 UTC (permalink / raw)
  To: Luis R. Rodriguez, Andrew Morton
  Cc: Dmitry Torokhov, Kees Cook, Jessica Yu, Rusty Russell,
	Michal Marek, Petr Mladek, Miroslav Benes, Josh Poimboeuf,
	Eric W. Biederman, Dan Carpenter, Colin Ian King, dcb314,
	linux-kselftest, linux-kernel, Shuah Khan

Hi Luis,

On 08/02/2017 04:55 PM, Luis R. Rodriguez wrote:
> On Wed, Aug 2, 2017 at 3:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>> On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>>
>>> 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]


>>>
>>> Fixes: 39258f448d71 ("kmod: add test driver to stress test the module loader")
>>> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
>>> ---
>>>  tools/testing/selftests/kmod/kmod.sh | 0
>>>  1 file changed, 0 insertions(+), 0 deletions(-)
>>>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
>>>
>>> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
>>> old mode 100644
>>> new mode 100755
>>
>> 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.
> 
> True if using regular diff, if using git, it will catch it though.
> 
>> It's more robust to not care about the x bit at all.  Something like
>> this?
>>
>> --- a/tools/testing/selftests/lib.mk~a
>> +++ a/tools/testing/selftests/lib.mk
>> @@ -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)
> 
> 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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 23:42       ` Andrew Morton
  2017-08-02 23:46         ` Luis R. Rodriguez
@ 2017-08-03  0:01         ` Shuah Khan
  1 sibling, 0 replies; 14+ messages in thread
From: Shuah Khan @ 2017-08-03  0:01 UTC (permalink / raw)
  To: Andrew Morton, Luis R. Rodriguez
  Cc: Dmitry Torokhov, Kees Cook, Jessica Yu, Rusty Russell,
	Michal Marek, Petr Mladek, Miroslav Benes, Josh Poimboeuf,
	Eric W. Biederman, Dan Carpenter, Colin Ian King, dcb314,
	linux-kselftest, linux-kernel, Shuah Khan


On 08/02/2017 05:42 PM, Andrew Morton wrote:
> On Wed, 2 Aug 2017 15:55:50 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> 
>> On Wed, Aug 2, 2017 at 3:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
>>> On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>>>
>>>> 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 <mcgrof@kernel.org>
>>>> ---
>>>>  tools/testing/selftests/kmod/kmod.sh | 0
>>>>  1 file changed, 0 insertions(+), 0 deletions(-)
>>>>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
>>>>
>>>> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
>>>> old mode 100644
>>>> new mode 100755
>>>
>>> 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.
>>
>> 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.

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

> tools/testing/selftests/kmod/kmod.sh
> tools/testing/selftests/sysctl/sysctl.sh

Yeah. sysctl needs to be fixed. Thanks for pointing this out.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-02 22:43   ` Andrew Morton
  2017-08-02 22:55     ` Luis R. Rodriguez
@ 2017-08-08  9:50     ` Michael Ellerman
  2017-08-08 17:52       ` Luis R. Rodriguez
  1 sibling, 1 reply; 14+ messages in thread
From: Michael Ellerman @ 2017-08-08  9:50 UTC (permalink / raw)
  To: Andrew Morton, Luis R. Rodriguez
  Cc: dmitry.torokhov, keescook, jeyu, rusty, mmarek, pmladek, mbenes,
	jpoimboe, ebiederm, shuah, dan.carpenter, colin.king, dcb314,
	linux-kselftest, linux-kernel

Andrew Morton <akpm@linux-foundation.org> writes:
> On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>
>> 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 <mcgrof@kernel.org>
>> ---
>>  tools/testing/selftests/kmod/kmod.sh | 0
>>  1 file changed, 0 insertions(+), 0 deletions(-)
>>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
>> 
>> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
>> old mode 100644
>> new mode 100755
>
> 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?
>
> --- a/tools/testing/selftests/lib.mk~a
> +++ a/tools/testing/selftests/lib.mk
> @@ -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.\x01.\x02.\x010.@�.\x02@@.\x19.@@.@.�.�.\x04.\x02.\x02.\x11.\x01.\x10.\x04.\x04.\x01.\x06����.\x10��.\x10�.�.\x01.\x06.�.�.\x10.�.\x10�.�.\x04.\x02.\x02.\x14.\x10DD.Q�td.\x10R�td.����.\x10��.\x03./lib64/ld64.so.2.\x10.GNU.\x02.\x14.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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] test_kmod: make selftest executable
  2017-08-08  9:50     ` Michael Ellerman
@ 2017-08-08 17:52       ` Luis R. Rodriguez
  0 siblings, 0 replies; 14+ messages in thread
From: Luis R. Rodriguez @ 2017-08-08 17:52 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Andrew Morton, Luis R. Rodriguez, dmitry.torokhov, keescook,
	jeyu, rusty, mmarek, pmladek, mbenes, jpoimboe, ebiederm, shuah,
	dan.carpenter, colin.king, dcb314, linux-kselftest, linux-kernel

On Tue, Aug 08, 2017 at 07:50:04PM +1000, Michael Ellerman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> > On Wed,  2 Aug 2017 14:14:46 -0700 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> >
> >> 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 <mcgrof@kernel.org>
> >> ---
> >>  tools/testing/selftests/kmod/kmod.sh | 0
> >>  1 file changed, 0 insertions(+), 0 deletions(-)
> >>  mode change 100644 => 100755 tools/testing/selftests/kmod/kmod.sh
> >> 
> >> diff --git a/tools/testing/selftests/kmod/kmod.sh b/tools/testing/selftests/kmod/kmod.sh
> >> old mode 100644
> >> new mode 100755
> >
> > 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?
> >
> > --- a/tools/testing/selftests/lib.mk~a
> > +++ a/tools/testing/selftests/lib.mk
> > @@ -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.\x01.\x02.\x010.@�.\x02@@.\x19.@@.@.�.�.\x04.\x02.\x02.\x11.\x01.\x10.\x04.\x04.\x01.\x06����.\x10��.\x10�.�.\x01.\x06.�.�.\x10.�.\x10�.�.\x04.\x02.\x02.\x14.\x10DD.Q�td.\x10R�td.����.\x10��.\x03./lib64/ld64.so.2.\x10.GNU.\x02.\x14.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

Which is precisely why I decided to just issue a warning instead:

https://lkml.kernel.org/r/20170803202436.6877-1-mcgrof@kernel.org

 Luis

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-08-08 17:52 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-02 21:14 [PATCH 0/5] test_kmod: fixes for v4.13-final Luis R. Rodriguez
2017-08-02 21:14 ` [PATCH 1/5] test_kmod: make selftest executable Luis R. Rodriguez
2017-08-02 22:43   ` Andrew Morton
2017-08-02 22:55     ` Luis R. Rodriguez
2017-08-02 23:42       ` Andrew Morton
2017-08-02 23:46         ` Luis R. Rodriguez
2017-08-03  0:01         ` Shuah Khan
2017-08-02 23:57       ` Shuah Khan
2017-08-08  9:50     ` Michael Ellerman
2017-08-08 17:52       ` Luis R. Rodriguez
2017-08-02 21:14 ` [PATCH 2/5] test_kmod: fix spelling mistake: "EMTPY" -> "EMPTY" Luis R. Rodriguez
2017-08-02 21:14 ` [PATCH 3/5] test_kmod: fix bug which allows negative values on two config options Luis R. Rodriguez
2017-08-02 21:14 ` [PATCH 4/5] test_kmod: fix the lock in register_test_dev_kmod() Luis R. Rodriguez
2017-08-02 21:14 ` [PATCH 5/5] test_kmod: fix small memory leak on filesystem tests Luis R. Rodriguez

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.