Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757738Ab3DPNw1 (ORCPT ); Tue, 16 Apr 2013 09:52:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35517 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755927Ab3DPNwY (ORCPT ); Tue, 16 Apr 2013 09:52:24 -0400 Date: Tue, 16 Apr 2013 15:49:13 +0200 From: Oleg Nesterov To: Michael Neuling Cc: Andrew Morton , Benjamin Herrenschmidt , Frederic Weisbecker , Jan Kratochvil , Ingo Molnar , Paul Mackerras , Paul Mundt , Prasad , Russell King , Will Deacon , linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/5] kill ptrace_{get,put}_breakpoints() Message-ID: <20130416134913.GC9189@redhat.com> References: <20130414160501.GA7612@redhat.com> <3883.1366096958@ale.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3883.1366096958@ale.ozlabs.ibm.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3989 Lines: 113 On 04/16, Michael Neuling wrote: > > > Benjamin, Paul, arch_dup_task_struct()->flush_ptrace_hw_breakpoint(src) > > on powerpc looks "obviously wrong". Don't we need > > > > - flush_ptrace_hw_breakpoint(src); > > + dst->thread->ptrace_bps[0] = NULL; > > Do you mean the following? > > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index 59dd545..559804e 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -911,7 +911,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct tas > flush_vsx_to_thread(src); > flush_spe_to_thread(src); > #ifdef CONFIG_HAVE_HW_BREAKPOINT > - flush_ptrace_hw_breakpoint(src); > + dst->thread.ptrace_bps[0] = NULL; > #endif /* CONFIG_HAVE_HW_BREAKPOINT */ Almost. This is what I think we should do, but it is pointless to do this in arch_dup_task_struct(), setup_thread_stack() will copy ptrace_bps[] from parent later. > Unable to handle kernel paging request for data at address 0x00100108 > Faulting instruction address: 0xc00000000014d5e4 > cpu 0x0: Vector: 300 (Data Access) at [c00000007e5836a0] > pc: c00000000014d5e4: .toggle_bp_slot+0x74/0x1c0 > lr: c00000000014dc14: .release_bp_slot+0x44/0x70 > sp: c00000007e583920 > msr: 9000000000009032 > dar: 100108 > dsisr: 42000000 > current = 0xc00000007e560000 > paca = 0xc00000000fe00000 softe: 0 irq_happened: 0x08 > pid = 1, comm = init > enter ? for help > [c00000007e5839d0] c00000000014dc14 .release_bp_slot+0x44/0x70 > [c00000007e583a50] c000000000144bbc .free_event+0x6c/0x1e0 > [c00000007e583ad0] c000000000144dc4 .perf_event_release_kernel+0x94/0x110 > [c00000007e583b60] c00000000014cf08 .unregister_hw_breakpoint+0x18/0x30 > [c00000007e583bd0] c00000000000e5f8 .ptrace_set_debugreg+0x158/0x230 > [c00000007e583cd0] c00000000000eb4c .arch_ptrace+0x43c/0x7b0 > [c00000007e583d90] c00000000008cbf8 .SyS_ptrace+0x98/0x170 > [c00000007e583e30] c000000000009d54 syscall_exit+0x0/0x98 > --- Exception: c01 (System Call) at 000000001001d1d4 > SP (3fffdf7459f0) is in userspace > > The crash seems to happen some time after the fork. Might be when the > new processes exits or get another ptrace call on it (I'm not sure which > one sorry). Yes, probably because both parent and child have the same ->ptrace_bps[] pointers. > Without your suggestion it doesn't crash this case (ie. mainline passes). This is clear. flush_ptrace_hw_breakpoint() nullifies ->ptrace_bps[], so setup_thread_stack() copies NULL. But, unless I missed something, this is wrong. Why should the parent lose its bps after fork? IOW, I think we need something like the patch below, but I do not have a powerpc machine for the testing. > Acked-by: Michael Neuling Thanks! Oleg. [PATCH] ptrace/powerpc: dont flush_ptrace_hw_breakpoint() on fork() arch_dup_task_struct() does flush_ptrace_hw_breakpoint(src), this is not what we want. We should clear child->thread.ptrace_bps[] copied by dup_task_struct(). --- x/arch/powerpc/kernel/process.c +++ x/arch/powerpc/kernel/process.c @@ -910,10 +910,6 @@ int arch_dup_task_struct(struct task_str flush_altivec_to_thread(src); flush_vsx_to_thread(src); flush_spe_to_thread(src); -#ifdef CONFIG_HAVE_HW_BREAKPOINT - flush_ptrace_hw_breakpoint(src); -#endif /* CONFIG_HAVE_HW_BREAKPOINT */ - *dst = *src; return 0; } @@ -984,6 +980,10 @@ int copy_thread(unsigned long clone_flag p->thread.ksp_limit = (unsigned long)task_stack_page(p) + _ALIGN_UP(sizeof(struct thread_info), 16); +#ifdef CONFIG_HAVE_HW_BREAKPOINT + p->thread.ptrace_bps[0] = NULL; +#endif + #ifdef CONFIG_PPC_STD_MMU_64 if (mmu_has_feature(MMU_FTR_SLB)) { unsigned long sp_vsid; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/