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

[Namhyung Kim]

Hello,

On Wed, Jun 1, 2016 at 3:17 PM, Omar Sandoval <osandov@xxxxxxxxxxx> wrote:
> From: Omar Sandoval <osandov@xxxxxx>
>
> 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@xxxxxx>
> ---
> 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.


> +
>  #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?

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
>