Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Michael Ellerman <mpe@ellerman.id.au>]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 03/08/2018 à 02:42, Murilo Opsfelder Araujo a écrit :
>> Hi, Christophe.
>> On Thu, Aug 02, 2018 at 07:26:20AM +0200, Christophe LEROY wrote:
>>> Le 01/08/2018 à 23:33, Murilo Opsfelder Araujo a écrit :
>>>> show_user_instructions() is a slightly modified version of
>>>> show_instructions() that allows userspace instruction dump.
>>>>
>>>> This will be useful within show_signal_msg() to dump userspace
>>>> instructions of the faulty location.
>>>>
>>>> Here is a sample of what show_user_instructions() outputs:
>>>>
>>>>     pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
>>>>     pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040
>>>>
>>>> The current->comm and current->pid printed can serve as a glue that
>>>> links the instructions dump to its originator, allowing messages to be
>>>> interleaved in the logs.
>>>>
>>>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>>>> index e9533b4d2f08..364645ac732c 100644
>>>> --- a/arch/powerpc/kernel/process.c
>>>> +++ b/arch/powerpc/kernel/process.c
>>>> @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
>>>>    	pr_cont("\n");
>>>>    }
>>>> +void show_user_instructions(struct pt_regs *regs)
>>>> +{
>>>> +	int i;
>>>> +	const char *prefix = KERN_INFO "%s[%d]: code: ";
>>>> +	unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
>>>> +					sizeof(int));
>>>> +
>>>> +	printk(prefix, current->comm, current->pid);
>>>
>>> Why not use pr_info() and remove KERN_INFO from *prefix ?
>> 
>> Because it doesn't compile:
>> 
>>    arch/powerpc/kernel/process.c:1317:10: error: expected ‘)’ before ‘prefix’
>>      pr_info(prefix, current->comm, current->pid);
>>              ^
>>    ./include/linux/printk.h:288:21: note: in definition of macro ‘pr_fmt’
>>     #define pr_fmt(fmt) fmt
>>                       ^
>> 
>> `pr_info(prefix, ...)` expands to `printk("\001" "6" prefix, ...)`,
>> which is an invalid string concatenation.
>> 
>> `pr_info("%s", ...)` expands to `printk("\001" "6" "%s", ...)`, which is
>> valid.
>
> Then what about using directly:
>
> pr_info("%s[%d]: code: ", ...);

Yeah that's better, I'll fix it up when applying.

>>>> +#if !defined(CONFIG_BOOKE)
>>>> +		/* If executing with the IMMU off, adjust pc rather
>>>> +		 * than print XXXXXXXX.
>>>> +		 */
>>>> +		if (!(regs->msr & MSR_IR))
>>>> +			pc = (unsigned long)phys_to_virt(pc);
>>>
>>> Shouldn't this be done outside of the loop, only once ?
>> 
>> I don't think so.
>> 
>> pc gets incremented at the bottom of the loop:
>> 
>>    pc += sizeof(int);
>> 
>> Adjusting pc is necessary at each iteration.  Leaving this block inside
>> the loop seems correct.
>
> This looks pretty strange.
> The first time, pc is a physical address, that you change to a virtual 
> address. Then when you increment it it is still a virtual address.
> So when you call phys_to_virt(pc) for the second time, pc is already a 
> virt address, so what happens indeed ?

Yeah that's a bit fishy.

On 64-bit it works because phys_to_virt() == __va() which is:

  #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET))

ie. it uses bitwise or, so __va(__va(x)) == __va(x).

But it looks like on 32-bit it's going to do the wrong thing. Do we ever
actually hit that case though, I'm not sure?


However for this patch I'll just remove the whole thing, because we
don't expect to be dumping user instructions in realmode.

cheers