Re: [PATCH 23/25] powerpc: Deliver SEGV signal on pkey violation

[Ram Pai]

On Tue, Oct 24, 2017 at 05:46:38PM +0200, Michael Ellerman wrote:
> Ram Pai <linux...@us.ibm.com> writes:
> 
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index ec74e20..f2a310d 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -265,6 +266,15 @@ void user_single_step_siginfo(struct task_struct *tsk,
> >     info->si_addr = (void __user *)regs->nip;
> >  }
> >  
> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > +static void fill_sig_info_pkey(int si_code, siginfo_t *info, unsigned long 
> > addr)
> > +{
> > +   if (info->si_signo != SIGSEGV || si_code != SEGV_PKUERR)
> 
> Just checking si_code is sufficient there I think.
> 
> > +           return;
> > +   info->si_pkey = get_paca()->paca_pkey;
> > +}
> > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> 
> This should define an empty version in the #else case, so we don't need
> the ifdef below.
> 
> > @@ -292,6 +302,18 @@ void _exception(int signr, struct pt_regs *regs, int 
> > code, unsigned long addr)
> >     info.si_signo = signr;
> >     info.si_code = code;
> >     info.si_addr = (void __user *) addr;
> > +
> > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > +   /*
> > +    * update the thread's pkey related fields.
> > +    * core-dump handlers and other sub-systems
> > +    * depend on those values.
> > +    */
> > +   thread_pkey_regs_save(&current->thread);
> 
> You shouldn't need to do this.
> 
> We're not putting any of the pkey regs in the signal frame, so you don't
> need to save before we do that. [And if you did the right place to do it
> would be in setup_sigcontext() (or the TM version).]

One of the comments in the past was the ability to capture AMR
register status in core-dumps and also the ability to modify the
AMR through ptrace. Thiago wrote a patch towards that. That patch
is one of the subsequent patches sent in this series.

I will move the code to pkey_get().

> 
> For ptrace and coredumps it should happen in pkey_get(), see eg.
> fpr_get() which does flush_fp_to_thread() as an example.

Ok.

> 
> > +   /* update the violated-key value */
> > +   fill_sig_info_pkey(code, &info, addr);
> > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> 
> > +
> >     force_sig_info(signr, &info, current);
> >  }
> 
> cheers

-- 
Ram Pai