| 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
| ||
|
Date: Fri, 19 Feb 2016 16:23:18 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Tom Zanussi <tom.zanussi@...ux.intel.com>
Cc: masami.hiramatsu.pt@...achi.com, namhyung@...nel.org,
josh@...htriplett.org, andi@...stfloor.org,
mathieu.desnoyers@...icios.com, peterz@...radead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v13 09/29] tracing: Add 'hist' event trigger command
On Thu, 10 Dec 2015 12:50:51 -0600
Tom Zanussi <tom.zanussi@...ux.intel.com> wrote:
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> new file mode 100644
> index 0000000..213aa79
> --- /dev/null
> +++ b/kernel/trace/trace_events_hist.c
> @@ -0,0 +1,837 @@
> +/*
> + * trace_events_hist - trace event hist triggers
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Copyright (C) 2015 Tom Zanussi <tom.zanussi@...ux.intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kallsyms.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/stacktrace.h>
> +
> +#include "tracing_map.h"
> +#include "trace.h"
> +
> +struct hist_field;
> +
> +typedef u64 (*hist_field_fn_t) (struct hist_field *field, void *event);
> +
> +struct hist_field {
> + struct ftrace_event_field *field;
> + unsigned long flags;
> + hist_field_fn_t fn;
> + unsigned int size;
> +};
> +
> +static u64 hist_field_counter(struct hist_field *field, void *event)
> +{
> + return 1;
> +}
> +
> +static u64 hist_field_string(struct hist_field *hist_field, void *event)
> +{
> + char *addr = (char *)(event + hist_field->field->offset);
> +
> + return (u64)(unsigned long)addr;
> +}
> +
> +#define DEFINE_HIST_FIELD_FN(type) \
> +static u64 hist_field_##type(struct hist_field *hist_field, void *event)\
> +{ \
> + type *addr = (type *)(event + hist_field->field->offset); \
> + \
> + return (u64)*addr; \
> +}
> +
> +DEFINE_HIST_FIELD_FN(s64);
> +DEFINE_HIST_FIELD_FN(u64);
> +DEFINE_HIST_FIELD_FN(s32);
> +DEFINE_HIST_FIELD_FN(u32);
> +DEFINE_HIST_FIELD_FN(s16);
> +DEFINE_HIST_FIELD_FN(u16);
> +DEFINE_HIST_FIELD_FN(s8);
> +DEFINE_HIST_FIELD_FN(u8);
> +
> +#define for_each_hist_field(i, hist_data) \
> + for (i = 0; i < hist_data->n_fields; i++)
> +
> +#define for_each_hist_val_field(i, hist_data) \
> + for (i = 0; i < hist_data->n_vals; i++)
> +
> +#define for_each_hist_key_field(i, hist_data) \
> + for (i = hist_data->n_vals; i < hist_data->n_fields; i++)
The above should have parenthesis around the macro variables.
e.g.
(hist_data)->n_fields
> +
> +#define HITCOUNT_IDX 0
> +#define HIST_KEY_MAX 1
> +#define HIST_KEY_SIZE_MAX MAX_FILTER_STR_VAL
> +
> +enum hist_field_flags {
> + HIST_FIELD_HITCOUNT = 1,
> + HIST_FIELD_KEY = 2,
> + HIST_FIELD_STRING = 4,
Note, please add "_FL" to the enums to denote they are flags (used for
bitmask).
e.g. HIST_FIELD_FL_HITCOUNT
> +};
> +
> +struct hist_trigger_attrs {
> + char *keys_str;
> + unsigned int map_bits;
> +};
> +
> +struct hist_trigger_data {
> + struct hist_field *fields[TRACING_MAP_FIELDS_MAX];
Hmm I'm confused, TRACING_MAP_FIELDS_MAX == 4
But below it we index into fields with n_fields. Which look to me can
be much bigger than 4.
Or is there some correlation between n_vals, n_keys and 4?
> + unsigned int n_vals;
> + unsigned int n_keys;
> + unsigned int n_fields;
> + unsigned int key_size;
> + struct tracing_map_sort_key sort_keys[TRACING_MAP_SORT_KEYS_MAX];
> + unsigned int n_sort_keys;
> + struct trace_event_file *event_file;
> + struct hist_trigger_attrs *attrs;
> + struct tracing_map *map;
> +};
> +
> +static hist_field_fn_t select_value_fn(int field_size, int field_is_signed)
> +{
> + hist_field_fn_t fn = NULL;
> +
> + switch (field_size) {
> + case 8:
> + if (field_is_signed)
> + fn = hist_field_s64;
> + else
> + fn = hist_field_u64;
> + break;
> + case 4:
> + if (field_is_signed)
> + fn = hist_field_s32;
> + else
> + fn = hist_field_u32;
> + break;
> + case 2:
> + if (field_is_signed)
> + fn = hist_field_s16;
> + else
> + fn = hist_field_u16;
> + break;
> + case 1:
> + if (field_is_signed)
> + fn = hist_field_s8;
> + else
> + fn = hist_field_u8;
> + break;
> + }
> +
> + return fn;
> +}
> +
> +static int parse_map_size(char *str)
> +{
> + unsigned long size, map_bits;
> + int ret;
> +
> + strsep(&str, "=");
> + if (!str) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = kstrtoul(str, 0, &size);
> + if (ret)
> + goto out;
> +
> + map_bits = ilog2(roundup_pow_of_two(size));
> + if (map_bits < TRACING_MAP_BITS_MIN ||
> + map_bits > TRACING_MAP_BITS_MAX)
> + ret = -EINVAL;
> + else
> + ret = map_bits;
> + out:
> + return ret;
> +}
> +
> +static void destroy_hist_trigger_attrs(struct hist_trigger_attrs *attrs)
> +{
> + if (!attrs)
> + return;
> +
> + kfree(attrs->keys_str);
> + kfree(attrs);
> +}
> +
> +static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
> +{
> + struct hist_trigger_attrs *attrs;
> + int ret = 0;
> +
> + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return ERR_PTR(-ENOMEM);
> +
> + while (trigger_str) {
> + char *str = strsep(&trigger_str, ":");
> +
> + if (!strncmp(str, "keys", strlen("keys")) ||
> + !strncmp(str, "key", strlen("key")))
nit, I prefer "strncmp(x,y,n) == 0" as it shows an equals, and, at
least to me, easier to quickly understand.
But not so nit, if !strncmp(str, "keys", strlen("keys")) then
!strcmp(str, "key", strlen("key")) is true!
It has the same result as:
if (!strncmp(str, "key", strlen("key")))
That is equivalent to if (x > 12 || x > 10) where
if (x > 10) will do.
Is that suppose to be a full match or partial match?
> + attrs->keys_str = kstrdup(str, GFP_KERNEL);
> + else if (!strncmp(str, "size", strlen("size"))) {
Is it OK to allow "sizeUgaBooga=10"?
Perhaps the compare should be against "size=" ?
Same for the keys above.
> + int map_bits = parse_map_size(str);
> +
> + if (map_bits < 0) {
> + ret = map_bits;
> + goto free;
> + }
> + attrs->map_bits = map_bits;
> + } else {
> + ret = -EINVAL;
> + goto free;
> + }
> + }
> +
> + if (!attrs->keys_str) {
> + ret = -EINVAL;
> + goto free;
> + }
> +
> + return attrs;
> + free:
> + destroy_hist_trigger_attrs(attrs);
> +
> + return ERR_PTR(ret);
> +}
> +
I stopped here. I'll review the rest later.
-- Steve
Powered by blists - more mailing lists