Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1727704 > unrolled thread

[PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
    [PATCH 07/14] power/avs: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
      Re: [PATCH 07/14] power/avs: Use %pS printk format for direct  addresses Nishanth Menon <nm@ti.com> - 2017-09-09 01:40 +0200
    [PATCH 03/14] x86: Use %pS printk format for symbols from direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
    [PATCH 06/14] md/bcache: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
      Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct  addresses Coly Li <i@coly.li> - 2017-09-07 07:00 +0200
        Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct  addresses Helge Deller <deller@gmx.de> - 2017-09-07 09:50 +0200
          Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct  addresses Coly Li <i@coly.li> - 2017-09-07 10:00 +0200
          Re: [PATCH 06/14] md/bcache: Use %pS printk format for direct  addresses Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-07 10:10 +0200
    [PATCH 01/14] arm: Use %pS printk format for symbols from direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
    [PATCH 14/14] sound/core: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
      Re: [PATCH 14/14] sound/core: Use %pS printk format for direct addresses Takashi Iwai <tiwai@suse.de> - 2017-09-07 10:40 +0200
    [PATCH 08/14] fs/f2fs: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
    [PATCH 09/14] fs/pstore: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
    [PATCH 04/14] ti_sci: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
      Re: [PATCH 04/14] ti_sci: Use %pS printk format for direct addresses Nishanth Menon <nm@ti.com> - 2017-09-09 01:40 +0200
        Re: [PATCH 04/14] ti_sci: Use %pS printk format for direct addresses Santosh Shilimkar <santosh.shilimkar@oracle.com> - 2017-09-09 02:40 +0200
    [PATCH 05/14] i915: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:30 +0200
    [PATCH 10/14] fs/xfs: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:40 +0200
      Re: [PATCH 10/14] fs/xfs: Use %pS printk format for direct addresses Christoph Hellwig <hch@infradead.org> - 2017-09-08 09:40 +0200
    [PATCH 13/14] netfilter/ipvs: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:40 +0200
    [PATCH 11/14] smp: Use %pF printk format specifier for function pointers Helge Deller <deller@gmx.de> - 2017-09-06 22:40 +0200
    [PATCH 02/14] um: Use %pS printk format for symbols from direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:40 +0200
    [PATCH 12/14] mm/memblock: Use %pS printk format for direct addresses Helge Deller <deller@gmx.de> - 2017-09-06 22:40 +0200
    Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-07 02:50 +0200
      Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Helge Deller <deller@gmx.de> - 2017-09-07 08:10 +0200
        Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-07 10:00 +0200
          Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-07 10:40 +0200
            Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Helge Deller <deller@gmx.de> - 2017-09-07 11:20 +0200
              Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-07 11:40 +0200
                Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-07 12:00 +0200
                  Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Helge Deller <deller@gmx.de> - 2017-09-07 14:40 +0200
                    RE: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages "Luck, Tony" <tony.luck@intel.com> - 2017-09-07 18:10 +0200
                      Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-08 08:30 +0200
                        Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages "Luck, Tony" <tony.luck@intel.com> - 2017-09-08 19:30 +0200
                          Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Helge Deller <deller@gmx.de> - 2017-09-08 20:30 +0200
                        Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Helge Deller <deller@gmx.de> - 2017-09-08 23:00 +0200
                        RE: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages "Yu, Fenghua" <fenghua.yu@intel.com> - 2017-09-09 00:30 +0200
                    Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Joe Perches <joe@perches.com> - 2017-09-07 19:00 +0200
    Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-08 08:30 +0200
      Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier  usages Helge Deller <deller@gmx.de> - 2017-09-08 22:50 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1727718 — [PATCH 13/14] netfilter/ipvs: Use %pS printk format for direct addresses

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 <deller@gmx.de>
Cc: Wensong Zhang <wensong@linux-vs.org>
Cc: netdev@vger.kernel.org
Cc: lvs-devel@vger.kernel.org
Cc: netfilter-devel@vger.kernel.org
---
 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

[toc] | [prev] | [next] | [standalone]

#1727719 — [PATCH 11/14] smp: Use %pF printk format specifier for function pointers

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 <deller@gmx.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 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

[toc] | [prev] | [next] | [standalone]

#1727720 — [PATCH 02/14] um: Use %pS printk format for symbols from direct addresses

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 <deller@gmx.de>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: user-mode-linux-devel@lists.sourceforge.net
---
 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

[toc] | [prev] | [next] | [standalone]

#1727721 — [PATCH 12/14] mm/memblock: Use %pS printk format for direct addresses

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 <deller@gmx.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 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

[toc] | [prev] | [next] | [standalone]

#1727858 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1727925 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1728003 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1728037 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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':
 

[toc] | [prev] | [next] | [standalone]

#1728072 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1728080 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

(Cc Tony, Fenghua, Benjamin, Paul, Michael)

a brief description:

original patch set:
  lkml.kernel.org/r/1504729681-3504-1-git-send-email-deller@gmx.de

start of this discussion:
  lkml.kernel.org/r/20170907075653.GA533@jagdpanzerIV.localdomain

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

[toc] | [prev] | [next] | [standalone]

#1728101 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1728200 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1728285 — RE: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1728625 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1729110 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1729184 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1729285 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

#1729334 — RE: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

> From: Sergey Senozhatsky [mailto:sergey.senozhatsky.work@gmail.com]
> 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

[toc] | [prev] | [next] | [standalone]

#1728318 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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.

[toc] | [prev] | [next] | [standalone]

#1728626 — Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

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

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web