linux-kernel - Re: [PATCH 11/12] hw-breakpoints: ftrace plugin for kernel symbol tracing using HW Breakpoint interfaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090605113657.GA6004@nowhere>
Date:	Fri, 5 Jun 2009 13:37:00 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
Cc:	David Daney <ddaney@...iumnetworks.com>,
	Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH 11/12] hw-breakpoints: ftrace plugin for kernel symbol
	tracing using HW Breakpoint interfaces

On Thu, Jun 04, 2009 at 09:06:58PM +0530, K.Prasad wrote:
> On Tue, Jun 02, 2009 at 04:12:08PM -0700, David Daney wrote:
> > Frederic Weisbecker wrote:
> >> From: K.Prasad <prasad@...ux.vnet.ibm.com>
> >>
> >> This patch adds an ftrace plugin to detect and profile memory access over kernel
> >> variables. It uses HW Breakpoint interfaces to 'watch memory addresses.
> >>
> >> Signed-off-by: K.Prasad <prasad@...ux.vnet.ibm.com>
> >> Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
> >> ---
> >>  kernel/trace/Kconfig          |   21 ++
> >>  kernel/trace/Makefile         |    1 +
> >>  kernel/trace/trace.h          |   23 ++
> >>  kernel/trace/trace_ksym.c     |  525 +++++++++++++++++++++++++++++++++++++++++
> >>  kernel/trace/trace_selftest.c |   53 ++++
> >>  5 files changed, 623 insertions(+), 0 deletions(-)
> >>  create mode 100644 kernel/trace/trace_ksym.c
> > [...]
> >> +	entry->ksym_hbp->info.name = ksymname;
> >> +	entry->ksym_hbp->info.type = op;
> >> +	entry->ksym_addr = entry->ksym_hbp->info.address = addr;
> >> +#ifdef CONFIG_X86
> >> +	entry->ksym_hbp->info.len = HW_BREAKPOINT_LEN_4;
> >> +#endif
> >
> > What if the symbol referred to an object of size other than 4?  This  
> > would clearly be incorrect in that case.
> >
> 
> I don't see a way in which we could automatically detect the size of a
> variable using the symbol name and use that as the 'len' (as this would
> be the ideal case). However, in this case we can circumvent this problem by
> using the least of the possible lengths (HW_BREAKPOINT_LEN_1 for x86)
> as it would be valid for symbols of all sizes. I will change this.
> Thanks for pointing it out!


Indeed, that looks a good solution.


 
> >> +	entry->ksym_hbp->triggered = (void *)ksym_hbp_handler;
> >> +
> >> +	ret = register_kernel_hw_breakpoint(entry->ksym_hbp);
> >
> > I hate to sound like a broken record, but could some one explain to me  
> > again why it is a good idea to design a new API that requires processor  
> > specific #ifdefs to be sprinkled all around generic kernel code?
> >
> > Back in:
> > http://lkml.org/lkml/2008/12/4/329
> > and
> > http://lkml.org/lkml/2009/5/21/189
> >
> > I raised doubts about this hw-breakpoint thing being generic and the  
> > responses made think that the processor specific portions would be  
> > isolated in the processor specific parts of the kernel.  I now see that  
> > I was wrong.
> >
> > When we add sparc, MIPS, ppc...  Support it would be nice to not have to  
> > add all our own #ifdefs to this, but instead have a generic interface  
> > that will not need changes.
> >
> > David Daney
> 
> One of the ways in which we could do this for 'len' field is to allow
> numeric lengths to be specified (as suggested by David Gibson in another
> mail thread), say "entry->ksym_hbp->info.len = 4;".
> 
> The length encoding based on register layout can be done in
> arch-specific code. I think this would make the code more usable and I
> should be implementing it in a patch soon.



Yeah, but are you sure every arch support length encoding based breakpoints?
May be such information should be passed in register_breakpoint instead and
let the arch answer whether it supports or not this operation...



> 
> As far as the 'type' information is concerned, some of the common
> breakpoint types (such as HW_BREAKPOINT_READ, HW_BREAKPOINT_WRITE,
> HW_BREAKPOINT_EXECUTE) can be defined in generic files and as NULL
> for those architectures that don't implement them.
> 
> However it might be a source of confusion to the end-user about
> supported breakpoint types on a given architecture.


Indeed but I think it's still worth. The Api documentation could
be thought with a section on which we can find the operations
that are supported by every archs.


> 
> Thanks,
> K.Prasad
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/