* [PATCH V2 2/4] powerpc/mm/radix: Move function from radix.h to pgtable-radix.c
From: Aneesh Kumar K.V @ 2018-05-29 14:28 UTC (permalink / raw)
To: benh, paulus, mpe, npiggin; +Cc: linuxppc-dev, Aneesh Kumar K.V
In-Reply-To: <20180529142841.19428-1-aneesh.kumar@linux.ibm.com>
In later patch we will update them which require them to be moved
to pgtable-radix.c. Keeping the function in radix.h results in
compile warning as below.
./arch/powerpc/include/asm/book3s/64/radix.h: In function ‘radix__ptep_set_access_flags’:
./arch/powerpc/include/asm/book3s/64/radix.h:196:28: error: dereferencing pointer to incomplete type ‘struct vm_area_struct’
struct mm_struct *mm = vma->vm_mm;
^~
./arch/powerpc/include/asm/book3s/64/radix.h:204:6: error: implicit declaration of function ‘atomic_read’; did you mean ‘__atomic_load’? [-Werror=implicit-function-declaration]
atomic_read(&mm->context.copros) > 0) {
^~~~~~~~~~~
__atomic_load
./arch/powerpc/include/asm/book3s/64/radix.h:204:21: error: dereferencing pointer to incomplete type ‘struct mm_struct’
atomic_read(&mm->context.copros) > 0) {
Instead of fixing header dependencies, we move the function to pgtable-radix.c
Also the function is now large to be a static inline . Doing the
move in separate patch helps in review.
No functional change in this patch. Only code movement.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/radix.h | 31 +++-------------------
arch/powerpc/mm/pgtable-radix.c | 22 +++++++++++++++
2 files changed, 25 insertions(+), 28 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
index 705193e7192f..36ed025b4e13 100644
--- a/arch/powerpc/include/asm/book3s/64/radix.h
+++ b/arch/powerpc/include/asm/book3s/64/radix.h
@@ -124,6 +124,9 @@ extern void radix__mark_rodata_ro(void);
extern void radix__mark_initmem_nx(void);
#endif
+extern void radix__ptep_set_access_flags(struct mm_struct *mm, pte_t *ptep,
+ pte_t entry, unsigned long address);
+
static inline unsigned long __radix_pte_update(pte_t *ptep, unsigned long clr,
unsigned long set)
{
@@ -190,34 +193,6 @@ static inline pte_t radix__ptep_get_and_clear_full(struct mm_struct *mm,
return __pte(old_pte);
}
-/*
- * Set the dirty and/or accessed bits atomically in a linux PTE, this
- * function doesn't need to invalidate tlb.
- */
-static inline void radix__ptep_set_access_flags(struct mm_struct *mm,
- pte_t *ptep, pte_t entry,
- unsigned long address)
-{
-
- unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED |
- _PAGE_RW | _PAGE_EXEC);
-
- if (cpu_has_feature(CPU_FTR_POWER9_DD1)) {
-
- unsigned long old_pte, new_pte;
-
- old_pte = __radix_pte_update(ptep, ~0, 0);
- /*
- * new value of pte
- */
- new_pte = old_pte | set;
- radix__flush_tlb_pte_p9_dd1(old_pte, mm, address);
- __radix_pte_update(ptep, 0, new_pte);
- } else
- __radix_pte_update(ptep, 0, set);
- asm volatile("ptesync" : : : "memory");
-}
-
static inline int radix__pte_same(pte_t pte_a, pte_t pte_b)
{
return ((pte_raw(pte_a) ^ pte_raw(pte_b)) == 0);
diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index ce24d72ea679..a6eec41dd347 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -1084,3 +1084,25 @@ int radix__has_transparent_hugepage(void)
return 0;
}
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
+
+void radix__ptep_set_access_flags(struct mm_struct *mm,
+ pte_t *ptep, pte_t entry,
+ unsigned long address)
+{
+ unsigned long set = pte_val(entry) & (_PAGE_DIRTY | _PAGE_ACCESSED |
+ _PAGE_RW | _PAGE_EXEC);
+
+ if (cpu_has_feature(CPU_FTR_POWER9_DD1)) {
+ unsigned long old_pte, new_pte;
+
+ old_pte = __radix_pte_update(ptep, ~0, 0);
+ /*
+ * new value of pte
+ */
+ new_pte = old_pte | set;
+ radix__flush_tlb_pte_p9_dd1(old_pte, mm, address);
+ __radix_pte_update(ptep, 0, new_pte);
+ } else
+ __radix_pte_update(ptep, 0, set);
+ asm volatile("ptesync" : : : "memory");
+}
--
2.17.0
^ permalink raw reply related
* [PATCH V2 1/4] powerpc/mm/hugetlb: Update huge_ptep_set_access_flags to call __ptep_set_access_flags directly
From: Aneesh Kumar K.V @ 2018-05-29 14:28 UTC (permalink / raw)
To: benh, paulus, mpe, npiggin; +Cc: linuxppc-dev, Aneesh Kumar K.V
In a later patch, we want to update __ptep_set_access_flags take page size
arg. This makes ptep_set_access_flags only work with mmu_virtual_psize.
To simplify the code make huge_ptep_set_access_flags directly call
__ptep_set_access_flags so that we can compute the hugetlb page size in
hugetlb function.
Now that ptep_set_access_flags won't be called for hugetlb remove
the is_vm_hugetlb_page() check and add the assert of pte lock
unconditionally.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/hugetlb.h | 19 +++--------------
arch/powerpc/mm/pgtable.c | 33 ++++++++++++++++++++++++++++--
2 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/hugetlb.h b/arch/powerpc/include/asm/hugetlb.h
index 78540c074d70..b4404a6da74f 100644
--- a/arch/powerpc/include/asm/hugetlb.h
+++ b/arch/powerpc/include/asm/hugetlb.h
@@ -166,22 +166,9 @@ static inline pte_t huge_pte_wrprotect(pte_t pte)
return pte_wrprotect(pte);
}
-static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma,
- unsigned long addr, pte_t *ptep,
- pte_t pte, int dirty)
-{
-#ifdef HUGETLB_NEED_PRELOAD
- /*
- * The "return 1" forces a call of update_mmu_cache, which will write a
- * TLB entry. Without this, platforms that don't do a write of the TLB
- * entry in the TLB miss handler asm will fault ad infinitum.
- */
- ptep_set_access_flags(vma, addr, ptep, pte, dirty);
- return 1;
-#else
- return ptep_set_access_flags(vma, addr, ptep, pte, dirty);
-#endif
-}
+extern int huge_ptep_set_access_flags(struct vm_area_struct *vma,
+ unsigned long addr, pte_t *ptep,
+ pte_t pte, int dirty);
static inline pte_t huge_ptep_get(pte_t *ptep)
{
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 9f361ae571e9..e70af9939379 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -221,14 +221,43 @@ int ptep_set_access_flags(struct vm_area_struct *vma, unsigned long address,
entry = set_access_flags_filter(entry, vma, dirty);
changed = !pte_same(*(ptep), entry);
if (changed) {
- if (!is_vm_hugetlb_page(vma))
- assert_pte_locked(vma->vm_mm, address);
+ assert_pte_locked(vma->vm_mm, address);
__ptep_set_access_flags(vma->vm_mm, ptep, entry, address);
flush_tlb_page(vma, address);
}
return changed;
}
+#ifdef CONFIG_HUGETLB_PAGE
+extern int huge_ptep_set_access_flags(struct vm_area_struct *vma,
+ unsigned long addr, pte_t *ptep,
+ pte_t pte, int dirty)
+{
+#ifdef HUGETLB_NEED_PRELOAD
+ /*
+ * The "return 1" forces a call of update_mmu_cache, which will write a
+ * TLB entry. Without this, platforms that don't do a write of the TLB
+ * entry in the TLB miss handler asm will fault ad infinitum.
+ */
+ ptep_set_access_flags(vma, addr, ptep, pte, dirty);
+ return 1;
+#else
+ int changed;
+
+ pte = set_access_flags_filter(pte, vma, dirty);
+ changed = !pte_same(*(ptep), pte);
+ if (changed) {
+#ifdef CONFIG_DEBUG_VM
+ assert_spin_locked(&vma->vm_mm->page_table_lock);
+#endif
+ __ptep_set_access_flags(vma->vm_mm, ptep, pte, addr);
+ flush_hugetlb_page(vma, addr);
+ }
+ return changed;
+#endif
+}
+#endif /* CONFIG_HUGETLB_PAGE */
+
#ifdef CONFIG_DEBUG_VM
void assert_pte_locked(struct mm_struct *mm, unsigned long addr)
{
--
2.17.0
^ permalink raw reply related
* Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()
From: Christophe LEROY @ 2018-05-29 14:13 UTC (permalink / raw)
To: Michal Suchanek, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Mauricio Faria de Oliveira, Nicholas Piggin,
Michael Neuling, linuxppc-dev, linux-kernel
In-Reply-To: <20180528131914.32231-1-msuchanek@suse.de>
Le 28/05/2018 à 15:19, Michal Suchanek a écrit :
> We now have barrier_nospec as mitigation so print it in
> cpu_show_spectre_v1 when enabled.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> arch/powerpc/kernel/security.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index 0239383c7e4d..a0c32d53980b 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, c
> if (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR))
> return sprintf(buf, "Not affected\n");
>
> - return sprintf(buf, "Vulnerable\n");
> + if (barrier_nospec_enabled)
> + return sprintf(buf, "Mitigation: __user pointer sanitization\n");
> + else
> + return sprintf(buf, "Vulnerable\n");
Checkpatch would tell you that an else is unneeded after a return. So
just leave it as it was before.
Christophe
> }
>
> ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, char *buf)
>
^ permalink raw reply
* Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()
From: kbuild test robot @ 2018-05-29 14:03 UTC (permalink / raw)
To: Michal Suchanek
Cc: kbuild-all, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Michal Suchanek, Mauricio Faria de Oliveira,
Nicholas Piggin, Michael Neuling, linuxppc-dev, linux-kernel
In-Reply-To: <20180528131914.32231-1-msuchanek@suse.de>
[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]
Hi Michal,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.17-rc7 next-20180529]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Michal-Suchanek/powerpc-64s-Enhance-the-information-in-cpu_show_spectre_v1/20180529-181036
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
arch/powerpc/kernel/security.c: In function 'cpu_show_spectre_v1':
>> arch/powerpc/kernel/security.c:54:6: error: 'barrier_nospec_enabled' undeclared (first use in this function); did you mean 'kernfs_ns_enabled'?
if (barrier_nospec_enabled)
^~~~~~~~~~~~~~~~~~~~~~
kernfs_ns_enabled
arch/powerpc/kernel/security.c:54:6: note: each undeclared identifier is reported only once for each function it appears in
arch/powerpc/kernel/security.c:58:1: error: control reaches end of non-void function [-Werror=return-type]
}
^
cc1: all warnings being treated as errors
vim +54 arch/powerpc/kernel/security.c
48
49 ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, char *buf)
50 {
51 if (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR))
52 return sprintf(buf, "Not affected\n");
53
> 54 if (barrier_nospec_enabled)
55 return sprintf(buf, "Mitigation: __user pointer sanitization\n");
56 else
57 return sprintf(buf, "Vulnerable\n");
58 }
59
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23378 bytes --]
^ permalink raw reply
* Re: [PATCH v4 4/4] powerpc/kbuild: move -mprofile-kernel check to Kconfig
From: Masahiro Yamada @ 2018-05-29 13:48 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Linux Kbuild mailing list, linuxppc-dev, Segher Boessenkool
In-Reply-To: <20180516141458.18996-5-npiggin@gmail.com>
2018-05-16 23:14 GMT+09:00 Nicholas Piggin <npiggin@gmail.com>:
> This eliminates the workaround that requires disabling
> -mprofile-kernel by default in Kconfig.
>
> [ Note: this depends on https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git kconfig-shell-v3 ]
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Since v3:
> - Moved a stray hunk back to patch 3 where it belongs.
>
> arch/powerpc/Kconfig | 16 +---------------
> arch/powerpc/Makefile | 14 ++------------
> 2 files changed, 3 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 512fcc177c87..af527f894f9b 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -460,23 +460,9 @@ config LD_HEAD_STUB_CATCH
>
> If unsure, say "N".
>
> -config DISABLE_MPROFILE_KERNEL
> - bool "Disable use of mprofile-kernel for kernel tracing"
> - depends on PPC64 && CPU_LITTLE_ENDIAN
> - default y
> - help
> - Selecting this options disables use of the mprofile-kernel ABI for
> - kernel tracing. That will cause options such as live patching
> - (CONFIG_LIVEPATCH) which depend on CONFIG_DYNAMIC_FTRACE_WITH_REGS to
> - be disabled also.
> -
> - If you have a toolchain which supports mprofile-kernel, then you can
> - disable this. Otherwise leave it enabled. If you're not sure, say
> - "Y".
> -
> config MPROFILE_KERNEL
> depends on PPC64 && CPU_LITTLE_ENDIAN
> - def_bool !DISABLE_MPROFILE_KERNEL
> + def_bool $(success $(srctree)/arch/powerpc/tools/gcc-check-mprofile-kernel.sh $(CC) -I$(srctree)/include -D__KERNEL__)
Your fix-up patch is good,
but could you check my comments below please?
> config IOMMU_HELPER
> def_bool PPC64
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 6faf1d6ad9dd..8f7a64fe7370 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -161,18 +161,8 @@ CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64
> endif
>
> ifdef CONFIG_MPROFILE_KERNEL
> - ifeq ($(shell $(srctree)/arch/powerpc/tools/gcc-check-mprofile-kernel.sh $(CC) -I$(srctree)/include -D__KERNEL__),OK)
You do not need to compare the returned string with "OK" any more.
Please remove the
echo "OK"
line from the shell script.
> - CC_FLAGS_FTRACE := -pg -mprofile-kernel
> - KBUILD_CPPFLAGS += -DCC_USING_MPROFILE_KERNEL
> - else
> - # If the user asked for mprofile-kernel but the toolchain doesn't
> - # support it, emit a warning and deliberately break the build later
> - # with mprofile-kernel-not-supported. We would prefer to make this an
> - # error right here, but then the user would never be able to run
> - # oldconfig to change their configuration.
> - $(warning Compiler does not support mprofile-kernel, set CONFIG_DISABLE_MPROFILE_KERNEL)
> - CC_FLAGS_FTRACE := -mprofile-kernel-not-supported
> - endif
> + CC_FLAGS_FTRACE := -pg -mprofile-kernel
> + KBUILD_CPPFLAGS += -DCC_USING_MPROFILE_KERNEL
Now, CONFIG_MPROFILE_KERNEL is equivalent to CC_USING_MPROFILE_KERNEL.
If you replace #ifdef CC_USING_MPROFILE_KERNEL
with #ifdef CONFIG_MPROFILE_KERNEL,
you can remove CC_USING_MRPROFILE_KERNEL entirely, right?
> endif
>
> CFLAGS-$(CONFIG_CELL_CPU) += $(call cc-option,-mcpu=cell)
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v4 1/4] powerpc/kbuild: set default generic machine type for 32-bit compile
From: Masahiro Yamada @ 2018-05-29 13:39 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Linux Kbuild mailing list, linuxppc-dev, Segher Boessenkool
In-Reply-To: <20180516141458.18996-2-npiggin@gmail.com>
2018-05-16 23:14 GMT+09:00 Nicholas Piggin <npiggin@gmail.com>:
> Some 64-bit toolchains uses the wrong ISA variant for compiling 32-bit
> kernels, even with -m32. Debian's powerpc64le is one such case, and
> that is because it is built with --with-cpu=power8.
>
> So when cross compiling a 32-bit kernel with a 64-bit toolchain, set
> -mcpu=powerpc initially, which is the generic 32-bit powerpc machine
> type and scheduling model. CPU and platform code can override this
> with subsequent -mcpu flags if necessary.
>
> This is not done for 32-bit toolchains otherwise it would override
> their defaults, which are presumably set appropriately for the
> environment (moreso than a 64-bit cross compiler).
>
> This fixes a lot of build failures due to incompatible assembly when
> compiling 32-bit kernel with th Debian powerpc64le 64-bit toolchain.
>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
Can you please remove the noise changes?
1/4 adds some blank lines, then 2/4 removes them.
> arch/powerpc/Makefile | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index 95813df90801..15ca4bafad82 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -24,9 +24,20 @@ ifeq ($(HAS_BIARCH),y)
> ifeq ($(CROSS32_COMPILE),)
> CROSS32CC := $(CC) -m32
> KBUILD_ARFLAGS += --target=elf32-powerpc
> +
This blank line will be removed by the next patch.
> +ifdef CONFIG_PPC32
> +# These options will be overridden by any -mcpu option that the CPU
> +# or platform code sets later on the command line, but they are needed
> +# to set a sane 32-bit cpu target for the 64-bit cross compiler which
> +# may default to the wrong ISA.
> +KBUILD_CFLAGS += -mcpu=powerpc
> +KBUILD_AFLAGS += -mcpu=powerpc
> +endif
> +
This blank line will be removed by the next patch.
> endif
> endif
>
> +
This blank line will be removed by the next patch.
> export CROSS32CC CROSS32AR
>
> ifeq ($(CROSS_COMPILE),)
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [next-20180517][ppc] watchdog: CPU 88 self-detected hard LOCKUP @ update_cfs_group+0x30/0x150
From: Abdul Haleem @ 2018-05-29 13:09 UTC (permalink / raw)
To: Nicholas Piggin
Cc: sachinp, Stephen Rothwell, linux-kernel, linux-next, linuxppc-dev
In-Reply-To: <20180521165056.5f3dceeb@roar.ozlabs.ibm.com>
On Mon, 2018-05-21 at 16:50 +1000, Nicholas Piggin wrote:
> Ah, it's POWER8.
>
> I'm betting we have a bug with nohz timer offloading somewhere.
>
> I *think* we may have seen similar on P9 as well, but that may be
> related to problems with stop states.
>
> Can you reproduce it easily? I'm thinking maybe adding some
> tracepoints that track decrementer settings and interrupts, and
> nohz offload activity might show something up.
Yes, the problem is reproducible consistently on our CI setup and today
It triggered on 4.17.0-rc6 (mainline) too.
--
Regard's
Abdul Haleem
IBM Linux Technology Centre
^ permalink raw reply
* [PATCH] powerpc/ptrace: Use copy_{from, to}_user() rather than open-coding
From: Michael Ellerman @ 2018-05-29 12:57 UTC (permalink / raw)
To: linuxppc-dev, viro; +Cc: malat
From: Al Viro <viro@ZenIV.linux.org.uk>
In PPC_PTRACE_GETHWDBGINFO and PPC_PTRACE_SETHWDEBUG we do an
access_ok() check and then __copy_{from,to}_user().
Instead we should just use copy_{from,to}_user() which does all that
for us and is less error prone.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/ptrace.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 0f63dd5972e9..9667666eb18e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -3082,27 +3082,19 @@ long arch_ptrace(struct task_struct *child, long request,
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#endif /* CONFIG_PPC_ADV_DEBUG_REGS */
- if (!access_ok(VERIFY_WRITE, datavp,
- sizeof(struct ppc_debug_info)))
+ if (copy_to_user(datavp, &dbginfo,
+ sizeof(struct ppc_debug_info)))
return -EFAULT;
- ret = __copy_to_user(datavp, &dbginfo,
- sizeof(struct ppc_debug_info)) ?
- -EFAULT : 0;
- break;
+ return 0;
}
case PPC_PTRACE_SETHWDEBUG: {
struct ppc_hw_breakpoint bp_info;
- if (!access_ok(VERIFY_READ, datavp,
- sizeof(struct ppc_hw_breakpoint)))
+ if (copy_from_user(&bp_info, datavp,
+ sizeof(struct ppc_hw_breakpoint)))
return -EFAULT;
- ret = __copy_from_user(&bp_info, datavp,
- sizeof(struct ppc_hw_breakpoint)) ?
- -EFAULT : 0;
- if (!ret)
- ret = ppc_set_hwdebug(child, &bp_info);
- break;
+ return ppc_set_hwdebug(child, &bp_info);
}
case PPC_PTRACE_DELHWDEBUG: {
--
2.14.1
^ permalink raw reply related
* Re: linux-next: boot messages from today's linux-next
From: Mathieu Malaterre @ 2018-05-29 12:39 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Michael Ellerman, Benjamin Herrenschmidt, PowerPC,
Linux-Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <20180529222327.29d16eb6@canb.auug.org.au>
Stephen,
On Tue, May 29, 2018 at 2:23 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi Mathieu,
>
> On Tue, 29 May 2018 13:56:48 +0200 Mathieu Malaterre <malat@debian.org> wrote:
>>
>> On Tue, May 29, 2018 at 1:02 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> > Hi all,
>> >
>> > My qemu boots of today's powerpc linux-next kernel produced the following
>> > boot message differences:
>> >
>> > -Max number of cores passed to firmware: 2048 (NR_CPUS = 2048)
>> > +Max number of cores passed to firmware: u (NR_CPUS = 2048)
>> >
>> > Seemingly caused by commit
>> >
>> > eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
>>
>> My bad. Patch is pending (thanks Michael!):
>>
>> https://patchwork.ozlabs.org/patch/921948/
>
> Umm, that fixes some other messages, but not this particular one.
Here is what I tried to say:
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 4d62f561f272..72ebe5896c1d 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -379,6 +379,11 @@ static void __init prom_printf(const char *format, ...)
}
prom_print_dec(vs);
break;
+ case 'u':
+ ++q;
+ vs = va_arg(args, unsigned long);
+ prom_print_dec(vs);
+ break;
case 'l':
++q;
if (*q == 0)
> --
> Cheers,
> Stephen Rothwell
^ permalink raw reply related
* [PATCH 2/2] powerpc: Add support for function error injection
From: Naveen N. Rao @ 2018-05-29 12:36 UTC (permalink / raw)
To: Ingo Molnar, Michael Ellerman, Masami Hiramatsu, Josef Bacik
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1527596631.git.naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/error-injection.h | 9 +++++++++
arch/powerpc/include/asm/ptrace.h | 5 +++++
3 files changed, 15 insertions(+)
create mode 100644 arch/powerpc/include/asm/error-injection.h
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 23247fa551e7..ed1ab693f945 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -194,6 +194,7 @@ config PPC
select HAVE_EBPF_JIT if PPC64
select HAVE_EFFICIENT_UNALIGNED_ACCESS if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
select HAVE_FTRACE_MCOUNT_RECORD
+ select HAVE_FUNCTION_ERROR_INJECTION
select HAVE_FUNCTION_GRAPH_TRACER
select HAVE_FUNCTION_TRACER
select HAVE_GCC_PLUGINS
diff --git a/arch/powerpc/include/asm/error-injection.h b/arch/powerpc/include/asm/error-injection.h
new file mode 100644
index 000000000000..b596eca04ef9
--- /dev/null
+++ b/arch/powerpc/include/asm/error-injection.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ERROR_INJECTION_H
+#define _ASM_ERROR_INJECTION_H
+
+#include <asm-generic/error-injection.h>
+
+#define ARCH_FUNC_RET "blr"
+
+#endif /* _ASM_ERROR_INJECTION_H */
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index e4923686e43a..c0705296c2f0 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -101,6 +101,11 @@ static inline long regs_return_value(struct pt_regs *regs)
return -regs->gpr[3];
}
+static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
+{
+ regs->gpr[3] = rc;
+}
+
#ifdef __powerpc64__
#define user_mode(regs) ((((regs)->msr) >> MSR_PR_LG) & 0x1)
#else
--
2.17.0
^ permalink raw reply related
* [PATCH 1/2] error-injection: Simplify arch specific helpers
From: Naveen N. Rao @ 2018-05-29 12:36 UTC (permalink / raw)
To: Ingo Molnar, Michael Ellerman, Masami Hiramatsu, Josef Bacik
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1527596631.git.naveen.n.rao@linux.vnet.ibm.com>
We already have an arch-independent way to set the instruction pointer
with instruction_pointer_set(). Using this allows us to get rid of the
need for override_function_with_return() that each architecture has to
implement.
Furthermore, just_return_func() only has to encode arch-specific
assembly instructions to return from a function. Introduce a macro
ARCH_FUNC_RET to provide the arch-specific instruction and move over
just_return_func() to generic code.
With these changes, architectures that already support kprobes, only
just need to ensure they provide regs_set_return_value(), GET_IP() (for
instruction_pointer_set()), and ARCH_FUNC_RET to support error
injection.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/x86/include/asm/error-injection.h | 6 +-----
arch/x86/lib/Makefile | 1 -
arch/x86/lib/error-inject.c | 20 --------------------
include/asm-generic/error-injection.h | 6 ++++++
include/linux/error-injection.h | 1 +
kernel/fail_function.c | 2 +-
kernel/trace/bpf_trace.c | 2 +-
lib/error-inject.c | 8 ++++++++
8 files changed, 18 insertions(+), 28 deletions(-)
delete mode 100644 arch/x86/lib/error-inject.c
diff --git a/arch/x86/include/asm/error-injection.h b/arch/x86/include/asm/error-injection.h
index 47b7a1296245..f3f22e237b86 100644
--- a/arch/x86/include/asm/error-injection.h
+++ b/arch/x86/include/asm/error-injection.h
@@ -2,12 +2,8 @@
#ifndef _ASM_ERROR_INJECTION_H
#define _ASM_ERROR_INJECTION_H
-#include <linux/compiler.h>
-#include <linux/linkage.h>
-#include <asm/ptrace.h>
#include <asm-generic/error-injection.h>
-asmlinkage void just_return_func(void);
-void override_function_with_return(struct pt_regs *regs);
+#define ARCH_FUNC_RET "ret"
#endif /* _ASM_ERROR_INJECTION_H */
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 25a972c61b0a..f23934bbaf4e 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -26,7 +26,6 @@ lib-y += memcpy_$(BITS).o
lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o insn-eval.o
lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
-lib-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o
lib-$(CONFIG_RETPOLINE) += retpoline.o
obj-y += msr.o msr-reg.o msr-reg-export.o hweight.o
diff --git a/arch/x86/lib/error-inject.c b/arch/x86/lib/error-inject.c
deleted file mode 100644
index 3cdf06128d13..000000000000
--- a/arch/x86/lib/error-inject.c
+++ /dev/null
@@ -1,20 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <linux/error-injection.h>
-#include <linux/kprobes.h>
-
-asmlinkage void just_return_func(void);
-
-asm(
- ".type just_return_func, @function\n"
- ".globl just_return_func\n"
- "just_return_func:\n"
- " ret\n"
- ".size just_return_func, .-just_return_func\n"
-);
-
-void override_function_with_return(struct pt_regs *regs)
-{
- regs->ip = (unsigned long)&just_return_func;
-}
-NOKPROBE_SYMBOL(override_function_with_return);
diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h
index 296c65442f00..8ac152cc204a 100644
--- a/include/asm-generic/error-injection.h
+++ b/include/asm-generic/error-injection.h
@@ -3,6 +3,9 @@
#define _ASM_GENERIC_ERROR_INJECTION_H
#if defined(__KERNEL__) && !defined(__ASSEMBLY__)
+#include <linux/compiler.h>
+#include <linux/linkage.h>
+
enum {
EI_ETYPE_NONE, /* Dummy value for undefined case */
EI_ETYPE_NULL, /* Return NULL if failure */
@@ -27,6 +30,9 @@ static struct error_injection_entry __used \
.addr = (unsigned long)fname, \
.etype = EI_ETYPE_##_etype, \
};
+
+asmlinkage void just_return_func(void);
+
#else
#define ALLOW_ERROR_INJECTION(fname, _etype)
#endif
diff --git a/include/linux/error-injection.h b/include/linux/error-injection.h
index 280c61ecbf20..f4a0b23423d2 100644
--- a/include/linux/error-injection.h
+++ b/include/linux/error-injection.h
@@ -4,6 +4,7 @@
#ifdef CONFIG_FUNCTION_ERROR_INJECTION
+#include <linux/types.h>
#include <asm/error-injection.h>
extern bool within_error_injection_list(unsigned long addr);
diff --git a/kernel/fail_function.c b/kernel/fail_function.c
index 1d5632d8bbcc..0ae2ca4a29e8 100644
--- a/kernel/fail_function.c
+++ b/kernel/fail_function.c
@@ -183,7 +183,7 @@ static int fei_kprobe_handler(struct kprobe *kp, struct pt_regs *regs)
if (should_fail(&fei_fault_attr, 1)) {
regs_set_return_value(regs, attr->retval);
- override_function_with_return(regs);
+ instruction_pointer_set(regs, (unsigned long)&just_return_func);
/* Kprobe specific fixup */
reset_current_kprobe();
preempt_enable_no_resched();
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 56ba0f2a01db..23f1f4ffda6c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -84,7 +84,7 @@ EXPORT_SYMBOL_GPL(trace_call_bpf);
BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
{
regs_set_return_value(regs, rc);
- override_function_with_return(regs);
+ instruction_pointer_set(regs, (unsigned long)&just_return_func);
return 0;
}
diff --git a/lib/error-inject.c b/lib/error-inject.c
index c0d4600f4896..7fdc92b5babc 100644
--- a/lib/error-inject.c
+++ b/lib/error-inject.c
@@ -20,6 +20,14 @@ struct ei_entry {
void *priv;
};
+asm(
+ ".type just_return_func, @function\n"
+ ".globl just_return_func\n"
+ "just_return_func:\n"
+ ARCH_FUNC_RET "\n"
+ ".size just_return_func, .-just_return_func\n"
+);
+
bool within_error_injection_list(unsigned long addr)
{
struct ei_entry *ent;
--
2.17.0
^ permalink raw reply related
* [PATCH 0/2] error-injection: simplify code and powerpc support
From: Naveen N. Rao @ 2018-05-29 12:36 UTC (permalink / raw)
To: Ingo Molnar, Michael Ellerman, Masami Hiramatsu, Josef Bacik
Cc: linuxppc-dev, linux-kernel
The first patch simplifies code around function error-injection by
limiting the need for arch-specific helpers. The second patch adds
support for powerpc.
- Naveen
Naveen N. Rao (2):
error-injection: Simplify arch specific helpers
powerpc: Add support for function error injection
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/error-injection.h | 9 +++++++++
arch/powerpc/include/asm/ptrace.h | 5 +++++
arch/x86/include/asm/error-injection.h | 6 +-----
arch/x86/lib/Makefile | 1 -
arch/x86/lib/error-inject.c | 20 --------------------
include/asm-generic/error-injection.h | 6 ++++++
include/linux/error-injection.h | 1 +
kernel/fail_function.c | 2 +-
kernel/trace/bpf_trace.c | 2 +-
lib/error-inject.c | 8 ++++++++
11 files changed, 33 insertions(+), 28 deletions(-)
create mode 100644 arch/powerpc/include/asm/error-injection.h
delete mode 100644 arch/x86/lib/error-inject.c
--
2.17.0
^ permalink raw reply
* Re: [PATCH] cpuidle/powernv : Add Description for cpuidle state
From: Michael Ellerman @ 2018-05-29 12:30 UTC (permalink / raw)
To: Stewart Smith, Abhishek Goel, rjw, daniel.lezcano, benh, paulus,
linux-pm, linuxppc-dev, linux-kernel
Cc: Abhishek Goel
In-Reply-To: <87d0xf6y16.fsf@linux.vnet.ibm.com>
Stewart Smith <stewart@linux.ibm.com> writes:
> Abhishek Goel <huntbag@linux.vnet.ibm.com> writes:
>> @@ -215,7 +216,7 @@ static inline void add_powernv_state(int index, const char *name,
>> u64 psscr_val, u64 psscr_mask)
>> {
>> strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
>> - strlcpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);
>> + strlcpy(powernv_states[index].desc, desc, CPUIDLE_DESC_LEN);
>
> We should still fall back to using name in the event of desc being null,
> as not all firmware will expose the descriptions.
>
>> @@ -311,6 +313,11 @@ static int powernv_add_idle_states(void)
>> pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n");
>> goto out;
>> }
>> + if (of_property_read_string_array(power_mgt,
>> + "ibm,cpu-idle-state-descs", descs, dt_idle_states) < 0) {
>> + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-descs in DT\n");
>> + goto out;
>> + }
>
> I don't think pr_warn is appropriate here, as for all current released
> firmware we don't have that property. I think perhaps just silently
> continuing on is okay, as we have to keep compatibility with that firmware.
+1
Anyone who's wondering why they're not seeing the names can just look at
the device tree on the machine to see if the property is there or not,
they don't need a printk in dmesg to tell them.
cheers
^ permalink raw reply
* Re: linux-next: boot messages from today's linux-next
From: Stephen Rothwell @ 2018-05-29 12:23 UTC (permalink / raw)
To: Mathieu Malaterre
Cc: Michael Ellerman, Benjamin Herrenschmidt, PowerPC,
Linux-Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <CA+7wUsw4Us749j8yO5zpqfMcYWPMujwkNq0CRSSWWVFDJXLS0w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 769 bytes --]
Hi Mathieu,
On Tue, 29 May 2018 13:56:48 +0200 Mathieu Malaterre <malat@debian.org> wrote:
>
> On Tue, May 29, 2018 at 1:02 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi all,
> >
> > My qemu boots of today's powerpc linux-next kernel produced the following
> > boot message differences:
> >
> > -Max number of cores passed to firmware: 2048 (NR_CPUS = 2048)
> > +Max number of cores passed to firmware: u (NR_CPUS = 2048)
> >
> > Seemingly caused by commit
> >
> > eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
>
> My bad. Patch is pending (thanks Michael!):
>
> https://patchwork.ozlabs.org/patch/921948/
Umm, that fixes some other messages, but not this particular one.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: linux-next: boot messages from today's linux-next
From: Mathieu Malaterre @ 2018-05-29 12:14 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Michael Ellerman, Benjamin Herrenschmidt, PowerPC,
Linux-Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <CA+7wUswm60dZAfnTYJ-2p3o1H1ubwn1_OH7GSu9Y_PRKKCHJog@mail.gmail.com>
Michael,
On Tue, May 29, 2018 at 2:00 PM, Mathieu Malaterre <malat@debian.org> wrote:
> On Tue, May 29, 2018 at 1:56 PM, Mathieu Malaterre <malat@debian.org> wrote:
>> On Tue, May 29, 2018 at 1:02 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>>> Hi all,
>>>
>>> My qemu boots of today's powerpc linux-next kernel produced the following
>>> boot message differences:
>>>
>>> -Max number of cores passed to firmware: 2048 (NR_CPUS = 2048)
>>> +Max number of cores passed to firmware: u (NR_CPUS = 2048)
>>>
>>> Seemingly caused by commit
>>>
>>> eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
>>
>> My bad. Patch is pending (thanks Michael!):
>>
>> https://patchwork.ozlabs.org/patch/921948/
>
> Nope, nevermind this is unrelated.
Could you update your patch "powerpc/prom: Fix %llx usage since
prom_printf() change" to also do "powerpc/prom: Fix %u usage since
prom_printf() change"
I did not pay attention that "%u" was also not supported in prom_printf.
I may be able to provide a patch tonight, if this is not acceptable,
please revert my faulty patch. Sorry for the mess.
>>> --
>>> Cheers,
>>> Stephen Rothwell
^ permalink raw reply
* Re: linux-next: boot messages from today's linux-next
From: Mathieu Malaterre @ 2018-05-29 12:00 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Michael Ellerman, Benjamin Herrenschmidt, PowerPC,
Linux-Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <CA+7wUsw4Us749j8yO5zpqfMcYWPMujwkNq0CRSSWWVFDJXLS0w@mail.gmail.com>
On Tue, May 29, 2018 at 1:56 PM, Mathieu Malaterre <malat@debian.org> wrote:
> On Tue, May 29, 2018 at 1:02 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> Hi all,
>>
>> My qemu boots of today's powerpc linux-next kernel produced the following
>> boot message differences:
>>
>> -Max number of cores passed to firmware: 2048 (NR_CPUS = 2048)
>> +Max number of cores passed to firmware: u (NR_CPUS = 2048)
>>
>> Seemingly caused by commit
>>
>> eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
>
> My bad. Patch is pending (thanks Michael!):
>
> https://patchwork.ozlabs.org/patch/921948/
Nope, nevermind this is unrelated.
>> --
>> Cheers,
>> Stephen Rothwell
^ permalink raw reply
* Re: [PATCH] powerpc/prom: Fix %llx usage since prom_printf() change
From: Mathieu Malaterre @ 2018-05-29 11:58 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <20180529101529.2583-1-mpe@ellerman.id.au>
On Tue, May 29, 2018 at 12:15 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> We recently added the __printf attribute to prom_printf(), which means
> GCC started warning about type/format mismatches. As part of that
> commit we changed some "%lx" formats to "%llx" where the type is
> actually unsigned long long.
>
> Unfortunately prom_printf() doesn't know how to print "%llx", it just
> prints a literal "lx", eg:
>
> reserved memory map:
> lx - lx
> lx - lx
Sorry about that. Patch looks good, will try to work on handling llx/llu.
Reviewed-by: Mathieu Malaterre <malat@debian.org>
> We should fix that at some point, but for now just cast the relevant
> values to unsigned long to get things printing again. On 64-bit that
> has no effect on the output, because both types are 64-bit wide. On
> 32-bit it means we're potentially not printing the high 32-bits of
> some values, but most of them are pointers anyway, and we've lived
> with that behaviour up until now.
>
> Fixes: eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kernel/prom_init.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 425992e393bc..662dd68c3fb8 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1580,7 +1580,7 @@ static void __init prom_instantiate_opal(void)
> return;
> }
>
> - prom_printf("instantiating opal at 0x%llx...", base);
> + prom_printf("instantiating opal at 0x%lx...", (unsigned long)base);
>
> if (call_prom_ret("call-method", 4, 3, rets,
> ADDR("load-opal-runtime"),
> @@ -1596,10 +1596,10 @@ static void __init prom_instantiate_opal(void)
>
> reserve_mem(base, size);
>
> - prom_debug("opal base = 0x%llx\n", base);
> - prom_debug("opal align = 0x%llx\n", align);
> - prom_debug("opal entry = 0x%llx\n", entry);
> - prom_debug("opal size = 0x%llx\n", size);
> + prom_debug("opal base = 0x%lx\n", (unsigned long)base);
> + prom_debug("opal align = 0x%lx\n", (unsigned long)align);
> + prom_debug("opal entry = 0x%lx\n", (unsigned long)entry);
> + prom_debug("opal size = 0x%lx\n", (unsigned long)size);
>
> prom_setprop(opal_node, "/ibm,opal", "opal-base-address",
> &base, sizeof(base));
> @@ -1734,7 +1734,7 @@ static void __init prom_instantiate_sml(void)
> if (base == 0)
> prom_panic("Could not allocate memory for sml\n");
>
> - prom_printf("instantiating sml at 0x%llx...", base);
> + prom_printf("instantiating sml at 0x%lx...", (unsigned long)base);
>
> memset((void *)base, 0, size);
>
> @@ -1753,7 +1753,7 @@ static void __init prom_instantiate_sml(void)
> prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> &size, sizeof(size));
>
> - prom_debug("sml base = 0x%llx\n", base);
> + prom_debug("sml base = 0x%lx\n", (unsigned long)base);
> prom_debug("sml size = 0x%x\n", size);
>
> prom_debug("prom_instantiate_sml: end...\n");
> @@ -1847,7 +1847,7 @@ static void __init prom_initialize_tce_table(void)
>
> prom_debug("TCE table: %s\n", path);
> prom_debug("\tnode = 0x%x\n", node);
> - prom_debug("\tbase = 0x%llx\n", base);
> + prom_debug("\tbase = 0x%lx\n", (unsigned long)base);
> prom_debug("\tsize = 0x%x\n", minsize);
>
> /* Initialize the table to have a one-to-one mapping
> @@ -2559,9 +2559,9 @@ static void __init flatten_device_tree(void)
> int i;
> prom_printf("reserved memory map:\n");
> for (i = 0; i < mem_reserve_cnt; i++)
> - prom_printf(" %llx - %llx\n",
> - be64_to_cpu(mem_reserve_map[i].base),
> - be64_to_cpu(mem_reserve_map[i].size));
> + prom_printf(" %lx - %lx\n",
> + (unsigned long)be64_to_cpu(mem_reserve_map[i].base),
> + (unsigned long)be64_to_cpu(mem_reserve_map[i].size));
> }
> #endif
> /* Bump mem_reserve_cnt to cause further reservations to fail
> --
> 2.14.1
>
^ permalink raw reply
* Re: linux-next: boot messages from today's linux-next
From: Mathieu Malaterre @ 2018-05-29 11:56 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Michael Ellerman, Benjamin Herrenschmidt, PowerPC,
Linux-Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <20180529210257.26da8de7@canb.auug.org.au>
On Tue, May 29, 2018 at 1:02 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi all,
>
> My qemu boots of today's powerpc linux-next kernel produced the following
> boot message differences:
>
> -Max number of cores passed to firmware: 2048 (NR_CPUS = 2048)
> +Max number of cores passed to firmware: u (NR_CPUS = 2048)
>
> Seemingly caused by commit
>
> eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
My bad. Patch is pending (thanks Michael!):
https://patchwork.ozlabs.org/patch/921948/
> --
> Cheers,
> Stephen Rothwell
^ permalink raw reply
* linux-next: boot messages from today's linux-next
From: Stephen Rothwell @ 2018-05-29 11:02 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, PowerPC
Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
Mathieu Malaterre
[-- Attachment #1: Type: text/plain, Size: 376 bytes --]
Hi all,
My qemu boots of today's powerpc linux-next kernel produced the following
boot message differences:
-Max number of cores passed to firmware: 2048 (NR_CPUS = 2048)
+Max number of cores passed to firmware: u (NR_CPUS = 2048)
Seemingly caused by commit
eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Kernel OOPS while running memory hotplug test on P6 machine
From: vrbagal1 @ 2018-05-29 10:40 UTC (permalink / raw)
To: linuxppc-dev
Greetings!!!
I am observing kernel oops while running memory hotplug test on P6
machine, on main line kernel.
Machine: Power 6
kernel version 4.17.0-rc6
gcc version 4.8.3 20140911 (Red Hat 4.8.3-7)
Trace:
10:36:09 Unable to handle kernel paging request for data at address
0xc000492746462a90
10:36:09 Faulting instruction address: 0xc00000000023d190
10:36:09 Oops: Kernel access of bad area, sig: 11 [#1]
10:36:09 BE SMP NR_CPUS=1024 NUMA pSeries
10:36:09 Dumping ftrace buffer:
10:36:09 (ftrace buffer empty)
10:36:09 Modules linked in: rpadlpar_io(E) rpaphp(E)
nf_conntrack_netbios_ns(E) nf_conntrack_broadcast(E) ip6t_REJECT(E)
nf_reject_ipv6(E) nf_conntrack_ipv6(E) nf_defrag_ipv6(E) ipt_REJECT(E)
nf_reject_ipv4(E) nf_conntrack_ipv4(E) nf_defrag_ipv4(E) cfg80211(E)
xt_conntrack(E) nf_conntrack(E) rfkill(E) libcrc32c(E) ebtable_nat(E)
ebtable_broute(E) bridge(E) stp(E) llc(E) ebtable_filter(E) ebtables(E)
ip6table_mangle(E) ip6table_security(E) ip6table_raw(E)
ip6table_filter(E) ip6_tables(E) iptable_mangle(E) iptable_security(E)
iptable_raw(E) iptable_filter(E) ip_tables(E) ses(E) enclosure(E)
osst(E) st(E) scsi_transport_sas(E) uio_pdrv_genirq(E) uio(E) ehea(E)
nfsd(E) auth_rpcgss(E) nfs_acl(E) lockd(E) sunrpc(E) grace(E) ipv6(E)
crc_ccitt(E) autofs4(E) ext4(E) mbcache(E) jbd2(E) sr_mod(E) cdrom(E)
10:36:09 sd_mod(E) dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E)
dax(E)
10:36:09 CPU: 2 PID: 7370 Comm: avocado Tainted: G E
4.17.0-rc6-autotest #1
10:36:09 NIP: c00000000023d190 LR: c0000000002cb88c CTR:
c0000000001259e0
10:36:09 REGS: c0000000fb0b7370 TRAP: 0300 Tainted: G E
(4.17.0-rc6-autotest)
10:36:09 MSR: 800000000200b032 <SF,VEC,EE,FP,ME,IR,DR,RI> CR: 24242488
XER: 00000000
10:36:09 CFAR: c000000000008924 DAR: c000492746462a90 DSISR: 40000000
SOFTE: 1
10:36:09 GPR00: c0000000002cb630 c0000000fb0b75f0 c000000001126400
c0000000d3333800
10:36:09 GPR04: 9249249249210100 0000000000000002 0000000000000007
0000000000057800
10:36:09 GPR08: 0000492492492490 c0000002b3fd0600 0092492492492101
0000000000000040
10:36:09 GPR12: 0000000024242488 c00000000eead000 00007fff95b31710
00000080dee637a8
10:36:09 GPR16: 0000000000000000 c0000000d3fd6920 0000000000000001
0000000000000001
10:36:09 GPR20: 0000000000000001 0000000000000000 000000000000cc00
00000000003fffff
10:36:09 GPR24: c00000000116cab0 0000000000000008 0000000000000000
0000000000000001
10:36:09 GPR28: c0000000012d1aa0 c0000000d3fd6920 c0000000d3fd6500
c0000000d3330000
10:36:09 NIP [c00000000023d190] .get_pfnblock_flags_mask+0x20/0xd0
10:36:09 LR [c0000000002cb88c] .unset_migratetype_isolate+0x2bc/0x340
10:36:09 Call Trace:
10:36:09 [c0000000fb0b75f0] [c0000000002cb630]
.unset_migratetype_isolate+0x60/0x340 (unreliable)
10:36:09 [c0000000fb0b76a0] [c0000000002cbc00]
.start_isolate_page_range+0x2f0/0x4c0
10:36:09 [c0000000fb0b77a0] [c0000000002c5a74]
.__offline_pages+0x114/0xaa0
10:36:09 [c0000000fb0b78f0] [c0000000005966d8]
.memory_subsys_offline+0x58/0xe0
10:36:09 [c0000000fb0b7970] [c000000000572ba8]
.device_offline+0xe8/0x130
10:36:09 [c0000000fb0b7a00] [c00000000059642c]
.store_mem_state+0x15c/0x180
10:36:09 [c0000000fb0b7a90] [c00000000056dc10] .dev_attr_store+0x30/0x60
10:36:09 [c0000000fb0b7b00] [c00000000037fd70] .sysfs_kf_write+0x60/0xa0
10:36:09 [c0000000fb0b7b70] [c00000000037eb40]
.kernfs_fop_write+0x180/0x250
10:36:09 [c0000000fb0b7c10] [c0000000002d2a4c] .__vfs_write+0x3c/0x1e0
10:36:09 [c0000000fb0b7cf0] [c0000000002d2e00] .vfs_write+0xc0/0x230
10:36:09 [c0000000fb0b7d90] [c0000000002d3124] .ksys_write+0x54/0x100
10:36:09 [c0000000fb0b7e30] [c00000000000b9d8] system_call+0x58/0x6c
10:36:09 Instruction dump:
10:36:09 4e800020 60000000 60000000 60000000 3d02001b 788ac202 3928b6a0
e9290000
10:36:09 2fa90000 419e006c 78886502 79081f24 <7d29402a> 2fa90000
419e000c 794a2428
10:36:09 ---[ end trace a4b1e0a92ed26a33 ]---
regards,
Venkat.
^ permalink raw reply
* [PATCH] powerpc/prom: Fix %llx usage since prom_printf() change
From: Michael Ellerman @ 2018-05-29 10:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: malat
We recently added the __printf attribute to prom_printf(), which means
GCC started warning about type/format mismatches. As part of that
commit we changed some "%lx" formats to "%llx" where the type is
actually unsigned long long.
Unfortunately prom_printf() doesn't know how to print "%llx", it just
prints a literal "lx", eg:
reserved memory map:
lx - lx
lx - lx
We should fix that at some point, but for now just cast the relevant
values to unsigned long to get things printing again. On 64-bit that
has no effect on the output, because both types are 64-bit wide. On
32-bit it means we're potentially not printing the high 32-bits of
some values, but most of them are pointers anyway, and we've lived
with that behaviour up until now.
Fixes: eae5f709a4d7 ("powerpc: Add __printf verification to prom_printf")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/prom_init.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 425992e393bc..662dd68c3fb8 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1580,7 +1580,7 @@ static void __init prom_instantiate_opal(void)
return;
}
- prom_printf("instantiating opal at 0x%llx...", base);
+ prom_printf("instantiating opal at 0x%lx...", (unsigned long)base);
if (call_prom_ret("call-method", 4, 3, rets,
ADDR("load-opal-runtime"),
@@ -1596,10 +1596,10 @@ static void __init prom_instantiate_opal(void)
reserve_mem(base, size);
- prom_debug("opal base = 0x%llx\n", base);
- prom_debug("opal align = 0x%llx\n", align);
- prom_debug("opal entry = 0x%llx\n", entry);
- prom_debug("opal size = 0x%llx\n", size);
+ prom_debug("opal base = 0x%lx\n", (unsigned long)base);
+ prom_debug("opal align = 0x%lx\n", (unsigned long)align);
+ prom_debug("opal entry = 0x%lx\n", (unsigned long)entry);
+ prom_debug("opal size = 0x%lx\n", (unsigned long)size);
prom_setprop(opal_node, "/ibm,opal", "opal-base-address",
&base, sizeof(base));
@@ -1734,7 +1734,7 @@ static void __init prom_instantiate_sml(void)
if (base == 0)
prom_panic("Could not allocate memory for sml\n");
- prom_printf("instantiating sml at 0x%llx...", base);
+ prom_printf("instantiating sml at 0x%lx...", (unsigned long)base);
memset((void *)base, 0, size);
@@ -1753,7 +1753,7 @@ static void __init prom_instantiate_sml(void)
prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
&size, sizeof(size));
- prom_debug("sml base = 0x%llx\n", base);
+ prom_debug("sml base = 0x%lx\n", (unsigned long)base);
prom_debug("sml size = 0x%x\n", size);
prom_debug("prom_instantiate_sml: end...\n");
@@ -1847,7 +1847,7 @@ static void __init prom_initialize_tce_table(void)
prom_debug("TCE table: %s\n", path);
prom_debug("\tnode = 0x%x\n", node);
- prom_debug("\tbase = 0x%llx\n", base);
+ prom_debug("\tbase = 0x%lx\n", (unsigned long)base);
prom_debug("\tsize = 0x%x\n", minsize);
/* Initialize the table to have a one-to-one mapping
@@ -2559,9 +2559,9 @@ static void __init flatten_device_tree(void)
int i;
prom_printf("reserved memory map:\n");
for (i = 0; i < mem_reserve_cnt; i++)
- prom_printf(" %llx - %llx\n",
- be64_to_cpu(mem_reserve_map[i].base),
- be64_to_cpu(mem_reserve_map[i].size));
+ prom_printf(" %lx - %lx\n",
+ (unsigned long)be64_to_cpu(mem_reserve_map[i].base),
+ (unsigned long)be64_to_cpu(mem_reserve_map[i].size));
}
#endif
/* Bump mem_reserve_cnt to cause further reservations to fail
--
2.14.1
^ permalink raw reply related
* Re: [PATCH v2] powerpc/64: Fix build failure with GCC 8.1
From: Christophe LEROY @ 2018-05-29 9:37 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Linux Kernel Mailing List, linuxppc-dev, Geoff Levand
In-Reply-To: <CAMuHMdXwO1_wrrTi40UmZC4o6KjvP0LF38m45n0ZNA2pDb_7Qg@mail.gmail.com>
Le 29/05/2018 à 11:05, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> On Tue, May 29, 2018 at 10:56 AM, Christophe LEROY
> <christophe.leroy@c-s.fr> wrote:
>> Le 29/05/2018 à 09:47, Geert Uytterhoeven a écrit :
>>> On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
>>> <christophe.leroy@c-s.fr> wrote:
>>>>
>>>> CC arch/powerpc/kernel/nvram_64.o
>>>> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
>>>> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound
>>>> 12 equals destination size [-Werror=stringop-truncation]
>>>> strncpy(new_part->header.name, name, 12);
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> CC arch/powerpc/kernel/trace/ftrace.o
>>>> In function 'make_field',
>>>> inlined from 'ps3_repository_read_boot_dat_address' at
>>>> arch/powerpc/platforms/ps3/repository.c:900:9:
>>>> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output
>>>> truncated before terminating nul copying 8 bytes from a string of the same
>>>> length [-Werror=stringop-truncation]
>>>> strncpy((char *)&n, text, 8);
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/arch/powerpc/kernel/nvram_64.c
>>>> +++ b/arch/powerpc/kernel/nvram_64.c
>>>> @@ -1039,7 +1039,7 @@ loff_t __init nvram_create_partition(const char
>>>> *name, int sig,
>>>> new_part->index = free_part->index;
>>>> new_part->header.signature = sig;
>>>> new_part->header.length = size;
>>>> - strncpy(new_part->header.name, name, 12);
>>>> + memcpy(new_part->header.name, name, strnlen(name,
>>>> sizeof(new_part->header.name)));
>>>
>>>
>>> The comment for nvram_header.lgnth says:
>>>
>>> /* Terminating null required only for names < 12 chars. */
>>>
>>> This will not terminate the string with a zero (the struct is
>>> allocated with kmalloc).
>>> So the original code is correct, the new one isn't.
>>
>> Right, then I have to first zeroize the destination.
>
> Using kzalloc() instead of kmalloc() will do.
>
> Still, papering around these warnings seems to obscure things, IMHO.
> And it increases code size, as you had to add a call to strnlen().
Right but then, what is the best solution to elimate that warning ?
Would it be better to enclose those two lines in:
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-truncation"
...
#pragma GCC diagnostic pop
Christophe
>
>>>> new_part->header.checksum = nvram_checksum(&new_part->header);
>>>>
>>>> rc = nvram_write_header(new_part);
>>>> diff --git a/arch/powerpc/platforms/ps3/repository.c
>>>> b/arch/powerpc/platforms/ps3/repository.c
>>>> index 50dbaf24b1ee..e49c887787c4 100644
>>>> --- a/arch/powerpc/platforms/ps3/repository.c
>>>> +++ b/arch/powerpc/platforms/ps3/repository.c
>>>> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64
>>>> index)
>>>>
>>>> static u64 make_field(const char *text, u64 index)
>>>> {
>>>> - u64 n;
>>>> + u64 n = 0;
>>>>
>>>> - strncpy((char *)&n, text, 8);
>>>> + memcpy((char *)&n, text, strnlen(text, sizeof(n)));
>>>
>>>
>>> This changes behavior: strncpy() fills the remainder of the buffer with
>>> zeroes. I don't remember the details of the PS3 repository structure,
>>> but given this writes to a fixed size u64 buffer, I'd expect the PS3
>>> hypervisor code to (1) rely on the zero padding, and (2) not need a zero
>>> terminator if there are 8 characters in the buffer, so probably the
>>> original code is correct, and the "fixed" code isn't.
>>
>> Here I have set n to 0 prior to the copy, so the buffer IS zero padded.
>
> Sorry, I missed that part.
>
> Gr{oetje,eeting}s,
>
> Geert
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/64: Fix build failure with GCC 8.1
From: Geert Uytterhoeven @ 2018-05-29 9:05 UTC (permalink / raw)
To: Christophe LEROY
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Linux Kernel Mailing List, linuxppc-dev, Geoff Levand
In-Reply-To: <1fa5b220-4d21-b4cd-33bf-8a3ce3178063@c-s.fr>
Hi Christophe,
On Tue, May 29, 2018 at 10:56 AM, Christophe LEROY
<christophe.leroy@c-s.fr> wrote:
> Le 29/05/2018 =C3=A0 09:47, Geert Uytterhoeven a =C3=A9crit :
>> On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
>> <christophe.leroy@c-s.fr> wrote:
>>>
>>> CC arch/powerpc/kernel/nvram_64.o
>>> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
>>> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound
>>> 12 equals destination size [-Werror=3Dstringop-truncation]
>>> strncpy(new_part->header.name, name, 12);
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> CC arch/powerpc/kernel/trace/ftrace.o
>>> In function 'make_field',
>>> inlined from 'ps3_repository_read_boot_dat_address' at
>>> arch/powerpc/platforms/ps3/repository.c:900:9:
>>> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output
>>> truncated before terminating nul copying 8 bytes from a string of the s=
ame
>>> length [-Werror=3Dstringop-truncation]
>>> strncpy((char *)&n, text, 8);
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>
>>
>> Thanks for your patch!
>>
>>> --- a/arch/powerpc/kernel/nvram_64.c
>>> +++ b/arch/powerpc/kernel/nvram_64.c
>>> @@ -1039,7 +1039,7 @@ loff_t __init nvram_create_partition(const char
>>> *name, int sig,
>>> new_part->index =3D free_part->index;
>>> new_part->header.signature =3D sig;
>>> new_part->header.length =3D size;
>>> - strncpy(new_part->header.name, name, 12);
>>> + memcpy(new_part->header.name, name, strnlen(name,
>>> sizeof(new_part->header.name)));
>>
>>
>> The comment for nvram_header.lgnth says:
>>
>> /* Terminating null required only for names < 12 chars. */
>>
>> This will not terminate the string with a zero (the struct is
>> allocated with kmalloc).
>> So the original code is correct, the new one isn't.
>
> Right, then I have to first zeroize the destination.
Using kzalloc() instead of kmalloc() will do.
Still, papering around these warnings seems to obscure things, IMHO.
And it increases code size, as you had to add a call to strnlen().
>>> new_part->header.checksum =3D nvram_checksum(&new_part->header=
);
>>>
>>> rc =3D nvram_write_header(new_part);
>>> diff --git a/arch/powerpc/platforms/ps3/repository.c
>>> b/arch/powerpc/platforms/ps3/repository.c
>>> index 50dbaf24b1ee..e49c887787c4 100644
>>> --- a/arch/powerpc/platforms/ps3/repository.c
>>> +++ b/arch/powerpc/platforms/ps3/repository.c
>>> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64
>>> index)
>>>
>>> static u64 make_field(const char *text, u64 index)
>>> {
>>> - u64 n;
>>> + u64 n =3D 0;
>>>
>>> - strncpy((char *)&n, text, 8);
>>> + memcpy((char *)&n, text, strnlen(text, sizeof(n)));
>>
>>
>> This changes behavior: strncpy() fills the remainder of the buffer with
>> zeroes. I don't remember the details of the PS3 repository structure,
>> but given this writes to a fixed size u64 buffer, I'd expect the PS3
>> hypervisor code to (1) rely on the zero padding, and (2) not need a zero
>> terminator if there are 8 characters in the buffer, so probably the
>> original code is correct, and the "fixed" code isn't.
>
> Here I have set n to 0 prior to the copy, so the buffer IS zero padded.
Sorry, I missed that part.
Gr{oetje,eeting}s,
Geert
--=20
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
.org
In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2] powerpc/64: Fix build failure with GCC 8.1
From: Christophe LEROY @ 2018-05-29 8:56 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Linux Kernel Mailing List, linuxppc-dev, Geoff Levand
In-Reply-To: <CAMuHMdVSKigQPUQH=BVc2fRSRmuEThLaf+V9ghMPDT9_m1C6BA@mail.gmail.com>
Le 29/05/2018 à 09:47, Geert Uytterhoeven a écrit :
> Hi Christophe,
>
> CC Geoff
>
> On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>> CC arch/powerpc/kernel/nvram_64.o
>> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
>> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound 12 equals destination size [-Werror=stringop-truncation]
>> strncpy(new_part->header.name, name, 12);
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> CC arch/powerpc/kernel/trace/ftrace.o
>> In function 'make_field',
>> inlined from 'ps3_repository_read_boot_dat_address' at arch/powerpc/platforms/ps3/repository.c:900:9:
>> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output truncated before terminating nul copying 8 bytes from a string of the same length [-Werror=stringop-truncation]
>> strncpy((char *)&n, text, 8);
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> Thanks for your patch!
>
>> --- a/arch/powerpc/kernel/nvram_64.c
>> +++ b/arch/powerpc/kernel/nvram_64.c
>> @@ -1039,7 +1039,7 @@ loff_t __init nvram_create_partition(const char *name, int sig,
>> new_part->index = free_part->index;
>> new_part->header.signature = sig;
>> new_part->header.length = size;
>> - strncpy(new_part->header.name, name, 12);
>> + memcpy(new_part->header.name, name, strnlen(name, sizeof(new_part->header.name)));
>
> The comment for nvram_header.lgnth says:
>
> /* Terminating null required only for names < 12 chars. */
>
> This will not terminate the string with a zero (the struct is
> allocated with kmalloc).
> So the original code is correct, the new one isn't.
Right, then I have to first zeroize the destination.
>
>> new_part->header.checksum = nvram_checksum(&new_part->header);
>>
>> rc = nvram_write_header(new_part);
>> diff --git a/arch/powerpc/platforms/ps3/repository.c b/arch/powerpc/platforms/ps3/repository.c
>> index 50dbaf24b1ee..e49c887787c4 100644
>> --- a/arch/powerpc/platforms/ps3/repository.c
>> +++ b/arch/powerpc/platforms/ps3/repository.c
>> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64 index)
>>
>> static u64 make_field(const char *text, u64 index)
>> {
>> - u64 n;
>> + u64 n = 0;
>>
>> - strncpy((char *)&n, text, 8);
>> + memcpy((char *)&n, text, strnlen(text, sizeof(n)));
>
> This changes behavior: strncpy() fills the remainder of the buffer with
> zeroes. I don't remember the details of the PS3 repository structure,
> but given this writes to a fixed size u64 buffer, I'd expect the PS3
> hypervisor code to (1) rely on the zero padding, and (2) not need a zero
> terminator if there are 8 characters in the buffer, so probably the
> original code is correct, and the "fixed" code isn't.
Here I have set n to 0 prior to the copy, so the buffer IS zero padded.
Christophe
>
> Has this been tested on a PS3?
>
>> return n + index;
>> }
>
> Gr{oetje,eeting}s,
>
> Geert
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/64: Fix build failure with GCC 8.1
From: Geert Uytterhoeven @ 2018-05-29 7:47 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Linux Kernel Mailing List, linuxppc-dev, Geoff Levand
In-Reply-To: <f5f2de3e1a39613f7303bfdc0d2f2210d4c91910.1527573345.git.christophe.leroy@c-s.fr>
Hi Christophe,
CC Geoff
On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
> CC arch/powerpc/kernel/nvram_64.o
> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound 12 equals destination size [-Werror=stringop-truncation]
> strncpy(new_part->header.name, name, 12);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> CC arch/powerpc/kernel/trace/ftrace.o
> In function 'make_field',
> inlined from 'ps3_repository_read_boot_dat_address' at arch/powerpc/platforms/ps3/repository.c:900:9:
> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output truncated before terminating nul copying 8 bytes from a string of the same length [-Werror=stringop-truncation]
> strncpy((char *)&n, text, 8);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Thanks for your patch!
> --- a/arch/powerpc/kernel/nvram_64.c
> +++ b/arch/powerpc/kernel/nvram_64.c
> @@ -1039,7 +1039,7 @@ loff_t __init nvram_create_partition(const char *name, int sig,
> new_part->index = free_part->index;
> new_part->header.signature = sig;
> new_part->header.length = size;
> - strncpy(new_part->header.name, name, 12);
> + memcpy(new_part->header.name, name, strnlen(name, sizeof(new_part->header.name)));
The comment for nvram_header.lgnth says:
/* Terminating null required only for names < 12 chars. */
This will not terminate the string with a zero (the struct is
allocated with kmalloc).
So the original code is correct, the new one isn't.
> new_part->header.checksum = nvram_checksum(&new_part->header);
>
> rc = nvram_write_header(new_part);
> diff --git a/arch/powerpc/platforms/ps3/repository.c b/arch/powerpc/platforms/ps3/repository.c
> index 50dbaf24b1ee..e49c887787c4 100644
> --- a/arch/powerpc/platforms/ps3/repository.c
> +++ b/arch/powerpc/platforms/ps3/repository.c
> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64 index)
>
> static u64 make_field(const char *text, u64 index)
> {
> - u64 n;
> + u64 n = 0;
>
> - strncpy((char *)&n, text, 8);
> + memcpy((char *)&n, text, strnlen(text, sizeof(n)));
This changes behavior: strncpy() fills the remainder of the buffer with
zeroes. I don't remember the details of the PS3 repository structure,
but given this writes to a fixed size u64 buffer, I'd expect the PS3
hypervisor code to (1) rely on the zero padding, and (2) not need a zero
terminator if there are 8 characters in the buffer, so probably the
original code is correct, and the "fixed" code isn't.
Has this been tested on a PS3?
> return n + index;
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox