| 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
| ||
|
Message-ID: <1446604653.17120.177.camel@tzanussi-mobl.amr.corp.intel.com>
Date: Tue, 03 Nov 2015 20:37:33 -0600
From: Tom Zanussi <tom.zanussi@...ux.intel.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: rostedt@...dmis.org, daniel.wagner@...-carit.de,
masami.hiramatsu.pt@...achi.com, josh@...htriplett.org,
andi@...stfloor.org, mathieu.desnoyers@...icios.com,
peterz@...radead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 17/28] tracing: Add hist trigger 'execname' modifier
On Mon, 2015-11-02 at 23:10 +0900, Namhyung Kim wrote:
> On Thu, Oct 22, 2015 at 01:14:21PM -0500, Tom Zanussi wrote:
> > Allow users to have pid fields displayed as program names in the output
> > by appending '.execname' to field names:
> >
> > # echo hist:keys=aaa.execname ... \
> > [ if filter] > event/trigger
>
> So in this case, 'aaa' should be 'common_pid', right?
>
Right.
> Also, it's just for display thus tasks which have different pid but
> same comm name will be saved in different entries, right? If so, it'd
> be better to note that somewhere as it might confuse users..
>
Right, good point, will document that.
>
> >
> > Signed-off-by: Tom Zanussi <tom.zanussi@...ux.intel.com>
> > Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> > ---
> > kernel/trace/trace.c | 3 +-
> > kernel/trace/trace_events_hist.c | 86 +++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 87 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 4f0968e..a143be0 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -3822,7 +3822,8 @@ static const char readme_msg[] =
> > "\t following modifiers to the field name, as applicable:\n\n"
> > "\t .hex display a number as a hex value\n"
> > "\t .sym display an address as a symbol\n"
> > - "\t .sym-offset display an address as a symbol and offset\n\n"
> > + "\t .sym-offset display an address as a symbol and offset\n"
> > + "\t .execname display a common_pid as a program name\n\n"
> > "\t The 'pause' parameter can be used to pause an existing hist\n"
> > "\t trigger or to start a hist trigger but not log any events\n"
> > "\t until told to do so. 'continue' can be used to start or\n"
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index e6e5c6e..656973a 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -75,6 +75,7 @@ enum hist_field_flags {
> > HIST_FIELD_HEX = 8,
> > HIST_FIELD_SYM = 16,
> > HIST_FIELD_SYM_OFFSET = 32,
> > + HIST_FIELD_EXECNAME = 64,
> > };
> >
> > struct hist_trigger_attrs {
> > @@ -218,6 +219,78 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
> > return ERR_PTR(ret);
> > }
> >
> > +static inline void save_comm(char *comm, struct task_struct *task)
> > +{
> > + if (!task->pid) {
> > + strcpy(comm, "<idle>");
> > + return;
> > + }
> > +
> > + if (WARN_ON_ONCE(task->pid < 0)) {
> > + strcpy(comm, "<XXX>");
> > + return;
> > + }
> > +
> > + if (task->pid > PID_MAX_DEFAULT) {
> > + strcpy(comm, "<...>");
> > + return;
> > + }
>
> I guess this comes from the saved_cmdlines code which uses a fixed
> array of PID_MAX_DEFAULT size. But as it doesn't use the array, we
> can simply copy the task->comm IMHO.
>
>
> > +
> > + memcpy(comm, task->comm, TASK_COMM_LEN);
> > +}
> > +
> > +static void hist_trigger_elt_free(struct tracing_map_elt *elt)
> > +{
> > + kfree((char *)elt->private_data);
> > +}
> > +
> > +static int hist_trigger_elt_alloc(struct tracing_map_elt *elt)
> > +{
> > + struct hist_trigger_data *hist_data = elt->map->private_data;
> > + struct hist_field *key_field;
> > + unsigned int i;
> > +
> > + for (i = hist_data->n_vals; i < hist_data->n_fields; i++) {
>
> I think it'd be better to add a list iterator macro like
> for_each_key_field() or something since it's easy to miss that the key
> fields always follows the value fields IMHO.
>
Yeah, that's a good idea (sometimes I even forget that myself ;-)
>
> > + key_field = hist_data->fields[i];
> > +
> > + if (key_field->flags & HIST_FIELD_EXECNAME) {
> > + unsigned int size = TASK_COMM_LEN + 1;
> > +
> > + elt->private_data = kzalloc(size, GFP_KERNEL);
> > + if (!elt->private_data)
> > + return -ENOMEM;
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void hist_trigger_elt_copy(struct tracing_map_elt *to,
> > + struct tracing_map_elt *from)
> > +{
> > + char *comm_from = from->private_data;
> > + char *comm_to = to->private_data;
> > +
> > + if (comm_from)
> > + memcpy(comm_to, comm_from, TASK_COMM_LEN + 1);
> > +}
> > +
> > +static void hist_trigger_elt_init(struct tracing_map_elt *elt)
> > +{
> > + char *comm = elt->private_data;
> > +
> > + if (comm)
> > + save_comm(comm, current);
> > +}
> > +
> > +static struct tracing_map_ops hist_trigger_ops = {
> > + .elt_alloc = hist_trigger_elt_alloc,
> > + .elt_copy = hist_trigger_elt_copy,
> > + .elt_free = hist_trigger_elt_free,
> > + .elt_init = hist_trigger_elt_init,
> > +};
>
> How about making it const?
>
Sure.
>
> > +
> > static void destroy_hist_field(struct hist_field *hist_field)
> > {
> > kfree(hist_field);
> > @@ -376,6 +449,9 @@ static int create_key_field(struct hist_trigger_data *hist_data,
> > flags |= HIST_FIELD_SYM;
> > else if (!strcmp(field_str, "sym-offset"))
> > flags |= HIST_FIELD_SYM_OFFSET;
> > + else if (!strcmp(field_str, "execname") &&
> > + !strcmp(field_name, "common_pid"))
> > + flags |= HIST_FIELD_EXECNAME;
> > else {
> > ret = -EINVAL;
> > goto out;
> > @@ -612,7 +688,7 @@ create_hist_data(unsigned int map_bits,
> > goto free;
> >
> > hist_data->map = tracing_map_create(map_bits, hist_data->key_size,
> > - NULL, hist_data);
> > + &hist_trigger_ops, hist_data);
>
> Why not using the hist_trigger_ops only if HIST_FIELD_EXECNAME is set
> to one of the key fields actually?
>
Right, they're only currently needed in that case, so should only be set
in that case, will change that.
Thanks,
Tom
> Thanks,
> Namhyung
>
>
> > if (IS_ERR(hist_data->map)) {
> > ret = PTR_ERR(hist_data->map);
> > hist_data->map = NULL;
> > @@ -732,6 +808,12 @@ hist_trigger_entry_print(struct seq_file *m,
> > sprint_symbol(str, uval);
> > seq_printf(m, "%s: [%llx] %-55s",
> > key_field->field->name, uval, str);
> > + } else if (key_field->flags & HIST_FIELD_EXECNAME) {
> > + char *comm = elt->private_data;
> > +
> > + uval = *(u64 *)(key + key_field->offset);
> > + seq_printf(m, "%s: %-16s[%10llu]",
> > + key_field->field->name, comm, uval);
> > } else if (key_field->flags & HIST_FIELD_STRING) {
> > seq_printf(m, "%s: %-50s", key_field->field->name,
> > (char *)(key + key_field->offset));
> > @@ -851,6 +933,8 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
> > flags_str = "sym";
> > else if (hist_field->flags & HIST_FIELD_SYM_OFFSET)
> > flags_str = "sym-offset";
> > + else if (hist_field->flags & HIST_FIELD_EXECNAME)
> > + flags_str = "execname";
> >
> > return flags_str;
> > }
> > --
> > 1.9.3
> >
--
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/
Powered by blists - more mailing lists