* [PATCH] powerpc: Fix possible deadlock on page fault
@ 2013-09-05 7:17 Aneesh Kumar K.V
2013-09-05 9:53 ` Paul Mackerras
0 siblings, 1 reply; 5+ messages in thread
From: Aneesh Kumar K.V @ 2013-09-05 7:17 UTC (permalink / raw)
To: benh, paulus; +Cc: linuxppc-dev, Aneesh Kumar K.V
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
stack_grow_into/14082 is trying to acquire lock:
(&mm->mmap_sem){++++++}, at: [<c000000000206d28>] .might_fault+0x78/0xe0
but task is already holding lock:
(&mm->mmap_sem){++++++}, at: [<c0000000007ffd8c>] .do_page_fault+0x24c/0x910
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&mm->mmap_sem);
lock(&mm->mmap_sem);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by stack_grow_into/14082:
#0: (&mm->mmap_sem){++++++}, at: [<c0000000007ffd8c>] .do_page_fault+0x24c/0x910
stack backtrace:
CPU: 21 PID: 14082 Comm: stack_grow_into Not tainted 3.10.0-10.el7.ppc64.debug #1
Call Trace:
[c0000003d396b850] [c000000000016e7c] .show_stack+0x7c/0x1f0 (unreliable)
[c0000003d396b920] [c000000000813fc8] .dump_stack+0x28/0x3c
[c0000003d396b990] [c000000000124b90] .__lock_acquire+0x1640/0x1800
[c0000003d396bab0] [c00000000012570c] .lock_acquire+0xac/0x250
[c0000003d396bb80] [c000000000206d54] .might_fault+0xa4/0xe0
[c0000003d396bbf0] [c0000000007ffe2c] .do_page_fault+0x2ec/0x910
[c0000003d396be30] [c0000000000092e8] handle_page_fault+0x10/0x30
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/mm/fault.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 8726779..dc2902a 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -206,7 +206,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
int trap = TRAP(regs);
int is_exec = trap == 0x400;
int fault;
- int rc = 0;
+ int rc = 0, store_update;
#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
/*
@@ -280,6 +280,13 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
+ /*
+ * We want to do this outside mmap_sem, because reading code around nip
+ * can result in fault, which will cause a deadlock when called with
+ * mmap_sem held
+ */
+ store_update = store_updates_sp(regs);
+
/* When running in the kernel we expect faults to occur only to
* addresses in user space. All other faults represent errors in the
* kernel and should generate an OOPS. Unfortunately, in the case of an
@@ -346,7 +353,7 @@ retry:
* expand the stack rather than segfaulting.
*/
if (address + 2048 < uregs->gpr[1]
- && (!user_mode(regs) || !store_updates_sp(regs)))
+ && (!user_mode(regs) || !store_update))
goto bad_area;
}
if (expand_stack(vma, address))
--
1.8.1.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix possible deadlock on page fault
2013-09-05 7:17 [PATCH] powerpc: Fix possible deadlock on page fault Aneesh Kumar K.V
@ 2013-09-05 9:53 ` Paul Mackerras
2013-09-05 11:48 ` Aneesh Kumar K.V
0 siblings, 1 reply; 5+ messages in thread
From: Paul Mackerras @ 2013-09-05 9:53 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev
On Thu, Sep 05, 2013 at 12:47:02PM +0530, Aneesh Kumar K.V wrote:
> @@ -280,6 +280,13 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>
> perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>
> + /*
> + * We want to do this outside mmap_sem, because reading code around nip
> + * can result in fault, which will cause a deadlock when called with
> + * mmap_sem held
> + */
> + store_update = store_updates_sp(regs);
We should only call store_updates_sp() if user_mode(regs); that was
the previous behaviour.
Paul.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix possible deadlock on page fault
2013-09-05 9:53 ` Paul Mackerras
@ 2013-09-05 11:48 ` Aneesh Kumar K.V
2013-09-05 11:50 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 5+ messages in thread
From: Aneesh Kumar K.V @ 2013-09-05 11:48 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev
Paul Mackerras <paulus@samba.org> writes:
> On Thu, Sep 05, 2013 at 12:47:02PM +0530, Aneesh Kumar K.V wrote:
>
>> @@ -280,6 +280,13 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>>
>> perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>>
>> + /*
>> + * We want to do this outside mmap_sem, because reading code around nip
>> + * can result in fault, which will cause a deadlock when called with
>> + * mmap_sem held
>> + */
>> + store_update = store_updates_sp(regs);
>
> We should only call store_updates_sp() if user_mode(regs); that was
> the previous behaviour.
Updated to
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 8726779..fad7af6 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -206,7 +206,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
int trap = TRAP(regs);
int is_exec = trap == 0x400;
int fault;
- int rc = 0;
+ int rc = 0, store_update = 0;
#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
/*
@@ -280,6 +280,14 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
+ /*
+ * We want to do this outside mmap_sem, because reading code around nip
+ * can result in fault, which will cause a deadlock when called with
+ * mmap_sem held
+ */
+ if (user_mode(regs))
+ store_update = store_updates_sp(regs);
+
/* When running in the kernel we expect faults to occur only to
* addresses in user space. All other faults represent errors in the
* kernel and should generate an OOPS. Unfortunately, in the case of an
@@ -345,8 +353,7 @@ retry:
* between the last mapped region and the stack will
* expand the stack rather than segfaulting.
*/
- if (address + 2048 < uregs->gpr[1]
- && (!user_mode(regs) || !store_updates_sp(regs)))
+ if (address + 2048 < uregs->gpr[1] && !store_update)
goto bad_area;
}
if (expand_stack(vma, address))
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix possible deadlock on page fault
2013-09-05 11:48 ` Aneesh Kumar K.V
@ 2013-09-05 11:50 ` Benjamin Herrenschmidt
2013-09-05 12:35 ` Aneesh Kumar K.V
0 siblings, 1 reply; 5+ messages in thread
From: Benjamin Herrenschmidt @ 2013-09-05 11:50 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, Paul Mackerras
On Thu, 2013-09-05 at 17:18 +0530, Aneesh Kumar K.V wrote:
> Paul Mackerras <paulus@samba.org> writes:
>
> > On Thu, Sep 05, 2013 at 12:47:02PM +0530, Aneesh Kumar K.V wrote:
> >
> >> @@ -280,6 +280,13 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
> >>
> >> perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
> >>
> >> + /*
> >> + * We want to do this outside mmap_sem, because reading code around nip
> >> + * can result in fault, which will cause a deadlock when called with
> >> + * mmap_sem held
> >> + */
> >> + store_update = store_updates_sp(regs);
> >
> > We should only call store_updates_sp() if user_mode(regs); that was
> > the previous behaviour.
>
> Updated to
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 8726779..fad7af6 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -206,7 +206,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
> int trap = TRAP(regs);
> int is_exec = trap == 0x400;
> int fault;
> - int rc = 0;
> + int rc = 0, store_update = 0;
Keep the "sp", in the name, it's confusing otherwise. It's not just
about "store update", it's about specifically recognizing instructions
used to update the stack frame in order to let them and only them
significantly lower the stack pointer.
Cheers,
Ben.
> #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> /*
> @@ -280,6 +280,14 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>
> perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>
> + /*
> + * We want to do this outside mmap_sem, because reading code around nip
> + * can result in fault, which will cause a deadlock when called with
> + * mmap_sem held
> + */
> + if (user_mode(regs))
> + store_update = store_updates_sp(regs);
> +
> /* When running in the kernel we expect faults to occur only to
> * addresses in user space. All other faults represent errors in the
> * kernel and should generate an OOPS. Unfortunately, in the case of an
> @@ -345,8 +353,7 @@ retry:
> * between the last mapped region and the stack will
> * expand the stack rather than segfaulting.
> */
> - if (address + 2048 < uregs->gpr[1]
> - && (!user_mode(regs) || !store_updates_sp(regs)))
> + if (address + 2048 < uregs->gpr[1] && !store_update)
> goto bad_area;
> }
> if (expand_stack(vma, address))
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix possible deadlock on page fault
2013-09-05 11:50 ` Benjamin Herrenschmidt
@ 2013-09-05 12:35 ` Aneesh Kumar K.V
0 siblings, 0 replies; 5+ messages in thread
From: Aneesh Kumar K.V @ 2013-09-05 12:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Paul Mackerras
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Thu, 2013-09-05 at 17:18 +0530, Aneesh Kumar K.V wrote:
>> Paul Mackerras <paulus@samba.org> writes:
>>
>> > On Thu, Sep 05, 2013 at 12:47:02PM +0530, Aneesh Kumar K.V wrote:
>> >
>> >> @@ -280,6 +280,13 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>> >>
>> >> perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>> >>
>> >> + /*
>> >> + * We want to do this outside mmap_sem, because reading code around nip
>> >> + * can result in fault, which will cause a deadlock when called with
>> >> + * mmap_sem held
>> >> + */
>> >> + store_update = store_updates_sp(regs);
>> >
>> > We should only call store_updates_sp() if user_mode(regs); that was
>> > the previous behaviour.
>>
>> Updated to
>>
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 8726779..fad7af6 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -206,7 +206,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>> int trap = TRAP(regs);
>> int is_exec = trap == 0x400;
>> int fault;
>> - int rc = 0;
>> + int rc = 0, store_update = 0;
>
> Keep the "sp", in the name, it's confusing otherwise. It's not just
> about "store update", it's about specifically recognizing instructions
> used to update the stack frame in order to let them and only them
> significantly lower the stack pointer.
>
Ok will do that. I posted a v2. So will wait for other feedback before i
post a new version.
-aneesh
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-05 12:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-05 7:17 [PATCH] powerpc: Fix possible deadlock on page fault Aneesh Kumar K.V
2013-09-05 9:53 ` Paul Mackerras
2013-09-05 11:48 ` Aneesh Kumar K.V
2013-09-05 11:50 ` Benjamin Herrenschmidt
2013-09-05 12:35 ` Aneesh Kumar K.V
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).