Message-ID: <1641863026.30821.1393424602830.JavaMail.zimbra@efficios.com>
Date:	Wed, 26 Feb 2014 14:23:22 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Rusty Russell <rusty@...tcorp.com.au>,
	David Howells <dhowells@...hat.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new
 TAINT_UNSIGNED_MODULE

----- Original Message -----
> From: "Steven Rostedt" <rostedt@...dmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>
> Cc: "Ingo Molnar" <mingo@...nel.org>, linux-kernel@...r.kernel.org, "Ingo Molnar" <mingo@...hat.com>, "Thomas
> Gleixner" <tglx@...utronix.de>, "Rusty Russell" <rusty@...tcorp.com.au>, "David Howells" <dhowells@...hat.com>,
> "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
> Sent: Monday, February 24, 2014 2:10:07 PM
> Subject: Re: [RFC PATCH] Fix: module signature vs tracepoints: add new TAINT_UNSIGNED_MODULE
> 
> On Mon, 24 Feb 2014 18:32:03 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> 
> 
> > > 
> > > The real answer to this is to enabled the tracepoints on module load,
> > > with a module parameter. We've talked about this before, and I also
> > > think there's some patches out there that do this. (I remember creating
> > > something myself). I'll have to go dig them out and we can work to get
> > > them in 3.15.
> > 
> > For the records, I still think that requiring users of tracing to add
> > module parameters specifying what tracing they need enabled is expecting
> > them to interact at the wrong interface level. This might be convenient
> > for kernel developers, but not for other types of kernel tracing end
> > users. From a user experience perspective, I think your "real answer"
> > is the wrong answer.
> 
> 
> Why? This is not a normal activity to for the user. You seem to have a
> few specific users, but those are exceptions, and this has nothing to
> do with normal kernel developer view.

The very fact that you present "normal kernel developer view" as being
the norm just tells how much we focus on different user bases. Since
the user-base you target are mostly kernel developers, it is
understandable that you dismiss our user base as being small and
specific.

There are many more Linux users than kernel developers out there. The
main difference is that the former category don't usually post on LKML.

> 
> Tracing a module that's being loaded is far from normal user activity.
> 
> Now, for boot up, I could see enabling all tracepoints. For that, we
> can add a hook to the module load that when a flag is set, we can
> enable all trace events in the module as it is loaded. That would work
> for boot up.
> 
> If this is such a user activity, please explain to me what scenario
> that requires tracing a module being loaded that could not easily be
> accomplished with a module parameter?

OK. Here is the scenario:

- We have users deploying tracing on their systems in flight recorder
  "snapshot" mode (writing into circular memory buffers, without any
  other type of I/O, except when a snapshot of the buffers is needed).
  They wish collect data from user-space and kernel-space, including
  from kernel modules. You can think of it like a detailed crash
  tracing for Linux distributions with very low overhead. Which
  tracepoints are enabled depends on configuration of this flight
  recorder tracing "session". There may be multiple concurrent tracing
  sessions enabled in parallel, trying each to pinpoint different kind
  of issues. Some are controlled by the distro end-user, others are
  automatically enabled by the distro if the user agrees to provide
  detailed bug report feedback to the distro.
- There, an automated analysis wants to hook on specific module
  tracepoints (e.g. the usb stack) which are loaded only when the
  devices are physically plugged into the computer.

It would not be appropriate to change a global state (whether the
tracepoint is enabled or not for this module) for something specific
to a subset of the tracing sessions. Moreover, you cannot expect an
issue-monitoring tool within the distribution to start modifying the
module parameters across the entire system. Chances are that it will
conflict with other tools and user-specific configuration if it try
to do so.

> 
> > 
> > > 
> > > > 
> > > > Another way to do this, without requiring changes to the existing
> > > > tracepoint_probe_register() API, is to simply add e.g. (better function
> > > > names welcome):
> > > > 
> > > > int tracepoint_has_callsites(const char *name)
> > > > {
> > > >     struct tracepoint_entry *entry;
> > > >     int ret = 0;
> > > > 
> > > >     mutex_lock(&tracepoints_mutex);
> > > >     entry = get_tracepoint(name);
> > > >     if (entry && entry->refcount) {
> > > >         ret = 1;
> > > >     }
> > > >     mutex_unlock(&tracepoints_mutex);
> > > >     return ret;
> > > > }
> > > 
> > > No, I'm not about to change the interface for something that is broken.
> > > tracepoint_probe_register() should fail if it does not register a
> > > tracepoint. It should not store it off for later. I'm not aware of any
> > > other "register" function in the kernel that does such a thing. Is
> > > there one that you know of?
> > 
> > see include/linux/notifier.h
> > 
> > You can register notifier callbacks without having any notifier call sites.
> > This is exactly what tracepoint.c is currently doing. The change you
> > propose
> > is the equivalent of refusing to register a notifier callback if there is
> > no call site for that notifier.
> 
> WTF! That's a horrible example. Yes, notifier is the infrastructure
> code (a header file), but where is there a registered list without
> callback sites? Look at include/linux/module.h! Do we allow to register
> module notifiers when modules are not configured in? NO!

See drivers/acpi/events.c: acpi_notifier_call_chain()

This notifier chain is defined within events.c, compiled whenever
ACPI is configured in the kernel (CONFIG_ACPI). All of its callers
are:

drivers/acpi/video.c: depends on CONFIG_ACPI_AC
acpi/ac.c: depends on CONFIG_ACPI_VIDEO

So yes, there are cases where the notifier block head is there and the
modules calling into it are not loaded.

> 
> The tracepoint code does much more than registering a notifier. It
> *enables* the tracepoint.

Per callsite tracepoint activation is merely an optimization over a
textbook callback mechanism. We want to skip the empty loop very quickly,
hence we skip over it with a conditional branch, or static jump (if
available).

The tracepoints know callsites registered from the core kernel and at
module load. Tracepoints also register probes for those callsites.
Whenever a registered probe matches a registered callsite, it is added
to the list of callbacks for that callsite. If there is at least one
probe in a callsite callback list, the callsite is enabled.

> I'll reverse your example on you. If you call
> a notifier that has nothing registered, it does the same work that it
> would do if something was registered to it. But the loop is empty, it
> just doesn't call anything.

A disabled tracepoint is just like an empty loop. The only difference
is that it is optimized for the common case: empty callback list.

> 
> The tracepoint code does the work to activate the call site. Here's
> where your analogy is broken. If I register a handler for a notifier,
> when the call site is hit, my handler will be called. Well, because of
> not doing anything different for non existent modules and modules that
> fail to have their call sites activated, the register returns success
> in both cases. That means, if I register a probe, it wont be called at
> the call site. That's a big f'ing bug.

If the probe and the callsite match, Tracepoints guarantee that the probe
is added to the module tracepoint callsite. The bug here is that
tracepoint.c silently ignored certain classes of tainted modules without
giving any error message to the user. I think your approach of logging
an error whenever tracepoints decide to disregard force-loaded modules
is a good approach to at least tell users there is something going
wrong. Splitting the "unsigned module" case from "force loaded" module
is going to let users have tracepoints behaving as expected in unsigned
modules within signed kernels, which I think is the second step in making
everything right.

Thanks,

Mathieu

> 
> 
> > 
> > > 
> > > > 
> > > > So tools which care about providing feedback to their users about the
> > > > fact that tracepoint callsites are not there can call this new
> > > > function.
> > > > The advantage is that it would not change the return values of the
> > > > existing
> > > > API. Also, it would be weird to have tracepoint_probe_register() return
> > > > an error value but leave the tracepoint in a registered state. Moving
> > > > this
> > > > into a separate query function seems more consistent IMHO.
> > > 
> > > Again, the existing API is not a user interface. It is free to change,
> > > and this change wont break any existing in-tree uses. In fact, the fact
> > > that you had this stupid way of doing it, *broke* the in-tree users!
> > > That is, no error messages were ever displayed when the probe was not
> > > registered. This is why I consider this a broken design.
> > > 
> > > If you want LTTng to enable tracepoints before loading, then have your
> > > module save off these these tracepoints and register a handler to
> > > be called when a module is loaded and enable them yourself.
> > 
> > That's a possible option.
> > 
> > > 
> > > For now, I'm going to push this in, and also mark it for stable.
> > 
> > I still disagree with this change.
> 
> Of course you do ;-)
> 
> -- Steve
> 

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

Powered by blists - more mailing lists