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

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:	Thu, 28 Aug 2014 16:01:12 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Daniel Thompson <daniel.thompson@...aro.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	kgdb-bugreport@...ts.sourceforge.net, patches@...aro.org,
	linaro-kernel@...ts.linaro.org,
	John Stultz <john.stultz@...aro.org>,
	Anton Vorontsov <anton.vorontsov@...aro.org>,
	Colin Cross <ccross@...roid.com>, kernel-team@...roid.com,
	Rob Herring <robherring2@...il.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Ben Dooks <ben.dooks@...ethink.co.uk>,
	Catalin Marinas <catalin.marinas@....com>,
	Dave Martin <Dave.Martin@....com>,
	Fabio Estevam <festevam@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Nicolas Pitre <nico@...aro.org>
Subject: Re: [PATCH v10 03/19] arm: fiq: Replace default FIQ handler

On Tue, Aug 19, 2014 at 07:12:07PM +0100, Daniel Thompson wrote:
> On 19/08/14 18:37, Russell King - ARM Linux wrote:
> > On Tue, Aug 19, 2014 at 05:45:53PM +0100, Daniel Thompson wrote:
> >> +int register_fiq_nmi_notifier(struct notifier_block *nb)
> >> +{
> >> +	return atomic_notifier_chain_register(&fiq_nmi_chain, nb);
> >> +}
> >> +
> >> +asmlinkage void __exception_irq_entry fiq_nmi_handler(struct pt_regs *regs)
> >> +{
> >> +	struct pt_regs *old_regs = set_irq_regs(regs);
> >> +
> >> +	nmi_enter();
> >> +	atomic_notifier_call_chain(&fiq_nmi_chain, (unsigned long)regs, NULL);
> >> +	nmi_exit();
> >> +	set_irq_regs(old_regs);
> >> +}
> > 
> > Really not happy with this.  What happens if a FIQ occurs while we're
> > inside register_fiq_nmi_notifier() - more specifically inside
> > atomic_notifier_chain_register() ?
> 
> Should depend on which side of the rcu update we're on.

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.

-- 
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/