linux-kernel - Re: [PATCH v15 02/23] tracing: Add 'hist' event trigger command

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160302093320.483228b8@gandalf.local.home>
Date:	Wed, 2 Mar 2016 09:33:20 -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 v15 02/23] tracing: Add 'hist' event trigger command

On Fri, 26 Feb 2016 10:01:05 -0600
Tom Zanussi <tom.zanussi@...ux.intel.com> wrote:


> +static int create_hitcount_val(struct hist_trigger_data *hist_data)
> +{
> +	hist_data->fields[HITCOUNT_IDX] =
> +		create_hist_field(NULL, HIST_FIELD_FL_HITCOUNT);
> +	if (!hist_data->fields[HITCOUNT_IDX])
> +		return -ENOMEM;
> +
> +	if (WARN_ON(++hist_data->n_vals > TRACING_MAP_VALS_MAX))

Never put side effects in a WARN_ON() macro. Some configs define
WARN_ON() as a nop. Which would break this code. Always do the
following:

	hist_data->n_vals++;

	if (WARN_ON(hist_data->n_vals > TRACING_MAP_VALS_MAX))
> +		return -EINVAL;

> +
> +	return 0;
> +}
> +
> +static int create_val_fields(struct hist_trigger_data *hist_data,
> +			     struct trace_event_file *file)
> +{
> +	int ret;
> +
> +	ret = create_hitcount_val(hist_data);
> +
> +	return ret;
> +}
> +
> +static int create_key_field(struct hist_trigger_data *hist_data,
> +			    unsigned int key_idx,
> +			    struct trace_event_file *file,
> +			    char *field_str)
> +{
> +	struct ftrace_event_field *field = NULL;
> +	unsigned long flags = 0;
> +	unsigned int key_size;
> +	int ret = 0;
> +
> +	if (WARN_ON(key_idx >= TRACING_MAP_FIELDS_MAX))
> +		return -EINVAL;
> +
> +	flags |= HIST_FIELD_FL_KEY;
> +
> +	field = trace_find_event_field(file->event_call, field_str);
> +	if (!field) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	key_size = field->size;
> +
> +	hist_data->fields[key_idx] = create_hist_field(field, flags);
> +	if (!hist_data->fields[key_idx]) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	key_size = ALIGN(key_size, sizeof(u64));
> +	hist_data->fields[key_idx]->size = key_size;
> +	hist_data->key_size = key_size;
> +	if (hist_data->key_size > HIST_KEY_SIZE_MAX) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (WARN_ON(++hist_data->n_keys > TRACING_MAP_KEYS_MAX))

Same here.

> +		return -EINVAL;
> +
> +	ret = key_size;
> + out:
> +	return ret;
> +}
> +

-- Steve