linux-kernel - Re: [PATCH V10 03/28] powerpc, ptrace: Enable in transaction NT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 16 Feb 2016 21:16:49 +1100
From:	Michael Ellerman <mpe@...erman.id.au>
To:	Denis Kirjanov <kda@...ux-powerpc.org>,
	Anshuman Khandual <khandual@...ux.vnet.ibm.com>
Cc:	mikey@...ling.org, james.hogan@...tec.com, avagin@...nvz.org,
	Paul.Clothier@...tec.com, davem@...emloft.net,
	peterz@...radead.org, palves@...hat.com, Ulrich.Weigand@...ibm.com,
	linux-kernel@...r.kernel.org, shuahkh@....samsung.com,
	dhowells@...hat.com, linuxppc-dev@...abs.org, kirjanov@...il.com,
	tglx@...utronix.de, oleg@...hat.com, davej@...hat.com,
	akpm@...ux-foundation.org, sukadev@...ux.vnet.ibm.com,
	emachado@...ux.vnet.ibm.com, sam.bobroff@....ibm.com
Subject: Re: [PATCH V10 03/28] powerpc, ptrace: Enable in transaction
 NT_PRFPREG ptrace requests

On Tue, 2016-02-16 at 12:09 +0300, Denis Kirjanov wrote:

> On 2/16/16, Anshuman Khandual <khandual@...ux.vnet.ibm.com> wrote:

> > This patch enables in transaction NT_PRFPREG ptrace requests.
> > The function fpr_get which gets the running value of all FPR
> > registers and the function fpr_set which sets the running
> > value of of all FPR registers work on the running set of FPR
> > registers whose location will be different if transaction is
> > active. This patch makes these functions adapt to situations
> > when the transaction is active.
> > 
> > Signed-off-by: Anshuman Khandual <khandual@...ux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/ptrace.c | 93
> > ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 89 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> > index 30a03c0..547a979 100644
> > --- a/arch/powerpc/kernel/ptrace.c
> > +++ b/arch/powerpc/kernel/ptrace.c
> > @@ -358,6 +358,29 @@ static int gpr_set(struct task_struct *target, const
> > struct user_regset *regset,
> >  	return ret;
> >  }
> > 
> > +/*
> > + * When the transaction is active, 'transact_fp' holds the current running
> > + * value of all FPR registers and 'fp_state' holds the last checkpointed
> > + * value of all FPR registers for the current transaction. When transaction
> > + * is not active 'fp_state' holds the current running state of all the FPR
> > + * registers. So this function which returns the current running values of
> > + * all the FPR registers, needs to know whether any transaction is active
> > + * or not.
> > + *
> > + * Userspace interface buffer layout:
> > + *
> > + * struct data {
> > + *	u64	fpr[32];
> > + *	u64	fpscr;
> > + * };
> > + *
> > + * There are two config options CONFIG_VSX and CONFIG_PPC_TRANSACTIONAL_MEM
> > + * which determines the final code in this function. All the combinations
> > of
> > + * these two config options are possible except the one below as
> > transactional
> > + * memory config pulls in CONFIG_VSX automatically.
> > + *
> > + *	!defined(CONFIG_VSX) && defined(CONFIG_PPC_TRANSACTIONAL_MEM)
> > + */
> >  static int fpr_get(struct task_struct *target, const struct user_regset
> > *regset,
> >  		   unsigned int pos, unsigned int count,
> >  		   void *kbuf, void __user *ubuf)
> > @@ -368,14 +391,31 @@ static int fpr_get(struct task_struct *target, const
> > struct user_regset *regset,
> >  #endif
> >  	flush_fp_to_thread(target);
> > 
> > -#ifdef CONFIG_VSX
> > +#if defined(CONFIG_VSX) && defined(CONFIG_PPC_TRANSACTIONAL_MEM)
> > +	/* copy to local buffer then write that out */
> > +	if (MSR_TM_ACTIVE(target->thread.regs->msr)) {
> > +		flush_altivec_to_thread(target);
> > +		flush_tmregs_to_thread(target);
> > +		for (i = 0; i < 32 ; i++)

> use ELF_NFPREG

> > +			buf[i] = target->thread.TS_TRANS_FPR(i);
> > +		buf[32] = target->thread.transact_fp.fpscr;

#define ELF_NFPREG	33	/* includes fpscr */

So it's not what he wants here. Unless you mean that he should use i < ELF_NFPREG - 1; ?

We already have several loops over the 32 fprs, I don't think hiding the "32"
behind a macro really buys us anything.

cheers