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]

Message-ID: <1554686902.35143.1394484906949.JavaMail.zimbra@efficios.com>
Date:	Mon, 10 Mar 2014 20:55:06 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Steven Rostedt <rostedt@...dmis.org>
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

----- Original Message -----
> 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>
> Sent: Monday, March 10, 2014 4:19:27 PM
> 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.

I'm OK with this change. I was merely pointing out that we need to document
this return value, since its semantic differs from what is usually expected.

I'll prepare a patch soon to document the return value.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
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/