[RFC,v3,08/19] arch: um: add shim to trap to allow installing a fault catcher for tests
diff mbox series

Message ID 20181128193636.254378-9-brendanhiggins@google.com
State Rejected
Headers show
Series
  • kunit: introduce KUnit, the Linux kernel unit testing framework
Related show

Commit Message

Brendan Higgins Nov. 28, 2018, 7:36 p.m. UTC
Add context to current thread that allows a test to specify that it
wants to skip the normal checks to run an installed fault catcher.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 arch/um/include/asm/processor-generic.h |  4 +++-
 arch/um/kernel/trap.c                   | 15 +++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

Comments

Luis Chamberlain Nov. 30, 2018, 3:34 a.m. UTC | #1
On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index cced829460427..bf90e678b3d71 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -201,6 +201,12 @@ void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
>  	segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
>  }
>  
> +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> +{
> +	current->thread.fault_addr = fault_addr;
> +	UML_LONGJMP(catcher, 1);
> +}
> +
>  /*
>   * We give a *copy* of the faultinfo in the regs to segv.
>   * This must be done, since nesting SEGVs could overwrite
> @@ -219,7 +225,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
>  	if (!is_user && regs)
>  		current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
>  
> -	if (!is_user && (address >= start_vm) && (address < end_vm)) {
> +	catcher = current->thread.fault_catcher;

This and..

> +	if (catcher && current->thread.is_running_test)
> +		segv_run_catcher(catcher, (void *) address);
> +	else if (!is_user && (address >= start_vm) && (address < end_vm)) {
>  		flush_tlb_kernel_vm();
>  		goto out;
>  	}

*not this*

> @@ -246,12 +255,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
>  		address = 0;
>  	}
>  
> -	catcher = current->thread.fault_catcher;
>  	if (!err)
>  		goto out;
>  	else if (catcher != NULL) {
> -		current->thread.fault_addr = (void *) address;
> -		UML_LONGJMP(catcher, 1);
> +		segv_run_catcher(catcher, (void *) address);
>  	}
>  	else if (current->thread.fault_addr != NULL)
>  		panic("fault_addr set but no fault catcher");

But with this seems one atomic change which should be submitted
separately, its just a helper. Think it would make the actual
change needed easier to review, ie, your needed changes would
be smaller and clearer for what you need.

  Luis
Luis Chamberlain Nov. 30, 2018, 3:41 a.m. UTC | #2
On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> +{
> +	current->thread.fault_addr = fault_addr;
> +	UML_LONGJMP(catcher, 1);
> +}

Some documentation about what this does exactly would be appreciated.
With the goal it may be useful to others wanting to consider support
for other archs -- if that actually ends up being desirable.

  Luis
Brendan Higgins Dec. 3, 2018, 11:34 p.m. UTC | #3
On Thu, Nov 29, 2018 at 7:34 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> > diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> > index cced829460427..bf90e678b3d71 100644
> > --- a/arch/um/kernel/trap.c
> > +++ b/arch/um/kernel/trap.c
> > @@ -201,6 +201,12 @@ void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
> >       segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
> >  }
> >
> > +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> > +{
> > +     current->thread.fault_addr = fault_addr;
> > +     UML_LONGJMP(catcher, 1);
> > +}
> > +
> >  /*
> >   * We give a *copy* of the faultinfo in the regs to segv.
> >   * This must be done, since nesting SEGVs could overwrite
> > @@ -219,7 +225,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> >       if (!is_user && regs)
> >               current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
> >
> > -     if (!is_user && (address >= start_vm) && (address < end_vm)) {
> > +     catcher = current->thread.fault_catcher;
>
> This and..
>
> > +     if (catcher && current->thread.is_running_test)
> > +             segv_run_catcher(catcher, (void *) address);
> > +     else if (!is_user && (address >= start_vm) && (address < end_vm)) {
> >               flush_tlb_kernel_vm();
> >               goto out;
> >       }
>
> *not this*

I don't understand. Are you saying the previous block of code is good
and this one is bad?

>
> > @@ -246,12 +255,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> >               address = 0;
> >       }
> >
> > -     catcher = current->thread.fault_catcher;
> >       if (!err)
> >               goto out;
> >       else if (catcher != NULL) {
> > -             current->thread.fault_addr = (void *) address;
> > -             UML_LONGJMP(catcher, 1);
> > +             segv_run_catcher(catcher, (void *) address);
> >       }
> >       else if (current->thread.fault_addr != NULL)
> >               panic("fault_addr set but no fault catcher");
>
> But with this seems one atomic change which should be submitted
> separately, its just a helper. Think it would make the actual
> change needed easier to review, ie, your needed changes would
> be smaller and clearer for what you need.

Are you suggesting that I pull out the bits needed to implement abort
in the next patch and squash it into this one?
Brendan Higgins Dec. 3, 2018, 11:37 p.m. UTC | #4
On Thu, Nov 29, 2018 at 7:41 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> > +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> > +{
> > +     current->thread.fault_addr = fault_addr;
> > +     UML_LONGJMP(catcher, 1);
> > +}
>
> Some documentation about what this does exactly would be appreciated.
> With the goal it may be useful to others wanting to consider support
> for other archs -- if that actually ends up being desirable.

Yeah, I should. Should this go in the wrapper around the abort() hack?
Or do you think I should write some documentation on how KUnit works
under Documentation/ ?
Luis Chamberlain Dec. 3, 2018, 11:46 p.m. UTC | #5
On Mon, Dec 03, 2018 at 03:34:57PM -0800, Brendan Higgins wrote:
> On Thu, Nov 29, 2018 at 7:34 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> > > diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> > > index cced829460427..bf90e678b3d71 100644
> > > --- a/arch/um/kernel/trap.c
> > > +++ b/arch/um/kernel/trap.c
> > > @@ -201,6 +201,12 @@ void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
> > >       segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
> > >  }
> > >
> > > +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> > > +{
> > > +     current->thread.fault_addr = fault_addr;
> > > +     UML_LONGJMP(catcher, 1);
> > > +}
> > > +
> > >  /*
> > >   * We give a *copy* of the faultinfo in the regs to segv.
> > >   * This must be done, since nesting SEGVs could overwrite
> > > @@ -219,7 +225,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> > >       if (!is_user && regs)
> > >               current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
> > >
> > > -     if (!is_user && (address >= start_vm) && (address < end_vm)) {
> > > +     catcher = current->thread.fault_catcher;
> >
> > This and..
> >
> > > +     if (catcher && current->thread.is_running_test)
> > > +             segv_run_catcher(catcher, (void *) address);
> > > +     else if (!is_user && (address >= start_vm) && (address < end_vm)) {
> > >               flush_tlb_kernel_vm();
> > >               goto out;
> > >       }
> >
> > *not this*
> 
> I don't understand. Are you saying the previous block of code is good
> and this one is bad?

No, I was saying that the above block of code is a functional change,
but I was also pointing out other areas which were not and could be
folded into a separate atomic patch where no functionality changes.

> > > @@ -246,12 +255,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> > >               address = 0;
> > >       }
> > >
> > > -     catcher = current->thread.fault_catcher;
> > >       if (!err)
> > >               goto out;
> > >       else if (catcher != NULL) {
> > > -             current->thread.fault_addr = (void *) address;
> > > -             UML_LONGJMP(catcher, 1);
> > > +             segv_run_catcher(catcher, (void *) address);
> > >       }
> > >       else if (current->thread.fault_addr != NULL)
> > >               panic("fault_addr set but no fault catcher");
> >
> > But with this seems one atomic change which should be submitted
> > separately, its just a helper. Think it would make the actual
> > change needed easier to review, ie, your needed changes would
> > be smaller and clearer for what you need.
> 
> Are you suggesting that I pull out the bits needed to implement abort
> in the next patch and squash it into this one?

No, I'm suggesting you can probably split this patch in 2, one which
wraps things with no functional changes, and another which adds your
changes.

  Luis
Brendan Higgins Dec. 4, 2018, 12:44 a.m. UTC | #6
On Mon, Dec 3, 2018 at 3:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Dec 03, 2018 at 03:34:57PM -0800, Brendan Higgins wrote:
> > On Thu, Nov 29, 2018 at 7:34 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > >
> > > On Wed, Nov 28, 2018 at 11:36:25AM -0800, Brendan Higgins wrote:
> > > > diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> > > > index cced829460427..bf90e678b3d71 100644
> > > > --- a/arch/um/kernel/trap.c
> > > > +++ b/arch/um/kernel/trap.c
> > > > @@ -201,6 +201,12 @@ void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
> > > >       segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
> > > >  }
> > > >
> > > > +static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
> > > > +{
> > > > +     current->thread.fault_addr = fault_addr;
> > > > +     UML_LONGJMP(catcher, 1);
> > > > +}
> > > > +
> > > >  /*
> > > >   * We give a *copy* of the faultinfo in the regs to segv.
> > > >   * This must be done, since nesting SEGVs could overwrite
> > > > @@ -219,7 +225,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> > > >       if (!is_user && regs)
> > > >               current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
> > > >
> > > > -     if (!is_user && (address >= start_vm) && (address < end_vm)) {
> > > > +     catcher = current->thread.fault_catcher;
> > >
> > > This and..
> > >
> > > > +     if (catcher && current->thread.is_running_test)
> > > > +             segv_run_catcher(catcher, (void *) address);
> > > > +     else if (!is_user && (address >= start_vm) && (address < end_vm)) {
> > > >               flush_tlb_kernel_vm();
> > > >               goto out;
> > > >       }
> > >
> > > *not this*
> >
> > I don't understand. Are you saying the previous block of code is good
> > and this one is bad?
>
> No, I was saying that the above block of code is a functional change,
> but I was also pointing out other areas which were not and could be
> folded into a separate atomic patch where no functionality changes.
>
> > > > @@ -246,12 +255,10 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> > > >               address = 0;
> > > >       }
> > > >
> > > > -     catcher = current->thread.fault_catcher;
> > > >       if (!err)
> > > >               goto out;
> > > >       else if (catcher != NULL) {
> > > > -             current->thread.fault_addr = (void *) address;
> > > > -             UML_LONGJMP(catcher, 1);
> > > > +             segv_run_catcher(catcher, (void *) address);
> > > >       }
> > > >       else if (current->thread.fault_addr != NULL)
> > > >               panic("fault_addr set but no fault catcher");
> > >
> > > But with this seems one atomic change which should be submitted
> > > separately, its just a helper. Think it would make the actual
> > > change needed easier to review, ie, your needed changes would
> > > be smaller and clearer for what you need.
> >
> > Are you suggesting that I pull out the bits needed to implement abort
> > in the next patch and squash it into this one?
>
> No, I'm suggesting you can probably split this patch in 2, one which
> wraps things with no functional changes, and another which adds your
> changes.
>

That makes sense.

Thanks for the clarification!

Patch
diff mbox series

diff --git a/arch/um/include/asm/processor-generic.h b/arch/um/include/asm/processor-generic.h
index b58b746d3f2ca..d566cd416ff02 100644
--- a/arch/um/include/asm/processor-generic.h
+++ b/arch/um/include/asm/processor-generic.h
@@ -27,6 +27,7 @@  struct thread_struct {
 	struct task_struct *prev_sched;
 	struct arch_thread arch;
 	jmp_buf switch_buf;
+	bool is_running_test;
 	struct {
 		int op;
 		union {
@@ -51,7 +52,8 @@  struct thread_struct {
 	.fault_addr		= NULL, \
 	.prev_sched		= NULL, \
 	.arch			= INIT_ARCH_THREAD, \
-	.request		= { 0 } \
+	.request		= { 0 }, \
+	.is_running_test	= false, \
 }
 
 static inline void release_thread(struct task_struct *task)
diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index cced829460427..bf90e678b3d71 100644
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -201,6 +201,12 @@  void segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
 	segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
 }
 
+static void segv_run_catcher(jmp_buf *catcher, void *fault_addr)
+{
+	current->thread.fault_addr = fault_addr;
+	UML_LONGJMP(catcher, 1);
+}
+
 /*
  * We give a *copy* of the faultinfo in the regs to segv.
  * This must be done, since nesting SEGVs could overwrite
@@ -219,7 +225,10 @@  unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
 	if (!is_user && regs)
 		current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
 
-	if (!is_user && (address >= start_vm) && (address < end_vm)) {
+	catcher = current->thread.fault_catcher;
+	if (catcher && current->thread.is_running_test)
+		segv_run_catcher(catcher, (void *) address);
+	else if (!is_user && (address >= start_vm) && (address < end_vm)) {
 		flush_tlb_kernel_vm();
 		goto out;
 	}
@@ -246,12 +255,10 @@  unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
 		address = 0;
 	}
 
-	catcher = current->thread.fault_catcher;
 	if (!err)
 		goto out;
 	else if (catcher != NULL) {
-		current->thread.fault_addr = (void *) address;
-		UML_LONGJMP(catcher, 1);
+		segv_run_catcher(catcher, (void *) address);
 	}
 	else if (current->thread.fault_addr != NULL)
 		panic("fault_addr set but no fault catcher");