MIPS: return after handling coprocessor 2 exception

MIPS: return after handling coprocessor 2 exception

[Jesper Nilsson]

Breaking here dropped us to the default code which always sends
a SIGILL to the current process, no matter what the CU2 notifier says.

Signed-off-by: Jesper Nilsson <jesper@jni.nu>
---
 traps.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 8bdd6a6..8527808 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -976,7 +976,7 @@ asmlinkage void do_cpu(struct pt_regs *regs)
 
 	case 2:
 		raw_notifier_call_chain(&cu2_chain, CU2_EXCEPTION, regs);
-		break;
+		return;
 
 	case 3:
 		break;

/^JN - Jesper Nilsson
--
                  Jesper Nilsson -- jesper_at_jni.nu

Re: MIPS: return after handling coprocessor 2 exception

[David Daney]

On 06/17/2010 06:25 AM, Jesper Nilsson wrote:
> Breaking here dropped us to the default code which always sends
> a SIGILL to the current process, no matter what the CU2 notifier says.
>
> Signed-off-by: Jesper Nilsson<jesper@jni.nu>
> ---
>   traps.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index 8bdd6a6..8527808 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -976,7 +976,7 @@ asmlinkage void do_cpu(struct pt_regs *regs)
>
>   	case 2:
>   		raw_notifier_call_chain(&cu2_chain, CU2_EXCEPTION, regs);
> -		break;
> +		return;
>

What happens when the call chain is empty, and the proper action *is* 
SIGILL?

David Daney

Re: MIPS: return after handling coprocessor 2 exception

[Jesper Nilsson]

On Thu, Jun 17, 2010 at 10:13:18AM -0700, David Daney wrote:
> On 06/17/2010 06:25 AM, Jesper Nilsson wrote:
> >Breaking here dropped us to the default code which always sends
> >a SIGILL to the current process, no matter what the CU2 notifier says.
> >
> >Signed-off-by: Jesper Nilsson<jesper@jni.nu>
> >---
> >  traps.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> >index 8bdd6a6..8527808 100644
> >--- a/arch/mips/kernel/traps.c
> >+++ b/arch/mips/kernel/traps.c
> >@@ -976,7 +976,7 @@ asmlinkage void do_cpu(struct pt_regs *regs)
> >
> >  	case 2:
> >  		raw_notifier_call_chain(&cu2_chain, CU2_EXCEPTION, regs);
> >-		break;
> >+		return;
> >
> 
> What happens when the call chain is empty, and the proper action *is* 
> SIGILL?

Well, since there is a default notifier installed at the end, it will
correctly return SIGILL.

See the definition of default_cu2_call in the same file.

> David Daney

/^JN - Jesper Nilsson
--
                  Jesper Nilsson -- jesper_at_jni.nu

Re: MIPS: return after handling coprocessor 2 exception

[Gleb O. Raiko]

On 17.06.2010 21:13, David Daney wrote:
> On 06/17/2010 06:25 AM, Jesper Nilsson wrote:
>> Breaking here dropped us to the default code which always sends
>> a SIGILL to the current process, no matter what the CU2 notifier says.
>>
>> Signed-off-by: Jesper Nilsson<jesper@jni.nu>
[...]
>> case 2:
>> raw_notifier_call_chain(&cu2_chain, CU2_EXCEPTION, regs);
>> - break;
>> + return;
>>
>
> What happens when the call chain is empty, and the proper action *is*
> SIGILL?

It's never empty, in fact. The default notifier declared at top of 
traps.c sends SIGILL. The problem that current code is sending SIGILL in 
all cases.

Gleb.

Re: MIPS: return after handling coprocessor 2 exception

[Ralf Baechle]

On Fri, Jun 18, 2010 at 11:54:38AM +0400, Gleb O. Raiko wrote:

> >What happens when the call chain is empty, and the proper action *is*
> >SIGILL?
> 
> It's never empty, in fact. The default notifier declared at top of
> traps.c sends SIGILL. The problem that current code is sending
> SIGILL in all cases.

That's not really a problem.  The design idea is that a the default
notifier has the lowest priority, that is any user notifier installed
should have higher priority resulting in it getting run first.  To avoid
the default notifier from getting executed such an extra notifier should
set NOTIFY_STOP_MASK in its return like:

static int default_cu2_call(struct notifier_block *nfb, unsigned long action,
        void *data)
{
	...

	return NOTIFY_OK | NOTIFY_STOP;
}

The notifier list could also be used for example by perf but there it
we'd want the notifier function not to return NOTIFY_STOP as the result;

Arguably sending the signal for CU2 instructions has been delegated to the
hook so the I agree that the break stateement should be replaced with a
return and will apply the patch.

  Ralf

Re: MIPS: return after handling coprocessor 2 exception

[Gleb O. Raiko]

On 18.06.2010 14:00, Ralf Baechle wrote:
> static int default_cu2_call(struct notifier_block *nfb, unsigned long action,
>          void *data)
> {
> 	...
>
> 	return NOTIFY_OK | NOTIFY_STOP;
NOTIFY_STOP implies NOTIFY_OK, so
	return NOTIFY_STOP;
shall be enough.
> }

> The notifier list could also be used for example by perf

Or octeon cop2 handler that just sends NOTIFY_BAD for getting the same 
behavior.

Gleb.

Re: MIPS: return after handling coprocessor 2 exception

[Ralf Baechle]

On Fri, Jun 18, 2010 at 02:35:54PM +0400, Gleb O. Raiko wrote:

> >static int default_cu2_call(struct notifier_block *nfb, unsigned long action,
> >         void *data)
> >{
> >	...
> >
> >	return NOTIFY_OK | NOTIFY_STOP;
> NOTIFY_STOP implies NOTIFY_OK, so
> 	return NOTIFY_STOP;
> shall be enough.

Correct - I was thinking NOTIFY_STOP_MASK.

> >}
> 
> >The notifier list could also be used for example by perf
> 
> Or octeon cop2 handler that just sends NOTIFY_BAD for getting the
> same behavior.

Bad karma to return an error for where none happened.

  Ralf