From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756945AbdIRSje (ORCPT ); Mon, 18 Sep 2017 14:39:34 -0400 Received: from mout.gmx.net ([212.227.15.19]:52139 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751550AbdIRSjc (ORCPT ); Mon, 18 Sep 2017 14:39:32 -0400 Date: Mon, 18 Sep 2017 20:39:02 +0200 From: Helge Deller To: Sergey Senozhatsky Cc: "Luck, Tony" , Fenghua Yu , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , "James E . J . Bottomley" , Helge Deller , Petr Mladek , Steven Rostedt , Andrew Morton , Jessica Yu , Alexei Starovoitov , linux-ia64@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/5] [RFC] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers Message-ID: <20170918183902.GA30752@p100.box> References: <20170916035347.19705-1-sergey.senozhatsky@gmail.com> <20170918174432.4fksyzco2g6gczwe@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170918174432.4fksyzco2g6gczwe@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Provags-ID: V03:K0:3I94tXjBDeuYOBUoXRt9hk2APt/72fUGNY6h2xkyCwOOfOJUQQo BuE4oSPSFxcdxrDyGkae+T2jars4bW/fkmQbKsCKcHtekKqr1CdL1yNUA5L0i7C6tuzBn8h 8JmDS5aeD0rcwLoSOWNGYxnoEshdVQlbcbFGDfiOKC0FVoZ3ygCupWoXDBEFkclAmsy7Qsz yRNquATqyydPm+iDi7eyg== X-UI-Out-Filterresults: notjunk:1;V01:K0:U0+EQo0xO1I=:LWYLB4M0rawWvvKT2Eviye opu1WJYv2yDVbWpoFHNYHVPkoSnHvil/Lj+hRowFqi5fXaBRX/Pxud2vOVsEQ2eiOkoRXX5jx A6XOSZv6UfMWtlZ7v/m0dmrbkc4HsLbnDBRf746iVlphPSMsMPQSb/x91ErARRBMO9hqBz98a xIdmP8AyPsd6FFio64Fi/ATaBrNM9xyuEreIJYbIvyfsT/XimpWEV3YXF/YDE3Vvcp1shU5rI nWrVpFujZzK+f5y0541Vfn4RTcUV3dJod2czmiTOPHyncTNScbH7jaSjbicRgaewj/b/8Dc/Q 66e8cpi0i4n0D9npG2GXBcOd6l7R72HQEcgDCVc1qEG9463TDyKWDJXQ4oJMzthChG/2Y/dx6 QTMylURowoHwXb/0sny42DtvQQpSAOsOZxuw+bn0jIDbR0SK9KLXtmeHXP9njr35cNMFWbcXs 86rE9swjLcA4wHbZwhbZD6RU6Ojn3xxv4EmEhUdvSg6d7w4w+xSDHjOeiwfQIH2QRWxMGAIlC yd8s7QbYZUpo8pcyjUfC23/copTBTEqJhuQHqprHYwSOubFdlvRjA9HzGdX5DYffR4jouNmFa 7FzVnCFy1dMpUh9vNWt4tcIbeEnkJO4toOc0R79hjPQcv91T0yg8lnt6SI6W+fcAFVqgLw60b 8l0Mgmr5Ww0Mpz0Lp7z8jKLtJp8KjeXDtzT6bqjkntSFQhkUzF4Tinn4Yc7rmHkDHl/u4BxFN 272oyO4bMMU8GI2k+pjB71QF9UmBKnaRqswDMR4CxvcO8WVeg7+mbDG5w2ein2QAvI3l/Kydk jr6f9g8yFpsSb0tUrn/A0uB5OyuIw== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Luck, Tony : > On Sat, Sep 16, 2017 at 12:53:42PM +0900, Sergey Senozhatsky wrote: > > Hello > > > > RFC > > > > On some arches C function pointers are indirect and point to > > a function descriptor, which contains the actual pointer to the code. > > This mostly doesn't matter, except for cases when people want to print > > out function pointers in symbolic format, because the usual '%pS/%ps' > > does not work on those arches as expected. That's the reason why we > > have '%pF/%pf', but since it's here because of a subtle ABI detail > > specific to some arches (ppc64/ia64/parisc64) it's easy to misuse > > '%pF/%pf' and '%pS/%ps' (see [1], for example). > > A few new warnings when building on ia64: > > arch/ia64/kernel/module.c:931: warning: passing argument 1 of 'dereference_function_descriptor' makes pointer from integer without a cast > arch/ia64/kernel/module.c:931: warning: return makes integer from pointer without a cast > kernel/kallsyms.c:325: warning: assignment makes integer from pointer without a cast > kernel/kallsyms.c:325: warning: passing argument 1 of 'dereference_kernel_function_descriptor' makes pointer from integer without a cast I got similiar warnings on parisc. This patch on top of yours fixed those: diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c index bc2eae8..4f34b46 100644 --- a/arch/parisc/kernel/module.c +++ b/arch/parisc/kernel/module.c @@ -66,6 +66,7 @@ #include #include +#include #if 0 #define DEBUGP printk @@ -959,12 +960,12 @@ void module_arch_cleanup(struct module *mod) unsigned long dereference_module_function_descriptor(struct module *mod, unsigned long addr) { - void *opd_sz = mod->arch.fdesc_offset + + unsigned long opd_sz = mod->arch.fdesc_offset + mod->arch.fdesc_max * sizeof(Elf64_Fdesc); if (addr < mod->arch.fdesc_offset || opd_sz < addr) return addr; - return dereference_function_descriptor(addr); + return (unsigned long) dereference_function_descriptor((void *) addr); } #endif diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index e2fc09e..76f4de6 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -322,7 +322,7 @@ const char *kallsyms_lookup(unsigned long addr, if (is_ksym_addr(addr)) { unsigned long pos; - addr = dereference_kernel_function_descriptor(addr); + addr = dereference_kernel_function_descriptor((void *) addr); pos = get_symbol_pos(addr, symbolsize, offset); /* Grab name */ kallsyms_expand_symbol(get_symbol_offset(pos), I did tried your testcases too. "echo 1 > /proc/sys/vm/drop_caches" gave correct output: printk#1 schedule_timeout+0x0/0x4a8 printk#2 schedule_timeout+0x0/0x4a8 printk#3 proc_sys_call_handler+0x120/0x180 printk#4 proc_sys_call_handler+0x120/0x180 printk#5 proc_sys_call_handler+0x120/0x180 printk#6 proc_sys_call_handler+0x120/0x180 and here is "modprobe zram": printk#7 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram] printk#8 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram] printk#9 do_one_initcall+0x194/0x290 printk#10 do_one_initcall+0x194/0x290 printk#11 do_one_initcall+0x194/0x290 printk#12 do_one_initcall+0x194/0x290 printk#13 zram_init+0x22c/0x2a0 [zram] printk#14 zram_init+0x22c/0x2a0 [zram] printk#15 zram_init+0x22c/0x2a0 [zram] printk#16 zram_init+0x22c/0x2a0 [zram] I wonder why printk#7 and printk#8 don't show "zram_init"... Regarding your patches: In arch/parisc/kernel/process.c: +void *dereference_kernel_function_descriptor(void *ptr) +{ + if (ptr < (void *)__start_opd || (void *)__end_opd < ptr) This needs to be (__end_opd is outside): + if (ptr < (void *)__start_opd || (void *)__end_opd <= ptr) The same is true for the checks in the other arches. I'd suggest to move the various extern char __start_opd[], __end_opd[]; out of arch//include/asm/sections.h and into I'll continue to test. Helge From mboxrd@z Thu Jan 1 00:00:00 1970 From: Helge Deller Date: Mon, 18 Sep 2017 18:39:02 +0000 Subject: Re: [PATCH 0/5] [RFC] printk/ia64/ppc64/parisc64: let's deprecate %pF/%pf printk specifiers Message-Id: <20170918183902.GA30752@p100.box> List-Id: References: <20170916035347.19705-1-sergey.senozhatsky@gmail.com> <20170918174432.4fksyzco2g6gczwe@intel.com> In-Reply-To: <20170918174432.4fksyzco2g6gczwe@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Sergey Senozhatsky Cc: "Luck, Tony" , Fenghua Yu , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , "James E . J . Bottomley" , Helge Deller , Petr Mladek , Steven Rostedt , Andrew Morton , Jessica Yu , Alexei Starovoitov , linux-ia64@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org * Luck, Tony : > On Sat, Sep 16, 2017 at 12:53:42PM +0900, Sergey Senozhatsky wrote: > > Hello > > > > RFC > > > > On some arches C function pointers are indirect and point to > > a function descriptor, which contains the actual pointer to the code. > > This mostly doesn't matter, except for cases when people want to print > > out function pointers in symbolic format, because the usual '%pS/%ps' > > does not work on those arches as expected. That's the reason why we > > have '%pF/%pf', but since it's here because of a subtle ABI detail > > specific to some arches (ppc64/ia64/parisc64) it's easy to misuse > > '%pF/%pf' and '%pS/%ps' (see [1], for example). > > A few new warnings when building on ia64: > > arch/ia64/kernel/module.c:931: warning: passing argument 1 of 'dereference_function_descriptor' makes pointer from integer without a cast > arch/ia64/kernel/module.c:931: warning: return makes integer from pointer without a cast > kernel/kallsyms.c:325: warning: assignment makes integer from pointer without a cast > kernel/kallsyms.c:325: warning: passing argument 1 of 'dereference_kernel_function_descriptor' makes pointer from integer without a cast I got similiar warnings on parisc. This patch on top of yours fixed those: diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c index bc2eae8..4f34b46 100644 --- a/arch/parisc/kernel/module.c +++ b/arch/parisc/kernel/module.c @@ -66,6 +66,7 @@ #include #include +#include #if 0 #define DEBUGP printk @@ -959,12 +960,12 @@ void module_arch_cleanup(struct module *mod) unsigned long dereference_module_function_descriptor(struct module *mod, unsigned long addr) { - void *opd_sz = mod->arch.fdesc_offset + + unsigned long opd_sz = mod->arch.fdesc_offset + mod->arch.fdesc_max * sizeof(Elf64_Fdesc); if (addr < mod->arch.fdesc_offset || opd_sz < addr) return addr; - return dereference_function_descriptor(addr); + return (unsigned long) dereference_function_descriptor((void *) addr); } #endif diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index e2fc09e..76f4de6 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -322,7 +322,7 @@ const char *kallsyms_lookup(unsigned long addr, if (is_ksym_addr(addr)) { unsigned long pos; - addr = dereference_kernel_function_descriptor(addr); + addr = dereference_kernel_function_descriptor((void *) addr); pos = get_symbol_pos(addr, symbolsize, offset); /* Grab name */ kallsyms_expand_symbol(get_symbol_offset(pos), I did tried your testcases too. "echo 1 > /proc/sys/vm/drop_caches" gave correct output: printk#1 schedule_timeout+0x0/0x4a8 printk#2 schedule_timeout+0x0/0x4a8 printk#3 proc_sys_call_handler+0x120/0x180 printk#4 proc_sys_call_handler+0x120/0x180 printk#5 proc_sys_call_handler+0x120/0x180 printk#6 proc_sys_call_handler+0x120/0x180 and here is "modprobe zram": printk#7 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram] printk#8 __UNIQUE_ID_vermagic8+0xb9a4/0xbd04 [zram] printk#9 do_one_initcall+0x194/0x290 printk#10 do_one_initcall+0x194/0x290 printk#11 do_one_initcall+0x194/0x290 printk#12 do_one_initcall+0x194/0x290 printk#13 zram_init+0x22c/0x2a0 [zram] printk#14 zram_init+0x22c/0x2a0 [zram] printk#15 zram_init+0x22c/0x2a0 [zram] printk#16 zram_init+0x22c/0x2a0 [zram] I wonder why printk#7 and printk#8 don't show "zram_init"... Regarding your patches: In arch/parisc/kernel/process.c: +void *dereference_kernel_function_descriptor(void *ptr) +{ + if (ptr < (void *)__start_opd || (void *)__end_opd < ptr) This needs to be (__end_opd is outside): + if (ptr < (void *)__start_opd || (void *)__end_opd <= ptr) The same is true for the checks in the other arches. I'd suggest to move the various extern char __start_opd[], __end_opd[]; out of arch//include/asm/sections.h and into I'll continue to test. Helge