Re: [PATCH v3 4/7] kunit: Handle test faults
On Tue, 19 Mar 2024 at 18:49, 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 -EINTR 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: Shuah Khan
> Reviewed-by: Kees Cook
> Reviewed-by: David Gow
> Tested-by: Rae Moar
> Signed-off-by: Mickaël Salaün
> Link: https://lore.kernel.org/r/20240319104857.70783-5-...@digikod.net
> ---
>
> Changes since v2:
> * s/-EFAULT/-EINTR/ in commit message as spotted by Rae.
> * Add a comment explaining vfork_done as suggested by David.
> * Add David's Reviewed-by.
> * Add Rae's Tested-by.
>
> Changes since v1:
> * Add Kees's Reviewed-by.
> ---
> include/kunit/try-catch.h | 3 ---
> lib/kunit/try-catch.c | 19 ---
> 2 files changed, 12 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..7a3910dd78a6 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);
It turns out kthread_exit() is not exported, so this doesn't work if
KUnit is built as a module.
I think the options we have are:
- Add EXPORT_SYMBOL(kthread_exit).
- Keep using out own completion, and kthread_complete_and_exit()
- try_get_module() before spawning the thread, and use
module_put_and_kthread_exit().
I think all of these would be okay, but I could've missed something.
-- David
> }
> 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 = &try_completion;
> try_catch->try_result = 0;
> task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> try_catch, "kunit_try_catch_thread");
> @@ -75,8 +76,12 @@ 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(&try_completion,
> + /*
> +* As for a vfork(2), task_struct->vfork_done (pointing to the
> +* underlying kthread->exited) can be used to wait for the end of a
> +* kernel thread.
> +*/
> + time_remaining = wait_for_completion_timeout(task_struct->vfork_done,
> kunit_test_ti
Re: [PATCH] x86, relocs: Ignore relocations in .notes section on walk_relocs
On Fri, Mar 22, 2024 at 04:40:11PM -0700, Kees Cook wrote:
> The earlier patch, commit aaa8736370db ("x86, relocs: Ignore relocations
> in .notes section"), landed via my tree. It was sent out on Feb 22nd
> (v1[1]) and got a suggestion from HPA and a Review from Juergen Gross.
> I sent v2 Feb 27th[2] and it sat ignored for two weeks.
s/ignored for two weeks/missed in the avalance of patches/
> Since it was a 10 year old kernel address exposure, I sent it to Linus
> on Mar 12th[3].
So is there some unwritten understanding somewhere which says that you
should take tip patches through your tree?
Maybe I've missed it.
If there isn't, should we agree on something?
Because there clearly is a need for clarification here...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH 1/1] arm64: syscall: Direct PRNG kstack randomization
On Sat, Mar 23, 2024, at 00:40, Jeremy Linton wrote: > On 3/8/24 14:29, Arnd Bergmann wrote: >> On Fri, Mar 8, 2024, at 17:49, Jeremy Linton wrote: >>> On 3/7/24 05:10, Arnd Bergmann wrote: I'm not sure I understand the logic. Do you mean that accessing CNTVCT itself is slow, or that reseeding based on CNTVCT is slow because of the overhead of reseeding? >>> >>> Slow, as in, its running at a much lower frequency than a cycle counter. >> >> Ok, I see. Would it be possible to use PMEVCNTR0 instead? > > So, I presume you actually mean PMCCNTR_EL0 because I don't see the > point of a dedicated event, although maybe... Right, that would make more sense. > So, the first and maybe largest problem is the PMxxx registers are all > optional because the PMU is optional! Although, they are strongly > recommended and most (AFAIK) machines do implement them. So, maybe if > its just using a cycle counter to dump some entropy into rnd_state it > becomes a statement that kstack randomization is slower or unsupported > if there isn't a PMU? I think that sounds workable, especially as there is already the randomize_kstack_offset=on/off conditional at boot time, it could fall back to just not randomizing and print a warning if the feature is enabled but unavailable at boot time. > But then we have to basically enable the PMU cycle counter globally, > which requires reworking how it works, because the cycle counter is > enabled/disabled and reset on the fly depending on whether the user is > trying to profile something. So, I have hacked that up, and it appears > to be working, although i'm not sure what kind of interaction will > happen with KVM yet. > > But I guess the larger question is whether its worth changing the PMU > behavior for this? I don't know too much about how the PMU works in detail, but I'm also worried about two possible issues that end up preventing us from using it in practice: - if enabling PMCCNTR_EL0 takes away one of the limited number of available counters, we probably don't want to go there - similarly, I would expect it to have a nonzero power consumption if the default is to have the clock disabled and non-counting. Probably not a big deal for server machines, but could be an issue on battery powered embedded devices. Arnd
[PATCH] x86/build: Clean up arch/x86/tools/relocs.c a bit
* Kees Cook wrote:
> On Fri, Mar 22, 2024 at 09:45:12AM +0100, Ingo Molnar wrote:
> >
> > * Kees Cook wrote:
> >
> > > On Sun, 17 Mar 2024 23:05:47 +0800, Guixiong Wei wrote:
> > > > The commit aaa8736370db ("x86, relocs: Ignore relocations in
> > > > .notes section") only ignore .note section on print_absolute_relocs,
> > > > but it also need to add on walk_relocs to avoid relocations in .note
> > > > section.
> > > >
> > > >
> > >
> > > Applied to for-next/hardening, thanks!
> > >
> > > [1/1] x86, relocs: Ignore relocations in .notes section on walk_relocs
> > > https://git.kernel.org/kees/c/6ba438a29b5d
> >
> > Please don't - these are x86 patches, plus it contains an eyesore - see
> > below ...
>
> Dropped.
>
> >
> > Thanks,
> >
> > Ingo
> >
> > relocs.c |3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> > Index: tip/arch/x86/tools/relocs.c
> > ===
> > --- tip.orig/arch/x86/tools/relocs.c
> > +++ tip/arch/x86/tools/relocs.c
> > @@ -752,9 +752,8 @@ static void walk_relocs(int (*process)(s
> > * values there are meant for pre-boot consumption (e.g.
> > * startup_xen).
> > */
> > - if (sec_applies->shdr.sh_type == SHT_NOTE) {
> > + if (sec_applies->shdr.sh_type == SHT_NOTE)
> > continue;
> > - }
>
> I think the patch was trying to follow the existing code style in the
> file. See line 733, for example:
>
> if (sec->shdr.sh_type != SHT_REL_TYPE) {
> continue;
> }
The 'existing code style' is a hodgepodge of CodingStyle compliant and
non-compliant bits - and our preference is to not propagate crap ...
Anyway, I cleaned it all up a bit in tip:x86/boot with the patch below.
Thanks,
Ingo
>
From: Ingo Molnar
Date: Sun, 24 Mar 2024 04:46:10 +0100
Subject: [PATCH] x86/build: Clean up arch/x86/tools/relocs.c a bit
So:
- Follow Documentation/CodingStyle for:
- curly braces
- variable definitions
- return statements
- etc.
- Fix unnecessary linebreaks
- Don't split user-visible error strings over multiple lines ...
- It's fine to use vertical alignment to make code more readable,
but it should be internally consistent for definitions visible
on a single page ...
- There's 40+ die() statements that are basically asserts and
never trigger. Make them single-line, to move them out of
sight as much as possible.
Signed-off-by: Ingo Molnar
---
arch/x86/tools/relocs.c | 362
1 file changed, 178 insertions(+), 184 deletions(-)
diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
index e7a44a7f617f..c101bed61940 100644
--- a/arch/x86/tools/relocs.c
+++ b/arch/x86/tools/relocs.c
@@ -11,41 +11,42 @@
#define Elf_Shdr ElfW(Shdr)
#define Elf_SymElfW(Sym)
-static Elf_Ehdrehdr;
-static unsigned long shnum;
-static unsigned intshstrndx;
-static unsigned intshsymtabndx;
-static unsigned intshxsymtabndx;
+static Elf_Ehdrehdr;
+static unsigned long shnum;
+static unsigned intshstrndx;
+static unsigned intshsymtabndx;
+static unsigned intshxsymtabndx;
static int sym_index(Elf_Sym *sym);
struct relocs {
- uint32_t*offset;
- unsigned long count;
- unsigned long size;
+ uint32_t*offset;
+ unsigned long count;
+ unsigned long size;
};
-static struct relocs relocs16;
-static struct relocs relocs32;
+static struct relocs relocs16;
+static struct relocs relocs32;
+
#if ELF_BITS == 64
-static struct relocs relocs32neg;
-static struct relocs relocs64;
-#define FMT PRIu64
+static struct relocs relocs32neg;
+static struct relocs relocs64;
+# define FMT PRIu64
#else
-#define FMT PRIu32
+# define FMT PRIu32
#endif
struct section {
- Elf_Shdr shdr;
- struct section *link;
- Elf_Sym*symtab;
- Elf32_Word *xsymtab;
- Elf_Rel*reltab;
- char *strtab;
+ Elf_Shdr shdr;
+ struct section *link;
+ Elf_Sym*symtab;
+ Elf32_Word *xsymtab;
+ Elf_Rel*reltab;
+ char *strtab;
};
-static struct section *secs;
+static struct section *secs;
-static const char * const sym_regex_kernel[S_NSYMTYPES] = {
+static const char * const sym_regex_kernel[S_NSYMTYPES] = {
/*
* Following symbols have been audited. There values are constant and do
* not change if bzImage is loaded
