From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161544AbcFBSVS (ORCPT ); Thu, 2 Jun 2016 14:21:18 -0400 Received: from mail-pa0-f54.google.com ([209.85.220.54]:35194 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932392AbcFBSVR (ORCPT ); Thu, 2 Jun 2016 14:21:17 -0400 Date: Thu, 2 Jun 2016 11:21:14 -0700 From: Omar Sandoval To: Namhyung Kim Cc: Steven Rostedt , Ingo Molnar , Masami Hiramatsu , LKML , kernel-team@fb.com, Omar Sandoval Subject: Re: [PATCH] tracing: expose current->comm to kprobe events Message-ID: <20160602182114.GA19967@vader.dhcp.thefacebook.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Fri, Jun 03, 2016 at 12:40:02AM +0900, Namhyung Kim wrote: > Hello, > > On Wed, Jun 1, 2016 at 3:17 PM, Omar Sandoval wrote: > > From: Omar Sandoval > > > > ftrace is very quick to give up on saving the task command line (see > > `trace_save_cmdline()`). The workaround for events which really care > > about the command line is to explicitly assign it as part of the entry. > > However, this doesn't work for kprobe events, as there's no > > straightforward way to get access to current->comm. Add a kprobe event > > variable $comm which provides exactly that. > > > > Signed-off-by: Omar Sandoval > > --- > > This is a problem I ran into while trying to debug an unrelated issue. I > > was considering trying to make ftrace try harder to save the command > > line, but that would most likely be at the expense at most events which > > don't care about the task comm. This is much less work. > > > > Documentation/trace/kprobetrace.txt | 2 ++ > > kernel/trace/trace_kprobe.c | 13 +++++++++++++ > > kernel/trace/trace_probe.c | 5 +++++ > > kernel/trace/trace_probe.h | 2 ++ > > kernel/trace/trace_uprobe.c | 7 +++++++ > > 5 files changed, 29 insertions(+) > > > > diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt > > index d68ea5fc812b..90914cbc1e51 100644 > > --- a/Documentation/trace/kprobetrace.txt > > +++ b/Documentation/trace/kprobetrace.txt > > @@ -40,6 +40,7 @@ Synopsis of kprobe_events > > $stackN : Fetch Nth entry of stack (N >= 0) > > $stack : Fetch stack address. > > $retval : Fetch return value.(*) > > + $comm : Fetch current task comm.(***) > > +|-offs(FETCHARG) : Fetch memory at FETCHARG +|- offs address.(**) > > NAME=FETCHARG : Set NAME as the argument name of FETCHARG. > > FETCHARG:TYPE : Set TYPE as the type of FETCHARG. Currently, basic types > > @@ -48,6 +49,7 @@ Synopsis of kprobe_events > > > > (*) only for return probe. > > (**) this is useful for fetching a field of data structures. > > + (***) you probably want this as a string, i.e., $comm:string > > > > Types > > ----- > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 5546eec0505f..e3a087552d49 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -214,6 +214,18 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > > } > > NOKPROBE_SYMBOL(FETCH_FUNC_NAME(memory, string_size)); > > > > +#define DEFINE_FETCH_comm(type) \ > > +void FETCH_FUNC_NAME(comm, type)(struct pt_regs *regs, \ > > + void *data, void *dest) \ > > +{ \ > > + fetch_memory_##type(regs, current->comm, dest); \ > > +} \ > > +NOKPROBE_SYMBOL(FETCH_FUNC_NAME(comm, type)); > > + > > +DEFINE_BASIC_FETCH_FUNCS(comm) > > +DEFINE_FETCH_comm(string) > > +DEFINE_FETCH_comm(string_size) > > I think you can define default type fetch functions (such as u8, ...) > to NULL to make sure it's used as string type only. Yeah, my original thinking was to not make $comm a special case, but it really is kind of useless to get it as anything other than a string. I'll also try to make the type default to string just for $comm. > > + > > #define DEFINE_FETCH_symbol(type) \ > > void FETCH_FUNC_NAME(symbol, type)(struct pt_regs *regs, void *data, void *dest)\ > > { \ > > @@ -587,6 +599,7 @@ static int create_trace_kprobe(int argc, char **argv) > > * $retval : fetch return value > > * $stack : fetch stack address > > * $stackN : fetch Nth of stack (N:0-) > > + * $comm : fetch current task comm > > * @ADDR : fetch memory at ADDR (ADDR should be in kernel) > > * @SYM[+|-offs] : fetch memory at SYM +|- offs (SYM is a data symbol) > > * %REG : fetch register REG > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > > index 1d372fa6fefb..918775fcacd3 100644 > > --- a/kernel/trace/trace_probe.c > > +++ b/kernel/trace/trace_probe.c > > @@ -348,6 +348,11 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t, > > } > > } else > > ret = -EINVAL; > > + } else if (strcmp(arg, "comm") == 0) { > > + if (is_kprobe) > > + f->fn = t->fetch[FETCH_MTD_comm]; > > + else > > + ret = -EINVAL; > > I think it should work with uprobes too. Why not enabling it to uprobes? You're right, it should work for uprobes, too, I'll add that. Thanks for taking a look! > Thanks, > Namhyung > > > > } else > > ret = -EINVAL; > > > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > > index f6398db09114..3f1a987e6dc1 100644 > > --- a/kernel/trace/trace_probe.h > > +++ b/kernel/trace/trace_probe.h > > @@ -102,6 +102,7 @@ enum { > > FETCH_MTD_reg = 0, > > FETCH_MTD_stack, > > FETCH_MTD_retval, > > + FETCH_MTD_comm, > > FETCH_MTD_memory, > > FETCH_MTD_symbol, > > FETCH_MTD_deref, > > @@ -213,6 +214,7 @@ DEFINE_FETCH_##method(u64) > > ASSIGN_FETCH_FUNC(reg, ftype), \ > > ASSIGN_FETCH_FUNC(stack, ftype), \ > > ASSIGN_FETCH_FUNC(retval, ftype), \ > > +ASSIGN_FETCH_FUNC(comm, ftype), \ > > ASSIGN_FETCH_FUNC(memory, ftype), \ > > ASSIGN_FETCH_FUNC(symbol, ftype), \ > > ASSIGN_FETCH_FUNC(deref, ftype), \ > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > > index c53485441c88..f7eedb191d1f 100644 > > --- a/kernel/trace/trace_uprobe.c > > +++ b/kernel/trace/trace_uprobe.c > > @@ -172,6 +172,13 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > > *(u32 *)dest = len; > > } > > > > +#define fetch_comm_u8 NULL > > +#define fetch_comm_u16 NULL > > +#define fetch_comm_u32 NULL > > +#define fetch_comm_u64 NULL > > +#define fetch_comm_string NULL > > +#define fetch_comm_string_size NULL > > + > > static unsigned long translate_user_vaddr(void *file_offset) > > { > > unsigned long base_addr; > > -- > > 2.8.3 > > -- Omar