[PATCH 06/14] md/bcache: Use %pS printk format for direct addresses

[PATCH 06/14] md/bcache: Use %pS printk format for direct addresses

[Helge Deller]

Use the %pS instead of the %pF printk format specifier for printing symbols
from direct addresses. This is needed for the ia64, ppc64 and parisc64
architectures.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: linux-bcache@vger.kernel.org
Cc: linux-raid@vger.kernel.org
---
 drivers/md/bcache/closure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
index 864e673..0b0c9bc 100644
--- a/drivers/md/bcache/closure.c
+++ b/drivers/md/bcache/closure.c
@@ -175,7 +175,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
 	list_for_each_entry(cl, &closure_list, all) {
 		int r = atomic_read(&cl->remaining);
 
-		seq_printf(f, "%p: %pF -> %pf p %p r %i ",
+		seq_printf(f, "%p: %pS -> %pf p %p r %i ",
 			   cl, (void *) cl->ip, cl->fn, cl->parent,
 			   r & CLOSURE_REMAINING_MASK);
 
@@ -187,7 +187,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
 			   r & CLOSURE_SLEEPING	? "Sl" : "");
 
 		if (r & CLOSURE_WAITING)
-			seq_printf(f, " W %pF\n",
+			seq_printf(f, " W %pS\n",
 				   (void *) cl->waiting_on);
 
 		seq_printf(f, "\n");
-- 
2.1.0

Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct addresses

[Coly Li]

On 2017/9/7 上午4:27, Helge Deller wrote:
> Use the %pS instead of the %pF printk format specifier for printing symbols
> from direct addresses. This is needed for the ia64, ppc64 and parisc64
> architectures.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: linux-bcache@vger.kernel.org
> Cc: linux-raid@vger.kernel.org
> ---
>  drivers/md/bcache/closure.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
> index 864e673..0b0c9bc 100644
> --- a/drivers/md/bcache/closure.c
> +++ b/drivers/md/bcache/closure.c
> @@ -175,7 +175,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
>  	list_for_each_entry(cl, &closure_list, all) {
>  		int r = atomic_read(&cl->remaining);
>  
> -		seq_printf(f, "%p: %pF -> %pf p %p r %i ",
> +		seq_printf(f, "%p: %pS -> %pf p %p r %i ",
>  			   cl, (void *) cl->ip, cl->fn, cl->parent,
>  			   r & CLOSURE_REMAINING_MASK);
>  
> @@ -187,7 +187,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
>  			   r & CLOSURE_SLEEPING	? "Sl" : "");
>  
>  		if (r & CLOSURE_WAITING)
> -			seq_printf(f, " W %pF\n",
> +			seq_printf(f, " W %pS\n",
>  				   (void *) cl->waiting_on);
>  
>  		seq_printf(f, "\n");
> 

It is unclear to me, that if %pF is used, on ia64/ppc64/parisc64 a
function descriptor conversion happens, what negative effect exactly
takes place ? Or you just want to unify the out put format, and get rid
of extra conversion information ?

Thanks.

-- 
Coly Li

Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct addresses

[Helge Deller]

On 07.09.2017 06:50, Coly Li wrote:
> On 2017/9/7 上午4:27, Helge Deller wrote:
>> Use the %pS instead of the %pF printk format specifier for printing symbols
>> from direct addresses. This is needed for the ia64, ppc64 and parisc64
>> architectures.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: linux-bcache@vger.kernel.org
>> Cc: linux-raid@vger.kernel.org
>> ---
>>  drivers/md/bcache/closure.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
>> index 864e673..0b0c9bc 100644
>> --- a/drivers/md/bcache/closure.c
>> +++ b/drivers/md/bcache/closure.c
>> @@ -175,7 +175,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
>>  	list_for_each_entry(cl, &closure_list, all) {
>>  		int r = atomic_read(&cl->remaining);
>>  
>> -		seq_printf(f, "%p: %pF -> %pf p %p r %i ",
>> +		seq_printf(f, "%p: %pS -> %pf p %p r %i ",
>>  			   cl, (void *) cl->ip, cl->fn, cl->parent,
>>  			   r & CLOSURE_REMAINING_MASK);
>>  
>> @@ -187,7 +187,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
>>  			   r & CLOSURE_SLEEPING	? "Sl" : "");
>>  
>>  		if (r & CLOSURE_WAITING)
>> -			seq_printf(f, " W %pF\n",
>> +			seq_printf(f, " W %pS\n",
>>  				   (void *) cl->waiting_on);
>>  
>>  		seq_printf(f, "\n");
>>
> 
> It is unclear to me, that if %pF is used, on ia64/ppc64/parisc64 a
> function descriptor conversion happens, what negative effect exactly
> takes place ?

On ia64/ppc64/parisc64 the kernel will crash here in the worst case, because
vsprintf() will try to read a pointer from that address and resolve it. 
But you hand over a return address (_RET_IP_) which should be 
resolved directly to a symbol. For that you need to use the %pS specifier,
which is what my patch does.

The patch has no negative effect on any platform. Output will still be the
same on x86/mips/arm/... as it has been before with %pF. The only positive
effect of the patch is that the seq_printf will now show correct output on 
ia64/ppc64/parisc64 too.

Helge

Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct addresses

[Coly Li]

On 2017/9/7 下午3:42, Helge Deller wrote:
> On 07.09.2017 06:50, Coly Li wrote:
>> On 2017/9/7 上午4:27, Helge Deller wrote:
>>> Use the %pS instead of the %pF printk format specifier for printing symbols
>>> from direct addresses. This is needed for the ia64, ppc64 and parisc64
>>> architectures.
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>> Cc: linux-bcache@vger.kernel.org
>>> Cc: linux-raid@vger.kernel.org
>>> ---
>>>  drivers/md/bcache/closure.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/md/bcache/closure.c b/drivers/md/bcache/closure.c
>>> index 864e673..0b0c9bc 100644
>>> --- a/drivers/md/bcache/closure.c
>>> +++ b/drivers/md/bcache/closure.c
>>> @@ -175,7 +175,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
>>>  	list_for_each_entry(cl, &closure_list, all) {
>>>  		int r = atomic_read(&cl->remaining);
>>>  
>>> -		seq_printf(f, "%p: %pF -> %pf p %p r %i ",
>>> +		seq_printf(f, "%p: %pS -> %pf p %p r %i ",
>>>  			   cl, (void *) cl->ip, cl->fn, cl->parent,
>>>  			   r & CLOSURE_REMAINING_MASK);
>>>  
>>> @@ -187,7 +187,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
>>>  			   r & CLOSURE_SLEEPING	? "Sl" : "");
>>>  
>>>  		if (r & CLOSURE_WAITING)
>>> -			seq_printf(f, " W %pF\n",
>>> +			seq_printf(f, " W %pS\n",
>>>  				   (void *) cl->waiting_on);
>>>  
>>>  		seq_printf(f, "\n");
>>>
>>
>> It is unclear to me, that if %pF is used, on ia64/ppc64/parisc64 a
>> function descriptor conversion happens, what negative effect exactly
>> takes place ?
> 
> On ia64/ppc64/parisc64 the kernel will crash here in the worst case, because
> vsprintf() will try to read a pointer from that address and resolve it. 
> But you hand over a return address (_RET_IP_) which should be 
> resolved directly to a symbol. For that you need to use the %pS specifier,
> which is what my patch does.
> 
> The patch has no negative effect on any platform. Output will still be the
> same on x86/mips/arm/... as it has been before with %pF. The only positive
> effect of the patch is that the seq_printf will now show correct output on 
> ia64/ppc64/parisc64 too.

Oh I see. The patch is OK to me, but could you please add the above
information in the commit log, it will help other bcache developers to
understand the patch. Thanks in advance for doing this.

BTW, I also suggest to fix vsprintf() for this issue. For most of
developers, we don't have sense for such issue on ia64/ppc64/parisc64,
it is still very probably someone else makes similar mistake in future.
If vsprintf() can be fixed, the potential risk can be safe.


-- 
Coly Li

Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct addresses

[Sergey Senozhatsky]

On (09/07/17 09:42), Helge Deller wrote:
> >> -		seq_printf(f, "%p: %pF -> %pf p %p r %i ",
> >> +		seq_printf(f, "%p: %pS -> %pf p %p r %i ",
> >>  			   cl, (void *) cl->ip, cl->fn, cl->parent,
> >>  			   r & CLOSURE_REMAINING_MASK);
> >>  
> >> @@ -187,7 +187,7 @@ static int debug_seq_show(struct seq_file *f, void *data)
> >>  			   r & CLOSURE_SLEEPING	? "Sl" : "");
> >>  
> >>  		if (r & CLOSURE_WAITING)
> >> -			seq_printf(f, " W %pF\n",
> >> +			seq_printf(f, " W %pS\n",
> >>  				   (void *) cl->waiting_on);
> >>  
> >>  		seq_printf(f, "\n");
> >>
> > 
> > It is unclear to me, that if %pF is used, on ia64/ppc64/parisc64 a
> > function descriptor conversion happens, what negative effect exactly
> > takes place ?
> 
> On ia64/ppc64/parisc64 the kernel will crash here in the worst case, because
> vsprintf() will try to read a pointer from that address and resolve it. 

probe_kernel_address() handles the page fault and returns -EFAULT if
you give it bad pointer. module_address_lookup() and get_symbol_pos()
seems to be smart enough not to crash on bad pointer as well. what am
I missing? could you please explain where we will crash?

	-ss