Re: [PATCH v2 23/25] powerpc/signal: Create 'unsafe' versions of copy_[ck][fpr/vsx]_to_user()

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 29/09/2020 à 04:04, Christopher M. Riedl a écrit :
> On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
>> For the non VSX version, that's trivial. Just use unsafe_copy_to_user()
>> instead of __copy_to_user().
>>
>> For the VSX version, remove the intermediate step through a buffer and
>> use unsafe_put_user() directly. This generates a far smaller code which
>> is acceptable to inline, see below:
>>
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/kernel/signal.h | 53 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 53 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
>> index f610cfafa478..2559a681536e 100644
>> --- a/arch/powerpc/kernel/signal.h
>> +++ b/arch/powerpc/kernel/signal.h
>> @@ -32,7 +32,54 @@ unsigned long copy_fpr_to_user(void __user *to,
>> struct task_struct *task);
>> unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct
>> *task);
>> unsigned long copy_fpr_from_user(struct task_struct *task, void __user
>> *from);
>> unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user
>> *from);
>> +
>> +#define unsafe_copy_fpr_to_user(to, task, label) do { \
>> + struct task_struct *__t = task; \
>> + u64 __user *buf = (u64 __user *)to; \
>> + int i; \
>> + \
>> + for (i = 0; i < ELF_NFPREG - 1 ; i++) \
>> + unsafe_put_user(__t->thread.TS_FPR(i), &buf[i], label); \
>> + unsafe_put_user(__t->thread.fp_state.fpscr, &buf[i], label); \
>> +} while (0)
>> +
> 
> I've been working on the PPC64 side of this "unsafe" rework using this
> series as a basis. One question here - I don't really understand what
> the benefit of re-implementing this logic in macros (similarly for the
> other copy_* functions below) is?

Not sure either.

The whole purpose is to not manage the error through a local var but exclusively use labels.
However, GCC is probably smart enough to understand it and drop the local var while inlining.

One important thing however is to make sure we won't end up with an outline function, otherwise you 
completely loose the benefit of the label stuff. And you get a function call inside a user access, 
which is what we want to avoid.

> 
> I am considering  a "__unsafe_copy_*" implementation in signal.c for
> each (just the original implementation w/ using the "unsafe_" variants
> of the uaccess stuff) which gets called by the "safe" functions w/ the
> appropriate "user_*_access_begin/user_*_access_end". Something like
> (pseudo-ish code):

Good idea, however ...

> 
> 	/* signal.c */
> 	unsigned long __unsafe_copy_fpr_to_user(...)
> 	{
> 		...
> 		unsafe_copy_to_user(..., bad);
> 		return 0;
> 	bad:
> 		return 1; /* -EFAULT? */
> 	}

This __unsafe_copy_fpr_to_user() has to be in signal.h and must be tagged 'static __always_inline' 
for the reasons explained above.

> 
> 	unsigned long copy_fpr_to_user(...)
> 	{
> 		unsigned long err;
> 		if (!user_write_access_begin(...))
> 			return 1; /* -EFAULT? */
> 
> 		err = __unsafe_copy_fpr_to_user(...);
> 
> 		user_write_access_end();
> 		return err;
> 	}
> 
> 	/* signal.h */
> 	unsigned long __unsafe_copy_fpr_to_user(...);
> 	#define unsafe_copy_fpr_to_user(..., label) \
> 		unsafe_op_wrap(__unsafe_copy_fpr_to_user(...), label)
> 
> This way there is a single implementation for each copy routine "body".
> The "unsafe" wrappers then just exist as simple macros similar to what
> x86 does: "unsafe_" macro wraps "__unsafe" functions for the goto label.

Yes, good point.

Christophe