Message-ID: <20160722101617.GA17821@e104818-lin.cambridge.arm.com>
Date:	Fri, 22 Jul 2016 11:16:17 +0100
From:	Catalin Marinas <catalin.marinas@....com>
To:	David Long <dave.long@...aro.org>
Cc:	Marc Zyngier <marc.zyngier@....com>,
	Huang Shijie <shijie.huang@....com>,
	James Morse <james.morse@....com>,
	Pratyush Anand <panand@...hat.com>,
	Sandeepa Prabhu <sandeepa.s.prabhu@...il.com>,
	Will Deacon <will.deacon@....com>,
	William Cohen <wcohen@...hat.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Steve Capper <steve.capper@...aro.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Li Bin <huawei.libin@...wei.com>,
	Jisheng Zhang <jszhang@...vell.com>,
	Mark Rutland <mark.rutland@....com>,
	Daniel Thompson <daniel.thompson@...aro.org>,
	Vladimir Murzin <Vladimir.Murzin@....com>,
	Petr Mladek <pmladek@...e.com>,
	Ard Biesheuvel <ard.biesheuvel@...aro.org>,
	Jens Wiklander <jens.wiklander@...aro.org>,
	Robin Murphy <robin.murphy@....com>,
	Mark Brown <broonie@...nel.org>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Dave P Martin <Dave.Martin@....com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	yalin wang <yalin.wang2010@...il.com>,
	Yang Shi <yang.shi@...aro.org>,
	Zi Shen Lim <zlim.lnx@...il.com>,
	John Blackwood <john.blackwood@...r.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Alex Bennée <alex.bennee@...aro.org>,
	Adam Buchbinder <adam.buchbinder@...il.com>,
	Christoffer Dall <christoffer.dall@...aro.org>
Subject: Re: [PATCH v15 04/10] arm64: Kprobes with single stepping support

On Thu, Jul 21, 2016 at 02:33:52PM -0400, David Long wrote:
> On 07/21/2016 01:23 PM, Marc Zyngier wrote:
> >On 21/07/16 17:33, David Long wrote:
> >>On 07/20/2016 12:09 PM, Marc Zyngier wrote:
> >>>On 08/07/16 17:35, David Long wrote:
> >>>>+#define MAX_INSN_SIZE			1
> >>>>+#define MAX_STACK_SIZE			128
> >>>
> >>>Where is that value coming from? Because even on my 6502, I have a 256
> >>>byte stack.
> >>>
> >>
> >>Although I don't claim to know the original author's thoughts I would
> >>guess it is based on the seven other existing implementations for
> >>kprobes on various architectures, all of which appear to use either 64
> >>or 128 for MAX_STACK_SIZE.  The code is not trying to duplicate the
> >>whole stack.
[...]
> >My main worry is that whatever value you pick, it is always going to be
> >wrong. This is used to preserve arguments that are passed on the stack,
> >as opposed to passed by registers). We have no idea of what is getting
> >passed there so saving nothing, 128 bytes or 2kB is about the same. It
> >is always wrong.
> >
> >A much better solution would be to check the frame pointer, and copy the
> >delta between FP and SP, assuming it fits inside the allocated buffer.
> >If it doesn't, or if FP is invalid, we just skip the hook, because we
> >can't reliably execute it.
> 
> Well, this is the way it works literally everywhere else. It is a documented
> limitation (Documentation/kprobes.txt). Said documentation may need to be
> changed along with the suggested fix.

The document states: "Up to MAX_STACK_SIZE bytes are copied". That means
the arch code could always copy less but never more than MAX_STACK_SIZE.
What we are proposing is that we should try to guess how much to copy
based on the FP value (caller's frame) and, if larger than
MAX_STACK_SIZE, skip the probe hook entirely. I don't think this goes
against the kprobes.txt document but at least it (a) may improve the
performance slightly by avoiding unnecessary copy and (b) it avoids
undefined behaviour if we ever encounter a jprobe with arguments passed
on the stack beyond MAX_STACK_SIZE.

-- 
Catalin

Powered by blists - more mailing lists