Messages in this thread | | | Date | Thu, 28 Aug 2014 09:15:51 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH v10 03/19] arm: fiq: Replace default FIQ handler |
| |
On Thu, Aug 28, 2014 at 04:54:25PM +0100, Daniel Thompson wrote: > On 28/08/14 16:01, Russell King - ARM Linux wrote: > > 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. > > Thanks for following this up. > > I originally formed the opinion RCU was safe from FIQ because it is also > used to manage the NMI notification handlers for x86 > (register_nmi_handler) and I understood the runtime constraints on FIQ > to be very similar. > > Note that x86 manages the notifiers itself so it uses > list_for_each_entry_rcu() rather atomic_notifier_call_chain() but > nevertheless I think this boils down to the same thing w.r.t. safety > concerns. > > > > 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. > > lockdep is automatically disabled by calling nmi_enter() so all the > lockdep calls should end up following the early exit path based on > current->lockdep_recursion.
Ah, that was what I was missing. Then the notification should be safe from NMI, so have at it! ;-)
Thanx, Paul
> > 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. > > >
| |