From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753859AbcETFQj (ORCPT ); Fri, 20 May 2016 01:16:39 -0400 Received: from mail-qg0-f41.google.com ([209.85.192.41]:33245 "EHLO mail-qg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750851AbcETFQh (ORCPT ); Fri, 20 May 2016 01:16:37 -0400 Subject: Re: [PATCH v12 05/10] arm64: Kprobes with single stepping support To: James Morse , Sandeepa Prabhu References: <1461783185-9056-1-git-send-email-dave.long@linaro.org> <1461783185-9056-6-git-send-email-dave.long@linaro.org> <57349AE2.3060507@arm.com> Cc: Catalin Marinas , Will Deacon , William Cohen , Pratyush Anand , Steve Capper , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Marc Zyngier , Dave P Martin , Mark Rutland , Robin Murphy , Ard Biesheuvel , Jens Wiklander , Christoffer Dall , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Yang Shi , Greg Kroah-Hartman , Viresh Kumar , "Suzuki K. Poulose" , Kees Cook , Zi Shen Lim , John Blackwood , Feng Kan , Balamurugan Shanmugam , Vladimir Murzin , Mark Salyzyn , Petr Mladek , Andrew Morton , Mark Brown From: David Long Message-ID: <573E9DB1.4070109@linaro.org> Date: Fri, 20 May 2016 01:16:33 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <57349AE2.3060507@arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/12/2016 11:01 AM, James Morse wrote: > Hi David, Sandeepa, > > On 27/04/16 19:53, David Long wrote: >> From: Sandeepa Prabhu > >> diff --git a/arch/arm64/kernel/kprobes.c b/arch/arm64/kernel/kprobes.c >> new file mode 100644 >> index 0000000..dfa1b1f >> --- /dev/null >> +++ b/arch/arm64/kernel/kprobes.c >> @@ -0,0 +1,520 @@ >> +/* >> + * arch/arm64/kernel/kprobes.c >> + * >> + * Kprobes support for ARM64 >> + * >> + * Copyright (C) 2013 Linaro Limited. >> + * Author: Sandeepa Prabhu >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + * >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "kprobes-arm64.h" >> + >> +#define MIN_STACK_SIZE(addr) min((unsigned long)MAX_STACK_SIZE, \ >> + (unsigned long)current_thread_info() + THREAD_START_SP - (addr)) > > What if we probe something called on the irq stack? > This needs the on_irq_stack() checks too, the start/end can be found from the > per-cpu irq_stack value. > > [ ... ] > OK. >> +int __kprobes setjmp_pre_handler(struct kprobe *p, struct pt_regs *regs) >> +{ >> + struct jprobe *jp = container_of(p, struct jprobe, kp); >> + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); >> + long stack_ptr = kernel_stack_pointer(regs); >> + >> + kcb->jprobe_saved_regs = *regs; >> + memcpy(kcb->jprobes_stack, (void *)stack_ptr, >> + MIN_STACK_SIZE(stack_ptr)); > > I wonder if we need this stack save/restore? > > The comment next to the equivalent code for x86 says: >> gcc assumes that the callee owns the argument space and could overwrite it, >> e.g. tailcall optimization. So, to be absolutely safe we also save and >> restore enough stack bytes to cover the argument area. > > On arm64 the first eight arguments are passed in registers, so we might not need > this stack copy. (sparc and powerpc work like this too, their versions of this > function don't copy chunks of the stack). > > ... then I went looking for functions with >8 arguments... > > Looking at the arm64 defconfig dwarf debug data, there are 71 of these that > don't get inlined, picking at random: >> rockchip_clk_register_pll() has 13 >> fib_dump_info() has 11 >> vma_merge() has 10 >> vring_create_virtqueue() has 10 > etc... > > So we do need this stack copying, so that we can probe these function without > risking the arguments being modified. > > It may be worth including a comment to the effect that this stack save/restore > is needed for functions that pass >8 arguments where the pre-handler may change > these values on the stack. > > I can add a comment. >> + preempt_enable_no_resched(); >> + return 1; >> +} >> + > > > Thanks, > > James > Thanks, -dl