RE: [PATCH v4 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

["Christopher M. Riedl" <cmr@codefail.de>]

On Mon Feb 1, 2021 at 10:15 AM CST, David Laight wrote:
> From: Christopher M. Riedl
> > Sent: 01 February 2021 15:56
> > 
> > On Thu Jan 28, 2021 at 4:38 AM CST, David Laight wrote:
> > > From: Christopher M. Riedl
> > > > Sent: 28 January 2021 04:04
> > > >
> > > > Reuse the "safe" implementation from signal.c except for calling
> > > > unsafe_copy_from_user() to copy into a local buffer.
> > > >
> > > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > > > ---
> > > >  arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
> > > >  1 file changed, 33 insertions(+)
> > > >
> > > > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> > > > index 2559a681536e..c18402d625f1 100644
> > > > --- a/arch/powerpc/kernel/signal.h
> > > > +++ b/arch/powerpc/kernel/signal.h
> > > > @@ -53,6 +53,33 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > > >  				&buf[i], label);\
> > > >  } while (0)
> > > >
> > > > +#define unsafe_copy_fpr_from_user(task, from, label)	do {		\
> > > > +	struct task_struct *__t = task;					\
> > > > +	u64 __user *__f = (u64 __user *)from;				\
> > > > +	u64 buf[ELF_NFPREG];						\
> > >
> > > How big is that buffer?
> > > Isn't is likely to be reasonably large compared to a reasonable
> > > kernel stack frame.
> > > Especially since this isn't even a leaf function.
> > >
> > 
> > I think Christophe answered this - I don't really have an opinion either
> > way. What would be a 'reasonable' kernel stack frame for reference?
>
> Zero :-)
>

Hehe good point!

> > 
> > > > +	int i;								\
> > > > +									\
> > > > +	unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double),	\
> > > > +				label);					\
> > > > +	for (i = 0; i < ELF_NFPREG - 1; i++)				\
> > > > +		__t->thread.TS_FPR(i) = buf[i];				\
> > > > +	__t->thread.fp_state.fpscr = buf[i];				\
> > > > +} while (0)
>
> On further reflection, since you immediately loop through the buffer
> why not just use user_access_begin() and unsafe_get_user() in the loop.

Christophe had suggested this a few revisions ago as well. When I tried
this approach, the signal handling performance took a pretty big hit:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-October/219351.html

I included some numbers on v3 as well but decided to drop the approach
altogether for this one since it just didn't seem worth the hit.

>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)