lkml.org 
[lkml]   [2018]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [REVIEW][PATCH 08/22] signal/mips: Use force_sig_fault where appropriate
From
Date
Hi Eric,

On 10/05/18 03:39, Eric W. Biederman wrote:
> Matt Redfearn <matt.redfearn@mips.com> writes:
>
>> Hi Eric,
>>
>> On 20/04/18 15:37, Eric W. Biederman wrote:
>>> Filling in struct siginfo before calling force_sig_info a tedious and
>>> error prone process, where once in a great while the wrong fields
>>> are filled out, and siginfo has been inconsistently cleared.
>>>
>>> Simplify this process by using the helper force_sig_fault. Which
>>> takes as a parameters all of the information it needs, ensures
>>> all of the fiddly bits of filling in struct siginfo are done properly
>>> and then calls force_sig_info.
>>>
>>> In short about a 5 line reduction in code for every time force_sig_info
>>> is called, which makes the calling function clearer.
>>>
>>> Cc: Ralf Baechle <ralf@linux-mips.org>
>>> Cc: James Hogan <jhogan@kernel.org>
>>> Cc: linux-mips@linux-mips.org
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>> ---
>>> arch/mips/kernel/traps.c | 65 ++++++++++++++----------------------------------
>>> arch/mips/mm/fault.c | 19 ++++----------
>>> 2 files changed, 23 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
>>> index 967e9e4e795e..66ec4b0b484d 100644
>>> --- a/arch/mips/kernel/traps.c
>>> +++ b/arch/mips/kernel/traps.c
>>> @@ -699,17 +699,11 @@ static int simulate_sync(struct pt_regs *regs, unsigned int opcode)
>>> asmlinkage void do_ov(struct pt_regs *regs)
>>> {
>>> enum ctx_state prev_state;
>>> - siginfo_t info;
>>> -
>>> - clear_siginfo(&info);
>>> - info.si_signo = SIGFPE;
>>> - info.si_code = FPE_INTOVF;
>>> - info.si_addr = (void __user *)regs->cp0_epc;
>>> prev_state = exception_enter();
>>> die_if_kernel("Integer overflow", regs);
>>> - force_sig_info(SIGFPE, &info, current);
>>> + force_sig_fault(SIGFPE, FPE_INTOVF, (void __user *)regs->cp0_epc, current);
>>> exception_exit(prev_state);
>>> }
>>> @@ -722,32 +716,27 @@ asmlinkage void do_ov(struct pt_regs *regs)
>>> void force_fcr31_sig(unsigned long fcr31, void __user *fault_addr,
>>> struct task_struct *tsk)
>>> {
>>> - struct siginfo si;
>>> -
>>> - clear_siginfo(&si);
>>> - si.si_addr = fault_addr;
>>> - si.si_signo = SIGFPE;
>>> + int si_code;
>>
>> This is giving build errors in Linux next
>> (https://storage.kernelci.org/next/master/next-20180509/mips/defconfig+kselftest/build.log)
>>
>> si_code would have ended up as 0 before from the clear_siginfo(), but perhaps
>
> And si_code 0 is not a valid si_code to use with a floating point
> siginfo layout.
>
>> int si_code = FPE_FLTUNK;
>>
>> Would make a more sensible default?
>
> FPE_FLTUNK would make a more sensible default.
>
> I seem to remember someone telling me that case can never happen in
> practice so I have simply not worried about it. Perhaps I am
> misremembering this.

It probably can't happen in practise - but the issue is that the kernel
doesn't even compile because -Werror=maybe-uninitialized results in a
build error since the compiler can't know that one of the branches will
definitely be taken to set si_code.

Thanks,
Matt

>
> Eric
>

\
 
 \ /
  Last update: 2018-05-10 10:03    [W:1.219 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site