This patch series fixes the wrong usages of the %pF and %pS printk format
specifiers throughout the kernel code.
Both specifiers have the same result on most architectures. But on ia64, ppc64
and parisc64 architectures the %pF specifier does an extra conversion because
there function pointers are actually function descriptors.
Helge
Helge Deller (14):
arm: Use %pS printk format for symbols from direct addresses
um: Use %pS printk format for symbols from direct addresses
x86: Use %pS printk format for symbols from direct addresses
ti_sci: Use %pS printk format for direct addresses
i915: Use %pS printk format for direct addresses
md/bcache: Use %pS printk format for direct addresses
power/avs: Use %pS printk format for direct addresses
fs/f2fs: Use %pS printk format for direct addresses
fs/pstore: Use %pS printk format for direct addresses
fs/xfs: Use %pS printk format for direct addresses
smp: Use %pF printk format specifier for function pointers
mm/memblock: Use %pS printk format for direct addresses
netfilter/ipvs: Use %pS printk format for direct addresses
sound/core: Use %pS printk format for direct addresses
arch/arm/mm/alignment.c | 2 +-
arch/um/kernel/sysrq.c | 2 +-
arch/x86/mm/extable.c | 4 ++--
arch/x86/xen/multicalls.c | 2 +-
drivers/firmware/ti_sci.c | 2 +-
drivers/gpu/drm/i915/intel_breadcrumbs.c | 2 +-
drivers/md/bcache/closure.c | 4 ++--
drivers/power/avs/smartreflex.c | 10 +++++-----
fs/f2fs/f2fs.h | 2 +-
fs/pstore/inode.c | 2 +-
fs/xfs/xfs_error.c | 2 +-
kernel/smp.c | 2 +-
mm/memblock.c | 14 +++++++-------
net/netfilter/ipvs/ip_vs_conn.c | 2 +-
net/netfilter/ipvs/ip_vs_ctl.c | 4 ++--
sound/core/device.c | 4 ++--
16 files changed, 30 insertions(+), 30 deletions(-)
--
2.1.0
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 <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Tony Luck <[email protected]>
---
fs/pstore/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index fefd226..59f65d7 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -116,7 +116,7 @@ static int pstore_ftrace_seq_show(struct seq_file *s, void *v)
rec = (struct pstore_ftrace_record *)(ps->record->buf + data->off);
- seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %pf <- %pF\n",
+ seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %ps <- %pS\n",
pstore_ftrace_decode_cpu(rec),
pstore_ftrace_read_timestamp(rec),
rec->ip, rec->parent_ip, (void *)rec->ip,
--
2.1.0
Use the %pS printk format for printing symbols from direct addresses.
On the x86 architecture there is actually no difference between %pS and %pF,
but for consistency throughout the kernel fix the wrong usage here too.
Signed-off-by: Helge Deller <[email protected]>
Cc: [email protected]
Cc: Ingo Molnar <[email protected]>
Cc: Boris Ostrovsky <[email protected]>
Cc: [email protected]
---
arch/x86/mm/extable.c | 4 ++--
arch/x86/xen/multicalls.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index c076f71..7137347 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -91,7 +91,7 @@ EXPORT_SYMBOL(ex_handler_ext);
bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)
{
- if (pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pF)\n",
+ if (pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n",
(unsigned int)regs->cx, regs->ip, (void *)regs->ip))
show_stack_regs(regs);
@@ -106,7 +106,7 @@ EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);
bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)
{
- if (pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pF)\n",
+ if (pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n",
(unsigned int)regs->cx, (unsigned int)regs->dx,
(unsigned int)regs->ax, regs->ip, (void *)regs->ip))
show_stack_regs(regs);
diff --git a/arch/x86/xen/multicalls.c b/arch/x86/xen/multicalls.c
index ea54a08..13598f9 100644
--- a/arch/x86/xen/multicalls.c
+++ b/arch/x86/xen/multicalls.c
@@ -103,7 +103,7 @@ void xen_mc_flush(void)
ret, smp_processor_id());
dump_stack();
for (i = 0; i < b->mcidx; i++) {
- printk(KERN_DEBUG " call %2d/%d: op=%lu arg=[%lx] result=%ld\t%pF\n",
+ printk(KERN_DEBUG " call %2d/%d: op=%lu arg=[%lx] result=%ld\t%pS\n",
i+1, b->mcidx,
b->debug[i].op,
b->debug[i].args[0],
--
2.1.0
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 <[email protected]>
Cc: Jaegeuk Kim <[email protected]>
Cc: Chao Yu <[email protected]>
Cc: [email protected]
---
fs/f2fs/f2fs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 94a88b2..9012a43 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1054,7 +1054,7 @@ struct f2fs_sb_info {
#ifdef CONFIG_F2FS_FAULT_INJECTION
#define f2fs_show_injection_info(type) \
- printk("%sF2FS-fs : inject %s in %s of %pF\n", \
+ printk("%sF2FS-fs : inject %s in %s of %pS\n", \
KERN_INFO, fault_name[type], \
__func__, __builtin_return_address(0))
static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
--
2.1.0
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 <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: [email protected]
---
drivers/power/avs/smartreflex.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
index 974fd68..89bf4d6 100644
--- a/drivers/power/avs/smartreflex.c
+++ b/drivers/power/avs/smartreflex.c
@@ -355,7 +355,7 @@ int sr_configure_errgen(struct omap_sr *sr)
u8 senp_shift, senn_shift;
if (!sr) {
- pr_warn("%s: NULL omap_sr from %pF\n",
+ pr_warn("%s: NULL omap_sr from %pS\n",
__func__, (void *)_RET_IP_);
return -EINVAL;
}
@@ -422,7 +422,7 @@ int sr_disable_errgen(struct omap_sr *sr)
u32 vpboundint_en, vpboundint_st;
if (!sr) {
- pr_warn("%s: NULL omap_sr from %pF\n",
+ pr_warn("%s: NULL omap_sr from %pS\n",
__func__, (void *)_RET_IP_);
return -EINVAL;
}
@@ -477,7 +477,7 @@ int sr_configure_minmax(struct omap_sr *sr)
u8 senp_shift, senn_shift;
if (!sr) {
- pr_warn("%s: NULL omap_sr from %pF\n",
+ pr_warn("%s: NULL omap_sr from %pS\n",
__func__, (void *)_RET_IP_);
return -EINVAL;
}
@@ -562,7 +562,7 @@ int sr_enable(struct omap_sr *sr, unsigned long volt)
int ret;
if (!sr) {
- pr_warn("%s: NULL omap_sr from %pF\n",
+ pr_warn("%s: NULL omap_sr from %pS\n",
__func__, (void *)_RET_IP_);
return -EINVAL;
}
@@ -614,7 +614,7 @@ int sr_enable(struct omap_sr *sr, unsigned long volt)
void sr_disable(struct omap_sr *sr)
{
if (!sr) {
- pr_warn("%s: NULL omap_sr from %pF\n",
+ pr_warn("%s: NULL omap_sr from %pS\n",
__func__, (void *)_RET_IP_);
return;
}
--
2.1.0
The debug functions uses wrongly the %pF instead of the %pS printk format
specifier for printing symbols for the address returned by
_builtin_return_address(0). Fix it for the ia64, ppc64 and parisc64
architectures.
Signed-off-by: Helge Deller <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: [email protected]
---
sound/core/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/device.c b/sound/core/device.c
index 8918838..cb0e46f 100644
--- a/sound/core/device.c
+++ b/sound/core/device.c
@@ -128,7 +128,7 @@ void snd_device_disconnect(struct snd_card *card, void *device_data)
if (dev)
__snd_device_disconnect(dev);
else
- dev_dbg(card->dev, "device disconnect %p (from %pF), not found\n",
+ dev_dbg(card->dev, "device disconnect %p (from %pS), not found\n",
device_data, __builtin_return_address(0));
}
EXPORT_SYMBOL_GPL(snd_device_disconnect);
@@ -152,7 +152,7 @@ void snd_device_free(struct snd_card *card, void *device_data)
if (dev)
__snd_device_free(dev);
else
- dev_dbg(card->dev, "device free %p (from %pF), not found\n",
+ dev_dbg(card->dev, "device free %p (from %pS), not found\n",
device_data, __builtin_return_address(0));
}
EXPORT_SYMBOL(snd_device_free);
--
2.1.0
Use the %pS printk format for printing symbols from direct addresses.
This is important for the ia64, ppc64 and parisc64 architectures, while on
other architectures there is no difference between %pS and %pF.
Fix it for consistency across the kernel.
Signed-off-by: Helge Deller <[email protected]>
Cc: Jani Nikula <[email protected]>
Cc: David Airlie <[email protected]>
Cc: [email protected]
---
drivers/gpu/drm/i915/intel_breadcrumbs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 4e00e5c..29c62d4 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -64,7 +64,7 @@ static unsigned long wait_timeout(void)
static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
{
- DRM_DEBUG_DRIVER("%s missed breadcrumb at %pF, irq posted? %s, current seqno=%x, last=%x\n",
+ DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS, irq posted? %s, current seqno=%x, last=%x\n",
engine->name, __builtin_return_address(0),
yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
&engine->irq_posted)),
--
2.1.0
Use the %pS printk format for printing symbols from direct addresses.
On ARM there is actually no difference between %pS and %pF, but for consistency
throughout the kernel fix the wrong usage here too.
Signed-off-by: Helge Deller <[email protected]>
Cc: Russell King <[email protected]>
Cc: [email protected]
---
arch/arm/mm/alignment.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 2c96190..20d721f 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -133,7 +133,7 @@ static const char *usermode_action[] = {
static int alignment_proc_show(struct seq_file *m, void *v)
{
seq_printf(m, "User:\t\t%lu\n", ai_user);
- seq_printf(m, "System:\t\t%lu (%pF)\n", ai_sys, ai_sys_last_pc);
+ seq_printf(m, "System:\t\t%lu (%pS)\n", ai_sys, ai_sys_last_pc);
seq_printf(m, "Skipped:\t%lu\n", ai_skipped);
seq_printf(m, "Half:\t\t%lu\n", ai_half);
seq_printf(m, "Word:\t\t%lu\n", ai_word);
--
2.1.0
Use the %pS printk format for printing symbols from direct addresses.
This is important for the ia64, ppc64 and parisc64 architectures, while on
other architectures there is no difference between %pS and %pF.
Fix it for consistency across the kernel.
Signed-off-by: Helge Deller <[email protected]>
Cc: Nishanth Menon <[email protected]>
Cc: Tero Kristo <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: [email protected]
---
drivers/firmware/ti_sci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
index 00cfed3..23b12d9 100644
--- a/drivers/firmware/ti_sci.c
+++ b/drivers/firmware/ti_sci.c
@@ -439,7 +439,7 @@ static inline int ti_sci_do_xfer(struct ti_sci_info *info,
/* And we wait for the response. */
timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
if (!wait_for_completion_timeout(&xfer->done, timeout)) {
- dev_err(dev, "Mbox timedout in resp(caller: %pF)\n",
+ dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
(void *)_RET_IP_);
ret = -ETIMEDOUT;
}
--
2.1.0
The debug code in memblock uses wrongly the %pF instead of the %pS printk
format specifier for printing symbols for the address returned by
_builtin_return_address(0)/_RET_IP_. Fix it for the ia64, ppc64 and parisc64
architectures.
Signed-off-by: Helge Deller <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
mm/memblock.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 9120578..7f1590d 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -597,7 +597,7 @@ int __init_memblock memblock_add(phys_addr_t base, phys_addr_t size)
{
phys_addr_t end = base + size - 1;
- memblock_dbg("memblock_add: [%pa-%pa] %pF\n",
+ memblock_dbg("memblock_add: [%pa-%pa] %pS\n",
&base, &end, (void *)_RET_IP_);
return memblock_add_range(&memblock.memory, base, size, MAX_NUMNODES, 0);
@@ -704,7 +704,7 @@ int __init_memblock memblock_free(phys_addr_t base, phys_addr_t size)
{
phys_addr_t end = base + size - 1;
- memblock_dbg(" memblock_free: [%pa-%pa] %pF\n",
+ memblock_dbg(" memblock_free: [%pa-%pa] %pS\n",
&base, &end, (void *)_RET_IP_);
kmemleak_free_part_phys(base, size);
@@ -715,7 +715,7 @@ int __init_memblock memblock_reserve(phys_addr_t base, phys_addr_t size)
{
phys_addr_t end = base + size - 1;
- memblock_dbg("memblock_reserve: [%pa-%pa] %pF\n",
+ memblock_dbg("memblock_reserve: [%pa-%pa] %pS\n",
&base, &end, (void *)_RET_IP_);
return memblock_add_range(&memblock.reserved, base, size, MAX_NUMNODES, 0);
@@ -1362,7 +1362,7 @@ void * __init memblock_virt_alloc_try_nid_nopanic(
phys_addr_t min_addr, phys_addr_t max_addr,
int nid)
{
- memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
+ memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pS\n",
__func__, (u64)size, (u64)align, nid, (u64)min_addr,
(u64)max_addr, (void *)_RET_IP_);
return memblock_virt_alloc_internal(size, align, min_addr,
@@ -1394,7 +1394,7 @@ void * __init memblock_virt_alloc_try_nid(
{
void *ptr;
- memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
+ memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pS\n",
__func__, (u64)size, (u64)align, nid, (u64)min_addr,
(u64)max_addr, (void *)_RET_IP_);
ptr = memblock_virt_alloc_internal(size, align,
@@ -1418,7 +1418,7 @@ void * __init memblock_virt_alloc_try_nid(
*/
void __init __memblock_free_early(phys_addr_t base, phys_addr_t size)
{
- memblock_dbg("%s: [%#016llx-%#016llx] %pF\n",
+ memblock_dbg("%s: [%#016llx-%#016llx] %pS\n",
__func__, (u64)base, (u64)base + size - 1,
(void *)_RET_IP_);
kmemleak_free_part_phys(base, size);
@@ -1438,7 +1438,7 @@ void __init __memblock_free_late(phys_addr_t base, phys_addr_t size)
{
u64 cursor, end;
- memblock_dbg("%s: [%#016llx-%#016llx] %pF\n",
+ memblock_dbg("%s: [%#016llx-%#016llx] %pS\n",
__func__, (u64)base, (u64)base + size - 1,
(void *)_RET_IP_);
kmemleak_free_part_phys(base, size);
--
2.1.0
Use the %pS printk format for printing symbols from direct addresses.
In usermode-linux there is actually no difference between %pS and %pF, but for
consistency throughout the kernel fix the wrong usage here too.
Signed-off-by: Helge Deller <[email protected]>
Cc: Jeff Dike <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: [email protected]
---
arch/um/kernel/sysrq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/um/kernel/sysrq.c b/arch/um/kernel/sysrq.c
index 6b995e8..05585ee 100644
--- a/arch/um/kernel/sysrq.c
+++ b/arch/um/kernel/sysrq.c
@@ -20,7 +20,7 @@
static void _print_addr(void *data, unsigned long address, int reliable)
{
- pr_info(" [<%08lx>] %s%pF\n", address, reliable ? "" : "? ",
+ pr_info(" [<%08lx>] %s%pS\n", address, reliable ? "" : "? ",
(void *)address);
}
--
2.1.0
Use the %pF instead of the %pS printk format specifier for printing function
pointers. This is needed for the ia64, ppc64 and parisc64 architectures.
Signed-off-by: Helge Deller <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: [email protected]
---
kernel/smp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/smp.c b/kernel/smp.c
index 81cfca9..238e9ec 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -230,7 +230,7 @@ static void flush_smp_call_function_queue(bool warn_cpu_offline)
* because we are not invoking the IPI handlers yet.
*/
llist_for_each_entry(csd, entry, llist)
- pr_warn("IPI callback %pS sent to offline CPU\n",
+ pr_warn("IPI callback %pF sent to offline CPU\n",
csd->func);
}
--
2.1.0
The debug and error printk functions in ipvs uses wrongly the %pF instead of
the %pS printk format specifier for printing symbols for the address returned
by _builtin_return_address(0). Fix it for the ia64, ppc64 and parisc64
architectures.
Signed-off-by: Helge Deller <[email protected]>
Cc: Wensong Zhang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
net/netfilter/ipvs/ip_vs_conn.c | 2 +-
net/netfilter/ipvs/ip_vs_ctl.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 3d2ac71a..f73561c 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -185,7 +185,7 @@ static inline int ip_vs_conn_hash(struct ip_vs_conn *cp)
hlist_add_head_rcu(&cp->c_list, &ip_vs_conn_tab[hash]);
ret = 1;
} else {
- pr_err("%s(): request for already hashed, called from %pF\n",
+ pr_err("%s(): request for already hashed, called from %pS\n",
__func__, __builtin_return_address(0));
ret = 0;
}
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 1fa3c23..88fc58a 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -300,7 +300,7 @@ static int ip_vs_svc_hash(struct ip_vs_service *svc)
unsigned int hash;
if (svc->flags & IP_VS_SVC_F_HASHED) {
- pr_err("%s(): request for already hashed, called from %pF\n",
+ pr_err("%s(): request for already hashed, called from %pS\n",
__func__, __builtin_return_address(0));
return 0;
}
@@ -334,7 +334,7 @@ static int ip_vs_svc_hash(struct ip_vs_service *svc)
static int ip_vs_svc_unhash(struct ip_vs_service *svc)
{
if (!(svc->flags & IP_VS_SVC_F_HASHED)) {
- pr_err("%s(): request for unhash flagged, called from %pF\n",
+ pr_err("%s(): request for unhash flagged, called from %pS\n",
__func__, __builtin_return_address(0));
return 0;
}
--
2.1.0
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 <[email protected]>
Cc: "Darrick J. Wong" <[email protected]>
Cc: [email protected]
---
fs/xfs/xfs_error.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 2f4feb9..028e50a 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -344,7 +344,7 @@ xfs_verifier_error(
{
struct xfs_mount *mp = bp->b_target->bt_mount;
- xfs_alert(mp, "Metadata %s detected at %pF, %s block 0x%llx",
+ xfs_alert(mp, "Metadata %s detected at %pS, %s block 0x%llx",
bp->b_error == -EFSBADCRC ? "CRC error" : "corruption",
__return_address, bp->b_ops->name, bp->b_bn);
--
2.1.0
On (09/06/17 22:27), Helge Deller wrote:
> This patch series fixes the wrong usages of the %pF and %pS printk format
> specifiers throughout the kernel code.
>
> Both specifiers have the same result on most architectures. But on ia64, ppc64
> and parisc64 architectures the %pF specifier does an extra conversion because
> there function pointers are actually function descriptors.
hm...
can we fix it in lib/vsprintf.c instead?
a) the patch set has "there is no problem on platform A, but still
let's just change it"
which is probably fine. but
b) you tweak/fix the currently existing users, OK. but there, most
likely, will be new users that will require fixes in the future.
-ss
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 <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
On 07.09.2017 02:45, Sergey Senozhatsky wrote:
> On (09/06/17 22:27), Helge Deller wrote:
>> This patch series fixes the wrong usages of the %pF and %pS printk format
>> specifiers throughout the kernel code.
>>
>> Both specifiers have the same result on most architectures. But on ia64, ppc64
>> and parisc64 architectures the %pF specifier does an extra conversion because
>> there function pointers are actually function descriptors.
>
> hm...
> can we fix it in lib/vsprintf.c instead?
There is nothing to fix in vsprintf, because it is already providing
both %pF and %pS for the two different architecture-specific API call
implementations.
> a) the patch set has "there is no problem on platform A, but still
> let's just change it"
>
> which is probably fine. but
>
> b) you tweak/fix the currently existing users, OK. but there, most
> likely, will be new users that will require fixes in the future.
Please read the documentation in Documentation/printk-formats.txt.
It's really as simple that if you use function pointers you *need to*
use %pF, and if you use raw addresses of functions, you *need to* use
%pS. If you use the correct specifier the output will automatically
be correct for all architectures. If you don't, then the output on
ia64, ppc64 and parisc64 architectures will be wrong and may lead
to kernel crashes in the worst case.
My patch series simply fixes that code which has it wrong.
It has no impact at all on users on x86, ARM, mips and
other platforms.
Helge
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 <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> ---
>> 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
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 <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> 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
Hello Helge,
On (09/07/17 08:01), Helge Deller wrote:
[..]
> > hm...
> > can we fix it in lib/vsprintf.c instead?
thanks for a quick reply.
> There is nothing to fix in vsprintf, because it is already providing
> both %pF and %pS for the two different architecture-specific API call
> implementations.
[..]
> ia64, ppc64 and parisc64 architectures will be wrong and may lead
> to kernel crashes in the worst case.
^^^^^^^^^^^^^^^^^
I was thinking about this part.
sorry, I don't have access to ia64/ppc64/parisc64 so can't check it or
test it. here is a question, does function descriptor belong to a special
section? can we check that supplied ptr belongs to a descriptor section
and avoid dereference_function_descriptor() if it doesn't? (just fall
through directly to symbol_string() in this case). is this possible?
I mean, there is no mechanism to prevent this type of wrongdoings in
the future, we can't scan the entire kernel for wrong pF/pS all the
time.
BTW, are we sure we can crash? when attempt to deference IP from
the given descriptor? shall we handle page fault in this case and
do something sane? just asking.
-ss
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
On (09/07/17 16:56), Sergey Senozhatsky wrote:
[..]
> BTW, are we sure we can crash? when attempt to deference IP from
> the given descriptor? shall we handle page fault in this case and
> do something sane? just asking.
I don't know... does the below code make any sense?
quick and dirty. NOT TESTED at all (not even compile tested).
we can avoid extra probe_kernel_address() on anything that is
not ia64, ppc64, etc.
basically it checks that it's safe to access ptr (we can access it
without page fault in __dereference_function_descriptor()). then
we do ptr->ip, and also check if it's safe, but in
dereference_function_descriptor().
I suppose somethign like
pr_err("%pF\n", 1);
can crash ia64, etc. correct?
well. not tested.
---
lib/vsprintf.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..0dc39b95e1d9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1593,6 +1593,16 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
int kptr_restrict __read_mostly;
+static void *__dereference_function_descriptor(void *ptr)
+{
+ void *p;
+
+ if (!probe_kernel_address(ptr, p))
+ return dereference_function_descriptor(ptr);
+
+ return ptr;
+}
+
/*
* Show a '%p' thing. A kernel extension is that the '%p' is followed
* by an extra set of alphanumeric characters that are extended format
@@ -1723,7 +1733,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
switch (*fmt) {
case 'F':
case 'f':
- ptr = dereference_function_descriptor(ptr);
+ ptr = __dereference_function_descriptor(ptr);
/* Fallthrough */
case 'S':
case 's':
On Wed, 06 Sep 2017 22:28:01 +0200,
Helge Deller wrote:
>
> The debug functions uses wrongly the %pF instead of the %pS printk format
> specifier for printing symbols for the address returned by
> _builtin_return_address(0). Fix it for the ia64, ppc64 and parisc64
> architectures.
>
> Signed-off-by: Helge Deller <[email protected]>
> Cc: Jaroslav Kysela <[email protected]>
> Cc: Takashi Iwai <[email protected]>
> Cc: [email protected]
Applied, thanks.
Takashi
On 07.09.2017 10:32, Sergey Senozhatsky wrote:
> On (09/07/17 16:56), Sergey Senozhatsky wrote:
> [..]
> 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?
Actually I never faced a kernel crash because of this on parisc.
Don't know for ia64 and ppc64.
>> BTW, are we sure we can crash? when attempt to deference IP from
>> the given descriptor? shall we handle page fault in this case and
>> do something sane? just asking.
>
> I don't know... does the below code make any sense?
No. Read below.
> basically it checks that it's safe to access ptr (we can access it
> without page fault in __dereference_function_descriptor()). then
> we do ptr->ip, and also check if it's safe, but in
> dereference_function_descriptor().
>
> I suppose somethign like
>
> pr_err("%pF\n", 1);
>
> can crash ia64, etc. correct?
On parisc it will not crash because we handle that one already.
For others I don't know.
But something like
pr_err("%pF\n", (unsigned long) (-1));
or any address above 0xffffffff00000000ULL might do bad things
on parisc, because it touches the I/O space and we don't check that yet.
> lib/vsprintf.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 86c3385b9eb3..0dc39b95e1d9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1593,6 +1593,16 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>
> int kptr_restrict __read_mostly;
>
> +static void *__dereference_function_descriptor(void *ptr)
> +{
> + void *p;
> +
> + if (!probe_kernel_address(ptr, p))
> + return dereference_function_descriptor(ptr);
> +
> + return ptr;
> +}
> +
> /*
> * Show a '%p' thing. A kernel extension is that the '%p' is followed
> * by an extra set of alphanumeric characters that are extended format
> @@ -1723,7 +1733,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> switch (*fmt) {
> case 'F':
> case 'f':
> - ptr = dereference_function_descriptor(ptr);
> + ptr = __dereference_function_descriptor(ptr);
This is not needed.
All affected arches (ia64, ppc64, parisc64) already call
probe_kernel_address() inside their dereference_function_descriptor() function.
So this patch just adds unnecessary overhead for all arches.
> ... here is a question, does function descriptor belong to a special
> section? can we check that supplied ptr belongs to a descriptor section
> and avoid dereference_function_descriptor() if it doesn't? (just fall
> through directly to symbol_string() in this case). is this possible?
I think theoretically yes.
On parisc ptr does *not* point to any code segment, and most likely it
points to the .data segment. I don't know if that's the case for ia64 and
ppc64 too.
I can look into adding such check-code, but even then the warning will
only show up if you run on ia64, ppc64 and parisc64.
Helge
(Cc Tony, Fenghua, Benjamin, Paul, Michael)
a brief description:
original patch set:
lkml.kernel.org/r/[email protected]
start of this discussion:
lkml.kernel.org/r/[email protected]
basically we are looking at possibilities to make %pF/%pS differences
less disturbing. Helge has discovered a number of wrong usages in the
kernel and has fixed the currently existing call sites; the question
is what we can do to avoid this type of the patch sets in the future?
/* assuming that no one reads printk documentation */
Hopefully you guys can help.
On (09/07/17 11:12), Helge Deller wrote:
[..]
> > - ptr = dereference_function_descriptor(ptr);
> > + ptr = __dereference_function_descriptor(ptr);
>
> This is not needed.
> All affected arches (ia64, ppc64, parisc64) already call
> probe_kernel_address() inside their dereference_function_descriptor() function.
> So this patch just adds unnecessary overhead for all arches.
good, thanks. honestly, I obviously didn't check what each platform
does. guilty! sort of.
> > ... here is a question, does function descriptor belong to a special
> > section? can we check that supplied ptr belongs to a descriptor section
> > and avoid dereference_function_descriptor() if it doesn't? (just fall
> > through directly to symbol_string() in this case). is this possible?
>
> I think theoretically yes.
> On parisc ptr does *not* point to any code segment, and most likely it
> points to the .data segment. I don't know if that's the case for ia64 and
> ppc64 too.
> I can look into adding such check-code, but even then the warning will
> only show up if you run on ia64, ppc64 and parisc64.
ok. personally I think that we need to start doing "does ptr belong to
descriptor segment/section/etc" thing and skip
dereference_function_descriptor() when it doesn't. [well, where possible.
hopefully on every affected platform... if this problem actually bothers
any one]. that seems like the way to fix the root cause of the problem.
because it's you, Petr and may be 7 more guys who knows the difference
between %pF/%pS. no one else has any idea at all. and no one actually
reads the printk() documentation, let's be real :)
-ss
On (09/07/17 18:36), Sergey Senozhatsky wrote:
[..]
> > I can look into adding such check-code, but even then the warning will
> > only show up if you run on ia64, ppc64 and parisc64.
sorry, not sure I understand the "warning" part.
what I'm thinking about is:
- every platform that needs descriptor dereference defines its own
function. otherwise dereference_descriptor(p) is just (p).
- so it's something like
arch/platform_abc/include/asm/sections.h
#undef dereference_function_descriptor
static inline void *dereference_function_descriptor(void *ptr)
{
if (not_a_function_descriptor(ptr))
return ptr;
if (!probe_kernel_address(....))
return function_ip;
return ptr;
}
- so then in lib/vsprintf.c we can do unconditionally
case F:
case f:
case S:
case s:
case B:
ptr = dereference_function_descriptor(ptr);
return symbol_string(....);
because platforms will take care of proper descriptor dereference,
when needed.
- and ideally we even can drop %pF-%pf. because there won't
be any difference between `S' and `F'.
something like this.
let's see if this is possible.
any thoughts?
-ss
On 07.09.2017 11:51, Sergey Senozhatsky wrote:
> On (09/07/17 18:36), Sergey Senozhatsky wrote:
> [..]
>>> I can look into adding such check-code, but even then the warning will
>>> only show up if you run on ia64, ppc64 and parisc64.
>
> sorry, not sure I understand the "warning" part.
I was thinking about adding code which warns at runtime if %pF/%pS is
presumable used wrongly.
You are thinking about code to work around the complexity by some
kind of autodetection.
> what I'm thinking about is:
>
> - every platform that needs descriptor dereference defines its own
> function. otherwise dereference_descriptor(p) is just (p).
>
> - so it's something like
>
> arch/platform_abc/include/asm/sections.h
>
> #undef dereference_function_descriptor
> static inline void *dereference_function_descriptor(void *ptr)
> {
> if (not_a_function_descriptor(ptr))
> return ptr;
I'm not sure if it's possible on ia64/ppc64/parisc64
to reliably detect if it's a function descriptor or not.
> if (!probe_kernel_address(....))
> return function_ip;
> return ptr;
> }
>
> - so then in lib/vsprintf.c we can do unconditionally
>
> case F:
> case f:
> case S:
> case s:
> case B:
> ptr = dereference_function_descriptor(ptr);
> return symbol_string(....);
>
> because platforms will take care of proper descriptor dereference,
> when needed.
Ok, but...
> - and ideally we even can drop %pF-%pf. because there won't
> be any difference between `S' and `F'.
> something like this.
> let's see if this is possible.
> any thoughts?
I see your idea, nevertheless, there *is* a difference between
a "pointer to some assembler statement" (%pS), and a
"pointer to a function" (%pF) on some architectures.
That's why %pF and %pS printk specifiers were introduced in 2008
by Linus in commit 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2.
People will probably get it wrong sometimes, and to try to avoid this
by some magic autodetection is IMHO the wrong solution.
Instead, maybe adding some checks to scripts/checkpatch.pl can help?
E.g. warn if %pF is used in combination with the keywords like
_builtin_return_address, _RET_IP_, and similar.
Helge
>> if (not_a_function_descriptor(ptr))
>> return ptr;
>
> I'm not sure if it's possible on ia64/ppc64/parisc64
> to reliably detect if it's a function descriptor or not.
Agreed. I don't know how to write this test (without changing the compiler to
put the pointers in a separate section ... and then changing the module loader
to keep a list of all these sections).
-Tony
On Thu, 2017-09-07 at 14:38 +0200, Helge Deller wrote:
> Instead, maybe adding some checks to scripts/checkpatch.pl can help?
> E.g. warn if %pF is used in combination with the keywords like
> _builtin_return_address, _RET_IP_, and similar.
coccinelle is probably a better tool for that.
On (09/07/17 16:05), Luck, Tony wrote:
[..]
> >> if (not_a_function_descriptor(ptr))
> >> return ptr;
> >
> > I'm not sure if it's possible on ia64/ppc64/parisc64
> > to reliably detect if it's a function descriptor or not.
>
> Agreed. I don't know how to write this test (without changing the compiler to
> put the pointers in a separate section ... and then changing the module loader
> to keep a list of all these sections).
let me try one more time :)
so below is a number of assumptions, let me know if anything is wrong
there.... and let's try to fix the "wrong bits" ;)
RFC
1) function descriptor table is in .data, not in .text
correct?
2) symbol resolution consists of 3 steps:
a) we check if this is a kernel symbol and resolve it if so
b) we check if the addr belongs to any module and resolve the addr
if so
c) we check if the addr is bpf and resolve it if so. let's skip this part.
so, for (a) we probably can do something like below. can't we?
// not tested, as usual.
---
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 127e7cfafa55..4807e204428e 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -319,6 +319,16 @@ const char *kallsyms_lookup(unsigned long addr,
namebuf[KSYM_NAME_LEN - 1] = 0;
namebuf[0] = 0;
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64) || defined(CONFIG_PARISC)
+ if (!is_ksym_addr(addr)) {
+ unsigned long deref_addr;
+
+ deref_addr = dereference_function_descriptor(addr);
+ if (is_ksym_addr(deref_addr))
+ addr = deref_addr;
+ }
+#endif
+
if (is_ksym_addr(addr)) {
unsigned long pos;
----
if the addr is not in kernel .text, then try dereferencing it and check
if the dereferenced addr is in kernel .text.
now, for (b) we can do something like below... probably.
if the addr is not module .text (not .data), then check if dereferenced
address is module .text (not .data).
---
diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..f81c67b745ff 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3865,6 +3865,16 @@ static inline int within(unsigned long addr, void *start, unsigned long size)
return ((void *)addr >= start && (void *)addr < start + size);
}
+static inline bool __mod_text_address(struct module *mod,
+ unsigned long addr)
+{
+ /* Make sure it's within the text section. */
+ if (!within(addr, mod->init_layout.base, mod->init_layout.text_size)
+ && !within(addr, mod->core_layout.base, mod->core_layout.text_size))
+ return false;
+ return true;
+}
+
#ifdef CONFIG_KALLSYMS
/*
* This ignores the intensely annoying "mapping symbols" found
@@ -3942,6 +3952,14 @@ const char *module_address_lookup(unsigned long addr,
preempt_disable();
mod = __module_address(addr);
if (mod) {
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64) || defined(CONFIG_PARISC)
+ unsigned long deref_addr;
+
+ if (!__mod_text_address(mod, addr))
+ deref_addr = dereference_function_descriptor(addr);
+ if (__mod_text_address(mod, deref_addr))
+ addr = deref_addr;
+#endif
if (modname)
*modname = mod->name;
ret = get_ksymbol(mod, addr, size, offset);
---
so there are probably some broken parts there. like...
I don't know. something.
so - what is broken, and how can we fix/tweak it? help me out.
btw, get_ksymbol() is actually interesting. it scans module's sections,
so if we are able to distinguish descriptor ELF sections, then we can
dereference addr only if it belong to descriptor table ELF section.
-ss
On (09/06/17 22:27), Helge Deller wrote:
> This patch series fixes the wrong usages of the %pF and %pS printk format
> specifiers throughout the kernel code.
>
> Both specifiers have the same result on most architectures. But on ia64, ppc64
> and parisc64 architectures the %pF specifier does an extra conversion because
> there function pointers are actually function descriptors.
Helge,
did you also grep for %pf? or just for %pF?
-ss
Looks fine,
Reviewed-by: Christoph Hellwig <[email protected]>
On Fri, Sep 08, 2017 at 03:18:30PM +0900, Sergey Senozhatsky wrote:
> if the addr is not in kernel .text, then try dereferencing it and check
> if the dereferenced addr is in kernel .text.
If it really is a function pointer, then we know that it is safe
to dereference. But if it isn't, then maybe not?
If it is a function pointer then dereferening will indeed give
us a .text address. But if it isn't, it might still give us a
.text address (we could reduce the probability of a false hit
by checking that the .text address was exactly on a symbol with
no offset ... but data values that happen to be the addresses of
function entry points are possible).
-Tony
On 08.09.2017 19:25, Luck, Tony wrote:
> On Fri, Sep 08, 2017 at 03:18:30PM +0900, Sergey Senozhatsky wrote:
>> if the addr is not in kernel .text, then try dereferencing it and check
>> if the dereferenced addr is in kernel .text.
>
> If it really is a function pointer, then we know that it is safe
> to dereference. But if it isn't, then maybe not?
>
> If it is a function pointer then dereferening will indeed give
> us a .text address. But if it isn't, it might still give us a
> .text address (we could reduce the probability of a false hit
> by checking that the .text address was exactly on a symbol with
> no offset ... but data values that happen to be the addresses of
> function entry points are possible).
I don't like this kind of trying to figure out at runtime at all.
It's too much guessing in here IMHO.
What about this idea:
For %pF we always have pointers to functions, e.g.:
printk("Going to call: %pF\n", gettimeofday);
printk("Going to call: %pF\n", p->func);
and for %pS most (if not all) usages use some kind of casting
from "unsigned long" to "void *", e.g.:
printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
printk("%s: called from %pS\n", __func__, (void *)__builtin_return_address(0));
printk("Faulted at %pS\n", (void *)regs->ip);
So, what if we for the %pS case simply take the type as it is
(unsigned long) and introduce a new printk-format, e.g. "%luS" ?
The %pS examples above then become:
printk("%s: called from %luS\n", __func__, _RET_IP_);
printk("%s: called from %luS\n", __func__, __builtin_return_address(0));
printk("Faulted at %luS\n", regs->ip);
That way we don't need type-casting, gain compile-time type
checks from the compiler, and we could add a checkpatch (or occinelle)
check which checks for the combination of %pF/%pS and "void*" keyword
and suggest to use %luS.
Opinions?
Helge
On 08.09.2017 08:23, Sergey Senozhatsky wrote:
> On (09/06/17 22:27), Helge Deller wrote:
>> This patch series fixes the wrong usages of the %pF and %pS printk format
>> specifiers throughout the kernel code.
>>
>> Both specifiers have the same result on most architectures. But on ia64, ppc64
>> and parisc64 architectures the %pF specifier does an extra conversion because
>> there function pointers are actually function descriptors.
>
> Helge,
>
> did you also grep for %pf? or just for %pF?
I grepped only for %pF and %pS.
There might be wrong %ps/%pf usages too.
Helge
On 08.09.2017 08:18, Sergey Senozhatsky wrote:
> On (09/07/17 16:05), Luck, Tony wrote:
> [..]
>>>> if (not_a_function_descriptor(ptr))
>>>> return ptr;
>>>
>>> I'm not sure if it's possible on ia64/ppc64/parisc64
>>> to reliably detect if it's a function descriptor or not.
>>
>> Agreed. I don't know how to write this test (without changing the compiler to
>> put the pointers in a separate section ... and then changing the module loader
>> to keep a list of all these sections).
>
> let me try one more time :)
>
> so below is a number of assumptions, let me know if anything is wrong
> there.... and let's try to fix the "wrong bits" ;)
>
>
> RFC
>
>
> 1) function descriptor table is in .data, not in .text
> correct?
>
> 2) symbol resolution consists of 3 steps:
>
> a) we check if this is a kernel symbol and resolve it if so
> b) we check if the addr belongs to any module and resolve the addr
> if so
> c) we check if the addr is bpf and resolve it if so. let's skip this part.
>
>
> so, for (a) we probably can do something like below. can't we?
> // not tested, as usual.
>
>
> ---
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 127e7cfafa55..4807e204428e 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -319,6 +319,16 @@ const char *kallsyms_lookup(unsigned long addr,
> namebuf[KSYM_NAME_LEN - 1] = 0;
> namebuf[0] = 0;
>
> +#if defined(CONFIG_IA64) || defined(CONFIG_PPC64) || defined(CONFIG_PARISC)
> + if (!is_ksym_addr(addr)) {
> + unsigned long deref_addr;
> +
> + deref_addr = dereference_function_descriptor(addr);
> + if (is_ksym_addr(deref_addr))
> + addr = deref_addr;
> + }
> +#endif
> +
> if (is_ksym_addr(addr)) {
> unsigned long pos;
>
>
> ----
>
> if the addr is not in kernel .text, then try dereferencing it and check
> if the dereferenced addr is in kernel .text.
>
>
>
> now, for (b) we can do something like below... probably.
>
> if the addr is not module .text (not .data), then check if dereferenced
> address is module .text (not .data).
>
>
> ---
>
> diff --git a/kernel/module.c b/kernel/module.c
> index de66ec825992..f81c67b745ff 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3865,6 +3865,16 @@ static inline int within(unsigned long addr, void *start, unsigned long size)
> return ((void *)addr >= start && (void *)addr < start + size);
> }
>
> +static inline bool __mod_text_address(struct module *mod,
> + unsigned long addr)
> +{
> + /* Make sure it's within the text section. */
> + if (!within(addr, mod->init_layout.base, mod->init_layout.text_size)
> + && !within(addr, mod->core_layout.base, mod->core_layout.text_size))
> + return false;
> + return true;
> +}
> +
> #ifdef CONFIG_KALLSYMS
> /*
> * This ignores the intensely annoying "mapping symbols" found
> @@ -3942,6 +3952,14 @@ const char *module_address_lookup(unsigned long addr,
> preempt_disable();
> mod = __module_address(addr);
> if (mod) {
> +#if defined(CONFIG_IA64) || defined(CONFIG_PPC64) || defined(CONFIG_PARISC)
> + unsigned long deref_addr;
> +
> + if (!__mod_text_address(mod, addr))
> + deref_addr = dereference_function_descriptor(addr);
> + if (__mod_text_address(mod, deref_addr))
> + addr = deref_addr;
> +#endif
> if (modname)
> *modname = mod->name;
> ret = get_ksymbol(mod, addr, size, offset);
>
> ---
>
> so there are probably some broken parts there. like...
> I don't know. something.
>
> so - what is broken, and how can we fix/tweak it? help me out.
Sergey, I'm sure there is a way how you can get it somehow to work the way
you describe above, but even then nobody can guarantee you that it
will work in 100% of the cases.
It's somehow like "we have %lu and %c specifiers, and it's basically
the same, so let's try to figure out at runtime which one should be
used based on analysis of what was given as argument".
It may work somehow, but not always.
What about the idea of a %luS specifier (or something other) ?
Helge
> From: Sergey Senozhatsky [mailto:[email protected]]
> On (09/07/17 16:05), Luck, Tony wrote:
> +static inline bool __mod_text_address(struct module *mod,
> + unsigned long addr) {
> + /* Make sure it's within the text section. */
> + if (!within(addr, mod->init_layout.base, mod->init_layout.text_size)
> + && !within(addr, mod->core_layout.base, mod-
> >core_layout.text_size))
> + return false;
> + return true;
> +}
The __mod_text_address() may be defined only on IA64, PPC64 and PARISC since it's only called in those cases.
> +
> #ifdef CONFIG_KALLSYMS
> /*
> * This ignores the intensely annoying "mapping symbols" found @@ -3942,6
> +3952,14 @@ const char *module_address_lookup(unsigned long addr,
> preempt_disable();
> mod = __module_address(addr);
> if (mod) {
> +#if defined(CONFIG_IA64) || defined(CONFIG_PPC64) ||
> defined(CONFIG_PARISC)
> + unsigned long deref_addr;
> +
> + if (!__mod_text_address(mod, addr))
> + deref_addr = dereference_function_descriptor(addr);
> + if (__mod_text_address(mod, deref_addr))
> + addr = deref_addr; #endif
Thanks.
-Fenghua
On 20:27-20170906, Helge Deller wrote:
> Use the %pS printk format for printing symbols from direct addresses.
> This is important for the ia64, ppc64 and parisc64 architectures, while on
> other architectures there is no difference between %pS and %pF.
> Fix it for consistency across the kernel.
>
> Signed-off-by: Helge Deller <[email protected]>
> Cc: Nishanth Menon <[email protected]>
> Cc: Tero Kristo <[email protected]>
> Cc: Santosh Shilimkar <[email protected]>
> Cc: [email protected]
OK with me. Thanks for doing the patch.
Acked-by: Nishanth Menon <[email protected]>
Santosh, Tero: maybe queue for an rc or later cycle, I think?
[...]
--
Regards,
Nishanth Menon
On 20:27-20170906, 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 <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: Nishanth Menon <[email protected]>
> Cc: [email protected]
Looks fine to me at least. Kevin/Rafael if you'd like to pick this up in
some rc or later cycle..
Acked-by: Nishanth Menon <[email protected]>
--
Regards,
Nishanth Menon
On 9/8/2017 4:30 PM, Nishanth Menon wrote:
> On 20:27-20170906, Helge Deller wrote:
>> Use the %pS printk format for printing symbols from direct addresses.
>> This is important for the ia64, ppc64 and parisc64 architectures, while on
>> other architectures there is no difference between %pS and %pF.
>> Fix it for consistency across the kernel.
>>
>> Signed-off-by: Helge Deller <[email protected]>
>> Cc: Nishanth Menon <[email protected]>
>> Cc: Tero Kristo <[email protected]>
>> Cc: Santosh Shilimkar <[email protected]>
>> Cc: [email protected]
>
> OK with me. Thanks for doing the patch.
> Acked-by: Nishanth Menon <[email protected]>
>
> Santosh, Tero: maybe queue for an rc or later cycle, I think?
>
Doesn't look a rc type fix so can wait for next one.
On Fri 2017-09-08 22:49:51, Helge Deller wrote:
> On 08.09.2017 08:18, Sergey Senozhatsky wrote:
> > On (09/07/17 16:05), Luck, Tony wrote:
> > [..]
> >>>> if (not_a_function_descriptor(ptr))
> >>>> return ptr;
> >>>
> >>> I'm not sure if it's possible on ia64/ppc64/parisc64
> >>> to reliably detect if it's a function descriptor or not.
> >>
> >> Agreed. I don't know how to write this test (without changing the compiler to
> >> put the pointers in a separate section ... and then changing the module loader
> >> to keep a list of all these sections).
> >
> > let me try one more time :)
> >
> > so below is a number of assumptions, let me know if anything is wrong
> > there.... and let's try to fix the "wrong bits" ;)
> >
> >
> > RFC
> >
> >
> > 1) function descriptor table is in .data, not in .text
> > correct?
> >
> > 2) symbol resolution consists of 3 steps:
> >
> > a) we check if this is a kernel symbol and resolve it if so
> > b) we check if the addr belongs to any module and resolve the addr
> > if so
> > c) we check if the addr is bpf and resolve it if so. let's skip this part.
> >
> >
> > so, for (a) we probably can do something like below. can't we?
> > // not tested, as usual.
> >
> >
> > so there are probably some broken parts there. like...
> > I don't know. something.
> >
> > so - what is broken, and how can we fix/tweak it? help me out.
>
> Sergey, I'm sure there is a way how you can get it somehow to work the way
> you describe above, but even then nobody can guarantee you that it
> will work in 100% of the cases.
It seems that dereferencing an invalid function descriptor is rather
safe because probe_kernel_address() prevents crashes.
The question is if we could get wrong results by the autodetection.
The following possibilities come to my mind:
First, if the variable used to store the function descriptor is on
stack and is not initialized. Then there is a non-trivial chance
that the garbage on the stack will be a real return address to an
existing function. Then the autodetection would help to hide this.
Second, if wonder if the address of the function descriptor
might be in callsyms as well. Note that global variables
are in kallsyms as well. Then we would always print
the name of this variable.
I do not have a strong opinion here. On one hand, it is clear
that %pS and %pF are often misused. But I am not sure if the above
possible problems are acceptable.
> It's somehow like "we have %lu and %c specifiers, and it's basically
> the same, so let's try to figure out at runtime which one should be
> used based on analysis of what was given as argument".
> It may work somehow, but not always.
I am not sure if I miss something. But the different output of
%lu and %c should be easy to distinguish. Also the difference
is the same on all architectures and should be well known.
This is not true for the %pS vs. %pF species.
> What about the idea of a %luS specifier (or something other) ?
I am not a big fun of this. IMHO, the relation between a pointer
and symbol name makes more sense that a relation between an
unsigned long and a symbol name. IMHO, this would just
add even more confusion.
Best Regards,
Petr
PS: I wonder if the improved documentation and fixing all occurrences
might be enough to reduce this mistake. I guess that most of them
were caused by copying the same pattern from an already broken code.
On Wed 2017-09-06 22:27:49, Helge Deller wrote:
> Use the %pS printk format for printing symbols from direct addresses.
> In usermode-linux there is actually no difference between %pS and %pF, but for
> consistency throughout the kernel fix the wrong usage here too.
>
> Signed-off-by: Helge Deller <[email protected]>
> Cc: Jeff Dike <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: [email protected]
> ---
> arch/um/kernel/sysrq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/um/kernel/sysrq.c b/arch/um/kernel/sysrq.c
> index 6b995e8..05585ee 100644
> --- a/arch/um/kernel/sysrq.c
> +++ b/arch/um/kernel/sysrq.c
> @@ -20,7 +20,7 @@
>
> static void _print_addr(void *data, unsigned long address, int reliable)
> {
> - pr_info(" [<%08lx>] %s%pF\n", address, reliable ? "" : "? ",
> + pr_info(" [<%08lx>] %s%pS\n", address, reliable ? "" : "? ",
> (void *)address);
This seems to be used to print addresses from the stack.
IMHO, we should use %pB here.
Best Regards,
Petr
On Wed 2017-09-06 22:27:47, Helge Deller wrote:
> This patch series fixes the wrong usages of the %pF and %pS printk format
> specifiers throughout the kernel code.
>
> Both specifiers have the same result on most architectures. But on ia64, ppc64
> and parisc64 architectures the %pF specifier does an extra conversion because
> there function pointers are actually function descriptors.
>
> Helge
>
> Helge Deller (14):
> arm: Use %pS printk format for symbols from direct addresses
> um: Use %pS printk format for symbols from direct addresses
IMHO, we should use %pB in this patch.
> x86: Use %pS printk format for symbols from direct addresses
> ti_sci: Use %pS printk format for direct addresses
> i915: Use %pS printk format for direct addresses
> md/bcache: Use %pS printk format for direct addresses
> power/avs: Use %pS printk format for direct addresses
> fs/f2fs: Use %pS printk format for direct addresses
> fs/pstore: Use %pS printk format for direct addresses
> fs/xfs: Use %pS printk format for direct addresses
> smp: Use %pF printk format specifier for function pointers
> mm/memblock: Use %pS printk format for direct addresses
> netfilter/ipvs: Use %pS printk format for direct addresses
> sound/core: Use %pS printk format for direct addresses
All other patches look fine. For the entire patchset, except
for the "um: " patch, feel free to use:
Reviewed-by: Petr Mladek <[email protected]>
Let me known if this is enough or if I should answer each patch
individually.
Best Regards,
Petr
Hi,
On (09/08/17 22:23), Yu, Fenghua wrote:
> > From: Sergey Senozhatsky [mailto:[email protected]]
> > On (09/07/17 16:05), Luck, Tony wrote:
> > +static inline bool __mod_text_address(struct module *mod,
> > + unsigned long addr) {
> > + /* Make sure it's within the text section. */
> > + if (!within(addr, mod->init_layout.base, mod->init_layout.text_size)
> > + && !within(addr, mod->core_layout.base, mod-
> > >core_layout.text_size))
> > + return false;
> > + return true;
> > +}
>
> The __mod_text_address() may be defined only on IA64, PPC64 and PARISC since it's only called in those cases.
sure. well, I didn't post the exact patch. __mod_text_address() was
supposed to be used from __module_text_address() /* I factored out
__mod_text_address() from that function, basically */
---
@@ -4305,9 +4323,7 @@ struct module *__module_text_address(unsigned long addr)
{
struct module *mod = __module_address(addr);
if (mod) {
- /* Make sure it's within the text section. */
- if (!within(addr, mod->init_layout.base, mod->init_layout.text_size)
- && !within(addr, mod->core_layout.base, mod->core_layout.text_size))
+ if (!__mod_text_address(mod, addr))
mod = NULL;
}
return mod;
---
-ss
Hi,
On (09/08/17 22:49), Helge Deller wrote:
[..]
> Sergey, I'm sure there is a way how you can get it somehow to work the way
> you describe above, but even then nobody can guarantee you that it
> will work in 100% of the cases.
>
> It's somehow like "we have %lu and %c specifiers, and it's basically
> the same, so let's try to figure out at runtime which one should be
> used based on analysis of what was given as argument".
> It may work somehow, but not always.
>
> What about the idea of a %luS specifier (or something other) ?
the idea is to have less format specifiers ;)
%pF/%pf is a subtle ABI detail, which made it to API.
I'm OK to keep %pf/%pF, if we won't be able to improve %ps/%pS.
otherwise, I'd prefer to get rid of it.
-ss
On (09/08/17 10:25), Luck, Tony wrote:
> On Fri, Sep 08, 2017 at 03:18:30PM +0900, Sergey Senozhatsky wrote:
> > if the addr is not in kernel .text, then try dereferencing it and check
> > if the dereferenced addr is in kernel .text.
>
> If it really is a function pointer, then we know that it is safe
> to dereference. But if it isn't, then maybe not?
yes. I thought about it - we can do %pS on a pointer to a global
structure. so that simple heuristic would not work reliably. we
parse ELF sections, and we know the address range of .opd section,
so we can check if the supplied pointer is within the .opd section
or not.
.opd does exist on ia64. not sure what's the name of ELF descriptor
section on ppc64/parisc64.
if we will be able to simply do
.opd->address <= ptr <= .opd->address + .opd->size
then it mostly should work for us. I guess.
> If it is a function pointer then dereferening will indeed give
> us a .text address. But if it isn't, it might still give us a
> .text address (we could reduce the probability of a false hit
> by checking that the .text address was exactly on a symbol with
> no offset ... but data values that happen to be the addresses of
> function entry points are possible).
hm. yes, need to think more.
-ss
On (09/08/17 20:28), Helge Deller wrote:
[..]
> I don't like this kind of trying to figure out at runtime at all.
> It's too much guessing in here IMHO.
well, may be we can avoid any guessing by checking that the
pointer belongs to .opd section.
for kernel we can add 2 new unsigned longs - __opd_start __opd_end -- and
tweak the corresponding arch/${FOO}/kernel/vmlinux.lds.S file to set those
two, the same way text, unwinding, etc. are set.
.... wait a second.
arch/ia64/kernel/vmlinux.lds.S already handles .opd section
.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
*(.opd)
}
it just doesn't save start/end addresses. so all we need to to
is
.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+ __opd_start = .;
*(.opd)
+ __opd_end = .;
}
and tweak symbol dereference
static inline void *dereference_function_descriptor(void *ptr)
{
struct fdesc *desc = ptr;
void *p;
+ if (prt < (void *)__start_opd || (void *)__end_opd < ptr)
+ return ptr;
+
if (!probe_kernel_address(&desc->ip, p))
ptr = p;
return ptr;
now, the modules.
module_frob_arch_sections() has the following lines
else if (strcmp(".opd", secstrings + s->sh_name) == 0)
mod->arch.opd = s;
so, once, again, we keep the .opd section info in memory. and we
also have the size of that section
mod->arch.opd->sh_size = fdescs * sizeof(struct fdesc);
so it seems that we've got what we need. need to provide arch callback
(same way as we do with dereference_function_descriptor() to properly
dereference modules' symbols).
so I think we almost have what we need to make ps/pS smart enough
on ppc64/ia64/parisc.
powerpc and parisc handle kernel .opd section as well:
arch/powerpc/kernel/vmlinux.lds.S: .opd
arch/parisc/kernel/vmlinux.lds.S: .opd
need to check more.
> What about this idea:
> For %pF we always have pointers to functions, e.g.:
> printk("Going to call: %pF\n", gettimeofday);
> printk("Going to call: %pF\n", p->func);
>
> and for %pS most (if not all) usages use some kind of casting
> from "unsigned long" to "void *", e.g.:
> printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
> printk("%s: called from %pS\n", __func__, (void *)__builtin_return_address(0));
> printk("Faulted at %pS\n", (void *)regs->ip);
>
> So, what if we for the %pS case simply take the type as it is
> (unsigned long) and introduce a new printk-format, e.g. "%luS" ?
> The %pS examples above then become:
> printk("%s: called from %luS\n", __func__, _RET_IP_);
> printk("%s: called from %luS\n", __func__, __builtin_return_address(0));
> printk("Faulted at %luS\n", regs->ip);
>
> That way we don't need type-casting, gain compile-time type
> checks from the compiler, and we could add a checkpatch (or occinelle)
> check which checks for the combination of %pF/%pS and "void*" keyword
> and suggest to use %luS.
>
> Opinions?
hm. sounds interesting. but I'm afraid people won't be so happy
to learn a new printk format specifier.
-ss
On (09/14/17 16:40), Sergey Senozhatsky wrote:
[..]
> powerpc and parisc handle kernel .opd section as well:
>
> arch/powerpc/kernel/vmlinux.lds.S: .opd
> arch/parisc/kernel/vmlinux.lds.S: .opd
for modules, arch-s define mod_arch_specific struct.
parisc has .opd
(fdesc offset should be the start of .opd,
fdesc_offset + sizeof(fdesc) * fdesc_count should be the .opd address range)
struct mod_arch_specific
{
unsigned long got_offset, got_count, got_max;
unsigned long fdesc_offset, fdesc_count, fdesc_max;
struct {
unsigned long stub_offset;
unsigned int stub_entries;
} *section;
int unwind_section;
struct unwind_table *unwind;
};
ia64 has .opd
struct mod_arch_specific {
struct elf64_shdr *core_plt; /* core PLT section */
struct elf64_shdr *init_plt; /* init PLT section */
struct elf64_shdr *got; /* global offset table */
struct elf64_shdr *opd; /* official procedure descriptors */
struct elf64_shdr *unwind; /* unwind-table section */
unsigned long gp; /* global-pointer for module */
void *core_unw_table; /* core unwind-table cookie returned by unwinder */
void *init_unw_table; /* init unwind-table cookie returned by unwinder */
unsigned int next_got_entry; /* index of next available got entry */
};
powerpc does not keep track of .opd, need to add
struct mod_arch_specific {
#ifdef __powerpc64__
unsigned int stubs_section; /* Index of stubs section in module */
unsigned int toc_section; /* What section is the TOC? */
bool toc_fixed; /* Have we fixed up .TOC.? */
#ifdef CONFIG_DYNAMIC_FTRACE
unsigned long toc;
unsigned long tramp;
#endif
#else /* powerpc64 */
/* Indices of PLT sections within module. */
unsigned int core_plt_section;
unsigned int init_plt_section;
#ifdef CONFIG_DYNAMIC_FTRACE
unsigned long tramp;
#endif
#endif /* powerpc64 */
/* List of BUG addresses, source line numbers and filenames */
struct list_head bug_list;
struct bug_entry *bug_table;
unsigned int num_bugs;
};
seems like we are looking at a solution here.
thoughts?
-ss
On 14.09.2017 10:03, Sergey Senozhatsky wrote:
> On (09/14/17 16:40), Sergey Senozhatsky wrote:
> [..]
>> powerpc and parisc handle kernel .opd section as well:
>>
>> arch/powerpc/kernel/vmlinux.lds.S: .opd
>> arch/parisc/kernel/vmlinux.lds.S: .opd
>
> for modules, arch-s define mod_arch_specific struct.
>
> parisc has .opd
>
> (fdesc offset should be the start of .opd,
> fdesc_offset + sizeof(fdesc) * fdesc_count should be the .opd address range)
>
> struct mod_arch_specific
> {
> unsigned long got_offset, got_count, got_max;
> unsigned long fdesc_offset, fdesc_count, fdesc_max;
> struct {
> unsigned long stub_offset;
> unsigned int stub_entries;
> } *section;
> int unwind_section;
> struct unwind_table *unwind;
> };
>
>
> ia64 has .opd
>
> struct mod_arch_specific {
> struct elf64_shdr *core_plt; /* core PLT section */
> struct elf64_shdr *init_plt; /* init PLT section */
> struct elf64_shdr *got; /* global offset table */
> struct elf64_shdr *opd; /* official procedure descriptors */
> struct elf64_shdr *unwind; /* unwind-table section */
> unsigned long gp; /* global-pointer for module */
>
> void *core_unw_table; /* core unwind-table cookie returned by unwinder */
> void *init_unw_table; /* init unwind-table cookie returned by unwinder */
> unsigned int next_got_entry; /* index of next available got entry */
> };
>
>
> powerpc does not keep track of .opd, need to add
>
> struct mod_arch_specific {
> #ifdef __powerpc64__
> unsigned int stubs_section; /* Index of stubs section in module */
> unsigned int toc_section; /* What section is the TOC? */
> bool toc_fixed; /* Have we fixed up .TOC.? */
> #ifdef CONFIG_DYNAMIC_FTRACE
> unsigned long toc;
> unsigned long tramp;
> #endif
>
> #else /* powerpc64 */
> /* Indices of PLT sections within module. */
> unsigned int core_plt_section;
> unsigned int init_plt_section;
> #ifdef CONFIG_DYNAMIC_FTRACE
> unsigned long tramp;
> #endif
> #endif /* powerpc64 */
>
> /* List of BUG addresses, source line numbers and filenames */
> struct list_head bug_list;
> struct bug_entry *bug_table;
> unsigned int num_bugs;
> };
>
>
> seems like we are looking at a solution here.
>
> thoughts?
The basic concept of your proposal may work, and since it will avoid such
coding issues in the future I think it's probably the best solution.
Will you come up with a patch ? (I won't have time the next few days).
If yes,I'd be happy to test it on parisc.
Helge
On (09/14/17 10:39), Helge Deller wrote:
[..]
> The basic concept of your proposal may work, and since it will avoid such
> coding issues in the future I think it's probably the best solution.
>
> Will you come up with a patch ? (I won't have time the next few days).
> If yes,I'd be happy to test it on parisc.
cool.
I think I can try to come up with something. I don't have any access
to affected H/W, so if I won't be able to bring up qemu images I'll
be happy to just hand it over to platform maintainers: arch-s like
parisc64 are way to exotic ;) my aim is a removal of %pf/%pF here.
let's hear from ia64 and ppc64 guys.
-ss
On 14.09.2017 11:27, Sergey Senozhatsky wrote:
> On (09/14/17 10:39), Helge Deller wrote:
> [..]
>> The basic concept of your proposal may work, and since it will avoid such
>> coding issues in the future I think it's probably the best solution.
>>
>> Will you come up with a patch ? (I won't have time the next few days).
>> If yes,I'd be happy to test it on parisc.
>
> cool.
>
> I think I can try to come up with something. I don't have any access
> to affected H/W, so if I won't be able to bring up qemu images
There is no qemu for parisc yet, but cross-compiling the kernel on Fedora or
Debian from x86_64 is easy and ready-to-go.
Otherwise it's not a problem, I can try in a few days.
> I'll be happy to just hand it over to platform maintainers: arch-s like
> parisc64 are way to exotic ;) my aim is a removal of %pf/%pF here.
>
> let's hear from ia64 and ppc64 guys.
Ok too.
Helge
> let's hear from ia64 and ppc64 guys.
If you write a patch, I can try it on some ia64 h/w.
Please include some test cases (perhaps as a second patch that adds a few good/bad %pF and %pS
to some code (both in base kernel, and in a module).
-Tony
On (09/14/17 16:01), Luck, Tony wrote:
>
> > let's hear from ia64 and ppc64 guys.
>
> If you write a patch, I can try it on some ia64 h/w.
>
> Please include some test cases (perhaps as a second patch that adds a few good/bad %pF and %pS
> to some code (both in base kernel, and in a module).
Hello,
I sent out an RFC patch set.
would the following test case work for you?
kernel)
echo 1 > /proc/sys/vm/drop_caches
module)
modprobe zram
echo 100M > /sys/block/zram0/disksize
---
drivers/block/zram/zram_drv.c | 15 +++++++++++++++
fs/drop_caches.c | 11 +++++++++++
2 files changed, 26 insertions(+)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 2981c27d3aae..ac92aaeaced0 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1508,6 +1508,12 @@ static int zram_add(void)
struct request_queue *queue;
int ret, device_id;
+ printk("printk#13 %pF\n", (void *)_RET_IP_);
+ printk("printk#14 %pS\n", (void *)_RET_IP_);
+
+ printk("printk#15 %pF\n", __builtin_return_address(0));
+ printk("printk#16 %pS\n", __builtin_return_address(0));
+
zram = kzalloc(sizeof(struct zram), GFP_KERNEL);
if (!zram)
return -ENOMEM;
@@ -1730,6 +1736,15 @@ static int __init zram_init(void)
{
int ret;
+ printk("printk#7 %pF\n", zram_init);
+ printk("printk#8 %pS\n", zram_init);
+
+ printk("printk#9 %pF\n", (void *)_RET_IP_);
+ printk("printk#10 %pS\n", (void *)_RET_IP_);
+
+ printk("printk#11 %pF\n", __builtin_return_address(0));
+ printk("printk#12 %pS\n", __builtin_return_address(0));
+
ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare",
zcomp_cpu_up_prepare, zcomp_cpu_dead);
if (ret < 0)
diff --git a/fs/drop_caches.c b/fs/drop_caches.c
index d72d52b90433..3db4adcac78d 100644
--- a/fs/drop_caches.c
+++ b/fs/drop_caches.c
@@ -39,11 +39,22 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused)
iput(toput_inode);
}
+#include <linux/sched.h>
+
int drop_caches_sysctl_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *length, loff_t *ppos)
{
int ret;
+ printk("printk#1 %pF\n", schedule_timeout);
+ printk("printk#2 %pS\n", schedule_timeout);
+
+ printk("printk#3 %pF\n", (void *)_RET_IP_);
+ printk("printk#4 %pS\n", (void *)_RET_IP_);
+
+ printk("printk#5 %pF\n", __builtin_return_address(0));
+ printk("printk#6 %pS\n", __builtin_return_address(0));
+
ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
if (ret)
return ret;
On Wed, Sep 06, 2017 at 10:27:57PM +0200, 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 <[email protected]>
> Cc: "Darrick J. Wong" <[email protected]>
> Cc: [email protected]
Reviewed-by: Darrick J. Wong <[email protected]>
> ---
> fs/xfs/xfs_error.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 2f4feb9..028e50a 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -344,7 +344,7 @@ xfs_verifier_error(
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
>
> - xfs_alert(mp, "Metadata %s detected at %pF, %s block 0x%llx",
> + xfs_alert(mp, "Metadata %s detected at %pS, %s block 0x%llx",
> bp->b_error == -EFSBADCRC ? "CRC error" : "corruption",
> __return_address, bp->b_ops->name, bp->b_bn);
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 12, 2017 at 2:10 PM, Petr Mladek <[email protected]> wrote:
> On Wed 2017-09-06 22:27:49, Helge Deller wrote:
>> Use the %pS printk format for printing symbols from direct addresses.
>> In usermode-linux there is actually no difference between %pS and %pF, but for
>> consistency throughout the kernel fix the wrong usage here too.
>>
>> Signed-off-by: Helge Deller <[email protected]>
>> Cc: Jeff Dike <[email protected]>
>> Cc: Richard Weinberger <[email protected]>
>> Cc: [email protected]
>> ---
>> arch/um/kernel/sysrq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/um/kernel/sysrq.c b/arch/um/kernel/sysrq.c
>> index 6b995e8..05585ee 100644
>> --- a/arch/um/kernel/sysrq.c
>> +++ b/arch/um/kernel/sysrq.c
>> @@ -20,7 +20,7 @@
>>
>> static void _print_addr(void *data, unsigned long address, int reliable)
>> {
>> - pr_info(" [<%08lx>] %s%pF\n", address, reliable ? "" : "? ",
>> + pr_info(" [<%08lx>] %s%pS\n", address, reliable ? "" : "? ",
>> (void *)address);
>
> This seems to be used to print addresses from the stack.
> IMHO, we should use %pB here.
%pWTF? ;)
Agreed, let's use %pB.
--
Thanks,
//richard
On Wed, Sep 06, 2017 at 10:27:52PM +0200, Helge Deller wrote:
> Use the %pS printk format for printing symbols from direct addresses.
> This is important for the ia64, ppc64 and parisc64 architectures, while on
> other architectures there is no difference between %pS and %pF.
> Fix it for consistency across the kernel.
i915.ko never runs on any of these, but if it helps with keeping things
consistent, why not.
Applied, thanks.
-Daniel
>
> Signed-off-by: Helge Deller <[email protected]>
> Cc: Jani Nikula <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: [email protected]
> ---
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 4e00e5c..29c62d4 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -64,7 +64,7 @@ static unsigned long wait_timeout(void)
>
> static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
> {
> - DRM_DEBUG_DRIVER("%s missed breadcrumb at %pF, irq posted? %s, current seqno=%x, last=%x\n",
> + DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS, irq posted? %s, current seqno=%x, last=%x\n",
> engine->name, __builtin_return_address(0),
> yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
> &engine->irq_posted)),
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Wed, Sep 6, 2017 at 1:28 PM Helge Deller <[email protected]> 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 <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Tony Luck <[email protected]>
I missed this email from some time ago. I've now applied it, thanks!
-Kees
> ---
> fs/pstore/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index fefd226..59f65d7 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -116,7 +116,7 @@ static int pstore_ftrace_seq_show(struct seq_file *s, void *v)
>
> rec = (struct pstore_ftrace_record *)(ps->record->buf + data->off);
>
> - seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %pf <- %pF\n",
> + seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %ps <- %pS\n",
> pstore_ftrace_decode_cpu(rec),
> pstore_ftrace_read_timestamp(rec),
> rec->ip, rec->parent_ip, (void *)rec->ip,
> --
> 2.1.0
>
--
Kees Cook
On Thu, Nov 29, 2018 at 03:26:51PM -0800, Kees Cook wrote:
> On Wed, Sep 6, 2017 at 1:28 PM Helge Deller <[email protected]> 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 <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Tony Luck <[email protected]>
>
> I missed this email from some time ago. I've now applied it, thanks!
Though it isn't really needed any more. See
04b8eb7a4ccd ("symbol lookup: introduce dereference_symbol_descriptor()")
which is part of the plan to deprecate %pF
-Tony
On Thu, Nov 29, 2018 at 3:49 PM Luck, Tony <[email protected]> wrote:
>
> On Thu, Nov 29, 2018 at 03:26:51PM -0800, Kees Cook wrote:
> > On Wed, Sep 6, 2017 at 1:28 PM Helge Deller <[email protected]> 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 <[email protected]>
> > > Cc: Kees Cook <[email protected]>
> > > Cc: Tony Luck <[email protected]>
> >
> > I missed this email from some time ago. I've now applied it, thanks!
>
> Though it isn't really needed any more. See
>
> 04b8eb7a4ccd ("symbol lookup: introduce dereference_symbol_descriptor()")
>
> which is part of the plan to deprecate %pF
Ah-ha! Okay then, nevermind. :) Thanks!
--
Kees Cook
On Mon, Oct 09, 2017 at 07:52:24AM +0200, Simon Horman wrote:
> On Wed, Sep 06, 2017 at 10:28:00PM +0200, Helge Deller wrote:
> > The debug and error printk functions in ipvs uses wrongly the %pF instead of
> > the %pS printk format specifier for printing symbols for the address returned
> > by _builtin_return_address(0). Fix it for the ia64, ppc64 and parisc64
> > architectures.
> >
> > Signed-off-by: Helge Deller <[email protected]>
> > Cc: Wensong Zhang <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
>
> Sorry for the delay in processing this.
>
> Acked-by: Simon Horman <[email protected]>
>
> Pablo, could you take this through the nf-next tree directly?
Applied, thanks.
From 1580758086302401746@xxx Mon Oct 09 05:53:07 +0000 2017
X-GM-THRID: 1577823759598431641
X-Gmail-Labels: Inbox,Category Forums
On Wed, Sep 06, 2017 at 10:28:00PM +0200, Helge Deller wrote:
> The debug and error printk functions in ipvs uses wrongly the %pF instead of
> the %pS printk format specifier for printing symbols for the address returned
> by _builtin_return_address(0). Fix it for the ia64, ppc64 and parisc64
> architectures.
>
> Signed-off-by: Helge Deller <[email protected]>
> Cc: Wensong Zhang <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Sorry for the delay in processing this.
Acked-by: Simon Horman <[email protected]>
Pablo, could you take this through the nf-next tree directly?
From 1577823759598431641@xxx Wed Sep 06 20:33:15 +0000 2017
X-GM-THRID: 1577823759598431641
X-Gmail-Labels: Inbox,Category Forums