Re: [PATCH] avr32: Don't acquire siglock when reading sighand action

[Matt Fleming <matt@console-pimps.org>]

On Sat, 2 Apr 2011 18:37:22 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 03/30, Matt Fleming wrote:
> >
> > From: Matt Fleming <matt.fleming@linux.intel.com>
> >
> > There's no need to acquire the siglock when simply reading the
> > action handler. We would need to lock it if we were modifying the
> > handler or we were protecting it from modification across function
> > calls, but if we're just reading it, there's no need to lock it
> 
> Agreed, ->siglock buys nothing.
> 
> But,
> 
> > --- a/arch/avr32/kernel/traps.c
> > +++ b/arch/avr32/kernel/traps.c
> > @@ -106,9 +106,7 @@ void _exception(long signr, struct pt_regs
> > *regs, int code, if (is_global_init(current)) {
> 
> is_global_init() is not right in theory, /sbin/init can be
> multithreaded. And, this doesn't cover the sub-namespace inits... I'd
> suggest to check SIGNAL_UNKILLABLE, but looking closer I think you
> can simply remove this code.
> 
> The comment says:
> 
> 	/*
> 	 * Init gets no signals that it doesn't have a handler for.
> 	 * That's all very well, but if it has caused a synchronous
> 	 * exception and we ignore the resulting signal, it will just
> 	 * generate the same exception over and over again and we get
> 	 * nowhere.  Better to kill it and let the kernel panic.
> 	 */
> 
> This is correct, but please look at force_sig_info(), it already does
> what you need:
> 
> 	* We don't want to have recursive SIGSEGV's etc, for example,
> 	* that is why we also clear SIGNAL_UNKILLABLE.

Aha! Yeah, you're totally correct - good spot. Interestingly, after a
bit of git archaeology it seems that this code was copied from powerpc,

commit 623b0355d5b1f9c6d05005b649a2f3a7b9fd7816
Author: Haavard Skinnemoen <hskinnemoen@atmel.com>
Date:   Tue Mar 13 17:59:11 2007 +0100

    [AVR32] Clean up exception handling code
    
      * Use generic BUG() handling
      * Remove some useless debug statements
      * Use a common function _exception() to send signals or oops when
        an exception can't be handled. This makes sure init doesn't
        enter an infinite exception loop as well. Borrowed from powerpc.
      * Add some basic exception tracing support to the page fault code.
      * Rework dump_stack(), show_regs() and friends and move everything
        into process.c
      * Print information about configuration options and chip type when
        oopsing
    
    Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>


... and Ben H deleted the copy in arch/powerpc in Nov 2009, 

commit a0592d42fe3e12966db02f5c41f1edae2e59c490
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date:   Tue Nov 10 15:13:15 2009 +0000

    powerpc: kill the obsolete code under is_global_init()
    
    The code under "if (is_global_init())" is bogus, and is_global_init()
    itself is not right in mt case.
    
    Contrary to what the comment says, nowadays force_sig_info() does kill
    init even if the handler is SIG_DFL. Note that force_sig_info() clears
    SIGNAL_UNKILLABLE exactly for this case.
    
    Signed-off-by: Oleg Nesterov <oleg@redhat.com>
    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>


I'll send a new patch that deletes this code for avr32.

-- 
Matt Fleming, Intel Open Source Technology Center