[REVIEW][PATCH 14/22] signal/riscv: Use force_sig_fault where appropriate

[REVIEW][PATCH 14/22] signal/riscv: Use force_sig_fault where appropriate

[Eric W. Biederman]

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: Palmer Dabbelt <palmer@sifive.com>
Cc: Albert Ou <albert@sifive.com>
Cc: linux-riscv at lists.infradead.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/riscv/kernel/traps.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 48aa6471cede..3087940008f4 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -66,14 +66,7 @@ void die(struct pt_regs *regs, const char *str)
 static inline void do_trap_siginfo(int signo, int code,
 	unsigned long addr, struct task_struct *tsk)
 {
-	siginfo_t info;
-
-	clear_siginfo(&info);
-	info.si_signo = signo;
-	info.si_errno = 0;
-	info.si_code = code;
-	info.si_addr = (void __user *)addr;
-	force_sig_info(signo, &info, tsk);
+	force_sig_fault(signo, code, (void __user *)addr, tsk);
 }
 
 void do_trap(struct pt_regs *regs, int signo, int code,
-- 
2.14.1

[REVIEW][PATCH 14/22] signal/riscv: Use force_sig_fault where appropriate

[Christoph Hellwig]

On Fri, Apr 20, 2018 at 09:38:03AM -0500, 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: Palmer Dabbelt <palmer@sifive.com>
> Cc: Albert Ou <albert@sifive.com>
> Cc: linux-riscv at lists.infradead.org
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  arch/riscv/kernel/traps.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 48aa6471cede..3087940008f4 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -66,14 +66,7 @@ void die(struct pt_regs *regs, const char *str)
>  static inline void do_trap_siginfo(int signo, int code,
>  	unsigned long addr, struct task_struct *tsk)
>  {
> -	siginfo_t info;
> -
> -	clear_siginfo(&info);
> -	info.si_signo = signo;
> -	info.si_errno = 0;
> -	info.si_code = code;
> -	info.si_addr = (void __user *)addr;
> -	force_sig_info(signo, &info, tsk);
> +	force_sig_fault(signo, code, (void __user *)addr, tsk);
>  }

Please kill the do_trap_siginfo helper and use force_sig_fault
directly in both callers.

[REVIEW][PATCH 23/22] signal/riscv: Replace do_trap_siginfo with force_sig_fault

[Eric W. Biederman]

The function force_sig_fault is just the generic version of
do_trap_siginfo with a (void __user *) instead of an unsigned long
parameter for the address.

So just use force_sig_fault to simplify the code.

Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Albert Ou <albert@sifive.com>
Cc: linux-riscv at lists.infradead.org
Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/riscv/kernel/traps.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 3087940008f4..b99d9dd21fd0 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -63,12 +63,6 @@ void die(struct pt_regs *regs, const char *str)
 		do_exit(SIGSEGV);
 }
 
-static inline void do_trap_siginfo(int signo, int code,
-	unsigned long addr, struct task_struct *tsk)
-{
-	force_sig_fault(signo, code, (void __user *)addr, tsk);
-}
-
 void do_trap(struct pt_regs *regs, int signo, int code,
 	unsigned long addr, struct task_struct *tsk)
 {
@@ -81,7 +75,7 @@ void do_trap(struct pt_regs *regs, int signo, int code,
 		show_regs(regs);
 	}
 
-	do_trap_siginfo(signo, code, addr, tsk);
+	force_sig_fault(signo, code, (void __user *)addr, tsk);
 }
 
 static void do_trap_error(struct pt_regs *regs, int signo, int code,
@@ -143,7 +137,7 @@ asmlinkage void do_trap_break(struct pt_regs *regs)
 	}
 #endif /* CONFIG_GENERIC_BUG */
 
-	do_trap_siginfo(SIGTRAP, TRAP_BRKPT, regs->sepc, current);
+	force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)(regs->sepc), current);
 	regs->sepc += 0x4;
 }
 
-- 
2.14.1

[REVIEW][PATCH 14/22] signal/riscv: Use force_sig_fault where appropriate

[Palmer Dabbelt]

On Fri, 20 Apr 2018 07:38:03 PDT (-0700), ebiederm at xmission.com 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: Palmer Dabbelt <palmer@sifive.com>
> Cc: Albert Ou <albert@sifive.com>
> Cc: linux-riscv at lists.infradead.org
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  arch/riscv/kernel/traps.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 48aa6471cede..3087940008f4 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -66,14 +66,7 @@ void die(struct pt_regs *regs, const char *str)
>  static inline void do_trap_siginfo(int signo, int code,
>  	unsigned long addr, struct task_struct *tsk)
>  {
> -	siginfo_t info;
> -
> -	clear_siginfo(&info);
> -	info.si_signo = signo;
> -	info.si_errno = 0;
> -	info.si_code = code;
> -	info.si_addr = (void __user *)addr;
> -	force_sig_info(signo, &info, tsk);
> +	force_sig_fault(signo, code, (void __user *)addr, tsk);
>  }
>
>  void do_trap(struct pt_regs *regs, int signo, int code,

If I understand this correctly, any change in behavior this causes would have 
been a bug on our end not filling out siginfo correctly?  In that case then 
feel free to add an

    Acked-by: Palmer Dabbelt <palmer@sifive.com>

Thanks!

[REVIEW][PATCH 14/22] signal/riscv: Use force_sig_fault where appropriate

[Eric W. Biederman]

Palmer Dabbelt <palmer@sifive.com> writes:

> On Fri, 20 Apr 2018 07:38:03 PDT (-0700), ebiederm at xmission.com 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: Palmer Dabbelt <palmer@sifive.com>
>> Cc: Albert Ou <albert@sifive.com>
>> Cc: linux-riscv at lists.infradead.org
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  arch/riscv/kernel/traps.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index 48aa6471cede..3087940008f4 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -66,14 +66,7 @@ void die(struct pt_regs *regs, const char *str)
>>  static inline void do_trap_siginfo(int signo, int code,
>>  	unsigned long addr, struct task_struct *tsk)
>>  {
>> -	siginfo_t info;
>> -
>> -	clear_siginfo(&info);
>> -	info.si_signo = signo;
>> -	info.si_errno = 0;
>> -	info.si_code = code;
>> -	info.si_addr = (void __user *)addr;
>> -	force_sig_info(signo, &info, tsk);
>> +	force_sig_fault(signo, code, (void __user *)addr, tsk);
>>  }
>>
>>  void do_trap(struct pt_regs *regs, int signo, int code,
>
> If I understand this correctly, any change in behavior this causes would have
> been a bug on our end not filling out siginfo correctly?  In that case then feel
> free to add an

If this change above causes any user visible changes it is a bug.
As Christoph rightly pointed out force_sig_fault is simply the arch
generic version of do_trap_siginfo. 

Eric