Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

Michael,

On (09/27/17 15:01), Michael Ellerman wrote:
> Sergey Senozhatsky  writes:
> 
> > On (09/22/17 16:48), Luck, Tony wrote:
> > [..]
> >> Tested patch series on ia64 successfully.
> >> 
> >> Tested-by: Tony Luck 
> >
> > thanks!
> >
> >> After this goes upstream, you should submit a patch to get rid of
> >> all uses of %pF (70 instances in 35 files) and %pf (63 in 34)
> >> 
> >> Perhaps break the patch by top-level directory (e.g. get all the %pF
> >> and %pF in the 17 files under drivers/ in one patch).
> >
> > frankly, I was going to have some sort of a lazy deprecation process:
> > didn't plan to send out a patch set that would hunt down all pf/pF-s.
> > hm...
> 
> That never works though, we have lots of cruft left over from times when
> that's happened and the conversion never quite got finished.

this time around it's different, I promise! :)


well...
I guess I can send out a tree wide pf/pF removal patch set. later.
when we will see that .opd based dereference does not make anyone
unhappy.

and I think we can't remove pf/pF from the kernel completely. it
will stay in vscnprintf() for some time. old habits die hard, I suppose,
there might be people using it for debugging/etc.


> At least if you send out the patches to do the removal they might
> eventually get merged.
> 
> > speaking of upstream, any objections if this patch set will go through
> > the printk tree, in one piece?
> 
> Do you mind putting it in a topic branch (based on rc2) and then merge
> that into the printk tree? That way I can merge the topic branch iff
> there are conflicts later down the line towards 4.15.

ok, let me re-spin the series. there are some changes here
and there, so I'll drop Tested-by/Reviewed-by tags and will
ask platforms' maintainers to re-test the patch set :(

if everything goes OK, then we can ask Petr to do the topic
branch (I don't have a kernel.org account).

-ss

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

On (09/27/17 16:26), Michael Ellerman wrote:
[..]
> > Tested-by: Santosh Sivaraj 
> 
> Thanks Santosh.
> 
> I also gave it a quick spin. I'll give you an ack for the powerpc changes.
> 
> Acked-by: Michael Ellerman  (powerpc)
> 
> 
> Thanks for cleaning this up Sergey.

thanks!

-ss

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Michael Ellerman]

Santosh Sivaraj  writes:

> * Sergey Senozhatsky  wrote (on 2017-09-20 
> 16:29:02 +):
>
>> Hello
>> 
>> RFC
>> 
>> On some arches C function pointers are indirect and point to
>> a function descriptor, which contains the actual pointer to the code.
>> This mostly doesn't matter, except for cases when people want to print
>> out function pointers in symbolic format, because the usual '%pS/%ps'
>> does not work on those arches as expected. That's the reason why we
>> have '%pF/%pf', but since it's here because of a subtle ABI detail
>> specific to some arches (ppc64/ia64/parisc64) it's easy to misuse
>> '%pF/%pf' and '%pS/%ps' (see [1], for example).
>> 
>> This patch set attempts to move ia64/ppc64/parisc64 C function
>> pointer ABI details out of printk() to arch code. Function dereference
>> code now checks if a pointer belongs to a .opd ELF section and dereferences
>> that pointer only if it does. The kernel and modules have their own .opd
>> sections that's why I use two different ARCH functions: for kernel and
>> for module pointer dereference.
>> 
>> I planned to remove dereference_function_descriptor() entirely,
>> but then I discovered a bunch other uses cases (kgdbts, init/main.c,
>> extable, etc.), so I decided to keep dereference_function_descriptor()
>> around because the main point of this patch set is to deprecate %pF/%pf.
>> But at the same time, I think I can go further and handle both kernel
>> and module descriptor dereference in dereference_function_descriptor().
>> We need a module pointer for module .opd check, so that will come at an
>> extra cost of module lookup (may be there will some other issues along
>> the way, haven't checked it).
>> 
>> Right now we've got:
>> 
>> - dereference_function_descriptor(addr)
>> a generic (old) function. it simply attempts to dereference
>> whatever pointer we give it.
>> 
>> - dereference_kernel_function_descriptor(addr)
>> dereferences a kernel pointer if it's within the kernel's .opd
>> section.
>> 
>> - dereference_module_function_descriptor(module, addr)
>> dereference a module pointer if it's within the module's .opd
>> section.
>> 
>> 
>> *** A BIG NOTE ***
>> I don't own ia64/ppc64/parisc64 hardware, so the patches are not
>> tested. Sorry about that!
>
> Tested patch series on ppc64 sucessfully.
>
> You may add tested by to the series.
>
> Tested-by: Santosh Sivaraj 

Thanks Santosh.

I also gave it a quick spin. I'll give you an ack for the powerpc changes.

Acked-by: Michael Ellerman  (powerpc)


Thanks for cleaning this up Sergey.

cheers

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Michael Ellerman]

Sergey Senozhatsky  writes:

> On (09/22/17 16:48), Luck, Tony wrote:
> [..]
>> Tested patch series on ia64 successfully.
>> 
>> Tested-by: Tony Luck 
>
> thanks!
>
>> After this goes upstream, you should submit a patch to get rid of
>> all uses of %pF (70 instances in 35 files) and %pf (63 in 34)
>> 
>> Perhaps break the patch by top-level directory (e.g. get all the %pF
>> and %pF in the 17 files under drivers/ in one patch).
>
> frankly, I was going to have some sort of a lazy deprecation process:
> didn't plan to send out a patch set that would hunt down all pf/pF-s.
> hm...

That never works though, we have lots of cruft left over from times when
that's happened and the conversion never quite got finished.

At least if you send out the patches to do the removal they might
eventually get merged.

> speaking of upstream, any objections if this patch set will go through
> the printk tree, in one piece?

Do you mind putting it in a topic branch (based on rc2) and then merge
that into the printk tree? That way I can merge the topic branch iff
there are conflicts later down the line towards 4.15.

cheers

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Helge Deller]

On 25.09.2017 18:29, Luck, Tony wrote:

speaking of upstream, any objections if this patch set will go through
the printk tree, in one piece?


Fine with me too.

Helge

RE: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Luck, Tony]

> speaking of upstream, any objections if this patch set will go through
> the printk tree, in one piece?

Seems to be a better idea than trying to coordinate pulls from three
separate "arch/"  trees.  Fine with me.

-Tony

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

On (09/22/17 16:48), Luck, Tony wrote:
[..]
> Tested patch series on ia64 successfully.
> 
> Tested-by: Tony Luck 

thanks!

> After this goes upstream, you should submit a patch to get rid of
> all uses of %pF (70 instances in 35 files) and %pf (63 in 34)
> 
> Perhaps break the patch by top-level directory (e.g. get all the %pF
> and %pF in the 17 files under drivers/ in one patch).

frankly, I was going to have some sort of a lazy deprecation process:
didn't plan to send out a patch set that would hunt down all pf/pF-s.
hm...


speaking of upstream, any objections if this patch set will go through
the printk tree, in one piece?

I'll wait for several more days and then resend v3 with updated
Documentation and tweaked checkpatch warning message.

-ss

RE: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Luck, Tony]

Tested patch series on ia64 successfully.

Tested-by: Tony Luck 

After this goes upstream, you should submit a patch to get rid of
all uses of %pF (70 instances in 35 files) and %pf (63 in 34)

Perhaps break the patch by top-level directory (e.g. get all the %pF
and %pF in the 17 files under drivers/ in one patch).

-Tony

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

On (09/22/17 11:04), Santosh Sivaraj wrote:
[..]
> > *** A BIG NOTE ***
> > I don't own ia64/ppc64/parisc64 hardware, so the patches are not
> > tested. Sorry about that!
> 
> Tested patch series on ppc64 sucessfully.
> 
> You may add tested by to the series.
> 
> Tested-by: Santosh Sivaraj 


thanks!

-ss

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Santosh Sivaraj]

* Sergey Senozhatsky  wrote (on 2017-09-20 
16:29:02 +):

> Hello
> 
> RFC
> 
> On some arches C function pointers are indirect and point to
> a function descriptor, which contains the actual pointer to the code.
> This mostly doesn't matter, except for cases when people want to print
> out function pointers in symbolic format, because the usual '%pS/%ps'
> does not work on those arches as expected. That's the reason why we
> have '%pF/%pf', but since it's here because of a subtle ABI detail
> specific to some arches (ppc64/ia64/parisc64) it's easy to misuse
> '%pF/%pf' and '%pS/%ps' (see [1], for example).
> 
> This patch set attempts to move ia64/ppc64/parisc64 C function
> pointer ABI details out of printk() to arch code. Function dereference
> code now checks if a pointer belongs to a .opd ELF section and dereferences
> that pointer only if it does. The kernel and modules have their own .opd
> sections that's why I use two different ARCH functions: for kernel and
> for module pointer dereference.
> 
> I planned to remove dereference_function_descriptor() entirely,
> but then I discovered a bunch other uses cases (kgdbts, init/main.c,
> extable, etc.), so I decided to keep dereference_function_descriptor()
> around because the main point of this patch set is to deprecate %pF/%pf.
> But at the same time, I think I can go further and handle both kernel
> and module descriptor dereference in dereference_function_descriptor().
> We need a module pointer for module .opd check, so that will come at an
> extra cost of module lookup (may be there will some other issues along
> the way, haven't checked it).
> 
> Right now we've got:
> 
> - dereference_function_descriptor(addr)
> a generic (old) function. it simply attempts to dereference
> whatever pointer we give it.
> 
> - dereference_kernel_function_descriptor(addr)
> dereferences a kernel pointer if it's within the kernel's .opd
> section.
> 
> - dereference_module_function_descriptor(module, addr)
> dereference a module pointer if it's within the module's .opd
> section.
> 
> 
> *** A BIG NOTE ***
> I don't own ia64/ppc64/parisc64 hardware, so the patches are not
> tested. Sorry about that!

Tested patch series on ppc64 sucessfully.

You may add tested by to the series.

Tested-by: Santosh Sivaraj 

Thanks,
Santosh

> 
> Another note:
> I need to check what is BPF symbol lookup and do we need to
> do any dereference there.
> 
> v2:
> -- convert dereference_function_descriptor() to unsigned long
> -- fix kernel descriptor range checks (Helge)
> -- fix parisc module descriptor range check (Helge)
> -- fix ppc64 module range check
> -- add checkpatch patch
> 
> 
> Sergey Senozhatsky (7):
>   switch dereference_function_descriptor() to `unsigned long'
>   sections: split dereference_function_descriptor()
>   ia64: Add .opd based function descriptor dereference
>   powerpc64: Add .opd based function descriptor dereference
>   parisc64: Add .opd based function descriptor dereference
>   symbol lookup: use new kernel and module dereference functions
>   checkpatch: add pF/pf deprecation warning
> 
>  Documentation/printk-formats.txt  | 15 +--
>  arch/ia64/include/asm/sections.h  | 16 
>  arch/ia64/kernel/module.c | 13 +
>  arch/ia64/kernel/vmlinux.lds.S|  2 ++
>  arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
>  arch/parisc/include/asm/sections.h|  4 +++-
>  arch/parisc/kernel/module.c   | 17 +
>  arch/parisc/kernel/process.c  | 15 ---
>  arch/parisc/kernel/vmlinux.lds.S  |  2 ++
>  arch/parisc/mm/init.c |  4 ++--
>  arch/powerpc/include/asm/module.h |  3 +++
>  arch/powerpc/include/asm/sections.h   | 17 ++---
>  arch/powerpc/kernel/module_64.c   | 16 
>  arch/powerpc/kernel/vmlinux.lds.S |  2 ++
>  drivers/misc/kgdbts.c |  2 +-
>  include/asm-generic/sections.h|  8 ++--
>  include/linux/moduleloader.h  |  4 
>  init/main.c   |  2 +-
>  kernel/extable.c  |  2 +-
>  kernel/kallsyms.c |  1 +
>  kernel/module.c   |  9 -
>  lib/vsprintf.c|  5 +
>  scripts/checkpatch.pl |  6 --
>  23 files changed, 132 insertions(+), 35 deletions(-)

-- 


signature.asc
Description: PGP signature

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

On (09/20/17 22:14), Helge Deller wrote:
> On 20.09.2017 18:29, Sergey Senozhatsky wrote:
> >  This patch set attempts to move ia64/ppc64/parisc64 C function
> > pointer ABI details out of printk() to arch code. Function dereference
> > code now checks if a pointer belongs to a .opd ELF section and dereferences
> > that pointer only if it does. The kernel and modules have their own .opd
> > sections that's why I use two different ARCH functions: for kernel and
> > for module pointer dereference.
> > ...> *** A BIG NOTE ***
> >  I don't own ia64/ppc64/parisc64 hardware, so the patches are not
> >  tested. Sorry about that!
> 
> 
> I just now tested your patch series successfully on parisc64.
> 
> You may add to the whole series:
> Tested-by: Helge Deller  # parisc64

thanks, Helge!

> > Another note:
> >  I need to check what is BPF symbol lookup and do we need to
> >  do any dereference there.
> 
> Not relevant for parisc, since we don't support it yet.

so that was my suspicion as well. at glance it didn't look like
bpf symbol resolution would work on platforms that do description
dereference.

-ss

Re: [RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Helge Deller]

On 20.09.2017 18:29, Sergey Senozhatsky wrote:

 This patch set attempts to move ia64/ppc64/parisc64 C function
pointer ABI details out of printk() to arch code. Function dereference
code now checks if a pointer belongs to a .opd ELF section and dereferences
that pointer only if it does. The kernel and modules have their own .opd
sections that's why I use two different ARCH functions: for kernel and
for module pointer dereference.
...> *** A BIG NOTE ***
 I don't own ia64/ppc64/parisc64 hardware, so the patches are not
 tested. Sorry about that!



I just now tested your patch series successfully on parisc64.

You may add to the whole series:
Tested-by: Helge Deller  # parisc64

 

Another note:
 I need to check what is BPF symbol lookup and do we need to
 do any dereference there.


Not relevant for parisc, since we don't support it yet.

Helge

[RFC][PATCH v2 0/7] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers

[Sergey Senozhatsky]

Hello

RFC

On some arches C function pointers are indirect and point to
a function descriptor, which contains the actual pointer to the code.
This mostly doesn't matter, except for cases when people want to print
out function pointers in symbolic format, because the usual '%pS/%ps'
does not work on those arches as expected. That's the reason why we
have '%pF/%pf', but since it's here because of a subtle ABI detail
specific to some arches (ppc64/ia64/parisc64) it's easy to misuse
'%pF/%pf' and '%pS/%ps' (see [1], for example).

This patch set attempts to move ia64/ppc64/parisc64 C function
pointer ABI details out of printk() to arch code. Function dereference
code now checks if a pointer belongs to a .opd ELF section and dereferences
that pointer only if it does. The kernel and modules have their own .opd
sections that's why I use two different ARCH functions: for kernel and
for module pointer dereference.

I planned to remove dereference_function_descriptor() entirely,
but then I discovered a bunch other uses cases (kgdbts, init/main.c,
extable, etc.), so I decided to keep dereference_function_descriptor()
around because the main point of this patch set is to deprecate %pF/%pf.
But at the same time, I think I can go further and handle both kernel
and module descriptor dereference in dereference_function_descriptor().
We need a module pointer for module .opd check, so that will come at an
extra cost of module lookup (may be there will some other issues along
the way, haven't checked it).

Right now we've got:

- dereference_function_descriptor(addr)
a generic (old) function. it simply attempts to dereference
whatever pointer we give it.

- dereference_kernel_function_descriptor(addr)
dereferences a kernel pointer if it's within the kernel's .opd
section.

- dereference_module_function_descriptor(module, addr)
dereference a module pointer if it's within the module's .opd
section.


*** A BIG NOTE ***
I don't own ia64/ppc64/parisc64 hardware, so the patches are not
tested. Sorry about that!

Another note:
I need to check what is BPF symbol lookup and do we need to
do any dereference there.

v2:
-- convert dereference_function_descriptor() to unsigned long
-- fix kernel descriptor range checks (Helge)
-- fix parisc module descriptor range check (Helge)
-- fix ppc64 module range check
-- add checkpatch patch


Sergey Senozhatsky (7):
  switch dereference_function_descriptor() to `unsigned long'
  sections: split dereference_function_descriptor()
  ia64: Add .opd based function descriptor dereference
  powerpc64: Add .opd based function descriptor dereference
  parisc64: Add .opd based function descriptor dereference
  symbol lookup: use new kernel and module dereference functions
  checkpatch: add pF/pf deprecation warning

 Documentation/printk-formats.txt  | 15 +--
 arch/ia64/include/asm/sections.h  | 16 
 arch/ia64/kernel/module.c | 13 +
 arch/ia64/kernel/vmlinux.lds.S|  2 ++
 arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
 arch/parisc/include/asm/sections.h|  4 +++-
 arch/parisc/kernel/module.c   | 17 +
 arch/parisc/kernel/process.c  | 15 ---
 arch/parisc/kernel/vmlinux.lds.S  |  2 ++
 arch/parisc/mm/init.c |  4 ++--
 arch/powerpc/include/asm/module.h |  3 +++
 arch/powerpc/include/asm/sections.h   | 17 ++---
 arch/powerpc/kernel/module_64.c   | 16 
 arch/powerpc/kernel/vmlinux.lds.S |  2 ++
 drivers/misc/kgdbts.c |  2 +-
 include/asm-generic/sections.h|  8 ++--
 include/linux/moduleloader.h  |  4 
 init/main.c   |  2 +-
 kernel/extable.c  |  2 +-
 kernel/kallsyms.c |  1 +
 kernel/module.c   |  9 -
 lib/vsprintf.c|  5 +
 scripts/checkpatch.pl |  6 --
 23 files changed, 132 insertions(+), 35 deletions(-)

-- 
2.14.1