[PATCH v2 1/5] powerpc/signal64: Access function descriptor with user access block

[PATCH v2 1/5] powerpc/signal64: Access function descriptor with user access block

[Christophe Leroy]

Access the function descriptor of the handler within a
user access block.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 1831bba0582e..790c450c2de8 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -936,8 +936,18 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		func_descr_t __user *funct_desc_ptr =
 			(func_descr_t __user *) ksig->ka.sa.sa_handler;
 
-		err |= get_user(regs->ctr, &funct_desc_ptr->entry);
-		err |= get_user(regs->gpr[2], &funct_desc_ptr->toc);
+		if (user_read_access_begin(funct_desc_ptr, sizeof(func_descr_t))) {
+			unsafe_get_user(regs->ctr, &funct_desc_ptr->entry, bad_funct_desc_block);
+			unsafe_get_user(regs->gpr[2], &funct_desc_ptr->toc, bad_funct_desc_block);
+		} else {
+			goto bad_funct_desc;
+bad_funct_desc_block:
+			user_read_access_end();
+bad_funct_desc:
+			signal_fault(current, regs, __func__, funct_desc_ptr);
+			return 1;
+		}
+		user_read_access_end();
 	}
 
 	/* enter the signal handler in native-endian mode */
-- 
2.25.0

[PATCH v2 2/5] powerpc/signal: Include the new stack frame inside the user access block

[Christophe Leroy]

Include the new stack frame inside the user access block and set it up
using unsafe_put_user().

On an mpc 8321 (book3s/32) the improvment is about 4% on a process
sending a signal to itself.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/signal_32.c | 29 +++++++++++++----------------
 arch/powerpc/kernel/signal_64.c | 14 +++++++-------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 0608581967f0..ff101e2b3bab 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -726,7 +726,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	struct rt_sigframe __user *frame;
 	struct mcontext __user *mctx;
 	struct mcontext __user *tm_mctx = NULL;
-	unsigned long newsp = 0;
+	unsigned long __user *newsp;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
 	/* Save the thread's msr before get_tm_stackpointer() changes it */
@@ -734,6 +734,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 
 	/* Set up Signal Frame */
 	frame = get_sigframe(ksig, tsk, sizeof(*frame), 1);
+	newsp = (unsigned long __user *)((unsigned long)frame - (__SIGNAL_FRAMESIZE + 16));
 	mctx = &frame->uc.uc_mcontext;
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	tm_mctx = &frame->uc_transact.uc_mcontext;
@@ -743,7 +744,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	else
 		prepare_save_user_regs(1);
 
-	if (!user_access_begin(frame, sizeof(*frame)))
+	if (!user_access_begin(newsp, __SIGNAL_FRAMESIZE + 16 + sizeof(*frame)))
 		goto badframe;
 
 	/* Put the siginfo & fill in most of the ucontext */
@@ -779,6 +780,9 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	}
 	unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
 
+	/* create a stack frame for the caller of the handler */
+	unsafe_put_user(regs->gpr[1], newsp, failed);
+
 	user_access_end();
 
 	if (copy_siginfo_to_user(&frame->info, &ksig->info))
@@ -790,13 +794,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	tsk->thread.fp_state.fpscr = 0;	/* turn off all fp exceptions */
 #endif
 
-	/* create a stack frame for the caller of the handler */
-	newsp = ((unsigned long)frame) - (__SIGNAL_FRAMESIZE + 16);
-	if (put_user(regs->gpr[1], (u32 __user *)newsp))
-		goto badframe;
-
 	/* Fill registers for signal handler */
-	regs->gpr[1] = newsp;
+	regs->gpr[1] = (unsigned long)newsp;
 	regs->gpr[3] = ksig->sig;
 	regs->gpr[4] = (unsigned long)&frame->info;
 	regs->gpr[5] = (unsigned long)&frame->uc;
@@ -826,7 +825,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	struct sigframe __user *frame;
 	struct mcontext __user *mctx;
 	struct mcontext __user *tm_mctx = NULL;
-	unsigned long newsp = 0;
+	unsigned long __user *newsp;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
 	/* Save the thread's msr before get_tm_stackpointer() changes it */
@@ -834,6 +833,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 
 	/* Set up Signal Frame */
 	frame = get_sigframe(ksig, tsk, sizeof(*frame), 1);
+	newsp = (unsigned long __user *)((unsigned long)frame - __SIGNAL_FRAMESIZE);
 	mctx = &frame->mctx;
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	tm_mctx = &frame->mctx_transact;
@@ -843,7 +843,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	else
 		prepare_save_user_regs(1);
 
-	if (!user_access_begin(frame, sizeof(*frame)))
+	if (!user_access_begin(newsp, __SIGNAL_FRAMESIZE + sizeof(*frame)))
 		goto badframe;
 	sc = (struct sigcontext __user *) &frame->sctx;
 
@@ -873,6 +873,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 		unsafe_put_user(PPC_RAW_SC(), &mctx->mc_pad[1], failed);
 		asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
 	}
+	/* create a stack frame for the caller of the handler */
+	unsafe_put_user(regs->gpr[1], newsp, failed);
 	user_access_end();
 
 	regs->link = tramp;
@@ -881,12 +883,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	tsk->thread.fp_state.fpscr = 0;	/* turn off all fp exceptions */
 #endif
 
-	/* create a stack frame for the caller of the handler */
-	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
-	if (put_user(regs->gpr[1], (u32 __user *)newsp))
-		goto badframe;
-
-	regs->gpr[1] = newsp;
+	regs->gpr[1] = (unsigned long)newsp;
 	regs->gpr[3] = ksig->sig;
 	regs->gpr[4] = (unsigned long) sc;
 	regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler);
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 790c450c2de8..2cca6c8febe1 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -847,13 +847,14 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		struct task_struct *tsk)
 {
 	struct rt_sigframe __user *frame;
-	unsigned long newsp = 0;
+	unsigned long __user *newsp;
 	long err = 0;
 	struct pt_regs *regs = tsk->thread.regs;
 	/* Save the thread's msr before get_tm_stackpointer() changes it */
 	unsigned long msr = regs->msr;
 
 	frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
+	newsp = (unsigned long __user *)((unsigned long)frame - __SIGNAL_FRAMESIZE);
 
 	/*
 	 * This only applies when calling unsafe_setup_sigcontext() and must be
@@ -862,7 +863,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	if (!MSR_TM_ACTIVE(msr))
 		prepare_setup_sigcontext(tsk);
 
-	if (!user_write_access_begin(frame, sizeof(*frame)))
+	if (!user_write_access_begin(newsp, __SIGNAL_FRAMESIZE + sizeof(*frame)))
 		goto badframe;
 
 	unsafe_put_user(&frame->info, &frame->pinfo, badframe_block);
@@ -900,6 +901,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	}
 
 	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
+	/* Allocate a dummy caller frame for the signal handler. */
+	unsafe_put_user(regs->gpr[1], newsp, badframe_block);
+
 	user_write_access_end();
 
 	/* Save the siginfo outside of the unsafe block. */
@@ -919,10 +923,6 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		regs_set_return_ip(regs, (unsigned long) &frame->tramp[0]);
 	}
 
-	/* Allocate a dummy caller frame for the signal handler. */
-	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
-	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
-
 	/* Set up "regs" so we "return" to the signal handler. */
 	if (is_elf2_task()) {
 		regs->ctr = (unsigned long) ksig->ka.sa.sa_handler;
@@ -952,7 +952,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 
 	/* enter the signal handler in native-endian mode */
 	regs_set_return_msr(regs, (regs->msr & ~MSR_LE) | (MSR_KERNEL & MSR_LE));
-	regs->gpr[1] = newsp;
+	regs->gpr[1] = (unsigned long)newsp;
 	regs->gpr[3] = ksig->sig;
 	regs->result = 0;
 	if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
-- 
2.25.0

[PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Christophe Leroy]

In the same spirit as commit fb05121fd6a2 ("signal: Add
unsafe_get_compat_sigset()"), implement an 'unsafe' version of
copy_siginfo_to_user() in order to use it within user access blocks.

For that, also add an 'unsafe' version of clear_user().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 include/linux/signal.h  | 15 +++++++++++++++
 include/linux/uaccess.h |  1 +
 kernel/signal.c         |  5 -----
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/linux/signal.h b/include/linux/signal.h
index 3454c7ff0778..659bd43daf10 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to,
 int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
 int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
 
+static __always_inline char __user *si_expansion(const siginfo_t __user *info)
+{
+	return ((char __user *)info) + sizeof(struct kernel_siginfo);
+}
+
+#define unsafe_copy_siginfo_to_user(to, from, label) do {		\
+	siginfo_t __user *__ucs_to = to;				\
+	const kernel_siginfo_t *__ucs_from = from;			\
+	char __user *__ucs_expansion = si_expansion(__ucs_to);		\
+									\
+	unsafe_copy_to_user(__ucs_to, __ucs_from,			\
+			    sizeof(struct kernel_siginfo), label);	\
+	unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label);	\
+} while (0)
+
 enum siginfo_layout {
 	SIL_KILL,
 	SIL_TIMER,
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index c05e903cef02..37073caac474 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count);
 #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
 #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e)
 #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e)
+#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e)
 static inline unsigned long user_access_save(void) { return 0UL; }
 static inline void user_access_restore(unsigned long flags) { }
 #endif
diff --git a/kernel/signal.c b/kernel/signal.c
index a3229add4455..83b5971e4304 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
 	return layout;
 }
 
-static inline char __user *si_expansion(const siginfo_t __user *info)
-{
-	return ((char __user *)info) + sizeof(struct kernel_siginfo);
-}
-
 int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
 {
 	char __user *expansion = si_expansion(to);
-- 
2.25.0

[PATCH v2 4/5] powerpc/uaccess: Add unsafe_clear_user()

[Christophe Leroy]

Implement unsafe_clear_user() for powerpc.
It's a copy/paste of unsafe_copy_to_user() with value 0 as source.

It may be improved in a later patch by using 'dcbz' instruction
to zeroize full cache lines at once.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/uaccess.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 22c79ab40006..962b675485ff 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -467,6 +467,26 @@ do {									\
 		unsafe_put_user(*(u8*)(_src + _i), (u8 __user *)(_dst + _i), e); \
 } while (0)
 
+#define unsafe_clear_user(d, l, e)					\
+do {									\
+	u8 __user *_dst = (u8 __user *)(d);				\
+	size_t _len = (l);						\
+	int _i;								\
+									\
+	for (_i = 0; _i < (_len & ~(sizeof(u64) - 1)); _i += sizeof(u64)) \
+		unsafe_put_user(0, (u64 __user *)(_dst + _i), e);	\
+	if (_len & 4) {							\
+		unsafe_put_user(0, (u32 __user *)(_dst + _i), e);	\
+		_i += 4;						\
+	}								\
+	if (_len & 2) {							\
+		unsafe_put_user(0, (u16 __user *)(_dst + _i), e);	\
+		_i += 2;						\
+	}								\
+	if (_len & 1)							\
+		unsafe_put_user(0, (u8 __user *)(_dst + _i), e);	\
+} while (0)
+
 #define HAVE_GET_KERNEL_NOFAULT
 
 #define __get_kernel_nofault(dst, src, type, err_label)			\
-- 
2.25.0

[PATCH v2 5/5] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Use unsafe_copy_siginfo_to_user() in order to do the copy
within the user access block.

On an mpc 8321 (book3s/32) the improvment is about 5% on a process
sending a signal to itself.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/signal_32.c | 13 ++++++-------
 arch/powerpc/kernel/signal_64.c |  5 +----
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index ff101e2b3bab..f9e16d108bc8 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -710,12 +710,6 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
 }
 #endif
 
-#ifdef CONFIG_PPC64
-
-#define copy_siginfo_to_user	copy_siginfo_to_user32
-
-#endif /* CONFIG_PPC64 */
-
 /*
  * Set up a signal frame for a "real-time" signal handler
  * (one which gets siginfo).
@@ -779,14 +773,19 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 		asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
 	}
 	unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
+#ifndef CONFIG_COMPAT
+	unsafe_copy_siginfo_to_user(&frame->info, &ksig->info, failed);
+#endif
 
 	/* create a stack frame for the caller of the handler */
 	unsafe_put_user(regs->gpr[1], newsp, failed);
 
 	user_access_end();
 
-	if (copy_siginfo_to_user(&frame->info, &ksig->info))
+#ifdef CONFIG_COMPAT
+	if (copy_siginfo_to_user32(&frame->info, &ksig->info))
 		goto badframe;
+#endif
 
 	regs->link = tramp;
 
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 2cca6c8febe1..82b73fbd937d 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -901,15 +901,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	}
 
 	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
+	unsafe_copy_siginfo_to_user(&frame->info, &ksig->info, badframe_block);
 	/* Allocate a dummy caller frame for the signal handler. */
 	unsafe_put_user(regs->gpr[1], newsp, badframe_block);
 
 	user_write_access_end();
 
-	/* Save the siginfo outside of the unsafe block. */
-	if (copy_siginfo_to_user(&frame->info, &ksig->info))
-		goto badframe;
-
 	/* Make sure signal handler doesn't get spurious FP exceptions */
 	tsk->thread.fp_state.fpscr = 0;
 
-- 
2.25.0

Re: [PATCH v2 1/5] powerpc/signal64: Access function descriptor with user access block

[Christoph Hellwig]

On Mon, Aug 23, 2021 at 03:35:51PM +0000, Christophe Leroy wrote:
> Access the function descriptor of the handler within a
> user access block.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 1831bba0582e..790c450c2de8 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -936,8 +936,18 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  		func_descr_t __user *funct_desc_ptr =
>  			(func_descr_t __user *) ksig->ka.sa.sa_handler;
>  
> -		err |= get_user(regs->ctr, &funct_desc_ptr->entry);
> -		err |= get_user(regs->gpr[2], &funct_desc_ptr->toc);
> +		if (user_read_access_begin(funct_desc_ptr, sizeof(func_descr_t))) {
> +			unsafe_get_user(regs->ctr, &funct_desc_ptr->entry, bad_funct_desc_block);
> +			unsafe_get_user(regs->gpr[2], &funct_desc_ptr->toc, bad_funct_desc_block);
> +		} else {
> +			goto bad_funct_desc;
> +bad_funct_desc_block:
> +			user_read_access_end();
> +bad_funct_desc:
> +			signal_fault(current, regs, __func__, funct_desc_ptr);
> +			return 1;
> +		}
> +		user_read_access_end();

Having a goto after an ele block, an then labels jumping into it is just
weird.

To make this somewhat readable just split it into a helper function:

static inline bool put_func_descr(func_descr_t __user *funct_desc_ptr,
		struct pt_regs *regs)
{
	if (!user_read_access_begin(funct_desc_ptr, sizeof(func_descr_t)))
		goto fault;
	unsafe_get_user(regs->ctr, &funct_desc_ptr->entry, uaccess_end);
	unsafe_get_user(regs->gpr[2], &funct_desc_ptr->toc, uaccess_end);
	user_read_access_end();
	return false;

uaccess_end:
	user_read_access_end();
fault:
	signal_fault(current, regs, __func__, funct_desc_ptr);
	return true;
}

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Christoph Hellwig]

On Mon, Aug 23, 2021 at 03:35:53PM +0000, Christophe Leroy wrote:
> In the same spirit as commit fb05121fd6a2 ("signal: Add
> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
> copy_siginfo_to_user() in order to use it within user access blocks.
> 
> For that, also add an 'unsafe' version of clear_user().

I'm a little worried about all these unsafe helper in powerpc and the
ever increasing scope of the unsafe sections.  Can you at least at
powerpc support to objtool to verify them?  objtool verifications has
helped to find quite a few bugs in unsafe sections on x86.

> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  include/linux/signal.h  | 15 +++++++++++++++
>  include/linux/uaccess.h |  1 +
>  kernel/signal.c         |  5 -----
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 3454c7ff0778..659bd43daf10 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to,
>  int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
>  int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
>  
> +static __always_inline char __user *si_expansion(const siginfo_t __user *info)
> +{
> +	return ((char __user *)info) + sizeof(struct kernel_siginfo);
> +}
> +
> +#define unsafe_copy_siginfo_to_user(to, from, label) do {		\
> +	siginfo_t __user *__ucs_to = to;				\
> +	const kernel_siginfo_t *__ucs_from = from;			\
> +	char __user *__ucs_expansion = si_expansion(__ucs_to);		\
> +									\
> +	unsafe_copy_to_user(__ucs_to, __ucs_from,			\
> +			    sizeof(struct kernel_siginfo), label);	\
> +	unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label);	\
> +} while (0)
> +
>  enum siginfo_layout {
>  	SIL_KILL,
>  	SIL_TIMER,
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index c05e903cef02..37073caac474 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count);
>  #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
>  #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e)
>  #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e)
> +#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e)
>  static inline unsigned long user_access_save(void) { return 0UL; }
>  static inline void user_access_restore(unsigned long flags) { }
>  #endif
> diff --git a/kernel/signal.c b/kernel/signal.c
> index a3229add4455..83b5971e4304 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
>  	return layout;
>  }
>  
> -static inline char __user *si_expansion(const siginfo_t __user *info)
> -{
> -	return ((char __user *)info) + sizeof(struct kernel_siginfo);
> -}
> -
>  int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
>  {
>  	char __user *expansion = si_expansion(to);
> -- 
> 2.25.0
> 
---end quoted text---

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Linus Torvalds]

On Wed, Sep 1, 2021 at 11:55 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> I'm a little worried about all these unsafe helper in powerpc and the
> ever increasing scope of the unsafe sections.  Can you at least at
> powerpc support to objtool to verify them?  objtool verifications has
> helped to find quite a few bugs in unsafe sections on x86.

.. yeah, objdump was particularly useful for the really subtle ones
where there are random function calls due to things like KASAN etc.

No human would ever have noticed "oh, we're walking the kernel stack
with user mode accesses enabled because the compiler inserted magical
debug code here". Objdump sees those things - assuming you teach it
about that special user space access enable/disable sequence.

            Linus

Re: [PATCH v2 5/5] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> Use unsafe_copy_siginfo_to_user() in order to do the copy
> within the user access block.
>
> On an mpc 8321 (book3s/32) the improvment is about 5% on a process
> sending a signal to itself.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

copy_siginfo_to_user is not the same as copy_siginfo_to_user32.

As in this patch breaks 32bit userspace on powerpc.


> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/signal_32.c | 13 ++++++-------
>  arch/powerpc/kernel/signal_64.c |  5 +----
>  2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index ff101e2b3bab..f9e16d108bc8 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -710,12 +710,6 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
>  }
>  #endif
>  
> -#ifdef CONFIG_PPC64
> -
> -#define copy_siginfo_to_user	copy_siginfo_to_user32
> -
> -#endif /* CONFIG_PPC64 */
> -
>  /*
>   * Set up a signal frame for a "real-time" signal handler
>   * (one which gets siginfo).
> @@ -779,14 +773,19 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>  		asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
>  	}
>  	unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
> +#ifndef CONFIG_COMPAT
> +	unsafe_copy_siginfo_to_user(&frame->info, &ksig->info, failed);
> +#endif
>  
>  	/* create a stack frame for the caller of the handler */
>  	unsafe_put_user(regs->gpr[1], newsp, failed);
>  
>  	user_access_end();
>  
> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
> +#ifdef CONFIG_COMPAT
> +	if (copy_siginfo_to_user32(&frame->info, &ksig->info))
>  		goto badframe;
> +#endif
>  
>  	regs->link = tramp;
>  
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 2cca6c8febe1..82b73fbd937d 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -901,15 +901,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	}
>  
>  	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
> +	unsafe_copy_siginfo_to_user(&frame->info, &ksig->info, badframe_block);
>  	/* Allocate a dummy caller frame for the signal handler. */
>  	unsafe_put_user(regs->gpr[1], newsp, badframe_block);
>  
>  	user_write_access_end();
>  
> -	/* Save the siginfo outside of the unsafe block. */
> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
> -		goto badframe;
> -
>  	/* Make sure signal handler doesn't get spurious FP exceptions */
>  	tsk->thread.fp_state.fpscr = 0;

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> In the same spirit as commit fb05121fd6a2 ("signal: Add
> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
> copy_siginfo_to_user() in order to use it within user access blocks.
>
> For that, also add an 'unsafe' version of clear_user().

Looking at your use cases you need the 32bit compat version of this
as well.

The 32bit compat version is too complicated to become a macro, so I
don't think you can make this work correctly for the 32bit compat case.

Probably-Not-by: "Eric W. Biederman" <ebiederm@xmission.com>

Eric

> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  include/linux/signal.h  | 15 +++++++++++++++
>  include/linux/uaccess.h |  1 +
>  kernel/signal.c         |  5 -----
>  3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 3454c7ff0778..659bd43daf10 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to,
>  int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
>  int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
>  
> +static __always_inline char __user *si_expansion(const siginfo_t __user *info)
> +{
> +	return ((char __user *)info) + sizeof(struct kernel_siginfo);
> +}
> +
> +#define unsafe_copy_siginfo_to_user(to, from, label) do {		\
> +	siginfo_t __user *__ucs_to = to;				\
> +	const kernel_siginfo_t *__ucs_from = from;			\
> +	char __user *__ucs_expansion = si_expansion(__ucs_to);		\
> +									\
> +	unsafe_copy_to_user(__ucs_to, __ucs_from,			\
> +			    sizeof(struct kernel_siginfo), label);	\
> +	unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label);	\
> +} while (0)
> +
>  enum siginfo_layout {
>  	SIL_KILL,
>  	SIL_TIMER,
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index c05e903cef02..37073caac474 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count);
>  #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
>  #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e)
>  #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e)
> +#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e)
>  static inline unsigned long user_access_save(void) { return 0UL; }
>  static inline void user_access_restore(unsigned long flags) { }
>  #endif
> diff --git a/kernel/signal.c b/kernel/signal.c
> index a3229add4455..83b5971e4304 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
>  	return layout;
>  }
>  
> -static inline char __user *si_expansion(const siginfo_t __user *info)
> -{
> -	return ((char __user *)info) + sizeof(struct kernel_siginfo);
> -}
> -
>  int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
>  {
>  	char __user *expansion = si_expansion(to);

Re: [PATCH v2 5/5] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 02/09/2021 à 20:38, Eric W. Biederman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> Use unsafe_copy_siginfo_to_user() in order to do the copy
>> within the user access block.
>>
>> On an mpc 8321 (book3s/32) the improvment is about 5% on a process
>> sending a signal to itself.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> copy_siginfo_to_user is not the same as copy_siginfo_to_user32.
> 
> As in this patch breaks 32bit userspace on powerpc.

I don't understand your comment. As you can see below, copy_siginfo_to_user32() is used in the 
compat case (ie 32 bit userspace on PPC64) and unsafe_copy_siginfo_to_user() is used on the 
non-compat case (ie 32 bit userspace on PPC32).

So what's the issue really ?

> 
> 
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/kernel/signal_32.c | 13 ++++++-------
>>   arch/powerpc/kernel/signal_64.c |  5 +----
>>   2 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>> index ff101e2b3bab..f9e16d108bc8 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -710,12 +710,6 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
>>   }
>>   #endif
>>   
>> -#ifdef CONFIG_PPC64
>> -
>> -#define copy_siginfo_to_user	copy_siginfo_to_user32
>> -
>> -#endif /* CONFIG_PPC64 */
>> -
>>   /*
>>    * Set up a signal frame for a "real-time" signal handler
>>    * (one which gets siginfo).
>> @@ -779,14 +773,19 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>>   		asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
>>   	}
>>   	unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
>> +#ifndef CONFIG_COMPAT
>> +	unsafe_copy_siginfo_to_user(&frame->info, &ksig->info, failed);
>> +#endif
>>   
>>   	/* create a stack frame for the caller of the handler */
>>   	unsafe_put_user(regs->gpr[1], newsp, failed);
>>   
>>   	user_access_end();
>>   
>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>> +#ifdef CONFIG_COMPAT
>> +	if (copy_siginfo_to_user32(&frame->info, &ksig->info))
>>   		goto badframe;
>> +#endif
>>   
>>   	regs->link = tramp;
>>   
>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>> index 2cca6c8febe1..82b73fbd937d 100644
>> --- a/arch/powerpc/kernel/signal_64.c
>> +++ b/arch/powerpc/kernel/signal_64.c
>> @@ -901,15 +901,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>   	}
>>   
>>   	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
>> +	unsafe_copy_siginfo_to_user(&frame->info, &ksig->info, badframe_block);
>>   	/* Allocate a dummy caller frame for the signal handler. */
>>   	unsafe_put_user(regs->gpr[1], newsp, badframe_block);
>>   
>>   	user_write_access_end();
>>   
>> -	/* Save the siginfo outside of the unsafe block. */
>> -	if (copy_siginfo_to_user(&frame->info, &ksig->info))
>> -		goto badframe;
>> -
>>   	/* Make sure signal handler doesn't get spurious FP exceptions */
>>   	tsk->thread.fp_state.fpscr = 0;

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> In the same spirit as commit fb05121fd6a2 ("signal: Add
>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
>> copy_siginfo_to_user() in order to use it within user access blocks.
>>
>> For that, also add an 'unsafe' version of clear_user().
> 
> Looking at your use cases you need the 32bit compat version of this
> as well.
> 
> The 32bit compat version is too complicated to become a macro, so I
> don't think you can make this work correctly for the 32bit compat case.

When looking into patch 5/5 that you nacked, I think you missed the fact that we keep using 
copy_siginfo_to_user32() as it for the 32 bit compat case.

> 
> Probably-Not-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> Eric
> 
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   include/linux/signal.h  | 15 +++++++++++++++
>>   include/linux/uaccess.h |  1 +
>>   kernel/signal.c         |  5 -----
>>   3 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/signal.h b/include/linux/signal.h
>> index 3454c7ff0778..659bd43daf10 100644
>> --- a/include/linux/signal.h
>> +++ b/include/linux/signal.h
>> @@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to,
>>   int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
>>   int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
>>   
>> +static __always_inline char __user *si_expansion(const siginfo_t __user *info)
>> +{
>> +	return ((char __user *)info) + sizeof(struct kernel_siginfo);
>> +}
>> +
>> +#define unsafe_copy_siginfo_to_user(to, from, label) do {		\
>> +	siginfo_t __user *__ucs_to = to;				\
>> +	const kernel_siginfo_t *__ucs_from = from;			\
>> +	char __user *__ucs_expansion = si_expansion(__ucs_to);		\
>> +									\
>> +	unsafe_copy_to_user(__ucs_to, __ucs_from,			\
>> +			    sizeof(struct kernel_siginfo), label);	\
>> +	unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label);	\
>> +} while (0)
>> +
>>   enum siginfo_layout {
>>   	SIL_KILL,
>>   	SIL_TIMER,
>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
>> index c05e903cef02..37073caac474 100644
>> --- a/include/linux/uaccess.h
>> +++ b/include/linux/uaccess.h
>> @@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count);
>>   #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
>>   #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e)
>>   #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e)
>> +#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e)
>>   static inline unsigned long user_access_save(void) { return 0UL; }
>>   static inline void user_access_restore(unsigned long flags) { }
>>   #endif
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index a3229add4455..83b5971e4304 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
>>   	return layout;
>>   }
>>   
>> -static inline char __user *si_expansion(const siginfo_t __user *info)
>> -{
>> -	return ((char __user *)info) + sizeof(struct kernel_siginfo);
>> -}
>> -
>>   int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
>>   {
>>   	char __user *expansion = si_expansion(to);

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>
>>> In the same spirit as commit fb05121fd6a2 ("signal: Add
>>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
>>> copy_siginfo_to_user() in order to use it within user access blocks.
>>>
>>> For that, also add an 'unsafe' version of clear_user().
>>
>> Looking at your use cases you need the 32bit compat version of this
>> as well.
>>
>> The 32bit compat version is too complicated to become a macro, so I
>> don't think you can make this work correctly for the 32bit compat case.
>
> When looking into patch 5/5 that you nacked, I think you missed the fact that we
> keep using copy_siginfo_to_user32() as it for the 32 bit compat case.

I did.  My mistake.

However that mistake was so easy I think it mirrors the comments others
have made that this looks like a maintenance hazard.

Is improving the performance of 32bit kernels interesting?
Is improving the performance of 32bit compat support interesting?

If performance one or either of those cases is interesting it looks like
we already have copy_siginfo_to_external32 the factor you would need
to build unsafe_copy_siginfo_to_user32.

So I am not going to say impossible but please make something
maintainable.  I unified all of the compat 32bit siginfo logic because
it simply did not get enough love and attention when it was implemented
per architecture.

In general I think that concern applies to this case as well.  We really
need an implementation that shares as much burden as possible with other
architectures.

Eric


>> Probably-Not-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>
>> Eric
>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>>   include/linux/signal.h  | 15 +++++++++++++++
>>>   include/linux/uaccess.h |  1 +
>>>   kernel/signal.c         |  5 -----
>>>   3 files changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/signal.h b/include/linux/signal.h
>>> index 3454c7ff0778..659bd43daf10 100644
>>> --- a/include/linux/signal.h
>>> +++ b/include/linux/signal.h
>>> @@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to,
>>>   int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
>>>   int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
>>>   +static __always_inline char __user *si_expansion(const siginfo_t __user
>>> *info)
>>> +{
>>> +	return ((char __user *)info) + sizeof(struct kernel_siginfo);
>>> +}
>>> +
>>> +#define unsafe_copy_siginfo_to_user(to, from, label) do {		\
>>> +	siginfo_t __user *__ucs_to = to;				\
>>> +	const kernel_siginfo_t *__ucs_from = from;			\
>>> +	char __user *__ucs_expansion = si_expansion(__ucs_to);		\
>>> +									\
>>> +	unsafe_copy_to_user(__ucs_to, __ucs_from,			\
>>> +			    sizeof(struct kernel_siginfo), label);	\
>>> +	unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label);	\
>>> +} while (0)
>>> +
>>>   enum siginfo_layout {
>>>   	SIL_KILL,
>>>   	SIL_TIMER,
>>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
>>> index c05e903cef02..37073caac474 100644
>>> --- a/include/linux/uaccess.h
>>> +++ b/include/linux/uaccess.h
>>> @@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count);
>>>   #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
>>>   #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e)
>>>   #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e)
>>> +#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e)
>>>   static inline unsigned long user_access_save(void) { return 0UL; }
>>>   static inline void user_access_restore(unsigned long flags) { }
>>>   #endif
>>> diff --git a/kernel/signal.c b/kernel/signal.c
>>> index a3229add4455..83b5971e4304 100644
>>> --- a/kernel/signal.c
>>> +++ b/kernel/signal.c
>>> @@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
>>>   	return layout;
>>>   }
>>>   -static inline char __user *si_expansion(const siginfo_t __user *info)
>>> -{
>>> -	return ((char __user *)info) + sizeof(struct kernel_siginfo);
>>> -}
>>> -
>>>   int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
>>>   {
>>>   	char __user *expansion = si_expansion(to);

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Christophe Leroy]

On 9/8/21 6:17 PM, Eric W. Biederman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :
>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>
>>>> In the same spirit as commit fb05121fd6a2 ("signal: Add
>>>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
>>>> copy_siginfo_to_user() in order to use it within user access blocks.
>>>>
>>>> For that, also add an 'unsafe' version of clear_user().
>>>
>>> Looking at your use cases you need the 32bit compat version of this
>>> as well.
>>>
>>> The 32bit compat version is too complicated to become a macro, so I
>>> don't think you can make this work correctly for the 32bit compat case.
>>
>> When looking into patch 5/5 that you nacked, I think you missed the fact that we
>> keep using copy_siginfo_to_user32() as it for the 32 bit compat case.
> 
> I did.  My mistake.
> 
> However that mistake was so easy I think it mirrors the comments others
> have made that this looks like a maintenance hazard.
> 
> Is improving the performance of 32bit kernels interesting?

Yes it is, and that's what this series do.

> Is improving the performance of 32bit compat support interesting?

For me this is a corner case, so I left it aside for now.

> 
> If performance one or either of those cases is interesting it looks like
> we already have copy_siginfo_to_external32 the factor you would need
> to build unsafe_copy_siginfo_to_user32.

I'm not sure I understand your saying here. What do you expect me to do 
with copy_siginfo_to_external32() ?

copy_siginfo_to_user32() is for compat only.

Native 32 bits powerpc use copy_siginfo_to_user()

> 
> So I am not going to say impossible but please make something
> maintainable.  I unified all of the compat 32bit siginfo logic because
> it simply did not get enough love and attention when it was implemented
> per architecture.

Yes, and ? I didn't do any modification to the compat case, so what you 
did remains.


> 
> In general I think that concern applies to this case as well.  We really
> need an implementation that shares as much burden as possible with other
> architectures.

I think yes, that's the reason why I made a generic 
unsafe_copy_siginfo_to_user() and didn't make a powerpc dedicated change.

Once this is merged any other architecture can use 
unsafe_copy_siginfo_to_user().

Did I miss something ?

Christophe

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> On 9/8/21 6:17 PM, Eric W. Biederman wrote:
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>
>>> Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :
>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>
>>>>> In the same spirit as commit fb05121fd6a2 ("signal: Add
>>>>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
>>>>> copy_siginfo_to_user() in order to use it within user access blocks.
>>>>>
>>>>> For that, also add an 'unsafe' version of clear_user().
>>>>
>>>> Looking at your use cases you need the 32bit compat version of this
>>>> as well.
>>>>
>>>> The 32bit compat version is too complicated to become a macro, so I
>>>> don't think you can make this work correctly for the 32bit compat case.
>>>
>>> When looking into patch 5/5 that you nacked, I think you missed the fact that we
>>> keep using copy_siginfo_to_user32() as it for the 32 bit compat case.
>>
>> I did.  My mistake.
>>
>> However that mistake was so easy I think it mirrors the comments others
>> have made that this looks like a maintenance hazard.
>>
>> Is improving the performance of 32bit kernels interesting?
>
> Yes it is, and that's what this series do.
>
>> Is improving the performance of 32bit compat support interesting?
>
> For me this is a corner case, so I left it aside for now.
>
>>
>> If performance one or either of those cases is interesting it looks like
>> we already have copy_siginfo_to_external32 the factor you would need
>> to build unsafe_copy_siginfo_to_user32.
>
> I'm not sure I understand your saying here. What do you expect me to
> do with copy_siginfo_to_external32() ?

Implement unsafe_copy_siginfo_to_user32.

> copy_siginfo_to_user32() is for compat only.
>
> Native 32 bits powerpc use copy_siginfo_to_user()

What you implemented doubles the number of test cases necessary to
compile test the 32bit ppc signal code, and makes the code noticeably
harder to follow.

Having a unsafe_copy_to_siginfo_to_user32 at least would allow the
number of test cases to remain the same as the current code.

>> So I am not going to say impossible but please make something
>> maintainable.  I unified all of the compat 32bit siginfo logic because
>> it simply did not get enough love and attention when it was implemented
>> per architecture.
>
> Yes, and ? I didn't do any modification to the compat case, so what
> you did remains.

You undid the unification between the 32bit code and the 32bit compat
code.

>> In general I think that concern applies to this case as well.  We really
>> need an implementation that shares as much burden as possible with other
>> architectures.
>
> I think yes, that's the reason why I made a generic
> unsafe_copy_siginfo_to_user() and didn't make a powerpc dedicated
> change.
>
> Once this is merged any other architecture can use
> unsafe_copy_siginfo_to_user().
>
> Did I miss something ?

Not dealing with the compat case and making the code signal stack frame
code noticeably more complicated.

If this optimization profitably applies to other architectures we need
to figure out how to implement unsafe_copy_siginfo_to_user32 or risk
making them all much worse to maintain.

Eric

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 11/09/2021 à 17:58, Eric W. Biederman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> On 9/8/21 6:17 PM, Eric W. Biederman wrote:
>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>
>>>> Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :
>>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>>>
>>>>>> In the same spirit as commit fb05121fd6a2 ("signal: Add
>>>>>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
>>>>>> copy_siginfo_to_user() in order to use it within user access blocks.
>>>>>>
>>>>>> For that, also add an 'unsafe' version of clear_user().
>>>>>
>>>>> Looking at your use cases you need the 32bit compat version of this
>>>>> as well.
>>>>>
>>>>> The 32bit compat version is too complicated to become a macro, so I
>>>>> don't think you can make this work correctly for the 32bit compat case.
>>>>
>>>> When looking into patch 5/5 that you nacked, I think you missed the fact that we
>>>> keep using copy_siginfo_to_user32() as it for the 32 bit compat case.
>>>
>>> I did.  My mistake.
>>>
>>> However that mistake was so easy I think it mirrors the comments others
>>> have made that this looks like a maintenance hazard.
>>>
>>> Is improving the performance of 32bit kernels interesting?
>>
>> Yes it is, and that's what this series do.
>>
>>> Is improving the performance of 32bit compat support interesting?
>>
>> For me this is a corner case, so I left it aside for now.
>>
>>>
>>> If performance one or either of those cases is interesting it looks like
>>> we already have copy_siginfo_to_external32 the factor you would need
>>> to build unsafe_copy_siginfo_to_user32.
>>
>> I'm not sure I understand your saying here. What do you expect me to
>> do with copy_siginfo_to_external32() ?
> 
> Implement unsafe_copy_siginfo_to_user32.

Ok, initialy I thought it would be a too big job but finaly that's not 
so big.


> 
>> copy_siginfo_to_user32() is for compat only.
>>
>> Native 32 bits powerpc use copy_siginfo_to_user()
> 
> What you implemented doubles the number of test cases necessary to
> compile test the 32bit ppc signal code, and makes the code noticeably
> harder to follow.

Yes and no.

We already have a different copy_siginfo_to_user() for compat and for 
native, why would anything be doubled ?

I agree it makes the code harder to follow though

> 
> Having a unsafe_copy_to_siginfo_to_user32 at least would allow the
> number of test cases to remain the same as the current code.

Not sure I follow you here, but regardless I have sent a v3 which 
tentatively implements copy_siginfo_to_user32() for the compat case.

> 
>>> So I am not going to say impossible but please make something
>>> maintainable.  I unified all of the compat 32bit siginfo logic because
>>> it simply did not get enough love and attention when it was implemented
>>> per architecture.
>>
>> Yes, and ? I didn't do any modification to the compat case, so what
>> you did remains.
> 
> You undid the unification between the 32bit code and the 32bit compat
> code.
> 
>>> In general I think that concern applies to this case as well.  We really
>>> need an implementation that shares as much burden as possible with other
>>> architectures.
>>
>> I think yes, that's the reason why I made a generic
>> unsafe_copy_siginfo_to_user() and didn't make a powerpc dedicated
>> change.
>>
>> Once this is merged any other architecture can use
>> unsafe_copy_siginfo_to_user().
>>
>> Did I miss something ?
> 
> Not dealing with the compat case and making the code signal stack frame
> code noticeably more complicated.
> 
> If this optimization profitably applies to other architectures we need
> to figure out how to implement unsafe_copy_siginfo_to_user32 or risk
> making them all much worse to maintain.
> 



Ok, let's see what you think about v3.

Thanks for you feedback
Christophe

Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 02/09/2021 à 08:54, Christoph Hellwig a écrit :
> On Mon, Aug 23, 2021 at 03:35:53PM +0000, Christophe Leroy wrote:
>> In the same spirit as commit fb05121fd6a2 ("signal: Add
>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
>> copy_siginfo_to_user() in order to use it within user access blocks.
>>
>> For that, also add an 'unsafe' version of clear_user().
> 
> I'm a little worried about all these unsafe helper in powerpc and the
> ever increasing scope of the unsafe sections.  Can you at least at
> powerpc support to objtool to verify them?  objtool verifications has
> helped to find quite a few bugs in unsafe sections on x86.

Ok, I've started looking at it, I have not found any work at all on 
objtool for powerpc. I'll see if I can draft something from the ARM64 
tentatives.

Christophe