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:54 AM CST, Gabriel Paubert wrote:
> On Mon, Feb 01, 2021 at 09:55:44AM -0600, Christopher M. Riedl wrote:
> > 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?
>
> See include/linux/poll.h, where the limit is of the order of 800 bytes
> and the number of entries in an on stack array is chosen at compile time
> (different between 32 and 64 bit for example).
>
> The values are used in do_sys_poll, which, with almost 1000 bytes of
> stack footprint, appears close to the top of "make checkstack". In
> addition do_sys_poll has to call the ->poll function of every file
> descriptor in its table, so it is not a tail function.
>
> This 264 bytes array looks reasonable, but please use 'make checkstack'
> to verify that the function's total stack usage stays within reason.

Neat, looks like total usage is a bit larger but still reasonable and
less than half of 800B:

0xc000000000017e900 __unsafe_restore_sigcontext.constprop.0 [vmlinux]:352

Thanks for the tip!

>
> Gabriel
>
> > 
> > > > +	int i;								\
> > > > +									\
> > > > +	unsafe_copy_from_user(buf, __f, ELF_NFPREG * sizeof(double),	\
> > >
> > > That really ought to be sizeof(buf).
> > >
> > 
> > Agreed, I will fix this. Thanks!
> > 
> > > David
> > >
> > >
> > > > +				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)
> > > > +
> > > > +#define unsafe_copy_vsx_from_user(task, from, label)	do {		\
> > > > +	struct task_struct *__t = task;					\
> > > > +	u64 __user *__f = (u64 __user *)from;				\
> > > > +	u64 buf[ELF_NVSRHALFREG];					\
> > > > +	int i;								\
> > > > +									\
> > > > +	unsafe_copy_from_user(buf, __f,					\
> > > > +				ELF_NVSRHALFREG * sizeof(double),	\
> > > > +				label);					\
> > > > +	for (i = 0; i < ELF_NVSRHALFREG ; i++)				\
> > > > +		__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];	\
> > > > +} while (0)
> > > > +
> > > > +
> > > >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > > >  #define unsafe_copy_ckfpr_to_user(to, task, label)	do {		\
> > > >  	struct task_struct *__t = task;					\
> > > > @@ -80,6 +107,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
> > > >  	unsafe_copy_to_user(to, (task)->thread.fp_state.fpr,	\
> > > >  			    ELF_NFPREG * sizeof(double), label)
> > > > 
> > > > +#define unsafe_copy_fpr_from_user(task, from, label)			\
> > > > +	unsafe_copy_from_user((task)->thread.fp_state.fpr, from,	\
> > > > +			    ELF_NFPREG * sizeof(double), label)
> > > > +
> > > >  static inline unsigned long
> > > >  copy_fpr_to_user(void __user *to, struct task_struct *task)
> > > >  {
> > > > @@ -115,6 +146,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
> > > >  #else
> > > >  #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
> > > > 
> > > > +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0)
> > > > +
> > > >  static inline unsigned long
> > > >  copy_fpr_to_user(void __user *to, struct task_struct *task)
> > > >  {
> > > > --
> > > > 2.26.1
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> > > MK1 1PT, UK
> > > Registration No: 1397386 (Wales)
> > 
>