Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1705689 > unrolled thread

Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with SIGFPE

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with  SIGFPE "Maciej W. Rozycki" <macro@imgtec.com> - 2017-08-07 18:20 +0200
    Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with SIGFPE Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-07 19:50 +0200
      Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with  SIGFPE Ralf Baechle <ralf@linux-mips.org> - 2017-08-07 22:00 +0200
    Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with SIGFPE ebiederm@xmission.com (Eric W. Biederman) - 2017-08-08 17:40 +0200
      Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with  SIGFPE "Maciej W. Rozycki" <macro@imgtec.com> - 2017-08-09 01:20 +0200

#1705689 — Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with SIGFPE

On Tue, 18 Jul 2017, Eric W. Biederman wrote:

> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index b68b4d0726d3..6c9cca9c5341 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -735,7 +735,7 @@ void force_fcr31_sig(unsigned long fcr31, void __user *fault_addr,
>  	else if (fcr31 & FPU_CSR_INE_X)
>  		si.si_code = FPE_FLTRES;
>  	else
> -		si.si_code = __SI_FAULT;
> +		si.si_code = FPE_FIXME;

 This is an "impossible" state to reach unless your hardware is on fire.  
One or more of the FCSR Cause bits will have been set (in `fcr31') or the 
FPE exception would not have happened.

 Of course there could be a simulator bug, or we could have breakage 
somewhere causing `process_fpemu_return' to be called with SIGFPE and 
inconsistent `fcr31'.  So we need to handle it somehow.

 So what would be the right value of `si_code' to use here for such an 
unexpected exception condition?  I think `BUG()' would be too big a 
hammer here.  Or wouldn't it?

  Maciej

[toc] | [next] | [standalone]

#1705744 — Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with SIGFPE

On Mon, Aug 7, 2017 at 9:18 AM, Maciej W. Rozycki <macro@imgtec.com> wrote:
>
>  So what would be the right value of `si_code' to use here for such an
> unexpected exception condition?  I think `BUG()' would be too big a
> hammer here.  Or wouldn't it?

Hell no. NEVER EVER BUG().

The only case to use BUG() is if there is some core data structure
(say, kernel stack) that is so corrupted that you know you cannot
continue. That's the *only* valid use.

If this is a "this condition cannot happen" issue, then just remove
the damn conditional. It's pointless. Adding a BUG() to show "this
cannot happen" is not acceptable.

                        Linus

[toc] | [prev] | [next] | [standalone]

#1705824

On Mon, Aug 07, 2017 at 10:41:39AM -0700, Linus Torvalds wrote:

> On Mon, Aug 7, 2017 at 9:18 AM, Maciej W. Rozycki <macro@imgtec.com> wrote:
> >
> >  So what would be the right value of `si_code' to use here for such an
> > unexpected exception condition?  I think `BUG()' would be too big a
> > hammer here.  Or wouldn't it?
> 
> Hell no. NEVER EVER BUG().
> 
> The only case to use BUG() is if there is some core data structure
> (say, kernel stack) that is so corrupted that you know you cannot
> continue. That's the *only* valid use.
> 
> If this is a "this condition cannot happen" issue, then just remove
> the damn conditional. It's pointless. Adding a BUG() to show "this
> cannot happen" is not acceptable.

I queued a patch to remove the code for 4.14.

  Ralf

[toc] | [prev] | [next] | [standalone]

#1706673 — Re: [PATCH 4/7] signal/mips: Document a conflict with SI_USER with SIGFPE

"Maciej W. Rozycki" <macro@imgtec.com> writes:

> On Tue, 18 Jul 2017, Eric W. Biederman wrote:
>
>> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
>> index b68b4d0726d3..6c9cca9c5341 100644
>> --- a/arch/mips/kernel/traps.c
>> +++ b/arch/mips/kernel/traps.c
>> @@ -735,7 +735,7 @@ void force_fcr31_sig(unsigned long fcr31, void __user *fault_addr,
>>  	else if (fcr31 & FPU_CSR_INE_X)
>>  		si.si_code = FPE_FLTRES;
>>  	else
>> -		si.si_code = __SI_FAULT;
>> +		si.si_code = FPE_FIXME;
>
>  This is an "impossible" state to reach unless your hardware is on fire.  
> One or more of the FCSR Cause bits will have been set (in `fcr31') or the 
> FPE exception would not have happened.
>
>  Of course there could be a simulator bug, or we could have breakage 
> somewhere causing `process_fpemu_return' to be called with SIGFPE and 
> inconsistent `fcr31'.  So we need to handle it somehow.
>
>  So what would be the right value of `si_code' to use here for such an 
> unexpected exception condition?  I think `BUG()' would be too big a 
> hammer here.  Or wouldn't it?

The possible solutions I can think of are:

WARN_ON_ONCE with a comment.

Add a new si_code to uapi/asm-generic/siginfo.h perhaps FPE_IMPOSSIBLE.
Like syscall numbers si_codes are cheap.

Call force_sig() instead of force_sig_info, using just a generic
si_code.

If this is truly impossible and the compiler doesn't complain just drop
the code.

Eric

[toc] | [prev] | [next] | [standalone]

#1706949

On Tue, 8 Aug 2017, Eric W. Biederman wrote:

> >  This is an "impossible" state to reach unless your hardware is on fire.  
> > One or more of the FCSR Cause bits will have been set (in `fcr31') or the 
> > FPE exception would not have happened.
> >
> >  Of course there could be a simulator bug, or we could have breakage 
> > somewhere causing `process_fpemu_return' to be called with SIGFPE and 
> > inconsistent `fcr31'.  So we need to handle it somehow.
> >
> >  So what would be the right value of `si_code' to use here for such an 
> > unexpected exception condition?  I think `BUG()' would be too big a 
> > hammer here.  Or wouldn't it?
> 
> The possible solutions I can think of are:
> 
> WARN_ON_ONCE with a comment.
> 
> Add a new si_code to uapi/asm-generic/siginfo.h perhaps FPE_IMPOSSIBLE.
> Like syscall numbers si_codes are cheap.

 I think we ought to do both.

 First, we have our own FP emulation code, which is changed from time to 
time, that uses the same exit path that the hardware exception does.  It 
could happen that we miss something and return SIGFPE from the emulation 
code without setting the cause bits appropriately.  This would be our own 
bug which might trigger exceedingly rarely and could then be caught by 
WARN_ON_ONCE or otherwise stay there forever in the absence of that check.

 Second, changing `si_code' from __SI_FAULT to 0 aka __SI_KILL will likely 
interfere with `copy_siginfo_to_user32' in arch/mips/kernel/signal32.c, 
making the userland lose the address of the faulting instruction in 32-bit 
software run on 64-bit hardware only, making our API inconsistent.  Using 
a distinct `si_code' value such as FPE_IMPOSSIBLE (though we might choose 
say FPE_FLTUNK for "FLoaTing point UNKnown" instead, for consistency; mind 
that most `si_code' macros have the same number of characters within 
groups associated with individual signals) for such odd traps is allowed 
by SUS and will prevent the inconsistency from happening, very cheaply as 
you say.

  Maciej

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web