linux-kernel - Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not set via debugfs

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]

Date:	Mon, 10 Mar 2014 16:19:27 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Johannes Berg <johannes.berg@...el.com>
Subject: Re: [for-next][PATCH 08/20] tracing: Warn if a tracepoint is not
 set via debugfs

On Mon, 10 Mar 2014 20:01:34 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
 
> >  	mutex_lock(&tracepoints_mutex);
> >  	old = tracepoint_add_probe(name, probe, data);
> > @@ -388,9 +393,13 @@ int tracepoint_probe_register(const char *name, void
> > *probe, void *data)
> >  		return PTR_ERR(old);
> >  	}
> >  	tracepoint_update_probes();		/* may update entry */
> > +	entry = get_tracepoint(name);
> > +	/* Make sure the entry was enabled */
> > +	if (!entry || !entry->enabled)
> > +		ret = -ENODEV;
> 
> Hi Steven,
> 
> Returning -ENODEV when the probe is still registered might come as a
> surprise to the caller. For instance, a caller may dynamically allocate
> name, probe, and/or data, it may want to free them when
> tracepoint_probe_register returns an error. But this "-ENODEV" return value
> is not really an error, and the parameters passed are still used.

It's an error when you wanted to enable a probe and the probe doesn't
exist. There are no in tree users of this call that expect it to work
when the probe does not exist.

> 
> If we go down this route, we might want at the very least to add documentation
> of tracepoint_probe_register() return values and their meaning
> in a comment on top of this function (perhaps also in the header). But
> even if we do so, this weird return value semantic with respect to use of the
> received parameters will likely cause memory corruption at some point.
> 
> Thoughts ?

Send a patch to document the return values. Your module can expect this
return value when it doesn't expect the probe to exist.

Again, it's really strange when you go to enable a probe, and there is
no probe to enable.

Note, I was nice. I removed the logic to unregister the probe in this
case.

Anyway, this should even help you. Before there was no way to enable a
probe and know if it was enabled or not. That is, if it didn't exist,
there was no feedback letting you know that.

If you expect to enable a probe that doesn't exist, then you can expect
this return value.

-- Steve
--
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/