Re: [PATCH v10 03/19] arm: fiq: Replace default FIQ handler

[Russell King - ARM Linux]

On Tue, Sep 02, 2014 at 12:49:16PM +0100, Daniel Thompson wrote:
> On 28/08/14 16:01, Russell King - ARM Linux wrote:
> > I just asked Paul McKenney, our RCU expert... essentially, yes, RCU
> > stuff itself is safe in this context.  However, RCU stuff can call into
> > lockdep if lockdep is configured, and there are questions over lockdep.
> > 
> > There's some things which can be done to reduce the lockdep exposure
> > to it, such as ensuring that rcu_read_lock() is first called outside
> > of FIQ context.
> > 
> > There's concerns with whether either printk() in check_flags() could
> > be reached too (flags there should always indicate that IRQs were
> > disabled, so that reduces down to a question about just the first
> > printk() there.)
> > 
> > There's also the very_verbose() stuff for RCU lockdep classes which
> > Paul says must not be enabled.
> > 
> > Lastly, Paul isn't a lockdep expert, but he sees nothing that prevents
> > lockdep doing the deadlock checking as a result of the above call.
> > 
> > So... this coupled with my feeling that notifiers make it too easy for
> > unreviewed code to be hooked into this path, I'm fairly sure that we
> > don't want to be calling atomic notifier chains from FIQ context.
> 
> Having esablished (elsewhere in the thread) that RCU usage is safe
> from FIQ I have been working on the assumption that your feeling
> regarding unreviewed code is sufficient on its own to avoid using
> notifiers (and also to avoid a list of function pointers like on x86).

Yes, it does, because unlike the x86 community, we have a wide range
of platforms, and platform code does not go through the same path or
get the same review as core ARM code.

As I already pointed out, with a notifier, it's very easy to sneak
something into the FIQ path by submitting a patch for platform code
which calls the registration function.  That's going to be pretty
difficult to spot amongst the 3000+ messages on the linux-arm-kernel
list each month in order to give it the review that it would need.
That's especially true as I now ignore almost all most platform
code patches as we have Arnd and Olof to look at that.

So, unless you can come up with a proposal which ensures that there
is sufficient review triggered when someone decides to call the
notifier registration function...

How about something like:

static const char *allowable_callers[] = {
	...
};

	snprintf(caller, sizeof(caller), "%pf", __builtin_return_address(0));

	for (i = 0; i < ARRAY_SIZE(allowable_callers); i++)
		if (!strcmp(caller, allowable_callers[i]))
			break;

	if (i == ARRAY_SIZE(allowable_callers)) {
		printk(KERN_ERR "%s is not permitted to register a FIQ notifer\n",
			caller);
		return;
	}

This gives us the advantage of using the notifier, but also gives us the
requirement that the file has to be modified to permit new registrations,
thereby triggering the closer review.

The other question I have is that if we permit kgdb and nmi tracing with
this mechanism, how do the hooked callers distinguish between these
different purposes?  I don't see how that works with your notifier
setup.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/