Re: [PATCH v2 4/7] kunit: Handle test faults
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün wrote:
>
> Previously, when a kernel test thread crashed (e.g. NULL pointer
> dereference, general protection fault), the KUnit test hanged for 30
> seconds and exited with a timeout error.
>
> Fix this issue by waiting on task_struct->vfork_done instead of the
> custom kunit_try_catch.try_completion, and track the execution state by
> initially setting try_result with -EFAULT and only setting it to 0 if
> the test passed.
>
> Fix kunit_generic_run_threadfn_adapter() signature by returning 0
> instead of calling kthread_complete_and_exit(). Because thread's exit
> code is never checked, always set it to 0 to make it clear.
>
> Fix the -EINTR error message, which couldn't be reached until now.
>
> This is tested with a following patch.
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Reviewed-by: Kees Cook
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-5-...@digikod.net
> ---
This works fine here, and looks good.
The use of task_struct->vfork_done is a bit confusing, as there's no
documentation (that I could find) about what vfork_done means for
kthreads. From the code, it looks to just be a copy of
kthread->exited, which is much more obvious a name.
Would it make sense to either (a) replace this with a call to
to_kthread(), and kthread->exited, or (b) add a comment explaining
what vfork_done means here. kthread_stop() itself is using
to_kthread() and kthread->exited -- even though task_struct is also
there -- so I'd feel a bit more comfortable with that option.
Otherwise,
Reviewed-by: David Gow
Cheers,
-- David
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
> include/kunit/try-catch.h | 3 ---
> lib/kunit/try-catch.c | 14 +++---
> 2 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> index c507dd43119d..7c966a1adbd3 100644
> --- a/include/kunit/try-catch.h
> +++ b/include/kunit/try-catch.h
> @@ -14,13 +14,11 @@
>
> typedef void (*kunit_try_catch_func_t)(void *);
>
> -struct completion;
> struct kunit;
>
> /**
> * struct kunit_try_catch - provides a generic way to run code which might
> fail.
> * @test: The test case that is currently being executed.
> - * @try_completion: Completion that the control thread waits on while test
> runs.
> * @try_result: Contains any errno obtained while running test case.
> * @try: The function, the test case, to attempt to run.
> * @catch: The function called if @try bails out.
> @@ -46,7 +44,6 @@ struct kunit;
> struct kunit_try_catch {
> /* private: internal use only. */
> struct kunit *test;
> - struct completion *try_completion;
> int try_result;
> kunit_try_catch_func_t try;
> kunit_try_catch_func_t catch;
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index cab8b24b5d5a..c6ee4db0b3bd 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -18,7 +18,7 @@
> void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
> {
> try_catch->try_result = -EFAULT;
> - kthread_complete_and_exit(try_catch->try_completion, -EFAULT);
> + kthread_exit(0);
> }
> EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
>
> @@ -26,9 +26,12 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> {
> struct kunit_try_catch *try_catch = data;
>
> + try_catch->try_result = -EINTR;
> try_catch->try(try_catch->context);
> + if (try_catch->try_result == -EINTR)
> + try_catch->try_result = 0;
>
> - kthread_complete_and_exit(try_catch->try_completion, 0);
> + return 0;
> }
>
> static unsigned long kunit_test_timeout(void)
> @@ -58,13 +61,11 @@ static unsigned long kunit_test_timeout(void)
>
> void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> {
> - DECLARE_COMPLETION_ONSTACK(try_completion);
> struct kunit *test = try_catch->test;
> struct task_struct *task_struct;
> int exit_code, time_remaining;
>
> try_catch->context = context;
> - try_catch->try_completion = _completion;
> try_catch->try_result = 0;
> task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> try_catch, "kunit_try_catch_thread");
> @@ -75,8 +76,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> }
> get_task_struct(task_struct);
> wake_up_process(task_struct);
> -
> - time_remaining = wait_for_completion_timeout(_completion,
> + time_remaining = wait_for_completion_timeout(task_struct->vfork_done,
> kunit_test_timeout());
> if (time_remaining == 0) {
> try_catch->try_result = -ETIMEDOUT;
> @@ -92,7 +92,7 @@ void kunit_try_catch_run(struct kunit_try_catch
Re: [PATCH v2 7/7] kunit: Add tests for fault
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün wrote:
>
> Add a test case to check NULL pointer dereference and make sure it would
> result as a failed test.
>
> The full kunit_fault test suite is marked as skipped when run on UML
> because it would result to a kernel panic.
>
> Tested with:
> ./tools/testing/kunit/kunit.py run --arch x86_64 kunit_fault
> ./tools/testing/kunit/kunit.py run --arch arm64 \
> --cross_compile=aarch64-linux-gnu- kunit_fault
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-8-...@digikod.net
> ---
>
> Changes since v1:
> * Removed the rodata and const test cases for now.
> * Replace CONFIG_X86 check with !CONFIG_UML, and remove the "_x86"
> references.
> ---
I think UML _should_ be able to handle this with signal handlers, but
I tested it and agree that it's broken, so disabling for now makes
sense.
In general, I'd prefer to have an empty test which SKIP()s here, but
since the suite is empty, KUnit does that anyway, so this is fine.
Reviewed-by: David Gow
Cheers,
-- David
> lib/kunit/kunit-test.c | 45 +-
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index f7980ef236a3..0fdca5fffaec 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -109,6 +109,48 @@ static struct kunit_suite kunit_try_catch_test_suite = {
> .test_cases = kunit_try_catch_test_cases,
> };
>
> +#ifndef CONFIG_UML
> +
> +static void kunit_test_null_dereference(void *data)
> +{
> + struct kunit *test = data;
> + int *null = NULL;
> +
> + *null = 0;
> +
> + KUNIT_FAIL(test, "This line should never be reached\n");
> +}
> +
> +static void kunit_test_fault_null_dereference(struct kunit *test)
> +{
> + struct kunit_try_catch_test_context *ctx = test->priv;
> + struct kunit_try_catch *try_catch = ctx->try_catch;
> +
> + kunit_try_catch_init(try_catch,
> +test,
> +kunit_test_null_dereference,
> +kunit_test_catch);
> + kunit_try_catch_run(try_catch, test);
> +
> + KUNIT_EXPECT_EQ(test, try_catch->try_result, -EINTR);
> + KUNIT_EXPECT_TRUE(test, ctx->function_called);
> +}
> +
> +#endif /* !CONFIG_UML */
> +
> +static struct kunit_case kunit_fault_test_cases[] = {
> +#ifndef CONFIG_UML
> + KUNIT_CASE(kunit_test_fault_null_dereference),
> +#endif /* !CONFIG_UML */
> + {}
> +};
> +
> +static struct kunit_suite kunit_fault_test_suite = {
> + .name = "kunit_fault",
> + .init = kunit_try_catch_test_init,
> + .test_cases = kunit_fault_test_cases,
> +};
> +
> /*
> * Context for testing test managed resources
> * is_resource_initialized is used to test arbitrary resources
> @@ -826,6 +868,7 @@ static struct kunit_suite kunit_current_test_suite = {
>
> kunit_test_suites(_try_catch_test_suite, _resource_test_suite,
> _log_test_suite, _status_test_suite,
> - _current_test_suite, _device_test_suite);
> + _current_test_suite, _device_test_suite,
> + _fault_test_suite);
>
> MODULE_LICENSE("GPL v2");
> --
> 2.44.0
>
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2 6/7] kunit: Print last test location on fault
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün wrote:
>
> This helps identify the location of test faults.
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Reviewed-by: Kees Cook
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-7-...@digikod.net
> ---
I like the idea of this, but am a little bit worried about how
confusing it might be, given that the location only updates on those
particular macros.
Maybe the answer is to make the __KUNIT_SAVE_LOC() macro, or something
equivalent, a supported API.
One possibility would be to have a KUNIT_MARKER() macro. If we really
wanted to, we could expand it to take a string so we can have a more
user-friendly KUNIT_MARKER(test, "parsing packet") description of
where things went wrong. Another could be to extend this to use the
code tagging framework[1], if that lands.
That being said, I think this is still an improvement without any of
those features. I've left a few comments below. Let me know what you
think.
Cheers,
-- David
[1]: https://lwn.net/Articles/906660/
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
> include/kunit/test.h | 24 +---
> lib/kunit/try-catch.c | 10 +++---
> 2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index fcb4a4940ace..f3aa66eb0087 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -301,6 +301,8 @@ struct kunit {
> struct list_head resources; /* Protected by lock. */
>
> char status_comment[KUNIT_STATUS_COMMENT_SIZE];
> + /* Saves the last seen test. Useful to help with faults. */
> + struct kunit_loc last_seen;
> };
>
> static inline void kunit_set_failure(struct kunit *test)
> @@ -567,6 +569,15 @@ void __printf(2, 3) kunit_log_append(struct
> string_stream *log, const char *fmt,
> #define kunit_err(test, fmt, ...) \
> kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__)
>
> +/*
> + * Must be called at the beginning of each KUNIT_*_ASSERTION().
> + * Cf. KUNIT_CURRENT_LOC.
> + */
> +#define _KUNIT_SAVE_LOC(test) do {
> \
> + WRITE_ONCE(test->last_seen.file, __FILE__);
> \
> + WRITE_ONCE(test->last_seen.line, __LINE__);
> \
> +} while (0)
Can we get rid of the leading '_', make this public, and document it?
If we want to rename it to KUNIT_MARKER() or similar, that might work
better, too.
> +
> /**
> * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity.
> * @test: The test context object.
> @@ -575,7 +586,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream
> *log, const char *fmt,
> * words, it does nothing and only exists for code clarity. See
> * KUNIT_EXPECT_TRUE() for more information.
> */
> -#define KUNIT_SUCCEED(test) do {} while (0)
> +#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
>
> void __noreturn __kunit_abort(struct kunit *test);
>
> @@ -601,14 +612,16 @@ void __kunit_do_failed_assertion(struct kunit *test,
> } while (0)
>
>
> -#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...)
> \
> +#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do {
> \
> + _KUNIT_SAVE_LOC(test);
> \
> _KUNIT_FAILED(test,
> \
> assert_type,
> \
> kunit_fail_assert,
> \
> kunit_fail_assert_format,
> \
> {},
> \
> fmt,
> \
> - ##__VA_ARGS__)
> + ##__VA_ARGS__);
> \
> +} while (0)
>
> /**
> * KUNIT_FAIL() - Always causes a test to fail when evaluated.
> @@ -637,6 +650,7 @@ void __kunit_do_failed_assertion(struct kunit *test,
> fmt,
> \
> ...)
> \
> do {
> \
> + _KUNIT_SAVE_LOC(test);
> \
> if (likely(!!(condition_) == !!expected_true_))
> \
> break;
> \
>
> \
> @@ -698,6 +712,7 @@ do {
> \
> .right_text = #right,
Re: [PATCH v2 5/7] kunit: Fix KUNIT_SUCCESS() calls in iov_iter tests
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün wrote:
>
> Fix KUNIT_SUCCESS() calls to pass a test argument.
>
> This is a no-op for now because this macro does nothing, but it will be
> required for the next commit.
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Reviewed-by: Kees Cook
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-6-...@digikod.net
> ---
This is a pretty straightforward fix.
I'm actually a bit surprised how many tests were actually using
KUNIT_SUCCEEDED().
Reviewed-by: David Gow
Thanks,
-- David
-- David
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
> lib/kunit_iov_iter.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/lib/kunit_iov_iter.c b/lib/kunit_iov_iter.c
> index 859b67c4d697..27e0c8ee71d8 100644
> --- a/lib/kunit_iov_iter.c
> +++ b/lib/kunit_iov_iter.c
> @@ -139,7 +139,7 @@ static void __init iov_kunit_copy_to_kvec(struct kunit
> *test)
> return;
> }
>
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> /*
> @@ -194,7 +194,7 @@ static void __init iov_kunit_copy_from_kvec(struct kunit
> *test)
> return;
> }
>
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> struct bvec_test_range {
> @@ -302,7 +302,7 @@ static void __init iov_kunit_copy_to_bvec(struct kunit
> *test)
> return;
> }
>
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> /*
> @@ -359,7 +359,7 @@ static void __init iov_kunit_copy_from_bvec(struct kunit
> *test)
> return;
> }
>
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> static void iov_kunit_destroy_xarray(void *data)
> @@ -453,7 +453,7 @@ static void __init iov_kunit_copy_to_xarray(struct kunit
> *test)
> return;
> }
>
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> /*
> @@ -516,7 +516,7 @@ static void __init iov_kunit_copy_from_xarray(struct
> kunit *test)
> return;
> }
>
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> /*
> @@ -596,7 +596,7 @@ static void __init iov_kunit_extract_pages_kvec(struct
> kunit *test)
> stop:
> KUNIT_EXPECT_EQ(test, size, 0);
> KUNIT_EXPECT_EQ(test, iter.count, 0);
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> /*
> @@ -674,7 +674,7 @@ static void __init iov_kunit_extract_pages_bvec(struct
> kunit *test)
> stop:
> KUNIT_EXPECT_EQ(test, size, 0);
> KUNIT_EXPECT_EQ(test, iter.count, 0);
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> /*
> @@ -753,7 +753,7 @@ static void __init iov_kunit_extract_pages_xarray(struct
> kunit *test)
> }
>
> stop:
> - KUNIT_SUCCEED();
> + KUNIT_SUCCEED(test);
> }
>
> static struct kunit_case __refdata iov_kunit_cases[] = {
> --
> 2.44.0
>
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2 3/7] kunit: Fix timeout message
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün wrote:
>
> The exit code is always checked, so let's properly handle the -ETIMEDOUT
> error code.
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Reviewed-by: Kees Cook
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-4-...@digikod.net
> ---
This looks good to me. It might make sense to use a switch statement
rather than a change of else-ifs if we end up handling more errors in
the future, but this is fine for now.
Reviewed-by: David Gow
Cheers,
-- David
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
> lib/kunit/try-catch.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index 73f5007f20ea..cab8b24b5d5a 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -79,7 +79,6 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> time_remaining = wait_for_completion_timeout(_completion,
> kunit_test_timeout());
> if (time_remaining == 0) {
> - kunit_err(test, "try timed out\n");
> try_catch->try_result = -ETIMEDOUT;
> kthread_stop(task_struct);
> }
> @@ -94,6 +93,8 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> try_catch->try_result = 0;
> else if (exit_code == -EINTR)
> kunit_err(test, "wake_up_process() was never called\n");
> + else if (exit_code == -ETIMEDOUT)
> + kunit_err(test, "try timed out\n");
> else if (exit_code)
> kunit_err(test, "Unknown error: %d\n", exit_code);
>
> --
> 2.44.0
>
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2 2/7] kunit: Fix kthread reference
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün wrote:
>
> There is a race condition when a kthread finishes after the deadline and
> before the call to kthread_stop(), which may lead to use after free.
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Reviewed-by: Kees Cook
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-3-...@digikod.net
> ---
Thanks: I've never been unlucky enough to hit this, but it's definitely a bug.
We should ideally mark it with a fixes tag:
Fixes: adf505457032 ("kunit: fix UAF when run kfence test case test_gfpzero")
Reviewed-by: David Gow
Cheers,
-- David
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
> lib/kunit/try-catch.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index a5cb2ef70a25..73f5007f20ea 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> #include "try-catch-impl.h"
>
> @@ -65,14 +66,15 @@ void kunit_try_catch_run(struct kunit_try_catch
> *try_catch, void *context)
> try_catch->context = context;
> try_catch->try_completion = _completion;
> try_catch->try_result = 0;
> - task_struct = kthread_run(kunit_generic_run_threadfn_adapter,
> - try_catch,
> - "kunit_try_catch_thread");
> + task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> +try_catch, "kunit_try_catch_thread");
> if (IS_ERR(task_struct)) {
> try_catch->try_result = PTR_ERR(task_struct);
> try_catch->catch(try_catch->context);
> return;
> }
> + get_task_struct(task_struct);
> + wake_up_process(task_struct);
>
> time_remaining = wait_for_completion_timeout(_completion,
> kunit_test_timeout());
> @@ -82,6 +84,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> kthread_stop(task_struct);
> }
>
> + put_task_struct(task_struct);
> exit_code = try_catch->try_result;
>
> if (!exit_code)
> --
> 2.44.0
>
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2 1/7] kunit: Handle thread creation error
On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün wrote:
>
> Previously, if a thread creation failed (e.g. -ENOMEM), the function was
> called (kunit_catch_run_case or kunit_catch_run_case_cleanup) without
> marking the test as failed. Instead, fill try_result with the error
> code returned by kthread_run(), which will mark the test as failed and
> print "internal error occurred...".
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Reviewed-by: Kees Cook
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-2-...@digikod.net
> ---
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
Thanks for catching this!
Reviewed-by: David Gow
Thanks,
-- David
-- David
> lib/kunit/try-catch.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index f7825991d576..a5cb2ef70a25 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -69,6 +69,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> try_catch,
> "kunit_try_catch_thread");
> if (IS_ERR(task_struct)) {
> + try_catch->try_result = PTR_ERR(task_struct);
> try_catch->catch(try_catch->context);
> return;
> }
> --
> 2.44.0
>
smime.p7s
Description: S/MIME Cryptographic Signature
[GIT PULL] pstore updates for v6.9-rc1
Hi Linus, Please pull these handful of pstore updates for v6.9-rc1. Details below. Thanks! -Kees The following changes since commit 41bccc98fb7931d63d03f326a746ac4d429c1dd3: Linux 6.8-rc2 (2024-01-28 17:01:12 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/pstore-v6.9-rc1 for you to fetch changes up to c8d25d696f526a42ad8cf615dc1131c0b00c662e: pstore/zone: Don't clear memory twice (2024-03-09 12:33:22 -0800) pstore updates for v6.9-rc1 - Make PSTORE_RAM available by default on arm64 (Nícolas F. R. A. Prado) - Allow for dynamic initialization in modular build (Guilherme G. Piccoli) - Add missing allocation failure check (Kunwu Chan) - Avoid duplicate memory zeroing (Christophe JAILLET) - Avoid potential double-free during pstorefs umount Christophe JAILLET (1): pstore/zone: Don't clear memory twice Guilherme G. Piccoli (1): efi: pstore: Allow dynamic initialization based on module parameter Kees Cook (1): pstore: inode: Only d_invalidate() is needed Kunwu Chan (1): pstore/zone: Add a null pointer check to the psz_kmsg_read Nícolas F. R. A. Prado (2): pstore/ram: Register to module device table arm64: defconfig: Enable PSTORE_RAM arch/arm64/configs/defconfig | 1 + drivers/firmware/efi/efi-pstore.c | 43 +++ fs/pstore/inode.c | 10 +++-- fs/pstore/ram.c | 1 + fs/pstore/zone.c | 3 ++- 5 files changed, 42 insertions(+), 16 deletions(-) -- Kees Cook
Re: [PATCH v2 4/7] kunit: Handle test faults
On Fri, Mar 1, 2024 at 2:40 PM Mickaël Salaün wrote:
>
> Previously, when a kernel test thread crashed (e.g. NULL pointer
> dereference, general protection fault), the KUnit test hanged for 30
> seconds and exited with a timeout error.
>
> Fix this issue by waiting on task_struct->vfork_done instead of the
> custom kunit_try_catch.try_completion, and track the execution state by
> initially setting try_result with -EFAULT and only setting it to 0 if
Hello!
Thanks for your patch! This has been tested and seems pretty good to
me but I just have a few questions. First, do you mean here "setting
try_result with -EINTR" instead?
But happy to add the tested-by.
Tested-by: Rae Moar
Thanks!
-Rae
> the test passed.
>
> Fix kunit_generic_run_threadfn_adapter() signature by returning 0
> instead of calling kthread_complete_and_exit(). Because thread's exit
> code is never checked, always set it to 0 to make it clear.
>
> Fix the -EINTR error message, which couldn't be reached until now.
>
> This is tested with a following patch.
>
> Cc: Brendan Higgins
> Cc: David Gow
> Cc: Rae Moar
> Cc: Shuah Khan
> Reviewed-by: Kees Cook
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240301194037.532117-5-...@digikod.net
> ---
>
> Changes since v1:
> * Added Kees's Reviewed-by.
> ---
> include/kunit/try-catch.h | 3 ---
> lib/kunit/try-catch.c | 14 +++---
> 2 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> index c507dd43119d..7c966a1adbd3 100644
> --- a/include/kunit/try-catch.h
> +++ b/include/kunit/try-catch.h
> @@ -14,13 +14,11 @@
>
> typedef void (*kunit_try_catch_func_t)(void *);
>
> -struct completion;
> struct kunit;
>
> /**
> * struct kunit_try_catch - provides a generic way to run code which might
> fail.
> * @test: The test case that is currently being executed.
> - * @try_completion: Completion that the control thread waits on while test
> runs.
> * @try_result: Contains any errno obtained while running test case.
> * @try: The function, the test case, to attempt to run.
> * @catch: The function called if @try bails out.
> @@ -46,7 +44,6 @@ struct kunit;
> struct kunit_try_catch {
> /* private: internal use only. */
> struct kunit *test;
> - struct completion *try_completion;
> int try_result;
> kunit_try_catch_func_t try;
> kunit_try_catch_func_t catch;
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index cab8b24b5d5a..c6ee4db0b3bd 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -18,7 +18,7 @@
> void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
> {
> try_catch->try_result = -EFAULT;
> - kthread_complete_and_exit(try_catch->try_completion, -EFAULT);
> + kthread_exit(0);
> }
> EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
>
> @@ -26,9 +26,12 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> {
> struct kunit_try_catch *try_catch = data;
>
> + try_catch->try_result = -EINTR;
> try_catch->try(try_catch->context);
> + if (try_catch->try_result == -EINTR)
> + try_catch->try_result = 0;
>
> - kthread_complete_and_exit(try_catch->try_completion, 0);
> + return 0;
Really my only question is why we do not need to still do a
kthread_exit(0) here? I realize we are not checking the thread's exit
code but isn't it safer to call kthread_exit(). I'm new to kthread so
I am not too sure.
> }
>
> static unsigned long kunit_test_timeout(void)
> @@ -58,13 +61,11 @@ static unsigned long kunit_test_timeout(void)
>
> void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
> {
> - DECLARE_COMPLETION_ONSTACK(try_completion);
> struct kunit *test = try_catch->test;
> struct task_struct *task_struct;
> int exit_code, time_remaining;
>
> try_catch->context = context;
> - try_catch->try_completion = _completion;
> try_catch->try_result = 0;
> task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> try_catch, "kunit_try_catch_thread");
> @@ -75,8 +76,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> }
> get_task_struct(task_struct);
> wake_up_process(task_struct);
> -
> - time_remaining = wait_for_completion_timeout(_completion,
> + time_remaining = wait_for_completion_timeout(task_struct->vfork_done,
> kunit_test_timeout());
> if (time_remaining == 0) {
> try_catch->try_result = -ETIMEDOUT;
> @@ -92,7 +92,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch,
> void *context)
> if (exit_code == -EFAULT)
> try_catch->try_result = 0;
> else if (exit_code == -EINTR)
> - kunit_err(test,
Re: [PATCH AUTOSEL 6.1 10/12] enic: Avoid false positive under FORTIFY_SOURCE
Hi! > From: Kees Cook > > [ Upstream commit 40b9385dd8e6a0515e1c9cd06a277483556b7286 ] > > FORTIFY_SOURCE has been ignoring 0-sized destinations while the kernel > code base has been converted to flexible arrays. In order to enforce > the 0-sized destinations (e.g. with __counted_by), the remaining 0-sized > destinations need to be handled. Unfortunately, struct vic_provinfo > resists full conversion, as it contains a flexible array of flexible > arrays, which is only possible with the 0-sized fake flexible array. > > Use unsafe_memcpy() to avoid future false positives under > CONFIG_FORTIFY_SOURCE. This prepares for future chagnes, but I don't believe we'll port them to stable. Best regards, Pavel -- People of Russia, stop Putin before his war on Ukraine escalates. signature.asc Description: PGP signature
Re: [PATCH v2] overflow: Change DEFINE_FLEX to take __counted_by member
On 3/11/2024 2:28 AM, Simon Horman wrote:
On Sat, Mar 09, 2024 at 12:32:45PM -0800, Kees Cook wrote:
On Fri, Mar 08, 2024 at 08:20:18PM +, Simon Horman wrote:
On Wed, Mar 06, 2024 at 03:51:36PM -0800, Kees Cook wrote:
The norm should be flexible array structures with __counted_by
annotations, so DEFINE_FLEX() is updated to expect that. Rename
the non-annotated version to DEFINE_RAW_FLEX(), and update the
few existing users.
Signed-off-by: Kees Cook
Hi Kees,
I'm unclear what this is based on, as it doesn't appear to apply
cleanly to net-next or the dev-queue branch of the iwl-next tree.
But I manually applied it to the latter and ran some checks.
It was based on v6.8-rc2, but it no longer applies cleanly to iwl-next:
https://lore.kernel.org/linux-next/20240307162958.02ec4...@canb.auug.org.au/
Is this something iwl-next can take for the v6.9 merge window? I can
send a rebased patch if that helps?
Thanks Kees,
I think that would help in the sense that from my POV it would
be more in fitting with the usual workflow for netdev patches.
But if the iwl maintainers think otherwise then I have no objections.
I can take this through iwl-next. A rebase would be great and if you
mark it for iwl-next ('PATCH iwl-next') so that everyone is clear on
target tree. Just to note since net-next is now closed, it would be
going to 6.10.
Thanks,
Tony
@@ -396,9 +396,9 @@ static inline size_t __must_check size_sub(size_t minuend,
size_t subtrahend)
* @name: Name for a variable to define.
* @member: Name of the array member.
* @count: Number of elements in the array; must be compile-time const.
- * @initializer: initializer expression (could be empty for no init).
+ * @initializer...: initializer expression (could be empty for no init).
Curiously kernel-doc --none seems happier without the line above changed.
I've fixed this up too:
https://lore.kernel.org/linux-next/202403071124.36DC2B617A@keescook/
--
Kees Cook
Re: [PATCH] exec: Simplify remove_arg_zero() error path
On Sat 09-03-24 13:48:30, Kees Cook wrote:
> We don't need the "out" label any more, so remove "ret" and return
> directly on error.
>
> Signed-off-by: Kees Cook
Looks good. Feel free to add:
Reviewed-by: Jan Kara
Honza
> ---
> Cc: Eric Biederman
> Cc: Alexander Viro
> Cc: Christian Brauner
> Cc: Jan Kara
> Cc: linux...@kvack.org
> Cc: linux-fsde...@vger.kernel.org
> ---
> fs/exec.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 715e1a8aa4f0..e7d9d6ad980b 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1720,7 +1720,6 @@ static int prepare_binprm(struct linux_binprm *bprm)
> */
> int remove_arg_zero(struct linux_binprm *bprm)
> {
> - int ret = 0;
> unsigned long offset;
> char *kaddr;
> struct page *page;
> @@ -1731,10 +1730,8 @@ int remove_arg_zero(struct linux_binprm *bprm)
> do {
> offset = bprm->p & ~PAGE_MASK;
> page = get_arg_page(bprm, bprm->p, 0);
> - if (!page) {
> - ret = -EFAULT;
> - goto out;
> - }
> + if (!page)
> + return -EFAULT;
> kaddr = kmap_local_page(page);
>
> for (; offset < PAGE_SIZE && kaddr[offset];
> @@ -1748,8 +1745,7 @@ int remove_arg_zero(struct linux_binprm *bprm)
> bprm->p++;
> bprm->argc--;
>
> -out:
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(remove_arg_zero);
>
> --
> 2.34.1
>
--
Jan Kara
SUSE Labs, CR
Re: [PATCH][next] net/smc: Avoid -Wflex-array-member-not-at-end warnings
On 2024/3/8 07:46, Gustavo A. R. Silva wrote:
On 3/7/24 02:17, Jan Karcher wrote:
On 04/03/2024 10:00, Wen Gu wrote:
On 2024/3/2 02:40, Gustavo A. R. Silva wrote:
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.
There are currently a couple of objects in `struct smc_clc_msg_proposal_area`
that contain a couple of flexible structures:
Thank you Gustavo for the proposal.
I had to do some reading to better understand what's happening and how your
patch solves this.
struct smc_clc_msg_proposal_area {
...
struct smc_clc_v2_extension pclc_v2_ext;
...
struct smc_clc_smcd_v2_extension pclc_smcd_v2_ext;
...
};
So, in order to avoid ending up with a couple of flexible-array members
in the middle of a struct, we use the `struct_group_tagged()` helper to
separate the flexible array from the rest of the members in the flexible
structure:
struct smc_clc_smcd_v2_extension {
struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
u8 system_eid[SMC_MAX_EID_LEN];
u8 reserved[16];
);
struct smc_clc_smcd_gid_chid gidchid[];
};
With the change described above, we now declare objects of the type of
the tagged struct without embedding flexible arrays in the middle of
another struct:
struct smc_clc_msg_proposal_area {
...
struct smc_clc_v2_extension_hdr pclc_v2_ext;
...
struct smc_clc_smcd_v2_extension_hdr pclc_smcd_v2_ext;
...
};
We also use `container_of()` when we need to retrieve a pointer to the
flexible structures.
So, with these changes, fix the following warnings:
In file included from net/smc/af_smc.c:42:
net/smc/smc_clc.h:186:49: warning: structure containing a flexible array member is not at the end of another
structure [-Wflex-array-member-not-at-end]
186 | struct smc_clc_v2_extension pclc_v2_ext;
| ^~~
net/smc/smc_clc.h:188:49: warning: structure containing a flexible array member is not at the end of another
structure [-Wflex-array-member-not-at-end]
188 | struct smc_clc_smcd_v2_extension pclc_smcd_v2_ext;
| ^~~~
Signed-off-by: Gustavo A. R. Silva
---
net/smc/smc_clc.c | 5 +++--
net/smc/smc_clc.h | 24 ++--
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index e55026c7529c..3094cfa1c458 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -853,8 +853,9 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct
smc_init_info *ini)
pclc_smcd = >pclc_smcd;
pclc_prfx = >pclc_prfx;
ipv6_prfx = pclc->pclc_prfx_ipv6;
- v2_ext = >pclc_v2_ext;
- smcd_v2_ext = >pclc_smcd_v2_ext;
+ v2_ext = container_of(>pclc_v2_ext, struct smc_clc_v2_extension,
_hdr);
+ smcd_v2_ext = container_of(>pclc_smcd_v2_ext,
+ struct smc_clc_smcd_v2_extension, hdr);
gidchids = pclc->pclc_gidchids;
trl = >pclc_trl;
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 7cc7070b9772..5b91a1947078 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -134,12 +134,14 @@ struct smc_clc_smcd_gid_chid {
*/
struct smc_clc_v2_extension {
- struct smc_clnt_opts_area_hdr hdr;
- u8 roce[16]; /* RoCEv2 GID */
- u8 max_conns;
- u8 max_links;
- __be16 feature_mask;
- u8 reserved[12];
+ struct_group_tagged(smc_clc_v2_extension_hdr, _hdr,
+ struct smc_clnt_opts_area_hdr hdr;
+ u8 roce[16]; /* RoCEv2 GID */
+ u8 max_conns;
+ u8 max_links;
+ __be16 feature_mask;
+ u8 reserved[12];
+ );
u8 user_eids[][SMC_MAX_EID_LEN];
};
@@ -159,8 +161,10 @@ struct smc_clc_msg_smcd { /* SMC-D GID information */
};
struct smc_clc_smcd_v2_extension {
- u8 system_eid[SMC_MAX_EID_LEN];
- u8 reserved[16];
+ struct_group_tagged(smc_clc_smcd_v2_extension_hdr, hdr,
+ u8 system_eid[SMC_MAX_EID_LEN];
+ u8 reserved[16];
+ );
struct smc_clc_smcd_gid_chid gidchid[];
};
@@ -183,9 +187,9 @@ struct smc_clc_msg_proposal_area {
struct smc_clc_msg_smcd pclc_smcd;
struct smc_clc_msg_proposal_prefix pclc_prfx;
struct smc_clc_ipv6_prefix pclc_prfx_ipv6[SMC_CLC_MAX_V6_PREFIX];
- struct smc_clc_v2_extension pclc_v2_ext;
+ struct smc_clc_v2_extension_hdr pclc_v2_ext;
u8 user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
- struct smc_clc_smcd_v2_extension pclc_smcd_v2_ext;
+ struct smc_clc_smcd_v2_extension_hdr pclc_smcd_v2_ext;
struct smc_clc_smcd_gid_chid
pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES];
struct smc_clc_msg_trail pclc_trl;
Thank you! Gustavo. This patch can fix this warning
Re: [PATCH v2] overflow: Change DEFINE_FLEX to take __counted_by member
On Sat, Mar 09, 2024 at 12:32:45PM -0800, Kees Cook wrote: > On Fri, Mar 08, 2024 at 08:20:18PM +, Simon Horman wrote: > > On Wed, Mar 06, 2024 at 03:51:36PM -0800, Kees Cook wrote: > > > The norm should be flexible array structures with __counted_by > > > annotations, so DEFINE_FLEX() is updated to expect that. Rename > > > the non-annotated version to DEFINE_RAW_FLEX(), and update the > > > few existing users. > > > > > > Signed-off-by: Kees Cook > > > > Hi Kees, > > > > I'm unclear what this is based on, as it doesn't appear to apply > > cleanly to net-next or the dev-queue branch of the iwl-next tree. > > But I manually applied it to the latter and ran some checks. > > It was based on v6.8-rc2, but it no longer applies cleanly to iwl-next: > https://lore.kernel.org/linux-next/20240307162958.02ec4...@canb.auug.org.au/ > > Is this something iwl-next can take for the v6.9 merge window? I can > send a rebased patch if that helps? Thanks Kees, I think that would help in the sense that from my POV it would be more in fitting with the usual workflow for netdev patches. But if the iwl maintainers think otherwise then I have no objections. > > > > @@ -396,9 +396,9 @@ static inline size_t __must_check size_sub(size_t > > > minuend, size_t subtrahend) > > > * @name: Name for a variable to define. > > > * @member: Name of the array member. > > > * @count: Number of elements in the array; must be compile-time const. > > > - * @initializer: initializer expression (could be empty for no init). > > > + * @initializer...: initializer expression (could be empty for no init). > > > > Curiously kernel-doc --none seems happier without the line above changed. > > I've fixed this up too: > https://lore.kernel.org/linux-next/202403071124.36DC2B617A@keescook/ > > -- > Kees Cook >
