| Message ID | 20181128193636.254378-9-brendanhiggins@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kunit: introduce KUnit, the Linux kernel unit testing framework | expand |
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
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
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?
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/ ?
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
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!
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");
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(-)