linux-kernel - Re: [for-next][PATCH 05/31] tracing: Have existing event_command.parse() implementations use helpers

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: <20220113162058.04731caf@gandalf.local.home>
Date:   Thu, 13 Jan 2022 16:20:58 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Daniel Bristot de Oliveira <bristot@...nel.org>
Cc:     Tom Zanussi <zanussi@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [for-next][PATCH 05/31] tracing: Have existing
 event_command.parse() implementations use helpers

On Thu, 13 Jan 2022 18:03:07 +0100
Daniel Bristot de Oliveira <bristot@...nel.org> wrote:

> I did some debug, and found that the histogram is working. The problem is that,
> to read the histogram I pause it to have consistent data:
> 
> in tools/tracing/rtla/osnoise_hist.c:
> osnoise_read_trace_hist() {
>  [...]
>         tracefs_hist_pause(tool->trace.inst, data->trace_hist);
> 
>         content = tracefs_event_file_read(tool->trace.inst, "osnoise",
>                                           "sample_threshold",
>                                           "hist", NULL);
>  [...]
> }
> 
> and, as far as I got, after this patch, pausing the histogram makes it to clear
> up. If I comment the "tracefs_hist_pause" line, "rtla osnoise hist" start
> working back again.
> 
> Thoughts?

This is all messed up. I'm removing this patch completely.

Tom, can you fix this. The issue is that it's putting too much policy into
the helper functions, which is big no no.

Specifically, we have:

int event_trigger_register(struct event_command *cmd_ops,
			   struct trace_event_file *file,
			   char *glob,
			   char *cmd,
			   char *param,
			   struct event_trigger_data *trigger_data,
			   int *n_registered)
{
	int ret;

	if (n_registered)
		*n_registered = 0;

	ret = cmd_ops->reg(glob, trigger_data, file);
	/*
	 * The above returns on success the # of functions enabled,
	 * but if it didn't find any functions it returns zero.
	 * Consider no functions a failure too.
	 */
	if (!ret) {
		cmd_ops->unreg(glob, trigger_data, file);
		ret = -ENOENT;
	} else if (ret > 0) {
		if (n_registered)
			*n_registered = ret;
		/* Just return zero, not the number of enabled functions */
		ret = 0;
	}

	return ret;
}


And in the case of pause, this *will* have ret = 0 on return. And what
happens is that it removes the trigger completely.

Look at the code in the histogram on the return:

	ret = event_trigger_register(cmd_ops, file, glob, cmd, param, trigger_data, &n_registered);
	if (ret < 0)
		goto out_free;
	if ((ret == 0) && (n_registered == 0)) {
		if (!(attrs->pause || attrs->cont || attrs->clear))
			ret = -ENOENT;
		goto out_free;
	}

It checks for 0 and 0 and only errors if it's not pause, cont, or clear.
Hence, all three are now broken due to this patch.

I will not be adding this to this merge window.

-- Steve