linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] kunit: test: Make filter strings in executor_test writable
@ 2023-08-30  0:21 David Gow
  2023-08-30  1:44 ` Guenter Roeck
  2023-08-30 21:34 ` Rae Moar
  0 siblings, 2 replies; 3+ messages in thread
From: David Gow @ 2023-08-30  0:21 UTC (permalink / raw)
  To: Shuah Khan, Rae Moar, Guenter Roeck
  Cc: Brendan Higgins, kunit-dev, linux-kselftest, linux-kernel, David Gow

KUnit's attribute filtering feature needs the filter strings passed in
to be writable, as it modifies them in-place during parsing. This works
for the filters passed on the kernel command line, but the string
literals used in the executor tests are at least theoretically read-only
(though they work on x86_64 for some reason). s390 wasn't fooled, and
crashed when these tests were run.

Use a 'char[]' instead, (and make an explicit variable for the current
filter in parse_filter_attr_test), which will store the string in a
writable segment.

Fixes: 76066f93f1df ("kunit: add tests for filtering attributes")
Closes: https://lore.kernel.org/linux-kselftest/55950256-c00a-4d21-a2c0-cf9f0e5b8a9a@roeck-us.net/
Signed-off-by: David Gow <davidgow@google.com>
---
 lib/kunit/executor_test.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index 4084071d0eb5..b4f6f96b2844 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -119,7 +119,7 @@ static void parse_filter_attr_test(struct kunit *test)
 {
 	int j, filter_count;
 	struct kunit_attr_filter *parsed_filters;
-	char *filters = "speed>slow, module!=example";
+	char filters[] = "speed>slow, module!=example", *filter = filters;
 	int err = 0;
 
 	filter_count = kunit_get_filter_count(filters);
@@ -128,7 +128,7 @@ static void parse_filter_attr_test(struct kunit *test)
 	parsed_filters = kunit_kcalloc(test, filter_count, sizeof(*parsed_filters),
 			GFP_KERNEL);
 	for (j = 0; j < filter_count; j++) {
-		parsed_filters[j] = kunit_next_attr_filter(&filters, &err);
+		parsed_filters[j] = kunit_next_attr_filter(&filter, &err);
 		KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter '%s'", filters[j]);
 	}
 
@@ -154,6 +154,7 @@ static void filter_attr_test(struct kunit *test)
 		.start = subsuite, .end = &subsuite[2],
 	};
 	struct kunit_suite_set got;
+	char filter[] = "speed>slow";
 	int err = 0;
 
 	subsuite[0] = alloc_fake_suite(test, "normal_suite", dummy_attr_test_cases);
@@ -168,7 +169,7 @@ static void filter_attr_test(struct kunit *test)
 	 * attribute is unset and thus, the filtering is based on the parent attribute
 	 * of slow.
 	 */
-	got = kunit_filter_suites(&suite_set, NULL, "speed>slow", NULL, &err);
+	got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
 	KUNIT_ASSERT_EQ(test, err, 0);
 	kfree_at_end(test, got.start);
@@ -191,12 +192,13 @@ static void filter_attr_empty_test(struct kunit *test)
 		.start = subsuite, .end = &subsuite[2],
 	};
 	struct kunit_suite_set got;
+	char filter[] = "module!=dummy";
 	int err = 0;
 
 	subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
 	subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
 
-	got = kunit_filter_suites(&suite_set, NULL, "module!=dummy", NULL, &err);
+	got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
 	KUNIT_ASSERT_EQ(test, err, 0);
 	kfree_at_end(test, got.start); /* just in case */
 
@@ -211,12 +213,13 @@ static void filter_attr_skip_test(struct kunit *test)
 		.start = subsuite, .end = &subsuite[1],
 	};
 	struct kunit_suite_set got;
+	char filter[] = "speed>slow";
 	int err = 0;
 
 	subsuite[0] = alloc_fake_suite(test, "suite", dummy_attr_test_cases);
 
 	/* Want: suite(slow, normal), NULL -> suite(slow with SKIP, normal), NULL */
-	got = kunit_filter_suites(&suite_set, NULL, "speed>slow", "skip", &err);
+	got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
 	KUNIT_ASSERT_EQ(test, err, 0);
 	kfree_at_end(test, got.start);
-- 
2.42.0.283.g2d96d420d3-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: test: Make filter strings in executor_test writable
  2023-08-30  0:21 [PATCH] kunit: test: Make filter strings in executor_test writable David Gow
@ 2023-08-30  1:44 ` Guenter Roeck
  2023-08-30 21:34 ` Rae Moar
  1 sibling, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2023-08-30  1:44 UTC (permalink / raw)
  To: David Gow
  Cc: Shuah Khan, Rae Moar, Brendan Higgins, kunit-dev,
	linux-kselftest, linux-kernel

On Wed, Aug 30, 2023 at 08:21:15AM +0800, David Gow wrote:
> KUnit's attribute filtering feature needs the filter strings passed in
> to be writable, as it modifies them in-place during parsing. This works
> for the filters passed on the kernel command line, but the string
> literals used in the executor tests are at least theoretically read-only
> (though they work on x86_64 for some reason). s390 wasn't fooled, and
> crashed when these tests were run.
> 
> Use a 'char[]' instead, (and make an explicit variable for the current
> filter in parse_filter_attr_test), which will store the string in a
> writable segment.
> 
> Fixes: 76066f93f1df ("kunit: add tests for filtering attributes")
> Closes: https://lore.kernel.org/linux-kselftest/55950256-c00a-4d21-a2c0-cf9f0e5b8a9a@roeck-us.net/
> Signed-off-by: David Gow <davidgow@google.com>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
>  lib/kunit/executor_test.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index 4084071d0eb5..b4f6f96b2844 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -119,7 +119,7 @@ static void parse_filter_attr_test(struct kunit *test)
>  {
>  	int j, filter_count;
>  	struct kunit_attr_filter *parsed_filters;
> -	char *filters = "speed>slow, module!=example";
> +	char filters[] = "speed>slow, module!=example", *filter = filters;
>  	int err = 0;
>  
>  	filter_count = kunit_get_filter_count(filters);
> @@ -128,7 +128,7 @@ static void parse_filter_attr_test(struct kunit *test)
>  	parsed_filters = kunit_kcalloc(test, filter_count, sizeof(*parsed_filters),
>  			GFP_KERNEL);
>  	for (j = 0; j < filter_count; j++) {
> -		parsed_filters[j] = kunit_next_attr_filter(&filters, &err);
> +		parsed_filters[j] = kunit_next_attr_filter(&filter, &err);
>  		KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter '%s'", filters[j]);
>  	}
>  
> @@ -154,6 +154,7 @@ static void filter_attr_test(struct kunit *test)
>  		.start = subsuite, .end = &subsuite[2],
>  	};
>  	struct kunit_suite_set got;
> +	char filter[] = "speed>slow";
>  	int err = 0;
>  
>  	subsuite[0] = alloc_fake_suite(test, "normal_suite", dummy_attr_test_cases);
> @@ -168,7 +169,7 @@ static void filter_attr_test(struct kunit *test)
>  	 * attribute is unset and thus, the filtering is based on the parent attribute
>  	 * of slow.
>  	 */
> -	got = kunit_filter_suites(&suite_set, NULL, "speed>slow", NULL, &err);
> +	got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>  	KUNIT_ASSERT_EQ(test, err, 0);
>  	kfree_at_end(test, got.start);
> @@ -191,12 +192,13 @@ static void filter_attr_empty_test(struct kunit *test)
>  		.start = subsuite, .end = &subsuite[2],
>  	};
>  	struct kunit_suite_set got;
> +	char filter[] = "module!=dummy";
>  	int err = 0;
>  
>  	subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
>  	subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
>  
> -	got = kunit_filter_suites(&suite_set, NULL, "module!=dummy", NULL, &err);
> +	got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
>  	KUNIT_ASSERT_EQ(test, err, 0);
>  	kfree_at_end(test, got.start); /* just in case */
>  
> @@ -211,12 +213,13 @@ static void filter_attr_skip_test(struct kunit *test)
>  		.start = subsuite, .end = &subsuite[1],
>  	};
>  	struct kunit_suite_set got;
> +	char filter[] = "speed>slow";
>  	int err = 0;
>  
>  	subsuite[0] = alloc_fake_suite(test, "suite", dummy_attr_test_cases);
>  
>  	/* Want: suite(slow, normal), NULL -> suite(slow with SKIP, normal), NULL */
> -	got = kunit_filter_suites(&suite_set, NULL, "speed>slow", "skip", &err);
> +	got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>  	KUNIT_ASSERT_EQ(test, err, 0);
>  	kfree_at_end(test, got.start);
> -- 
> 2.42.0.283.g2d96d420d3-goog
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: test: Make filter strings in executor_test writable
  2023-08-30  0:21 [PATCH] kunit: test: Make filter strings in executor_test writable David Gow
  2023-08-30  1:44 ` Guenter Roeck
@ 2023-08-30 21:34 ` Rae Moar
  1 sibling, 0 replies; 3+ messages in thread
From: Rae Moar @ 2023-08-30 21:34 UTC (permalink / raw)
  To: David Gow
  Cc: Shuah Khan, Guenter Roeck, Brendan Higgins, kunit-dev,
	linux-kselftest, linux-kernel

On Tue, Aug 29, 2023 at 8:21 PM David Gow <davidgow@google.com> wrote:
>
> KUnit's attribute filtering feature needs the filter strings passed in
> to be writable, as it modifies them in-place during parsing. This works
> for the filters passed on the kernel command line, but the string
> literals used in the executor tests are at least theoretically read-only
> (though they work on x86_64 for some reason). s390 wasn't fooled, and
> crashed when these tests were run.
>
> Use a 'char[]' instead, (and make an explicit variable for the current
> filter in parse_filter_attr_test), which will store the string in a
> writable segment.
>
> Fixes: 76066f93f1df ("kunit: add tests for filtering attributes")
> Closes: https://lore.kernel.org/linux-kselftest/55950256-c00a-4d21-a2c0-cf9f0e5b8a9a@roeck-us.net/
> Signed-off-by: David Gow <davidgow@google.com>

Hello!

This looks good to me. Thanks for doing this!

Reviewed-by: Rae Moar <rmoar@google.com>

-Rae

> ---
>  lib/kunit/executor_test.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index 4084071d0eb5..b4f6f96b2844 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -119,7 +119,7 @@ static void parse_filter_attr_test(struct kunit *test)
>  {
>         int j, filter_count;
>         struct kunit_attr_filter *parsed_filters;
> -       char *filters = "speed>slow, module!=example";
> +       char filters[] = "speed>slow, module!=example", *filter = filters;
>         int err = 0;
>
>         filter_count = kunit_get_filter_count(filters);
> @@ -128,7 +128,7 @@ static void parse_filter_attr_test(struct kunit *test)
>         parsed_filters = kunit_kcalloc(test, filter_count, sizeof(*parsed_filters),
>                         GFP_KERNEL);
>         for (j = 0; j < filter_count; j++) {
> -               parsed_filters[j] = kunit_next_attr_filter(&filters, &err);
> +               parsed_filters[j] = kunit_next_attr_filter(&filter, &err);
>                 KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter '%s'", filters[j]);
>         }
>
> @@ -154,6 +154,7 @@ static void filter_attr_test(struct kunit *test)
>                 .start = subsuite, .end = &subsuite[2],
>         };
>         struct kunit_suite_set got;
> +       char filter[] = "speed>slow";
>         int err = 0;
>
>         subsuite[0] = alloc_fake_suite(test, "normal_suite", dummy_attr_test_cases);
> @@ -168,7 +169,7 @@ static void filter_attr_test(struct kunit *test)
>          * attribute is unset and thus, the filtering is based on the parent attribute
>          * of slow.
>          */
> -       got = kunit_filter_suites(&suite_set, NULL, "speed>slow", NULL, &err);
> +       got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>         KUNIT_ASSERT_EQ(test, err, 0);
>         kfree_at_end(test, got.start);
> @@ -191,12 +192,13 @@ static void filter_attr_empty_test(struct kunit *test)
>                 .start = subsuite, .end = &subsuite[2],
>         };
>         struct kunit_suite_set got;
> +       char filter[] = "module!=dummy";
>         int err = 0;
>
>         subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
>         subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
>
> -       got = kunit_filter_suites(&suite_set, NULL, "module!=dummy", NULL, &err);
> +       got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
>         KUNIT_ASSERT_EQ(test, err, 0);
>         kfree_at_end(test, got.start); /* just in case */
>
> @@ -211,12 +213,13 @@ static void filter_attr_skip_test(struct kunit *test)
>                 .start = subsuite, .end = &subsuite[1],
>         };
>         struct kunit_suite_set got;
> +       char filter[] = "speed>slow";
>         int err = 0;
>
>         subsuite[0] = alloc_fake_suite(test, "suite", dummy_attr_test_cases);
>
>         /* Want: suite(slow, normal), NULL -> suite(slow with SKIP, normal), NULL */
> -       got = kunit_filter_suites(&suite_set, NULL, "speed>slow", "skip", &err);
> +       got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>         KUNIT_ASSERT_EQ(test, err, 0);
>         kfree_at_end(test, got.start);
> --
> 2.42.0.283.g2d96d420d3-goog
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-30 21:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-30  0:21 [PATCH] kunit: test: Make filter strings in executor_test writable David Gow
2023-08-30  1:44 ` Guenter Roeck
2023-08-30 21:34 ` Rae Moar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).