Subject: Re: [PATCH] powerpc: Fix possible deadlock on page fault

From: "Aneesh Kumar K.V" <[email-masked]>
Date: Thu, 05 Sep 2013 18:05:12 +0530

Benjamin Herrenschmidt <[email-masked]> writes:

> On Thu, 2013-09-05 at 17:18 +0530, Aneesh Kumar K.V wrote:
>> Paul Mackerras <[email-masked]> 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