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

	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.


[1] https://marc.info/?l=linux-kernel&m=150472969730573

Sergey Senozhatsky (5):
  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

 Documentation/printk-formats.txt          | 15 +++++----------
 arch/ia64/include/asm/sections.h          | 14 +++++++++++++-
 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        |  3 +++
 arch/parisc/kernel/module.c               | 14 ++++++++++++++
 arch/parisc/kernel/process.c              | 10 ++++++++++
 arch/parisc/kernel/vmlinux.lds.S          |  2 ++
 arch/powerpc/include/asm/module.h         |  3 +++
 arch/powerpc/include/asm/sections.h       | 13 +++++++++++++
 arch/powerpc/kernel/module_64.c           | 16 ++++++++++++++++
 arch/powerpc/kernel/vmlinux.lds.S         |  2 ++
 include/asm-generic/sections.h            |  4 ++--
 include/linux/moduleloader.h              |  4 ++++
 kernel/kallsyms.c                         |  1 +
 kernel/module.c                           |  7 +++++++
 lib/vsprintf.c                            |  5 +----
 18 files changed, 113 insertions(+), 17 deletions(-)

-- 
2.14.1

There are two format specifiers to print out a pointer in symbolic
format: '%pS/%ps' and '%pF/%pf'. On most architectures, the two
mean exactly the same thing, but some architectures (ia64, ppc64,
parisc64) use an indirect pointer for C function pointers, where
the function pointer points to a function descriptor (which in
turn contains the actual pointer to the code). The '%pF/%pf, when
used appropriately, automatically does the appropriate function
descriptor dereference on such architectures.

The "when used appropriately" part is tricky. Basically this is
a subtle ABI detail, specific to some platforms, that made it to
the API level and people can be unaware of it and miss the whole
"we need to dereference the function" business out. [1] proves
that point (note that it fixes only '%pF' and '%pS', there might
be '%pf' and '%ps' cases as well).

It appears that we can handle everything within the affected
arches and make '%pS/%ps' smart enough to retire '%pF/%pf'.
Function descriptors live in .opd elf section and all affected
arches (ia64, ppc64, parisc64) handle it properly for kernel
and modules. So we, technically, can decide if the dereference
is needed by simply looking at the pointer: if it belongs to
.opd section then we need to dereference it.

The kernel and modules have their own .opd sections, obviously,
that's why we need to split dereference_function_descriptor()
and use separate kernel and module dereference arch callbacks.

This patch does the first step, it
a) adds dereference_kernel_function_descriptor() function.
b) adds a weak alias to dereference_module_function_descriptor()
   function.

So, for the time being, we will have:
1) dereference_function_descriptor()
   A generic function, that simply dereferences the pointer. There is
   bunch of places that call it: kgdbts, init/main.c, extable, etc.

2) dereference_kernel_function_descriptor()
   A function to call on kernel symbols that does kernel .opd section
   address range test.

3) dereference_module_function_descriptor()
   A function to call on modules' symbols that does modules' .opd
   section address range test.

[1] https://marc.info/?l=linux-kernel&m=150472969730573

Signed-off-by: Sergey Senozhatsky <redacted>
---
 include/asm-generic/sections.h | 4 ++--
 include/linux/moduleloader.h   | 4 ++++
 kernel/module.c                | 6 ++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index e5da44eddd2f..21d2165e531a 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h

@@ -49,10 +49,10 @@ extern char __ctors_start[], __ctors_end[];
 
 extern __visible const void __nosave_begin, __nosave_end;
 
-/* function descriptor handling (if any).  Override
- * in asm/sections.h */
+/* Function descriptor handling (if any).  Override in asm/sections.h */
 #ifndef dereference_function_descriptor
 #define dereference_function_descriptor(p) (p)
+#define dereference_kernel_function_descriptor(p) (p)
 #endif
 
 /* random extra sections (if any).  Override

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 4d0cb9bba93e..172904e9cded 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h

@@ -85,6 +85,10 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+/* Dereference module function descriptor */
+unsigned long dereference_module_function_descriptor(struct module *mod,
+						     unsigned long addr);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)

diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..87cdb46863cd 100644
--- a/kernel/module.c
+++ b/kernel/module.c

@@ -2121,6 +2121,12 @@ void __weak module_arch_freeing_init(struct module *mod)
 {
 }
 
+unsigned long __weak dereference_module_function_descriptor(struct module *mod,
+							    unsigned long addr)
+{
+	return addr;
+}
+
 /* Free a module, remove from lists, etc. */
 static void free_module(struct module *mod)
 {

-- 
2.14.1

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for IA64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd];

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end].

Signed-off-by: Sergey Senozhatsky <redacted>
---
 arch/ia64/include/asm/sections.h | 14 +++++++++++++-
 arch/ia64/kernel/module.c        | 13 +++++++++++++
 arch/ia64/kernel/vmlinux.lds.S   |  2 ++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 2ab2003698ef..bff3f3535609 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h

@@ -25,8 +25,11 @@ extern char __start_gate_fsyscall_patchlist[], __end_gate_fsyscall_patchlist[];
 extern char __start_gate_brl_fsys_bubble_down_patchlist[], __end_gate_brl_fsys_bubble_down_patchlist[];
 extern char __start_unwind[], __end_unwind[];
 extern char __start_ivt_text[], __end_ivt_text[];
+extern char __start_opd[], __end_opd[];
 
 #undef dereference_function_descriptor
+#undef dereference_kernel_function_descriptor
+
 static inline void *dereference_function_descriptor(void *ptr)
 {
 	struct fdesc *desc = ptr;

@@ -37,6 +40,15 @@ static inline void *dereference_function_descriptor(void *ptr)
 	return ptr;
 }
 
+static inline void *dereference_kernel_function_descriptor(void *ptr)
+{
+	/*
+	 * Check if the ptr is a function descriptor and thus needs to
+	 * be dereferenced.
+	 */
+	if (ptr < (void *)__start_opd || (void *)__end_opd < ptr)
+		return ptr;
+	return dereference_function_descriptor(ptr);
+}
 
 #endif /* _ASM_IA64_SECTIONS_H */
-

diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index d1d945c6bd05..d42f1e19d75d 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c

@@ -35,6 +35,7 @@
 
 #include <asm/patch.h>
 #include <asm/unaligned.h>
+#include <asm/sections.h>
 
 #define ARCH_MODULE_DEBUG 0
 

@@ -917,3 +918,15 @@ module_arch_cleanup (struct module *mod)
 	if (mod->arch.core_unw_table)
 		unw_remove_unwind_table(mod->arch.core_unw_table);
 }
+
+unsigned long dereference_module_function_descriptor(struct module *mod,
+					     unsigned long addr)
+{
+	Elf64_Shdr *opd = mod->arch.opd;
+
+	if (addr < opd->sh_addr ||
+			(opd->sh_addr + opd->sh_size) < addr)
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}

diff --git a/arch/ia64/kernel/vmlinux.lds.S b/arch/ia64/kernel/vmlinux.lds.S
index 798026dde52e..f872ba5ff82a 100644
--- a/arch/ia64/kernel/vmlinux.lds.S
+++ b/arch/ia64/kernel/vmlinux.lds.S

@@ -107,7 +107,9 @@ SECTIONS {
 	RODATA
 
 	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	}
 
 	/*

-- 
2.14.1

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for powerpc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd];

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end].

Signed-off-by: Sergey Senozhatsky <redacted>
---
 arch/powerpc/include/asm/module.h   |  3 +++
 arch/powerpc/include/asm/sections.h | 13 +++++++++++++
 arch/powerpc/kernel/module_64.c     | 16 ++++++++++++++++
 arch/powerpc/kernel/vmlinux.lds.S   |  2 ++
 4 files changed, 34 insertions(+)

diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 6c0132c7212f..7e28442827f1 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h

@@ -45,6 +45,9 @@ struct mod_arch_specific {
 	unsigned long tramp;
 #endif
 
+	/* For module function descriptor dereference */
+	unsigned long start_opd;
+	unsigned long end_opd;
 #else /* powerpc64 */
 	/* Indices of PLT sections within module. */
 	unsigned int core_plt_section;

diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 7902d6358854..7cc4db86952b 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h

@@ -16,6 +16,9 @@ extern char __end_interrupts[];
 extern char __prom_init_toc_start[];
 extern char __prom_init_toc_end[];
 
+extern char __start_opd[];
+extern char __end_opd[];
+
 static inline int in_kernel_text(unsigned long addr)
 {
 	if (addr >= (unsigned long)_stext && addr < (unsigned long)__init_end)

@@ -66,6 +69,8 @@ static inline int overlaps_kvm_tmp(unsigned long start, unsigned long end)
 
 #ifdef PPC64_ELF_ABI_v1
 #undef dereference_function_descriptor
+#undef dereference_kernel_function_descriptor
+
 static inline void *dereference_function_descriptor(void *ptr)
 {
 	struct ppc64_opd_entry *desc = ptr;

@@ -75,6 +80,14 @@ static inline void *dereference_function_descriptor(void *ptr)
 		ptr = p;
 	return ptr;
 }
+
+static inline void *dereference_kernel_function_descriptor(void *ptr)
+{
+	if (ptr < (void *)__start_opd || (void *)__end_opd < ptr)
+		return ptr;
+
+	return dereference_function_descriptor(ptr);
+}
 #endif /* PPC64_ELF_ABI_v1 */
 
 #endif

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 0b0f89685b67..52aa5d668364 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c

@@ -344,6 +344,11 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
 		else if (strcmp(secstrings+sechdrs[i].sh_name,"__versions")==0)
 			dedotify_versions((void *)hdr + sechdrs[i].sh_offset,
 					  sechdrs[i].sh_size);
+		else if (strcmp(secstrings + sechdrs[i].sh_name, ".opd")==0) {
+			me->arch.start_opd = sechdrs[i].sh_offset;
+			me->arch.end_opd = me->arch.start_opd +
+					sechdrs[i].sh_size;
+		}
 
 		/* We don't handle .init for the moment: rename to _init */
 		while ((p = strstr(secstrings + sechdrs[i].sh_name, ".init")))

@@ -712,6 +717,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	return 0;
 }
 
+#ifdef PPC64_ELF_ABI_v1
+unsigned long dereference_module_function_descriptor(struct module *mod,
+						     unsigned long addr)
+{
+	if (addr < mod->arch.start_opd || mod->arch.end_opd < addr)
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}
+#endif /* PPC64_ELF_ABI_v1 */
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 #ifdef CC_USING_MPROFILE_KERNEL

diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 882628fa6987..70e10251e083 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S

@@ -277,7 +277,9 @@ SECTIONS
 	}
 
 	.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	}
 
 	. = ALIGN(256);

-- 
2.14.1

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for parisc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd];

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end].

Signed-off-by: Sergey Senozhatsky <redacted>
---
 arch/parisc/boot/compressed/vmlinux.lds.S |  2 ++
 arch/parisc/include/asm/sections.h        |  3 +++
 arch/parisc/kernel/module.c               | 14 ++++++++++++++
 arch/parisc/kernel/process.c              | 10 ++++++++++
 arch/parisc/kernel/vmlinux.lds.S          |  2 ++
 5 files changed, 31 insertions(+)

diff --git a/arch/parisc/boot/compressed/vmlinux.lds.S b/arch/parisc/boot/compressed/vmlinux.lds.S
index a4ce3314e78e..4ebd4e65524c 100644
--- a/arch/parisc/boot/compressed/vmlinux.lds.S
+++ b/arch/parisc/boot/compressed/vmlinux.lds.S

@@ -29,7 +29,9 @@ SECTIONS
 	. = ALIGN(16);
 	/* Linkage tables */
 	.opd : {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	} PROVIDE (__gp = .);
 	.plt : {
 		*(.plt)

diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index 9d13c3507ad6..e3cde650b2f9 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h

@@ -6,7 +6,10 @@
 
 #ifdef CONFIG_64BIT
 #undef dereference_function_descriptor
+#undef dereference_kernel_function_descriptor
+
 void *dereference_function_descriptor(void *);
+void *dereference_kernel_function_descriptor(void *);
 #endif
 
 #endif

diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index f1a76935a314..bc2eae8634fd 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c

@@ -954,3 +954,17 @@ void module_arch_cleanup(struct module *mod)
 {
 	deregister_unwind_table(mod);
 }
+
+#ifdef CONFIG_64BIT
+unsigned long dereference_module_function_descriptor(struct module *mod,
+					     unsigned long addr)
+{
+	void *opd_sz = mod->arch.fdesc_offset +
+		       mod->arch.fdesc_max * sizeof(Elf64_Fdesc);
+
+	if (addr < mod->arch.fdesc_offset || opd_sz < addr)
+		return addr;
+
+	return dereference_function_descriptor(addr);
+}
+#endif

diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index 30f92391a93e..f30776bdaa79 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c

@@ -267,6 +267,8 @@ get_wchan(struct task_struct *p)
 }
 
 #ifdef CONFIG_64BIT
+extern char __start_opd[], __end_opd[];
+
 void *dereference_function_descriptor(void *ptr)
 {
 	Elf64_Fdesc *desc = ptr;

@@ -276,6 +278,14 @@ void *dereference_function_descriptor(void *ptr)
 		ptr = p;
 	return ptr;
 }
+
+void *dereference_kernel_function_descriptor(void *ptr)
+{
+	if (ptr < (void *)__start_opd || (void *)__end_opd < ptr)
+		return ptr;
+
+	return dereference_function_descriptor(ptr);
+}
 #endif
 
 static inline unsigned long brk_rnd(void)

diff --git a/arch/parisc/kernel/vmlinux.lds.S b/arch/parisc/kernel/vmlinux.lds.S
index ffe2cbf52d1a..ab030895dd1e 100644
--- a/arch/parisc/kernel/vmlinux.lds.S
+++ b/arch/parisc/kernel/vmlinux.lds.S

@@ -99,7 +99,9 @@ SECTIONS
 	. = ALIGN(16);
 	/* Linkage tables */
 	.opd : {
+		__start_opd = .;
 		*(.opd)
+		__end_opd = .;
 	} PROVIDE (__gp = .);
 	.plt : {
 		*(.plt)

-- 
2.14.1

Call appropriate function descriptor dereference ARCH callbacks:
- dereference_kernel_function_descriptor() if the pointer is a
  kernel symbol;

- dereference_module_function_descriptor() if the pointer is a
  module symbol.

This patch also removes dereference_function_descriptor() from
'%pF/%pf' vsprintf handler, because it has the same behavior with
'%pS/%ps' now.

Signed-off-by: Sergey Senozhatsky <redacted>
---
 Documentation/printk-formats.txt | 15 +++++----------
 kernel/kallsyms.c                |  1 +
 kernel/module.c                  |  1 +
 lib/vsprintf.c                   |  5 +----
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 361789df51ec..b2afafc84638 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt

@@ -50,26 +50,23 @@ Symbols/Function Pointers
 
 ::
 
+	%pS	versatile_init+0x0/0x110
+	%ps	versatile_init
 	%pF	versatile_init+0x0/0x110
 	%pf	versatile_init
-	%pS	versatile_init+0x0/0x110
 	%pSR	versatile_init+0x9/0x110
 		(with __builtin_extract_return_addr() translation)
-	%ps	versatile_init
 	%pB	prev_fn_of_versatile_init+0x88/0x88
 
-The ``F`` and ``f`` specifiers are for printing function pointers,
-for example, f->func, &gettimeofday. They have the same result as
-``S`` and ``s`` specifiers. But they do an extra conversion on
-ia64, ppc64 and parisc64 architectures where the function pointers
-are actually function descriptors.
-
 The ``S`` and ``s`` specifiers can be used for printing symbols
 from direct addresses, for example, __builtin_return_address(0),
 (void *)regs->ip. They result in the symbol name with (``S``) or
 without (``s``) offsets. If KALLSYMS are disabled then the symbol
 address is printed instead.
 
+Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
+and thus deprecated.
+
 The ``B`` specifier results in the symbol name with offsets and should be
 used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur

@@ -77,8 +74,6 @@ when tail-call``s are used and marked with the noreturn GCC attribute.
 
 Examples::
 
-	printk("Going to call: %pF\n", gettimeofday);
-	printk("Going to call: %pF\n", p->func);
 	printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
 	printk("%s: called from %pS\n", __func__,
 				(void *)__builtin_return_address(0));

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 127e7cfafa55..e2fc09ea9509 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c

@@ -322,6 +322,7 @@ const char *kallsyms_lookup(unsigned long addr,
 	if (is_ksym_addr(addr)) {
 		unsigned long pos;
 
+		addr = dereference_kernel_function_descriptor(addr);
 		pos = get_symbol_pos(addr, symbolsize, offset);
 		/* Grab name */
 		kallsyms_expand_symbol(get_symbol_offset(pos),

diff --git a/kernel/module.c b/kernel/module.c
index 87cdb46863cd..4f591f2bbf5a 100644
--- a/kernel/module.c
+++ b/kernel/module.c

@@ -3948,6 +3948,7 @@ const char *module_address_lookup(unsigned long addr,
 	preempt_disable();
 	mod = __module_address(addr);
 	if (mod) {
+		addr = dereference_module_function_descriptor(mod, addr);
 		if (modname)
 			*modname = mod->name;
 		ret = get_ksymbol(mod, addr, size, offset);

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..bf04b4f5d8e7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

@@ -40,7 +40,6 @@
 #include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
 #include <asm/page.h>		/* for PAGE_SIZE */
-#include <asm/sections.h>	/* for dereference_function_descriptor() */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
 
 #include <linux/string_helpers.h>

@@ -1721,10 +1720,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 
 	switch (*fmt) {
-	case 'F':
+	case 'F': /* %pF and %pf are kept for compatibility reasons only */
 	case 'f':
-		ptr = dereference_function_descriptor(ptr);
-		/* Fallthrough */
 	case 'S':
 	case 's':
 	case 'B':

-- 
2.14.1

On 2017/09/16 12:53PM, Sergey Senozhatsky wrote:

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for powerpc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd];

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end].

Would it be simpler to just use kernel_text_address() and dereference 
everything else? See commit 83e840c770f2c5 ("powerpc64/elfv1: Only 
dereference function descriptor for non-text symbols") for a related 
patch.

- Naveen

On (09/16/17 15:13), Naveen N. Rao wrote:
[..]

Would it be simpler to just use kernel_text_address() and dereference 
everything else? See commit 83e840c770f2c5 ("powerpc64/elfv1: Only 
dereference function descriptor for non-text symbols") for a related 
patch.

I had this idea, see

lkml.kernel.org/r/20170908172528.qc2vdtxzqh777k6o@intel.com


	-ss

On Sat, Sep 16, 2017 at 12:53:42PM +0900, Sergey Senozhatsky wrote:

	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).

A few new warnings when building on ia64:

arch/ia64/kernel/module.c:931: warning: passing argument 1 of 'dereference_function_descriptor' makes pointer from integer without a cast
arch/ia64/kernel/module.c:931: warning: return makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: assignment makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: passing argument 1 of 'dereference_kernel_function_descriptor' makes pointer from integer without a cast

Tried out the module case with a simple Hello-world test case.
This code:

char buf[1];

int init_module(void)
{
	printk(KERN_INFO "Hello world 1.\n");

	printk("using %%p  my init_module is at %p\n", init_module);
	printk("using %%pF my init_module is at %pF\n", init_module);
	printk("using %%pS my init_module is at %pS\n", init_module);

	printk("using %%p  my buf is at %p\n", buf);
	printk("using %%pF my buf is at %pF\n", buf);
	printk("using %%pS my buf is at %pS\n", buf);

	return 0;
}

Gave this console output:

Hello world 1.
using %p  my init_module is at a000000203bf0328
using %pF my init_module is at init_module+0x0/0x140 [hello_1]
using %pS my init_module is at init_module+0x0/0x140 [hello_1]
using %p  my buf is at a000000203bf0648
using %pF my buf is at buf+0x0/0xfffffffffffffb58 [hello_1]
using %pS my buf is at buf+0x0/0xfffffffffffffb58 [hello_1]


Which looks like what you wanted. People unaware of the vagaries
of ppc64/ia64/parisc64 can use the wrong %p[SF] variant, but still
get the right output.

-Tony

* Luck, Tony [off-list ref]:

On Sat, Sep 16, 2017 at 12:53:42PM +0900, Sergey Senozhatsky wrote:

quoted

	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).

A few new warnings when building on ia64:

arch/ia64/kernel/module.c:931: warning: passing argument 1 of 'dereference_function_descriptor' makes pointer from integer without a cast
arch/ia64/kernel/module.c:931: warning: return makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: assignment makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: passing argument 1 of 'dereference_kernel_function_descriptor' makes pointer from integer without a cast

I got similiar warnings on parisc.
This patch on top of yours fixed those:

diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index bc2eae8..4f34b46 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c

@@ -66,6 +66,7 @@
 
 #include <asm/pgtable.h>
 #include <asm/unwind.h>
+#include <asm/sections.h>
 
 #if 0
 #define DEBUGP printk

@@ -959,12 +960,12 @@ void module_arch_cleanup(struct module *mod)
 unsigned long dereference_module_function_descriptor(struct module *mod,
 					     unsigned long addr)
 {
-	void *opd_sz = mod->arch.fdesc_offset +
+	unsigned long opd_sz = mod->arch.fdesc_offset +
 		       mod->arch.fdesc_max * sizeof(Elf64_Fdesc);
 
 	if (addr < mod->arch.fdesc_offset || opd_sz < addr)
 		return addr;
 
-	return dereference_function_descriptor(addr);
+	return (unsigned long) dereference_function_descriptor((void *) addr);
 }
 #endif

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index e2fc09e..76f4de6 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c

@@ -322,7 +322,7 @@ const char *kallsyms_lookup(unsigned long addr,
 	if (is_ksym_addr(addr)) {
 		unsigned long pos;
 
-		addr = dereference_kernel_function_descriptor(addr);
+		addr = dereference_kernel_function_descriptor((void *) addr);
 		pos = get_symbol_pos(addr, symbolsize, offset);
 		/* Grab name */
 		kallsyms_expand_symbol(get_symbol_offset(pos),

I did tried your testcases too.

"echo 1 > /proc/sys/vm/drop_caches" gave correct output:
 printk#1 schedule_timeout+0x0/0x4a8
 printk#2 schedule_timeout+0x0/0x4a8
 printk#3 proc_sys_call_handler+0x120/0x180
 printk#4 proc_sys_call_handler+0x120/0x180
 printk#5 proc_sys_call_handler+0x120/0x180
 printk#6 proc_sys_call_handler+0x120/0x180

and here is "modprobe zram":
 printk#7 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
 printk#8 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
 printk#9 do_one_initcall+0x194/0x290
 printk#10 do_one_initcall+0x194/0x290
 printk#11 do_one_initcall+0x194/0x290
 printk#12 do_one_initcall+0x194/0x290
 printk#13 zram_init+0x22c/0x2a0 [zram]
 printk#14 zram_init+0x22c/0x2a0 [zram]
 printk#15 zram_init+0x22c/0x2a0 [zram]
 printk#16 zram_init+0x22c/0x2a0 [zram]

I wonder why printk#7 and printk#8 don't show "zram_init"...


Regarding your patches:

In arch/parisc/kernel/process.c:
+void *dereference_kernel_function_descriptor(void *ptr)
+{
+       if (ptr < (void *)__start_opd || (void *)__end_opd < ptr)

This needs to be (__end_opd is outside):
+       if (ptr < (void *)__start_opd || (void *)__end_opd <= ptr)

The same is true for the checks in the other arches.


I'd suggest to move the various
	extern char __start_opd[], __end_opd[];
out of arch/<arch>/include/asm/sections.h and into <asm-generic/sections.h>


I'll continue to test.

Helge

On (09/18/17 20:39), Helge Deller wrote:
[..]

quoted

A few new warnings when building on ia64:

arch/ia64/kernel/module.c:931: warning: passing argument 1 of 'dereference_function_descriptor' makes pointer from integer without a cast
arch/ia64/kernel/module.c:931: warning: return makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: assignment makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: passing argument 1 of 'dereference_kernel_function_descriptor' makes pointer from integer without a cast

I got similiar warnings on parisc.
This patch on top of yours fixed those:

Tony, Helge,

thanks for the reports!

I'll simply convert everything to `unsigned long'. including the
dereference_function_descriptor() function [I believe there are
still some casts happening when we pass addr from kernel/module
dereference functions to dereference_function_descriptor(), or
when we return `void *' back to symbol resolution code, etc.)
besides, it seems that everything that uses
dereference_function_descriptor() wants `unsigned long' anyway:

drivers/misc/kgdbts.c:  addr = (unsigned long) dereference_function_descriptor((void *)addr);
init/main.c:    addr = (unsigned long) dereference_function_descriptor(fn);
kernel/extable.c:       addr = (unsigned long) dereference_function_descriptor(ptr);
kernel/module.c:        unsigned long a = (unsigned long)dereference_function_descriptor(addr);

so I'll just switch it to ulong.

I did tried your testcases too.

"echo 1 > /proc/sys/vm/drop_caches" gave correct output:
 printk#1 schedule_timeout+0x0/0x4a8
 printk#2 schedule_timeout+0x0/0x4a8
 printk#3 proc_sys_call_handler+0x120/0x180
 printk#4 proc_sys_call_handler+0x120/0x180
 printk#5 proc_sys_call_handler+0x120/0x180
 printk#6 proc_sys_call_handler+0x120/0x180

and here is "modprobe zram":
 printk#7 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
 printk#8 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
 printk#9 do_one_initcall+0x194/0x290
 printk#10 do_one_initcall+0x194/0x290
 printk#11 do_one_initcall+0x194/0x290
 printk#12 do_one_initcall+0x194/0x290
 printk#13 zram_init+0x22c/0x2a0 [zram]
 printk#14 zram_init+0x22c/0x2a0 [zram]
 printk#15 zram_init+0x22c/0x2a0 [zram]
 printk#16 zram_init+0x22c/0x2a0 [zram]

I wonder why printk#7 and printk#8 don't show "zram_init"...

interesting... what does the unpatched kernel show?

Regarding your patches:

In arch/parisc/kernel/process.c:
+void *dereference_kernel_function_descriptor(void *ptr)
+{
+       if (ptr < (void *)__start_opd || (void *)__end_opd < ptr)

This needs to be (__end_opd is outside):
+       if (ptr < (void *)__start_opd || (void *)__end_opd <= ptr)

The same is true for the checks in the other arches.

um... yeah. __end_opd is definitely not a valid place for a descriptor!
I think I had `if (!(ptr >= __start_opd && ptr < __end_opd))' which I
wrongly converted. "shame, shame, shame".

thanks!

I'd suggest to move the various
	extern char __start_opd[], __end_opd[];
out of arch/<arch>/include/asm/sections.h and into <asm-generic/sections.h>

ok, will take a look.

	-ss

On (09/18/17 10:44), Luck, Tony wrote:
[..]

A few new warnings when building on ia64:

arch/ia64/kernel/module.c:931: warning: passing argument 1 of 'dereference_function_descriptor' makes pointer from integer without a cast
arch/ia64/kernel/module.c:931: warning: return makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: assignment makes integer from pointer without a cast
kernel/kallsyms.c:325: warning: passing argument 1 of 'dereference_kernel_function_descriptor' makes pointer from integer without a cast

got it, will address in v2.

[..]

Which looks like what you wanted. People unaware of the vagaries
of ppc64/ia64/parisc64 can use the wrong %p[SF] variant, but still
get the right output.

thanks!

	-ss

"Naveen N. Rao" [off-list ref] writes:

On 2017/09/16 12:53PM, Sergey Senozhatsky wrote:

quoted

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for powerpc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd];

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end].

Would it be simpler to just use kernel_text_address() and dereference 
everything else? See commit 83e840c770f2c5 ("powerpc64/elfv1: Only 
dereference function descriptor for non-text symbols") for a related 
patch.

Yeah that would be a lot simpler and probably work perfectly well.

cheers

On (09/19/17 20:22), Michael Ellerman wrote:

quoted

On 2017/09/16 12:53PM, Sergey Senozhatsky wrote:

quoted

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for powerpc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd];

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end].

Would it be simpler to just use kernel_text_address() and dereference 
everything else? See commit 83e840c770f2c5 ("powerpc64/elfv1: Only 
dereference function descriptor for non-text symbols") for a related 
patch.

Yeah that would be a lot simpler and probably work perfectly well.

unlike ppc_function_entry(), printk() can get called on any symbol,
not just function pointers.

for example,

cat /proc/kallsyms | grep shrinker_rwsem
ffffffff81a4b1e0 d shrinker_rwsem

or

cat /proc/kallsyms | grep vm_total_pages
ffffffff81dcd418 B vm_total_pages


and so on.

	-ss

From: Sergey Senozhatsky

Sent: 19 September 2017 03:06

...

I'll simply convert everything to `unsigned long'. including the
dereference_function_descriptor() function [I believe there are
still some casts happening when we pass addr from kernel/module
dereference functions to dereference_function_descriptor(), or
when we return `void *' back to symbol resolution code, etc.)
besides, it seems that everything that uses
dereference_function_descriptor() wants `unsigned long' anyway:

Using 'unsigned long' for any kind of pointer is an accident
waiting do happen.
It also makes it difficult to typecheck the function calls.
Using 'void *' isn't any better.
Either a pointer to an undefined struct, or a struct containing
a single 'char' member, is likely to be safest.

	David

On 19.09.2017 04:05, Sergey Senozhatsky wrote:

On (09/18/17 20:39), Helge Deller wrote:

quoted

I did tried your testcases [on parisc] too.

...

quoted

and here is "modprobe zram":
  printk#7 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
  printk#8 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
  printk#9 do_one_initcall+0x194/0x290
  printk#10 do_one_initcall+0x194/0x290
  printk#11 do_one_initcall+0x194/0x290
  printk#12 do_one_initcall+0x194/0x290
  printk#13 zram_init+0x22c/0x2a0 [zram]
  printk#14 zram_init+0x22c/0x2a0 [zram]
  printk#15 zram_init+0x22c/0x2a0 [zram]
  printk#16 zram_init+0x22c/0x2a0 [zram]

I wonder why printk#7 and printk#8 don't show "zram_init"...

interesting... what does the unpatched kernel show?

Really strange.
The unpatched kernel shows __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 too.
The symbol should be known, because later on in printk13 it shows correctly zram_init.
I'll need to dig deeper into it, but at least the regression is not due
to your patch.

Helge

* Helge Deller [off-list ref]:

On 19.09.2017 04:05, Sergey Senozhatsky wrote:

quoted

On (09/18/17 20:39), Helge Deller wrote:

quoted

I did tried your testcases [on parisc] too.

...

quoted

quoted

and here is "modprobe zram":
 printk#7 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
 printk#8 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram]
 printk#9 do_one_initcall+0x194/0x290
 printk#10 do_one_initcall+0x194/0x290
 printk#11 do_one_initcall+0x194/0x290
 printk#12 do_one_initcall+0x194/0x290
 printk#13 zram_init+0x22c/0x2a0 [zram]
 printk#14 zram_init+0x22c/0x2a0 [zram]
 printk#15 zram_init+0x22c/0x2a0 [zram]
 printk#16 zram_init+0x22c/0x2a0 [zram]

I wonder why printk#7 and printk#8 don't show "zram_init"...

interesting... what does the unpatched kernel show?

Really strange.
The unpatched kernel shows __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 too.
The symbol should be known, because later on in printk13 it shows correctly zram_init.
I'll need to dig deeper into it, but at least the regression is not due
to your patch.

Sergey, I was wrong with this assumption.

Your implementation of dereference_module_function_descriptor() in
arch/parisc/kernel/module.c is faulty.
mod->arch.fdesc_offset is relative to the base address of the module,
so you need to add to mod->core_layout.base.

Here is the relevant patch to fix this issue (against mainline).
Additionally I compare against mod->arch.fdesc_count instead of
mod->arch.fdesc_max.
Can you please fold it into your patch
[PATCH 4/5] parisc64: Add .opd based function descriptor dereference
for the next round?

Thanks,
Helge

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index f1a7693..ae3e6c5 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c

@@ -66,6 +66,7 @@
 
 #include <asm/pgtable.h>
 #include <asm/unwind.h>
+#include <asm/sections.h>
 
 #if 0
 #define DEBUGP printk

@@ -954,3 +955,19 @@ void module_arch_cleanup(struct module *mod)
 {
 	deregister_unwind_table(mod);
 }
+
+#ifdef CONFIG_64BIT
+unsigned long dereference_module_function_descriptor(struct module *mod,
+					     unsigned long addr)
+{
+	unsigned long opd_start = (Elf64_Addr) mod->core_layout.base
+					+ mod->arch.fdesc_offset;
+	unsigned long opd_end = opd_start
+			+ mod->arch.fdesc_count * sizeof(Elf64_Fdesc);
+
+	if (addr < opd_start || addr >= opd_end)
+		return addr;
+
+	return (unsigned long) dereference_function_descriptor((void *) addr);
+}
+#endif

On 19.09.2017 15:38, David Laight wrote:

From: Sergey Senozhatsky

quoted

Sent: 19 September 2017 03:06

...

quoted

I'll simply convert everything to `unsigned long'. including the
dereference_function_descriptor() function [I believe there are
still some casts happening when we pass addr from kernel/module
dereference functions to dereference_function_descriptor(), or
when we return `void *' back to symbol resolution code, etc.)
besides, it seems that everything that uses
dereference_function_descriptor() wants `unsigned long' anyway:

Using 'unsigned long' for any kind of pointer is an accident
waiting do happen.
It also makes it difficult to typecheck the function calls.
Using 'void *' isn't any better.
Either a pointer to an undefined struct, or a struct containing
a single 'char' member, is likely to be safest.

David, you might be right in most cases, but in this case I'd prefer
unsigned long too. I think this will create the least amount of
typecasts here.

Helge

On (09/19/17 22:03), Helge Deller wrote:
[..]

Your implementation of dereference_module_function_descriptor() in
arch/parisc/kernel/module.c is faulty.
mod->arch.fdesc_offset is relative to the base address of the module,
so you need to add to mod->core_layout.base.

aha, got it. I should have figured that out.
many thanks!

Here is the relevant patch to fix this issue (against mainline).
Additionally I compare against mod->arch.fdesc_count instead of
mod->arch.fdesc_max.

hmm, .fdesc_max looked relevant to me. it's count_fdescs() - the
number of R_PARISC_FPTR64 relocation entries.

but ok, will use .fdesc_count.

Can you please fold it into your patch
[PATCH 4/5] parisc64: Add .opd based function descriptor dereference
for the next round?

sure, will fold. + SoB.

I think I'll try to re-spin the series today (or tomorrow, I'm slightly
overloaded with another stuff right now). I've received enough bug reports
no need to wait for another week ;)

	-ss

Sergey Senozhatsky [off-list ref] writes:

On (09/19/17 20:22), Michael Ellerman wrote:

quoted

quoted

On 2017/09/16 12:53PM, Sergey Senozhatsky wrote:

quoted

We are moving towards separate kernel and module function descriptor
dereference callbacks. This patch enables it for powerpc64.

For pointers that belong to the kernel
-  Added __start_opd and __end_opd pointers, to track the kernel
   .opd section address range;

-  Added dereference_kernel_function_descriptor(). Now we
   will dereference only function pointers that are within
   [__start_opd, __end_opd];

For pointers that belong to a module
-  Added dereference_module_function_descriptor() to handle module
   function descriptor dereference. Now we will dereference only
   pointers that are within [module->opd.start, module->opd.end].

Would it be simpler to just use kernel_text_address() and dereference 
everything else? See commit 83e840c770f2c5 ("powerpc64/elfv1: Only 
dereference function descriptor for non-text symbols") for a related 
patch.

Yeah that would be a lot simpler and probably work perfectly well.

unlike ppc_function_entry(), printk() can get called on any symbol,
not just function pointers.

for example,

cat /proc/kallsyms | grep shrinker_rwsem
ffffffff81a4b1e0 d shrinker_rwsem

Yep, good point. So your patch is probably good then. Maybe someone
other than me can find time to test it ;)

cheers

On (09/20/17 11:51), Michael Ellerman wrote:
[..]

quoted

unlike ppc_function_entry(), printk() can get called on any symbol,
not just function pointers.

for example,

cat /proc/kallsyms | grep shrinker_rwsem
ffffffff81a4b1e0 d shrinker_rwsem

Yep, good point. So your patch is probably good then. Maybe someone
other than me can find time to test it ;)

Hi,

I'll re-spin the series today/tomorrow.

	-ss

From: Helge Deller

Sent: 19 September 2017 21:08

...

quoted

Using 'unsigned long' for any kind of pointer is an accident
waiting do happen.
It also makes it difficult to typecheck the function calls.
Using 'void *' isn't any better.
Either a pointer to an undefined struct, or a struct containing
a single 'char' member, is likely to be safest.

=20
David, you might be right in most cases, but in this case I'd prefer
unsigned long too. I think this will create the least amount of
typecasts here.

I've not looked at the specifics case...

Another option is using a struct with a single member and
passing it by value.
This could be used for things like user-space pointers or
even errno values.
The only problem is old ABI where even small structures are
always passed by reference.

	David

On 20.09.2017 10:41, David Laight wrote:

From: Helge Deller

quoted

Sent: 19 September 2017 21:08

...

quoted

quoted

Using 'unsigned long' for any kind of pointer is an accident
waiting do happen.
It also makes it difficult to typecheck the function calls.
Using 'void *' isn't any better.
Either a pointer to an undefined struct, or a struct containing
a single 'char' member, is likely to be safest.

David, you might be right in most cases, but in this case I'd prefer
unsigned long too. I think this will create the least amount of
typecasts here.

I've not looked at the specifics case...

Another option is using a struct with a single member and
passing it by value.

Actually, we do already have correct structs which could be referenced:
parisc: struct Elf64_Fdesc
ia64:	struct fdesc
ppc64:	struct ppc64_opd_entry

One could "#define platform_opd_entry" to each of those depending on the platform and use it.
It might be misleading though, because the pointer which is handed over to
dereference_function_descriptor() can be such a pointer but isn't necessary.
I'll leave it up to Sergey to decide.

Helge

On (09/20/17 12:20), Helge Deller wrote:
[..]

quoted

I've not looked at the specifics case...

Another option is using a struct with a single member and
passing it by value.

Actually, we do already have correct structs which could be referenced:
parisc: struct Elf64_Fdesc
ia64:	struct fdesc
ppc64:	struct ppc64_opd_entry

One could "#define platform_opd_entry" to each of those depending on the platform and use it.
It might be misleading though, because the pointer which is handed over to
dereference_function_descriptor() can be such a pointer but isn't necessary.
I'll leave it up to Sergey to decide.

Hello,

I think I'll keep 'unsigned long' for now.

	-ss