| Message ID | mvm5zqmu35d.fsf@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: fix locking violation in page fault handler | expand |
On 7.05.19 г. 10:36 ч., Andreas Schwab wrote: > When a user mode process accesses an address in the vmalloc area > do_page_fault tries to unlock the mmap semaphore when it isn't locked. > > Signed-off-by: Andreas Schwab <schwab@suse.de> > --- > arch/riscv/mm/fault.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > index 88401d5125bc..c51878e5a66a 100644 > --- a/arch/riscv/mm/fault.c > +++ b/arch/riscv/mm/fault.c > @@ -181,6 +181,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs) > up_read(&mm->mmap_sem); > /* User mode accesses just cause a SIGSEGV */ > if (user_mode(regs)) { > +bad_area_do_trap: > do_trap(regs, SIGSEGV, code, addr, tsk); > return; > } > @@ -230,7 +231,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs) > int index; > > if (user_mode(regs)) > - goto bad_area; > + goto bad_area_do_trap; > > /* > * Synchronize this task's top level page-table > In this case I think it will be a lot cleaner if you just duplicated the do_trap call. On a slightly different note - is there any reason why do_page_fault is such a spaghetti mess? At the very least the code under no_context label could go into it's own function since it just kills the process and never returns? Furthermore the whole vmalloc_fault just cries for being factored out in a function, it's explicitly in it's own block.
On Mai 07 2019, Nikolay Borisov <nborisov@suse.com> wrote: > At the very least the code under > no_context label could go into it's own function since it just kills the > process and never returns? This is not true. Andreas.
On 7.05.19 г. 17:12 ч., Andreas Schwab wrote: > On Mai 07 2019, Nikolay Borisov <nborisov@suse.com> wrote: > >> At the very least the code under >> no_context label could go into it's own function since it just kills the >> process and never returns? > > This is not true. Be more specific, according to do_task_dead after the last __schedule is called the calling context is no more? > > Andreas. >
On Mai 07 2019, Nikolay Borisov <nborisov@suse.com> wrote: > On 7.05.19 г. 17:12 ч., Andreas Schwab wrote: >> On Mai 07 2019, Nikolay Borisov <nborisov@suse.com> wrote: >> >>> At the very least the code under >>> no_context label could go into it's own function since it just kills the >>> process and never returns? >> >> This is not true. > > Be more specific, according to do_task_dead after the last __schedule is > called the calling context is no more? /* Are we prepared to handle this kernel fault? */ if (fixup_exception(regs)) return; Andreas.
On Tue, 07 May 2019 00:36:46 PDT (-0700), schwab@suse.de wrote: > When a user mode process accesses an address in the vmalloc area > do_page_fault tries to unlock the mmap semaphore when it isn't locked. > > Signed-off-by: Andreas Schwab <schwab@suse.de> > --- > arch/riscv/mm/fault.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > index 88401d5125bc..c51878e5a66a 100644 > --- a/arch/riscv/mm/fault.c > +++ b/arch/riscv/mm/fault.c > @@ -181,6 +181,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs) > up_read(&mm->mmap_sem); > /* User mode accesses just cause a SIGSEGV */ > if (user_mode(regs)) { > +bad_area_do_trap: > do_trap(regs, SIGSEGV, code, addr, tsk); > return; > } > @@ -230,7 +231,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs) > int index; > > if (user_mode(regs)) > - goto bad_area; > + goto bad_area_do_trap; > > /* > * Synchronize this task's top level page-table I got lost with all the gotos, I think something like this is cleaner diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c index 26293bc053a8..cec8be9e2d6a 100644 --- a/arch/riscv/mm/fault.c +++ b/arch/riscv/mm/fault.c @@ -229,8 +229,9 @@ asmlinkage void do_page_fault(struct pt_regs *regs) pte_t *pte_k; int index; + /* User mode accesses just cause a SIGSEGV */ if (user_mode(regs)) - goto bad_area; + return do_trap(regs, SIGSEGV, code, addr, tsk); /* * Synchronize this task's top level page-table Unless anyone has a better idea? Either way: Reviewed-by: Palmer Dabbelt <palmer@sifive.com> LMK if you, or anyone else, has a preference. I'm assuming this will go in through my tree, so I've picked up my version for now :)
On Mai 07 2019, Palmer Dabbelt <palmer@sifive.com> wrote: > LMK if you, or anyone else, has a preference. I'm assuming this will go in > through my tree, so I've picked up my version for now :) You did? Andreas.
On Thu, 16 May 2019 00:42:01 PDT (-0700), schwab@suse.de wrote: > On Mai 07 2019, Palmer Dabbelt <palmer@sifive.com> wrote: > >> LMK if you, or anyone else, has a preference. I'm assuming this will go in >> through my tree, so I've picked up my version for now :) > > You did? It ended up landing in Linus' tree as 8fef9900d43f ("riscv: fix locking violation in page fault handler"), so it looks like I did manage avoid losing this one.
diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c index 88401d5125bc..c51878e5a66a 100644 --- a/arch/riscv/mm/fault.c +++ b/arch/riscv/mm/fault.c @@ -181,6 +181,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs) up_read(&mm->mmap_sem); /* User mode accesses just cause a SIGSEGV */ if (user_mode(regs)) { +bad_area_do_trap: do_trap(regs, SIGSEGV, code, addr, tsk); return; } @@ -230,7 +231,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs) int index; if (user_mode(regs)) - goto bad_area; + goto bad_area_do_trap; /* * Synchronize this task's top level page-table
When a user mode process accesses an address in the vmalloc area do_page_fault tries to unlock the mmap semaphore when it isn't locked. Signed-off-by: Andreas Schwab <schwab@suse.de> --- arch/riscv/mm/fault.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)