From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vincent Chen Subject: Re: [PATCH] signal/nds32: More information in do_revinsn Date: Thu, 26 Apr 2018 11:02:54 +0800 Message-ID: References: <878t9ilmhv.fsf@xmission.com> <20180420010408.24438-5-ebiederm@xmission.com> <87in8fuuva.fsf_-_@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <87in8fuuva.fsf_-_@xmission.com> Sender: linux-kernel-owner@vger.kernel.org To: "Eric W. Biederman" Cc: linux-arch , Linux Kernel Mailing List , linux-api@vger.kernel.org, Vincent Chen , Greentime Hu , Arnd Bergmann List-Id: linux-api@vger.kernel.org 2018-04-26 0:13 GMT+08:00 Eric W. Biederman : > > While reviewing f6ed1ecad56f ("signal/nds32: Use force_sig(SIGILL) in do_revisn") > Vincent Chen asked if it was possible to provide more information in do_revinsn > with force_sig_fault. > > That seems reasonable and the appropirate si_code appears to be > ILL_ILLOPC (illegal opcode) as the printk indicates this code path > is triggered when a reserved instruction is exectured. > I think error code is set as ILL_ILLOPC is a good choice for normal reserved instruction but it seems a little bit inappropriate for nds32. For nds32 reserved exception, It is possibly triggered by illegal opcode, operand and immediate. Unfortunately, do_revisn() can not identify the actual reason now. Therefore, I chose ILLTRAP to avoid confusing user when the reserved exception is cauesd by wrong immediate or operand. > So update do_revinsn to use force_sig_fault(SIGILL, ILL_ILLOPC, ...). > Giving userspace a much better experience when soemone attempts > to execute a reserved instruction. > > Cc: Greentime Hu > Cc: Vincent Chen > Suggested-by: Vincent Chen > Signed-off-by: "Eric W. Biederman" > --- > > Vincent I have updated this from your suggestion to use > ILL_ILLOPC as that appears the more appropriate si_code. > Normally I expect you could just update your nds32 tree and > make this kind of change but since I am touching this code anyway I will > be happy to take this change along with the others. > > Does this look good to you? > It is fine to commit this change along with the others. Vincent Chen > arch/nds32/kernel/traps.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c > index a6205fd4db52..5c2f61835ab9 100644 > --- a/arch/nds32/kernel/traps.c > +++ b/arch/nds32/kernel/traps.c > @@ -349,7 +349,8 @@ void do_revinsn(struct pt_regs *regs) > show_regs(regs); > if (!user_mode(regs)) > do_exit(SIGILL); > - force_sig(SIGILL, current); > + force_sig_fault(SIGILL, ILL_ILLOPC, > + (void __user *)instruction_pointer(regs), current); > } > > #ifdef CONFIG_ALIGNMENT_TRAP > -- > 2.14.1 >