Re: [PATCH] tracing: expose current->comm to kprobe events

[Omar Sandoval <osandov@osandov.com>]

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 <osandov@osandov.com> wrote:
> > From: Omar Sandoval <osandov@fb.com>
> >
> > 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 <osandov@fb.com>
> > ---
> > 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