[PATCH RESEND v3 1/6] powerpc/signal64: Access function descriptor with user access block

[PATCH RESEND v3 1/6] 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>
---
Resending with correct date, sorry for the noise, my new PC seems to have used the fake date from Git instead of adding proper Date: from current date/time.


v3: Flatten the change to avoid nested gotos.
---
 arch/powerpc/kernel/signal_64.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 1831bba0582e..7b1cd50bc4fb 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -936,8 +936,13 @@ 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)))
+			goto badfunc;
+
+		unsafe_get_user(regs->ctr, &funct_desc_ptr->entry, badfunc_block);
+		unsafe_get_user(regs->gpr[2], &funct_desc_ptr->toc, badfunc_block);
+
+		user_read_access_end();
 	}
 
 	/* enter the signal handler in native-endian mode */
@@ -962,5 +967,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 badframe:
 	signal_fault(current, regs, "handle_rt_signal64", frame);
 
+	return 1;
+
+badfunc_block:
+	user_read_access_end();
+badfunc:
+	signal_fault(current, regs, __func__, (void __user *)ksig->ka.sa.sa_handler);
+
 	return 1;
 }
-- 
2.31.1

[PATCH RESEND v3 1/6] 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>
---
Resending with correct date, sorry for the noise, my new PC seems to have used the fake date from Git instead of adding proper Date: from current date/time.


v3: Flatten the change to avoid nested gotos.
---
 arch/powerpc/kernel/signal_64.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 1831bba0582e..7b1cd50bc4fb 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -936,8 +936,13 @@ 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)))
+			goto badfunc;
+
+		unsafe_get_user(regs->ctr, &funct_desc_ptr->entry, badfunc_block);
+		unsafe_get_user(regs->gpr[2], &funct_desc_ptr->toc, badfunc_block);
+
+		user_read_access_end();
 	}
 
 	/* enter the signal handler in native-endian mode */
@@ -962,5 +967,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 badframe:
 	signal_fault(current, regs, "handle_rt_signal64", frame);
 
+	return 1;
+
+badfunc_block:
+	user_read_access_end();
+badfunc:
+	signal_fault(current, regs, __func__, (void __user *)ksig->ka.sa.sa_handler);
+
 	return 1;
 }
-- 
2.31.1

[PATCH RESEND v3 2/6] 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 7b1cd50bc4fb..d80ff83cacb9 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;
@@ -947,7 +947,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.31.1

[PATCH RESEND v3 2/6] 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 7b1cd50bc4fb..d80ff83cacb9 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;
@@ -947,7 +947,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.31.1

[PATCH RESEND v3 3/6] 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().

This commit adds the generic fallback for unsafe_clear_user().
Architectures wanting to use unsafe_copy_siginfo_to_user() within a
user_access_begin() section have to make sure they have their
own unsafe_clear_user().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Added precision about unsafe_clear_user() in commit message.
---
 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 3f96a6374e4f..70ea7e741427 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 952741f6d0f9..23f168730b7e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3324,11 +3324,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.31.1

[PATCH RESEND v3 3/6] 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().

This commit adds the generic fallback for unsafe_clear_user().
Architectures wanting to use unsafe_copy_siginfo_to_user() within a
user_access_begin() section have to make sure they have their
own unsafe_clear_user().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Added precision about unsafe_clear_user() in commit message.
---
 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 3f96a6374e4f..70ea7e741427 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 952741f6d0f9..23f168730b7e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3324,11 +3324,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.31.1

[PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()

[Christophe Leroy]

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

To do so, we need inline version of copy_siginfo_to_external32() as we
don't want any function call inside user access blocks.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 include/linux/compat.h |  83 +++++++++++++++++++++++++++++-
 include/linux/signal.h |  58 +++++++++++++++++++++
 kernel/signal.c        | 114 +----------------------------------------
 3 files changed, 141 insertions(+), 114 deletions(-)

diff --git a/include/linux/compat.h b/include/linux/compat.h
index 8e0598c7d1d1..68823f4b86ee 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -412,6 +412,19 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
 #ifndef copy_siginfo_to_user32
 #define copy_siginfo_to_user32 __copy_siginfo_to_user32
 #endif
+
+#ifdef CONFIG_COMPAT
+#define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
+	struct compat_siginfo __user *__ucs_to = to;			\
+	const struct kernel_siginfo *__ucs_from = from;			\
+	struct compat_siginfo __ucs_new = {0};				\
+									\
+	__copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
+	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
+			    sizeof(struct compat_siginfo), label);	\
+} while (0)
+#endif
+
 int get_compat_sigevent(struct sigevent *event,
 		const struct compat_sigevent __user *u_event);
 
@@ -992,15 +1005,81 @@ static inline bool in_compat_syscall(void) { return false; }
  * appropriately converted them already.
  */
 #ifndef compat_ptr
-static inline void __user *compat_ptr(compat_uptr_t uptr)
+static __always_inline void __user *compat_ptr(compat_uptr_t uptr)
 {
 	return (void __user *)(unsigned long)uptr;
 }
 #endif
 
-static inline compat_uptr_t ptr_to_compat(void __user *uptr)
+static __always_inline compat_uptr_t ptr_to_compat(void __user *uptr)
 {
 	return (u32)(unsigned long)uptr;
 }
 
+static __always_inline void
+__copy_siginfo_to_external32(struct compat_siginfo *to,
+			     const struct kernel_siginfo *from)
+{
+	to->si_signo = from->si_signo;
+	to->si_errno = from->si_errno;
+	to->si_code  = from->si_code;
+	switch(__siginfo_layout(from->si_signo, from->si_code)) {
+	case SIL_KILL:
+		to->si_pid = from->si_pid;
+		to->si_uid = from->si_uid;
+		break;
+	case SIL_TIMER:
+		to->si_tid     = from->si_tid;
+		to->si_overrun = from->si_overrun;
+		to->si_int     = from->si_int;
+		break;
+	case SIL_POLL:
+		to->si_band = from->si_band;
+		to->si_fd   = from->si_fd;
+		break;
+	case SIL_FAULT:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		break;
+	case SIL_FAULT_TRAPNO:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_trapno = from->si_trapno;
+		break;
+	case SIL_FAULT_MCEERR:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_addr_lsb = from->si_addr_lsb;
+		break;
+	case SIL_FAULT_BNDERR:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_lower = ptr_to_compat(from->si_lower);
+		to->si_upper = ptr_to_compat(from->si_upper);
+		break;
+	case SIL_FAULT_PKUERR:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_pkey = from->si_pkey;
+		break;
+	case SIL_FAULT_PERF_EVENT:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_perf_data = from->si_perf_data;
+		to->si_perf_type = from->si_perf_type;
+		break;
+	case SIL_CHLD:
+		to->si_pid = from->si_pid;
+		to->si_uid = from->si_uid;
+		to->si_status = from->si_status;
+		to->si_utime = from->si_utime;
+		to->si_stime = from->si_stime;
+		break;
+	case SIL_RT:
+		to->si_pid = from->si_pid;
+		to->si_uid = from->si_uid;
+		to->si_int = from->si_int;
+		break;
+	case SIL_SYS:
+		to->si_call_addr = ptr_to_compat(from->si_call_addr);
+		to->si_syscall   = from->si_syscall;
+		to->si_arch      = from->si_arch;
+		break;
+	}
+}
+
 #endif /* _LINUX_COMPAT_H */
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 70ea7e741427..637260bc193d 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -65,6 +65,64 @@ enum siginfo_layout {
 	SIL_SYS,
 };
 
+static const struct {
+	unsigned char limit, layout;
+} sig_sicodes[] = {
+	[SIGILL]  = { NSIGILL,  SIL_FAULT },
+	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
+	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
+	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
+	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
+#if defined(SIGEMT)
+	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
+#endif
+	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
+	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
+	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
+};
+
+static __always_inline enum
+siginfo_layout __siginfo_layout(unsigned sig, int si_code)
+{
+	enum siginfo_layout layout = SIL_KILL;
+
+	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
+		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
+		    (si_code <= sig_sicodes[sig].limit)) {
+			layout = sig_sicodes[sig].layout;
+			/* Handle the exceptions */
+			if ((sig == SIGBUS) &&
+			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
+				layout = SIL_FAULT_MCEERR;
+			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
+				layout = SIL_FAULT_BNDERR;
+#ifdef SEGV_PKUERR
+			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
+				layout = SIL_FAULT_PKUERR;
+#endif
+			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
+				layout = SIL_FAULT_PERF_EVENT;
+			else if (IS_ENABLED(CONFIG_SPARC) &&
+				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
+				layout = SIL_FAULT_TRAPNO;
+			else if (IS_ENABLED(CONFIG_ALPHA) &&
+				 ((sig == SIGFPE) ||
+				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
+				layout = SIL_FAULT_TRAPNO;
+		}
+		else if (si_code <= NSIGPOLL)
+			layout = SIL_POLL;
+	} else {
+		if (si_code == SI_TIMER)
+			layout = SIL_TIMER;
+		else if (si_code == SI_SIGIO)
+			layout = SIL_POLL;
+		else if (si_code < 0)
+			layout = SIL_RT;
+	}
+	return layout;
+}
+
 enum siginfo_layout siginfo_layout(unsigned sig, int si_code);
 
 /*
diff --git a/kernel/signal.c b/kernel/signal.c
index 23f168730b7e..0d402bdb174e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3249,22 +3249,6 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
 }
 #endif
 
-static const struct {
-	unsigned char limit, layout;
-} sig_sicodes[] = {
-	[SIGILL]  = { NSIGILL,  SIL_FAULT },
-	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
-	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
-	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
-	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
-#if defined(SIGEMT)
-	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
-#endif
-	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
-	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
-	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
-};
-
 static bool known_siginfo_layout(unsigned sig, int si_code)
 {
 	if (si_code == SI_KERNEL)
@@ -3286,42 +3270,7 @@ static bool known_siginfo_layout(unsigned sig, int si_code)
 
 enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
 {
-	enum siginfo_layout layout = SIL_KILL;
-	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
-		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
-		    (si_code <= sig_sicodes[sig].limit)) {
-			layout = sig_sicodes[sig].layout;
-			/* Handle the exceptions */
-			if ((sig == SIGBUS) &&
-			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
-				layout = SIL_FAULT_MCEERR;
-			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
-				layout = SIL_FAULT_BNDERR;
-#ifdef SEGV_PKUERR
-			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
-				layout = SIL_FAULT_PKUERR;
-#endif
-			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
-				layout = SIL_FAULT_PERF_EVENT;
-			else if (IS_ENABLED(CONFIG_SPARC) &&
-				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
-				layout = SIL_FAULT_TRAPNO;
-			else if (IS_ENABLED(CONFIG_ALPHA) &&
-				 ((sig == SIGFPE) ||
-				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
-				layout = SIL_FAULT_TRAPNO;
-		}
-		else if (si_code <= NSIGPOLL)
-			layout = SIL_POLL;
-	} else {
-		if (si_code == SI_TIMER)
-			layout = SIL_TIMER;
-		else if (si_code == SI_SIGIO)
-			layout = SIL_POLL;
-		else if (si_code < 0)
-			layout = SIL_RT;
-	}
-	return layout;
+	return __siginfo_layout(sig, si_code);
 }
 
 int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
@@ -3389,66 +3338,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to,
 {
 	memset(to, 0, sizeof(*to));
 
-	to->si_signo = from->si_signo;
-	to->si_errno = from->si_errno;
-	to->si_code  = from->si_code;
-	switch(siginfo_layout(from->si_signo, from->si_code)) {
-	case SIL_KILL:
-		to->si_pid = from->si_pid;
-		to->si_uid = from->si_uid;
-		break;
-	case SIL_TIMER:
-		to->si_tid     = from->si_tid;
-		to->si_overrun = from->si_overrun;
-		to->si_int     = from->si_int;
-		break;
-	case SIL_POLL:
-		to->si_band = from->si_band;
-		to->si_fd   = from->si_fd;
-		break;
-	case SIL_FAULT:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		break;
-	case SIL_FAULT_TRAPNO:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_trapno = from->si_trapno;
-		break;
-	case SIL_FAULT_MCEERR:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_addr_lsb = from->si_addr_lsb;
-		break;
-	case SIL_FAULT_BNDERR:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_lower = ptr_to_compat(from->si_lower);
-		to->si_upper = ptr_to_compat(from->si_upper);
-		break;
-	case SIL_FAULT_PKUERR:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_pkey = from->si_pkey;
-		break;
-	case SIL_FAULT_PERF_EVENT:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_perf_data = from->si_perf_data;
-		to->si_perf_type = from->si_perf_type;
-		break;
-	case SIL_CHLD:
-		to->si_pid = from->si_pid;
-		to->si_uid = from->si_uid;
-		to->si_status = from->si_status;
-		to->si_utime = from->si_utime;
-		to->si_stime = from->si_stime;
-		break;
-	case SIL_RT:
-		to->si_pid = from->si_pid;
-		to->si_uid = from->si_uid;
-		to->si_int = from->si_int;
-		break;
-	case SIL_SYS:
-		to->si_call_addr = ptr_to_compat(from->si_call_addr);
-		to->si_syscall   = from->si_syscall;
-		to->si_arch      = from->si_arch;
-		break;
-	}
+	__copy_siginfo_to_external32(to, from);
 }
 
 int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
-- 
2.31.1

[PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()

[Christophe Leroy]

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

To do so, we need inline version of copy_siginfo_to_external32() as we
don't want any function call inside user access blocks.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 include/linux/compat.h |  83 +++++++++++++++++++++++++++++-
 include/linux/signal.h |  58 +++++++++++++++++++++
 kernel/signal.c        | 114 +----------------------------------------
 3 files changed, 141 insertions(+), 114 deletions(-)

diff --git a/include/linux/compat.h b/include/linux/compat.h
index 8e0598c7d1d1..68823f4b86ee 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -412,6 +412,19 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
 #ifndef copy_siginfo_to_user32
 #define copy_siginfo_to_user32 __copy_siginfo_to_user32
 #endif
+
+#ifdef CONFIG_COMPAT
+#define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
+	struct compat_siginfo __user *__ucs_to = to;			\
+	const struct kernel_siginfo *__ucs_from = from;			\
+	struct compat_siginfo __ucs_new = {0};				\
+									\
+	__copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
+	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
+			    sizeof(struct compat_siginfo), label);	\
+} while (0)
+#endif
+
 int get_compat_sigevent(struct sigevent *event,
 		const struct compat_sigevent __user *u_event);
 
@@ -992,15 +1005,81 @@ static inline bool in_compat_syscall(void) { return false; }
  * appropriately converted them already.
  */
 #ifndef compat_ptr
-static inline void __user *compat_ptr(compat_uptr_t uptr)
+static __always_inline void __user *compat_ptr(compat_uptr_t uptr)
 {
 	return (void __user *)(unsigned long)uptr;
 }
 #endif
 
-static inline compat_uptr_t ptr_to_compat(void __user *uptr)
+static __always_inline compat_uptr_t ptr_to_compat(void __user *uptr)
 {
 	return (u32)(unsigned long)uptr;
 }
 
+static __always_inline void
+__copy_siginfo_to_external32(struct compat_siginfo *to,
+			     const struct kernel_siginfo *from)
+{
+	to->si_signo = from->si_signo;
+	to->si_errno = from->si_errno;
+	to->si_code  = from->si_code;
+	switch(__siginfo_layout(from->si_signo, from->si_code)) {
+	case SIL_KILL:
+		to->si_pid = from->si_pid;
+		to->si_uid = from->si_uid;
+		break;
+	case SIL_TIMER:
+		to->si_tid     = from->si_tid;
+		to->si_overrun = from->si_overrun;
+		to->si_int     = from->si_int;
+		break;
+	case SIL_POLL:
+		to->si_band = from->si_band;
+		to->si_fd   = from->si_fd;
+		break;
+	case SIL_FAULT:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		break;
+	case SIL_FAULT_TRAPNO:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_trapno = from->si_trapno;
+		break;
+	case SIL_FAULT_MCEERR:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_addr_lsb = from->si_addr_lsb;
+		break;
+	case SIL_FAULT_BNDERR:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_lower = ptr_to_compat(from->si_lower);
+		to->si_upper = ptr_to_compat(from->si_upper);
+		break;
+	case SIL_FAULT_PKUERR:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_pkey = from->si_pkey;
+		break;
+	case SIL_FAULT_PERF_EVENT:
+		to->si_addr = ptr_to_compat(from->si_addr);
+		to->si_perf_data = from->si_perf_data;
+		to->si_perf_type = from->si_perf_type;
+		break;
+	case SIL_CHLD:
+		to->si_pid = from->si_pid;
+		to->si_uid = from->si_uid;
+		to->si_status = from->si_status;
+		to->si_utime = from->si_utime;
+		to->si_stime = from->si_stime;
+		break;
+	case SIL_RT:
+		to->si_pid = from->si_pid;
+		to->si_uid = from->si_uid;
+		to->si_int = from->si_int;
+		break;
+	case SIL_SYS:
+		to->si_call_addr = ptr_to_compat(from->si_call_addr);
+		to->si_syscall   = from->si_syscall;
+		to->si_arch      = from->si_arch;
+		break;
+	}
+}
+
 #endif /* _LINUX_COMPAT_H */
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 70ea7e741427..637260bc193d 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -65,6 +65,64 @@ enum siginfo_layout {
 	SIL_SYS,
 };
 
+static const struct {
+	unsigned char limit, layout;
+} sig_sicodes[] = {
+	[SIGILL]  = { NSIGILL,  SIL_FAULT },
+	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
+	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
+	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
+	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
+#if defined(SIGEMT)
+	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
+#endif
+	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
+	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
+	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
+};
+
+static __always_inline enum
+siginfo_layout __siginfo_layout(unsigned sig, int si_code)
+{
+	enum siginfo_layout layout = SIL_KILL;
+
+	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
+		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
+		    (si_code <= sig_sicodes[sig].limit)) {
+			layout = sig_sicodes[sig].layout;
+			/* Handle the exceptions */
+			if ((sig == SIGBUS) &&
+			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
+				layout = SIL_FAULT_MCEERR;
+			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
+				layout = SIL_FAULT_BNDERR;
+#ifdef SEGV_PKUERR
+			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
+				layout = SIL_FAULT_PKUERR;
+#endif
+			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
+				layout = SIL_FAULT_PERF_EVENT;
+			else if (IS_ENABLED(CONFIG_SPARC) &&
+				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
+				layout = SIL_FAULT_TRAPNO;
+			else if (IS_ENABLED(CONFIG_ALPHA) &&
+				 ((sig == SIGFPE) ||
+				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
+				layout = SIL_FAULT_TRAPNO;
+		}
+		else if (si_code <= NSIGPOLL)
+			layout = SIL_POLL;
+	} else {
+		if (si_code == SI_TIMER)
+			layout = SIL_TIMER;
+		else if (si_code == SI_SIGIO)
+			layout = SIL_POLL;
+		else if (si_code < 0)
+			layout = SIL_RT;
+	}
+	return layout;
+}
+
 enum siginfo_layout siginfo_layout(unsigned sig, int si_code);
 
 /*
diff --git a/kernel/signal.c b/kernel/signal.c
index 23f168730b7e..0d402bdb174e 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3249,22 +3249,6 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
 }
 #endif
 
-static const struct {
-	unsigned char limit, layout;
-} sig_sicodes[] = {
-	[SIGILL]  = { NSIGILL,  SIL_FAULT },
-	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
-	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
-	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
-	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
-#if defined(SIGEMT)
-	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
-#endif
-	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
-	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
-	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
-};
-
 static bool known_siginfo_layout(unsigned sig, int si_code)
 {
 	if (si_code == SI_KERNEL)
@@ -3286,42 +3270,7 @@ static bool known_siginfo_layout(unsigned sig, int si_code)
 
 enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
 {
-	enum siginfo_layout layout = SIL_KILL;
-	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
-		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
-		    (si_code <= sig_sicodes[sig].limit)) {
-			layout = sig_sicodes[sig].layout;
-			/* Handle the exceptions */
-			if ((sig == SIGBUS) &&
-			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
-				layout = SIL_FAULT_MCEERR;
-			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
-				layout = SIL_FAULT_BNDERR;
-#ifdef SEGV_PKUERR
-			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
-				layout = SIL_FAULT_PKUERR;
-#endif
-			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
-				layout = SIL_FAULT_PERF_EVENT;
-			else if (IS_ENABLED(CONFIG_SPARC) &&
-				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
-				layout = SIL_FAULT_TRAPNO;
-			else if (IS_ENABLED(CONFIG_ALPHA) &&
-				 ((sig == SIGFPE) ||
-				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
-				layout = SIL_FAULT_TRAPNO;
-		}
-		else if (si_code <= NSIGPOLL)
-			layout = SIL_POLL;
-	} else {
-		if (si_code == SI_TIMER)
-			layout = SIL_TIMER;
-		else if (si_code == SI_SIGIO)
-			layout = SIL_POLL;
-		else if (si_code < 0)
-			layout = SIL_RT;
-	}
-	return layout;
+	return __siginfo_layout(sig, si_code);
 }
 
 int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
@@ -3389,66 +3338,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to,
 {
 	memset(to, 0, sizeof(*to));
 
-	to->si_signo = from->si_signo;
-	to->si_errno = from->si_errno;
-	to->si_code  = from->si_code;
-	switch(siginfo_layout(from->si_signo, from->si_code)) {
-	case SIL_KILL:
-		to->si_pid = from->si_pid;
-		to->si_uid = from->si_uid;
-		break;
-	case SIL_TIMER:
-		to->si_tid     = from->si_tid;
-		to->si_overrun = from->si_overrun;
-		to->si_int     = from->si_int;
-		break;
-	case SIL_POLL:
-		to->si_band = from->si_band;
-		to->si_fd   = from->si_fd;
-		break;
-	case SIL_FAULT:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		break;
-	case SIL_FAULT_TRAPNO:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_trapno = from->si_trapno;
-		break;
-	case SIL_FAULT_MCEERR:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_addr_lsb = from->si_addr_lsb;
-		break;
-	case SIL_FAULT_BNDERR:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_lower = ptr_to_compat(from->si_lower);
-		to->si_upper = ptr_to_compat(from->si_upper);
-		break;
-	case SIL_FAULT_PKUERR:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_pkey = from->si_pkey;
-		break;
-	case SIL_FAULT_PERF_EVENT:
-		to->si_addr = ptr_to_compat(from->si_addr);
-		to->si_perf_data = from->si_perf_data;
-		to->si_perf_type = from->si_perf_type;
-		break;
-	case SIL_CHLD:
-		to->si_pid = from->si_pid;
-		to->si_uid = from->si_uid;
-		to->si_status = from->si_status;
-		to->si_utime = from->si_utime;
-		to->si_stime = from->si_stime;
-		break;
-	case SIL_RT:
-		to->si_pid = from->si_pid;
-		to->si_uid = from->si_uid;
-		to->si_int = from->si_int;
-		break;
-	case SIL_SYS:
-		to->si_call_addr = ptr_to_compat(from->si_call_addr);
-		to->si_syscall   = from->si_syscall;
-		to->si_arch      = from->si_arch;
-		break;
-	}
+	__copy_siginfo_to_external32(to, from);
 }
 
 int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
-- 
2.31.1

Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()

[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_user32() in order to use it within user access blocks.
>
> To do so, we need inline version of copy_siginfo_to_external32() as we
> don't want any function call inside user access blocks.

I don't understand.  What is wrong with?

#define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
	struct compat_siginfo __user *__ucs_to = to;			\
	const struct kernel_siginfo *__ucs_from = from;			\
	struct compat_siginfo __ucs_new;				\
									\
	copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
			    sizeof(struct compat_siginfo), label);	\
} while (0)

Your replacement of "memset(to, 0, sizeof(*to))" with
"struct compat_siginfo __ucs_new = {0}".  is actively unsafe as the
compiler is free not to initialize any holes in the structure to 0 in
the later case.

Is there something about the unsafe macros that I am not aware of that
makes it improper to actually call C functions?  Is that a requirement
for the instructions that change the user space access behavior?

From the looks of this change all that you are doing is making it so
that all of copy_siginfo_to_external32 is being inlined.  If that is not
a hard requirement of the instructions it seems like the wrong thing to
do here. copy_siginfo_to_external32 has not failures so it does not need
to be inlined so you can jump to the label.

Eric

>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  include/linux/compat.h |  83 +++++++++++++++++++++++++++++-
>  include/linux/signal.h |  58 +++++++++++++++++++++
>  kernel/signal.c        | 114 +----------------------------------------
>  3 files changed, 141 insertions(+), 114 deletions(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 8e0598c7d1d1..68823f4b86ee 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -412,6 +412,19 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
>  #ifndef copy_siginfo_to_user32
>  #define copy_siginfo_to_user32 __copy_siginfo_to_user32
>  #endif
> +
> +#ifdef CONFIG_COMPAT
> +#define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
> +	struct compat_siginfo __user *__ucs_to = to;			\
> +	const struct kernel_siginfo *__ucs_from = from;			\
> +	struct compat_siginfo __ucs_new = {0};				\
> +									\
> +	__copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
> +	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
> +			    sizeof(struct compat_siginfo), label);	\
> +} while (0)
> +#endif
> +
>  int get_compat_sigevent(struct sigevent *event,
>  		const struct compat_sigevent __user *u_event);
>  
> @@ -992,15 +1005,81 @@ static inline bool in_compat_syscall(void) { return false; }
>   * appropriately converted them already.
>   */
>  #ifndef compat_ptr
> -static inline void __user *compat_ptr(compat_uptr_t uptr)
> +static __always_inline void __user *compat_ptr(compat_uptr_t uptr)
>  {
>  	return (void __user *)(unsigned long)uptr;
>  }
>  #endif
>  
> -static inline compat_uptr_t ptr_to_compat(void __user *uptr)
> +static __always_inline compat_uptr_t ptr_to_compat(void __user *uptr)
>  {
>  	return (u32)(unsigned long)uptr;
>  }
>  
> +static __always_inline void
> +__copy_siginfo_to_external32(struct compat_siginfo *to,
> +			     const struct kernel_siginfo *from)
> +{
> +	to->si_signo = from->si_signo;
> +	to->si_errno = from->si_errno;
> +	to->si_code  = from->si_code;
> +	switch(__siginfo_layout(from->si_signo, from->si_code)) {
> +	case SIL_KILL:
> +		to->si_pid = from->si_pid;
> +		to->si_uid = from->si_uid;
> +		break;
> +	case SIL_TIMER:
> +		to->si_tid     = from->si_tid;
> +		to->si_overrun = from->si_overrun;
> +		to->si_int     = from->si_int;
> +		break;
> +	case SIL_POLL:
> +		to->si_band = from->si_band;
> +		to->si_fd   = from->si_fd;
> +		break;
> +	case SIL_FAULT:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		break;
> +	case SIL_FAULT_TRAPNO:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_trapno = from->si_trapno;
> +		break;
> +	case SIL_FAULT_MCEERR:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_addr_lsb = from->si_addr_lsb;
> +		break;
> +	case SIL_FAULT_BNDERR:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_lower = ptr_to_compat(from->si_lower);
> +		to->si_upper = ptr_to_compat(from->si_upper);
> +		break;
> +	case SIL_FAULT_PKUERR:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_pkey = from->si_pkey;
> +		break;
> +	case SIL_FAULT_PERF_EVENT:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_perf_data = from->si_perf_data;
> +		to->si_perf_type = from->si_perf_type;
> +		break;
> +	case SIL_CHLD:
> +		to->si_pid = from->si_pid;
> +		to->si_uid = from->si_uid;
> +		to->si_status = from->si_status;
> +		to->si_utime = from->si_utime;
> +		to->si_stime = from->si_stime;
> +		break;
> +	case SIL_RT:
> +		to->si_pid = from->si_pid;
> +		to->si_uid = from->si_uid;
> +		to->si_int = from->si_int;
> +		break;
> +	case SIL_SYS:
> +		to->si_call_addr = ptr_to_compat(from->si_call_addr);
> +		to->si_syscall   = from->si_syscall;
> +		to->si_arch      = from->si_arch;
> +		break;
> +	}
> +}
> +
>  #endif /* _LINUX_COMPAT_H */
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 70ea7e741427..637260bc193d 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -65,6 +65,64 @@ enum siginfo_layout {
>  	SIL_SYS,
>  };
>  
> +static const struct {
> +	unsigned char limit, layout;
> +} sig_sicodes[] = {
> +	[SIGILL]  = { NSIGILL,  SIL_FAULT },
> +	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
> +	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
> +	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
> +	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
> +#if defined(SIGEMT)
> +	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
> +#endif
> +	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
> +	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
> +	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
> +};
> +
> +static __always_inline enum
> +siginfo_layout __siginfo_layout(unsigned sig, int si_code)
> +{
> +	enum siginfo_layout layout = SIL_KILL;
> +
> +	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
> +		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
> +		    (si_code <= sig_sicodes[sig].limit)) {
> +			layout = sig_sicodes[sig].layout;
> +			/* Handle the exceptions */
> +			if ((sig == SIGBUS) &&
> +			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
> +				layout = SIL_FAULT_MCEERR;
> +			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
> +				layout = SIL_FAULT_BNDERR;
> +#ifdef SEGV_PKUERR
> +			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
> +				layout = SIL_FAULT_PKUERR;
> +#endif
> +			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
> +				layout = SIL_FAULT_PERF_EVENT;
> +			else if (IS_ENABLED(CONFIG_SPARC) &&
> +				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
> +				layout = SIL_FAULT_TRAPNO;
> +			else if (IS_ENABLED(CONFIG_ALPHA) &&
> +				 ((sig == SIGFPE) ||
> +				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
> +				layout = SIL_FAULT_TRAPNO;
> +		}
> +		else if (si_code <= NSIGPOLL)
> +			layout = SIL_POLL;
> +	} else {
> +		if (si_code == SI_TIMER)
> +			layout = SIL_TIMER;
> +		else if (si_code == SI_SIGIO)
> +			layout = SIL_POLL;
> +		else if (si_code < 0)
> +			layout = SIL_RT;
> +	}
> +	return layout;
> +}
> +
>  enum siginfo_layout siginfo_layout(unsigned sig, int si_code);
>  
>  /*
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 23f168730b7e..0d402bdb174e 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3249,22 +3249,6 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
>  }
>  #endif
>  
> -static const struct {
> -	unsigned char limit, layout;
> -} sig_sicodes[] = {
> -	[SIGILL]  = { NSIGILL,  SIL_FAULT },
> -	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
> -	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
> -	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
> -	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
> -#if defined(SIGEMT)
> -	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
> -#endif
> -	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
> -	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
> -	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
> -};
> -
>  static bool known_siginfo_layout(unsigned sig, int si_code)
>  {
>  	if (si_code == SI_KERNEL)
> @@ -3286,42 +3270,7 @@ static bool known_siginfo_layout(unsigned sig, int si_code)
>  
>  enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
>  {
> -	enum siginfo_layout layout = SIL_KILL;
> -	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
> -		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
> -		    (si_code <= sig_sicodes[sig].limit)) {
> -			layout = sig_sicodes[sig].layout;
> -			/* Handle the exceptions */
> -			if ((sig == SIGBUS) &&
> -			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
> -				layout = SIL_FAULT_MCEERR;
> -			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
> -				layout = SIL_FAULT_BNDERR;
> -#ifdef SEGV_PKUERR
> -			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
> -				layout = SIL_FAULT_PKUERR;
> -#endif
> -			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
> -				layout = SIL_FAULT_PERF_EVENT;
> -			else if (IS_ENABLED(CONFIG_SPARC) &&
> -				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
> -				layout = SIL_FAULT_TRAPNO;
> -			else if (IS_ENABLED(CONFIG_ALPHA) &&
> -				 ((sig == SIGFPE) ||
> -				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
> -				layout = SIL_FAULT_TRAPNO;
> -		}
> -		else if (si_code <= NSIGPOLL)
> -			layout = SIL_POLL;
> -	} else {
> -		if (si_code == SI_TIMER)
> -			layout = SIL_TIMER;
> -		else if (si_code == SI_SIGIO)
> -			layout = SIL_POLL;
> -		else if (si_code < 0)
> -			layout = SIL_RT;
> -	}
> -	return layout;
> +	return __siginfo_layout(sig, si_code);
>  }
>  
>  int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
> @@ -3389,66 +3338,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to,
>  {
>  	memset(to, 0, sizeof(*to));
>  
> -	to->si_signo = from->si_signo;
> -	to->si_errno = from->si_errno;
> -	to->si_code  = from->si_code;
> -	switch(siginfo_layout(from->si_signo, from->si_code)) {
> -	case SIL_KILL:
> -		to->si_pid = from->si_pid;
> -		to->si_uid = from->si_uid;
> -		break;
> -	case SIL_TIMER:
> -		to->si_tid     = from->si_tid;
> -		to->si_overrun = from->si_overrun;
> -		to->si_int     = from->si_int;
> -		break;
> -	case SIL_POLL:
> -		to->si_band = from->si_band;
> -		to->si_fd   = from->si_fd;
> -		break;
> -	case SIL_FAULT:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		break;
> -	case SIL_FAULT_TRAPNO:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_trapno = from->si_trapno;
> -		break;
> -	case SIL_FAULT_MCEERR:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_addr_lsb = from->si_addr_lsb;
> -		break;
> -	case SIL_FAULT_BNDERR:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_lower = ptr_to_compat(from->si_lower);
> -		to->si_upper = ptr_to_compat(from->si_upper);
> -		break;
> -	case SIL_FAULT_PKUERR:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_pkey = from->si_pkey;
> -		break;
> -	case SIL_FAULT_PERF_EVENT:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_perf_data = from->si_perf_data;
> -		to->si_perf_type = from->si_perf_type;
> -		break;
> -	case SIL_CHLD:
> -		to->si_pid = from->si_pid;
> -		to->si_uid = from->si_uid;
> -		to->si_status = from->si_status;
> -		to->si_utime = from->si_utime;
> -		to->si_stime = from->si_stime;
> -		break;
> -	case SIL_RT:
> -		to->si_pid = from->si_pid;
> -		to->si_uid = from->si_uid;
> -		to->si_int = from->si_int;
> -		break;
> -	case SIL_SYS:
> -		to->si_call_addr = ptr_to_compat(from->si_call_addr);
> -		to->si_syscall   = from->si_syscall;
> -		to->si_arch      = from->si_arch;
> -		break;
> -	}
> +	__copy_siginfo_to_external32(to, from);
>  }
>  
>  int __copy_siginfo_to_user32(struct compat_siginfo __user *to,

Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()

[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_user32() in order to use it within user access blocks.
>
> To do so, we need inline version of copy_siginfo_to_external32() as we
> don't want any function call inside user access blocks.

I don't understand.  What is wrong with?

#define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
	struct compat_siginfo __user *__ucs_to = to;			\
	const struct kernel_siginfo *__ucs_from = from;			\
	struct compat_siginfo __ucs_new;				\
									\
	copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
			    sizeof(struct compat_siginfo), label);	\
} while (0)

Your replacement of "memset(to, 0, sizeof(*to))" with
"struct compat_siginfo __ucs_new = {0}".  is actively unsafe as the
compiler is free not to initialize any holes in the structure to 0 in
the later case.

Is there something about the unsafe macros that I am not aware of that
makes it improper to actually call C functions?  Is that a requirement
for the instructions that change the user space access behavior?

From the looks of this change all that you are doing is making it so
that all of copy_siginfo_to_external32 is being inlined.  If that is not
a hard requirement of the instructions it seems like the wrong thing to
do here. copy_siginfo_to_external32 has not failures so it does not need
to be inlined so you can jump to the label.

Eric

>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  include/linux/compat.h |  83 +++++++++++++++++++++++++++++-
>  include/linux/signal.h |  58 +++++++++++++++++++++
>  kernel/signal.c        | 114 +----------------------------------------
>  3 files changed, 141 insertions(+), 114 deletions(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 8e0598c7d1d1..68823f4b86ee 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -412,6 +412,19 @@ int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
>  #ifndef copy_siginfo_to_user32
>  #define copy_siginfo_to_user32 __copy_siginfo_to_user32
>  #endif
> +
> +#ifdef CONFIG_COMPAT
> +#define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
> +	struct compat_siginfo __user *__ucs_to = to;			\
> +	const struct kernel_siginfo *__ucs_from = from;			\
> +	struct compat_siginfo __ucs_new = {0};				\
> +									\
> +	__copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
> +	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
> +			    sizeof(struct compat_siginfo), label);	\
> +} while (0)
> +#endif
> +
>  int get_compat_sigevent(struct sigevent *event,
>  		const struct compat_sigevent __user *u_event);
>  
> @@ -992,15 +1005,81 @@ static inline bool in_compat_syscall(void) { return false; }
>   * appropriately converted them already.
>   */
>  #ifndef compat_ptr
> -static inline void __user *compat_ptr(compat_uptr_t uptr)
> +static __always_inline void __user *compat_ptr(compat_uptr_t uptr)
>  {
>  	return (void __user *)(unsigned long)uptr;
>  }
>  #endif
>  
> -static inline compat_uptr_t ptr_to_compat(void __user *uptr)
> +static __always_inline compat_uptr_t ptr_to_compat(void __user *uptr)
>  {
>  	return (u32)(unsigned long)uptr;
>  }
>  
> +static __always_inline void
> +__copy_siginfo_to_external32(struct compat_siginfo *to,
> +			     const struct kernel_siginfo *from)
> +{
> +	to->si_signo = from->si_signo;
> +	to->si_errno = from->si_errno;
> +	to->si_code  = from->si_code;
> +	switch(__siginfo_layout(from->si_signo, from->si_code)) {
> +	case SIL_KILL:
> +		to->si_pid = from->si_pid;
> +		to->si_uid = from->si_uid;
> +		break;
> +	case SIL_TIMER:
> +		to->si_tid     = from->si_tid;
> +		to->si_overrun = from->si_overrun;
> +		to->si_int     = from->si_int;
> +		break;
> +	case SIL_POLL:
> +		to->si_band = from->si_band;
> +		to->si_fd   = from->si_fd;
> +		break;
> +	case SIL_FAULT:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		break;
> +	case SIL_FAULT_TRAPNO:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_trapno = from->si_trapno;
> +		break;
> +	case SIL_FAULT_MCEERR:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_addr_lsb = from->si_addr_lsb;
> +		break;
> +	case SIL_FAULT_BNDERR:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_lower = ptr_to_compat(from->si_lower);
> +		to->si_upper = ptr_to_compat(from->si_upper);
> +		break;
> +	case SIL_FAULT_PKUERR:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_pkey = from->si_pkey;
> +		break;
> +	case SIL_FAULT_PERF_EVENT:
> +		to->si_addr = ptr_to_compat(from->si_addr);
> +		to->si_perf_data = from->si_perf_data;
> +		to->si_perf_type = from->si_perf_type;
> +		break;
> +	case SIL_CHLD:
> +		to->si_pid = from->si_pid;
> +		to->si_uid = from->si_uid;
> +		to->si_status = from->si_status;
> +		to->si_utime = from->si_utime;
> +		to->si_stime = from->si_stime;
> +		break;
> +	case SIL_RT:
> +		to->si_pid = from->si_pid;
> +		to->si_uid = from->si_uid;
> +		to->si_int = from->si_int;
> +		break;
> +	case SIL_SYS:
> +		to->si_call_addr = ptr_to_compat(from->si_call_addr);
> +		to->si_syscall   = from->si_syscall;
> +		to->si_arch      = from->si_arch;
> +		break;
> +	}
> +}
> +
>  #endif /* _LINUX_COMPAT_H */
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index 70ea7e741427..637260bc193d 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -65,6 +65,64 @@ enum siginfo_layout {
>  	SIL_SYS,
>  };
>  
> +static const struct {
> +	unsigned char limit, layout;
> +} sig_sicodes[] = {
> +	[SIGILL]  = { NSIGILL,  SIL_FAULT },
> +	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
> +	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
> +	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
> +	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
> +#if defined(SIGEMT)
> +	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
> +#endif
> +	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
> +	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
> +	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
> +};
> +
> +static __always_inline enum
> +siginfo_layout __siginfo_layout(unsigned sig, int si_code)
> +{
> +	enum siginfo_layout layout = SIL_KILL;
> +
> +	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
> +		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
> +		    (si_code <= sig_sicodes[sig].limit)) {
> +			layout = sig_sicodes[sig].layout;
> +			/* Handle the exceptions */
> +			if ((sig == SIGBUS) &&
> +			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
> +				layout = SIL_FAULT_MCEERR;
> +			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
> +				layout = SIL_FAULT_BNDERR;
> +#ifdef SEGV_PKUERR
> +			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
> +				layout = SIL_FAULT_PKUERR;
> +#endif
> +			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
> +				layout = SIL_FAULT_PERF_EVENT;
> +			else if (IS_ENABLED(CONFIG_SPARC) &&
> +				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
> +				layout = SIL_FAULT_TRAPNO;
> +			else if (IS_ENABLED(CONFIG_ALPHA) &&
> +				 ((sig == SIGFPE) ||
> +				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
> +				layout = SIL_FAULT_TRAPNO;
> +		}
> +		else if (si_code <= NSIGPOLL)
> +			layout = SIL_POLL;
> +	} else {
> +		if (si_code == SI_TIMER)
> +			layout = SIL_TIMER;
> +		else if (si_code == SI_SIGIO)
> +			layout = SIL_POLL;
> +		else if (si_code < 0)
> +			layout = SIL_RT;
> +	}
> +	return layout;
> +}
> +
>  enum siginfo_layout siginfo_layout(unsigned sig, int si_code);
>  
>  /*
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 23f168730b7e..0d402bdb174e 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3249,22 +3249,6 @@ COMPAT_SYSCALL_DEFINE2(rt_sigpending, compat_sigset_t __user *, uset,
>  }
>  #endif
>  
> -static const struct {
> -	unsigned char limit, layout;
> -} sig_sicodes[] = {
> -	[SIGILL]  = { NSIGILL,  SIL_FAULT },
> -	[SIGFPE]  = { NSIGFPE,  SIL_FAULT },
> -	[SIGSEGV] = { NSIGSEGV, SIL_FAULT },
> -	[SIGBUS]  = { NSIGBUS,  SIL_FAULT },
> -	[SIGTRAP] = { NSIGTRAP, SIL_FAULT },
> -#if defined(SIGEMT)
> -	[SIGEMT]  = { NSIGEMT,  SIL_FAULT },
> -#endif
> -	[SIGCHLD] = { NSIGCHLD, SIL_CHLD },
> -	[SIGPOLL] = { NSIGPOLL, SIL_POLL },
> -	[SIGSYS]  = { NSIGSYS,  SIL_SYS },
> -};
> -
>  static bool known_siginfo_layout(unsigned sig, int si_code)
>  {
>  	if (si_code == SI_KERNEL)
> @@ -3286,42 +3270,7 @@ static bool known_siginfo_layout(unsigned sig, int si_code)
>  
>  enum siginfo_layout siginfo_layout(unsigned sig, int si_code)
>  {
> -	enum siginfo_layout layout = SIL_KILL;
> -	if ((si_code > SI_USER) && (si_code < SI_KERNEL)) {
> -		if ((sig < ARRAY_SIZE(sig_sicodes)) &&
> -		    (si_code <= sig_sicodes[sig].limit)) {
> -			layout = sig_sicodes[sig].layout;
> -			/* Handle the exceptions */
> -			if ((sig == SIGBUS) &&
> -			    (si_code >= BUS_MCEERR_AR) && (si_code <= BUS_MCEERR_AO))
> -				layout = SIL_FAULT_MCEERR;
> -			else if ((sig == SIGSEGV) && (si_code == SEGV_BNDERR))
> -				layout = SIL_FAULT_BNDERR;
> -#ifdef SEGV_PKUERR
> -			else if ((sig == SIGSEGV) && (si_code == SEGV_PKUERR))
> -				layout = SIL_FAULT_PKUERR;
> -#endif
> -			else if ((sig == SIGTRAP) && (si_code == TRAP_PERF))
> -				layout = SIL_FAULT_PERF_EVENT;
> -			else if (IS_ENABLED(CONFIG_SPARC) &&
> -				 (sig == SIGILL) && (si_code == ILL_ILLTRP))
> -				layout = SIL_FAULT_TRAPNO;
> -			else if (IS_ENABLED(CONFIG_ALPHA) &&
> -				 ((sig == SIGFPE) ||
> -				  ((sig == SIGTRAP) && (si_code == TRAP_UNK))))
> -				layout = SIL_FAULT_TRAPNO;
> -		}
> -		else if (si_code <= NSIGPOLL)
> -			layout = SIL_POLL;
> -	} else {
> -		if (si_code == SI_TIMER)
> -			layout = SIL_TIMER;
> -		else if (si_code == SI_SIGIO)
> -			layout = SIL_POLL;
> -		else if (si_code < 0)
> -			layout = SIL_RT;
> -	}
> -	return layout;
> +	return __siginfo_layout(sig, si_code);
>  }
>  
>  int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from)
> @@ -3389,66 +3338,7 @@ void copy_siginfo_to_external32(struct compat_siginfo *to,
>  {
>  	memset(to, 0, sizeof(*to));
>  
> -	to->si_signo = from->si_signo;
> -	to->si_errno = from->si_errno;
> -	to->si_code  = from->si_code;
> -	switch(siginfo_layout(from->si_signo, from->si_code)) {
> -	case SIL_KILL:
> -		to->si_pid = from->si_pid;
> -		to->si_uid = from->si_uid;
> -		break;
> -	case SIL_TIMER:
> -		to->si_tid     = from->si_tid;
> -		to->si_overrun = from->si_overrun;
> -		to->si_int     = from->si_int;
> -		break;
> -	case SIL_POLL:
> -		to->si_band = from->si_band;
> -		to->si_fd   = from->si_fd;
> -		break;
> -	case SIL_FAULT:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		break;
> -	case SIL_FAULT_TRAPNO:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_trapno = from->si_trapno;
> -		break;
> -	case SIL_FAULT_MCEERR:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_addr_lsb = from->si_addr_lsb;
> -		break;
> -	case SIL_FAULT_BNDERR:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_lower = ptr_to_compat(from->si_lower);
> -		to->si_upper = ptr_to_compat(from->si_upper);
> -		break;
> -	case SIL_FAULT_PKUERR:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_pkey = from->si_pkey;
> -		break;
> -	case SIL_FAULT_PERF_EVENT:
> -		to->si_addr = ptr_to_compat(from->si_addr);
> -		to->si_perf_data = from->si_perf_data;
> -		to->si_perf_type = from->si_perf_type;
> -		break;
> -	case SIL_CHLD:
> -		to->si_pid = from->si_pid;
> -		to->si_uid = from->si_uid;
> -		to->si_status = from->si_status;
> -		to->si_utime = from->si_utime;
> -		to->si_stime = from->si_stime;
> -		break;
> -	case SIL_RT:
> -		to->si_pid = from->si_pid;
> -		to->si_uid = from->si_uid;
> -		to->si_int = from->si_int;
> -		break;
> -	case SIL_SYS:
> -		to->si_call_addr = ptr_to_compat(from->si_call_addr);
> -		to->si_syscall   = from->si_syscall;
> -		to->si_arch      = from->si_arch;
> -		break;
> -	}
> +	__copy_siginfo_to_external32(to, from);
>  }
>  
>  int __copy_siginfo_to_user32(struct compat_siginfo __user *to,

Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()

[Christophe Leroy]

Le 13/09/2021 à 17:54, 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_user32() in order to use it within user access blocks.
>>
>> To do so, we need inline version of copy_siginfo_to_external32() as we
>> don't want any function call inside user access blocks.
> 
> I don't understand.  What is wrong with?
> 
> #define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
> 	struct compat_siginfo __user *__ucs_to = to;			\
> 	const struct kernel_siginfo *__ucs_from = from;			\
> 	struct compat_siginfo __ucs_new;				\
> 									\
> 	copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
> 	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
> 			    sizeof(struct compat_siginfo), label);	\
> } while (0)

As far as I understood, it is forbidden to call functions within user 
access blocks.

On powerpc it doesn't matter (yet), but as far as I understand x86 as a 
tool called "objtool" to enforce that.


> 
> Your replacement of "memset(to, 0, sizeof(*to))" with
> "struct compat_siginfo __ucs_new = {0}".  is actively unsafe as the
> compiler is free not to initialize any holes in the structure to 0 in
> the later case.

Ah ? I didn't know that.
Maybe we can make as exception for memset(). Or we can hard-code a 
zeroizing loop.

> 
> Is there something about the unsafe macros that I am not aware of that
> makes it improper to actually call C functions?  Is that a requirement
> for the instructions that change the user space access behavior?

See see 
https://lore.kernel.org/lkml/20190318155142.025214872@infradead.org/T/ ?

> 
>  From the looks of this change all that you are doing is making it so
> that all of copy_siginfo_to_external32 is being inlined.  If that is not
> a hard requirement of the instructions it seems like the wrong thing to
> do here. copy_siginfo_to_external32 has not failures so it does not need
> to be inlined so you can jump to the label.

Yes that's what I did, make sure everything is inlined. Or maybe I 
misunderstood something ?

Christophe

Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()

[Christophe Leroy]

Le 13/09/2021 à 17:54, 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_user32() in order to use it within user access blocks.
>>
>> To do so, we need inline version of copy_siginfo_to_external32() as we
>> don't want any function call inside user access blocks.
> 
> I don't understand.  What is wrong with?
> 
> #define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
> 	struct compat_siginfo __user *__ucs_to = to;			\
> 	const struct kernel_siginfo *__ucs_from = from;			\
> 	struct compat_siginfo __ucs_new;				\
> 									\
> 	copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
> 	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
> 			    sizeof(struct compat_siginfo), label);	\
> } while (0)

As far as I understood, it is forbidden to call functions within user 
access blocks.

On powerpc it doesn't matter (yet), but as far as I understand x86 as a 
tool called "objtool" to enforce that.


> 
> Your replacement of "memset(to, 0, sizeof(*to))" with
> "struct compat_siginfo __ucs_new = {0}".  is actively unsafe as the
> compiler is free not to initialize any holes in the structure to 0 in
> the later case.

Ah ? I didn't know that.
Maybe we can make as exception for memset(). Or we can hard-code a 
zeroizing loop.

> 
> Is there something about the unsafe macros that I am not aware of that
> makes it improper to actually call C functions?  Is that a requirement
> for the instructions that change the user space access behavior?

See see 
https://lore.kernel.org/lkml/20190318155142.025214872@infradead.org/T/ ?

> 
>  From the looks of this change all that you are doing is making it so
> that all of copy_siginfo_to_external32 is being inlined.  If that is not
> a hard requirement of the instructions it seems like the wrong thing to
> do here. copy_siginfo_to_external32 has not failures so it does not need
> to be inlined so you can jump to the label.

Yes that's what I did, make sure everything is inlined. Or maybe I 
misunderstood something ?

Christophe

[PATCH RESEND v3 5/6] 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.31.1

[PATCH RESEND v3 5/6] 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.31.1

[PATCH RESEND v3 6/6] 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>
---
v3: Don't leave compat aside, use the new unsafe_copy_siginfo_to_user32()
---
 arch/powerpc/kernel/signal_32.c | 8 +++-----
 arch/powerpc/kernel/signal_64.c | 5 +----
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index ff101e2b3bab..3a2db8af2d65 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -710,9 +710,9 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
 }
 #endif
 
-#ifdef CONFIG_PPC64
+#ifndef CONFIG_PPC64
 
-#define copy_siginfo_to_user	copy_siginfo_to_user32
+#define unsafe_copy_siginfo_to_user32	unsafe_copy_siginfo_to_user
 
 #endif /* CONFIG_PPC64 */
 
@@ -779,15 +779,13 @@ 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);
+	unsafe_copy_siginfo_to_user32(&frame->info, &ksig->info, 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))
-		goto badframe;
-
 	regs->link = tramp;
 
 #ifdef CONFIG_PPC_FPU_REGS
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index d80ff83cacb9..56c0c74aa28c 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.31.1

[PATCH RESEND v3 6/6] 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>
---
v3: Don't leave compat aside, use the new unsafe_copy_siginfo_to_user32()
---
 arch/powerpc/kernel/signal_32.c | 8 +++-----
 arch/powerpc/kernel/signal_64.c | 5 +----
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index ff101e2b3bab..3a2db8af2d65 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -710,9 +710,9 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
 }
 #endif
 
-#ifdef CONFIG_PPC64
+#ifndef CONFIG_PPC64
 
-#define copy_siginfo_to_user	copy_siginfo_to_user32
+#define unsafe_copy_siginfo_to_user32	unsafe_copy_siginfo_to_user
 
 #endif /* CONFIG_PPC64 */
 
@@ -779,15 +779,13 @@ 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);
+	unsafe_copy_siginfo_to_user32(&frame->info, &ksig->info, 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))
-		goto badframe;
-
 	regs->link = tramp;
 
 #ifdef CONFIG_PPC_FPU_REGS
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index d80ff83cacb9..56c0c74aa28c 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.31.1

Re: [PATCH RESEND v3 6/6] 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.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v3: Don't leave compat aside, use the new unsafe_copy_siginfo_to_user32()
> ---
>  arch/powerpc/kernel/signal_32.c | 8 +++-----
>  arch/powerpc/kernel/signal_64.c | 5 +----
>  2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index ff101e2b3bab..3a2db8af2d65 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -710,9 +710,9 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
>  }
>  #endif
>  
> -#ifdef CONFIG_PPC64
> +#ifndef CONFIG_PPC64
>  
> -#define copy_siginfo_to_user	copy_siginfo_to_user32
> +#define unsafe_copy_siginfo_to_user32	unsafe_copy_siginfo_to_user
>  
>  #endif /* CONFIG_PPC64 */

Any particular reason to reverse the sense of this #ifdef?

Otherwise this change looks much cleaner.

Eric


>  
> @@ -779,15 +779,13 @@ 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);
> +	unsafe_copy_siginfo_to_user32(&frame->info, &ksig->info, 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))
> -		goto badframe;
> -
>  	regs->link = tramp;
>  
>  #ifdef CONFIG_PPC_FPU_REGS
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index d80ff83cacb9..56c0c74aa28c 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 RESEND v3 6/6] 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.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v3: Don't leave compat aside, use the new unsafe_copy_siginfo_to_user32()
> ---
>  arch/powerpc/kernel/signal_32.c | 8 +++-----
>  arch/powerpc/kernel/signal_64.c | 5 +----
>  2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index ff101e2b3bab..3a2db8af2d65 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -710,9 +710,9 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
>  }
>  #endif
>  
> -#ifdef CONFIG_PPC64
> +#ifndef CONFIG_PPC64
>  
> -#define copy_siginfo_to_user	copy_siginfo_to_user32
> +#define unsafe_copy_siginfo_to_user32	unsafe_copy_siginfo_to_user
>  
>  #endif /* CONFIG_PPC64 */

Any particular reason to reverse the sense of this #ifdef?

Otherwise this change looks much cleaner.

Eric


>  
> @@ -779,15 +779,13 @@ 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);
> +	unsafe_copy_siginfo_to_user32(&frame->info, &ksig->info, 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))
> -		goto badframe;
> -
>  	regs->link = tramp;
>  
>  #ifdef CONFIG_PPC_FPU_REGS
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index d80ff83cacb9..56c0c74aa28c 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 RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> 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.

If you can't make function calls from an unsafe macro there is another
way to handle this that doesn't require everything to be inline.

From a safety perspective it is probably even a better approach.

Eric

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 0608581967f0..1b30bb78b863 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -205,6 +205,12 @@ struct sigframe {
 	int			abigap[56];
 };
 
+#ifdef CONFIG_PPC64
+# define user_siginfo_t		compat_siginfo_t
+#else
+# define user_siginfo_t		siginfo_t
+#endif
+
 /*
  *  When we have rt signals to deliver, we set up on the
  *  user stack, going down from the original stack pointer:
@@ -217,11 +223,7 @@ struct sigframe {
  *
  */
 struct rt_sigframe {
-#ifdef CONFIG_PPC64
-	compat_siginfo_t info;
-#else
-	struct siginfo info;
-#endif
+	user_siginfo_t info;
 	struct ucontext	uc;
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	struct ucontext	uc_transact;
@@ -712,7 +714,7 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
 
 #ifdef CONFIG_PPC64
 
-#define copy_siginfo_to_user	copy_siginfo_to_user32
+#define copy_siginfo_to_external	copy_siginfo_to_external32
 
 #endif /* CONFIG_PPC64 */
 
@@ -731,6 +733,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	struct pt_regs *regs = tsk->thread.regs;
 	/* Save the thread's msr before get_tm_stackpointer() changes it */
 	unsigned long msr = regs->msr;
+	user_siginfo_t uinfo;
 
 	/* Set up Signal Frame */
 	frame = get_sigframe(ksig, tsk, sizeof(*frame), 1);
@@ -743,6 +746,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	else
 		prepare_save_user_regs(1);
 
+	copy_siginfo_to_external(&uinfo, &ksig->info);
+
 	if (!user_access_begin(frame, sizeof(*frame)))
 		goto badframe;
 
@@ -778,12 +783,10 @@ 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);
+	unsafe_copy_to_user(&frame->info, &uinfo, failed);
 
 	user_access_end();
 
-	if (copy_siginfo_to_user(&frame->info, &ksig->info))
-		goto badframe;
-
 	regs->link = tramp;
 
 #ifdef CONFIG_PPC_FPU_REGS

Eric

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> 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.

If you can't make function calls from an unsafe macro there is another
way to handle this that doesn't require everything to be inline.

From a safety perspective it is probably even a better approach.

Eric

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 0608581967f0..1b30bb78b863 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -205,6 +205,12 @@ struct sigframe {
 	int			abigap[56];
 };
 
+#ifdef CONFIG_PPC64
+# define user_siginfo_t		compat_siginfo_t
+#else
+# define user_siginfo_t		siginfo_t
+#endif
+
 /*
  *  When we have rt signals to deliver, we set up on the
  *  user stack, going down from the original stack pointer:
@@ -217,11 +223,7 @@ struct sigframe {
  *
  */
 struct rt_sigframe {
-#ifdef CONFIG_PPC64
-	compat_siginfo_t info;
-#else
-	struct siginfo info;
-#endif
+	user_siginfo_t info;
 	struct ucontext	uc;
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	struct ucontext	uc_transact;
@@ -712,7 +714,7 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
 
 #ifdef CONFIG_PPC64
 
-#define copy_siginfo_to_user	copy_siginfo_to_user32
+#define copy_siginfo_to_external	copy_siginfo_to_external32
 
 #endif /* CONFIG_PPC64 */
 
@@ -731,6 +733,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	struct pt_regs *regs = tsk->thread.regs;
 	/* Save the thread's msr before get_tm_stackpointer() changes it */
 	unsigned long msr = regs->msr;
+	user_siginfo_t uinfo;
 
 	/* Set up Signal Frame */
 	frame = get_sigframe(ksig, tsk, sizeof(*frame), 1);
@@ -743,6 +746,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	else
 		prepare_save_user_regs(1);
 
+	copy_siginfo_to_external(&uinfo, &ksig->info);
+
 	if (!user_access_begin(frame, sizeof(*frame)))
 		goto badframe;
 
@@ -778,12 +783,10 @@ 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);
+	unsafe_copy_to_user(&frame->info, &uinfo, failed);
 
 	user_access_end();
 
-	if (copy_siginfo_to_user(&frame->info, &ksig->info))
-		goto badframe;
-
 	regs->link = tramp;
 
 #ifdef CONFIG_PPC_FPU_REGS

Eric

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 13/09/2021 à 18:21, Eric W. Biederman a écrit :
> ebiederm@xmission.com (Eric W. Biederman) writes:
> 
>> 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.
> 
> If you can't make function calls from an unsafe macro there is another
> way to handle this that doesn't require everything to be inline.
> 
>  From a safety perspective it is probably even a better approach.

Yes but that's exactly what I wanted to avoid for the native ppc32 case: 
this double hop means useless pressure on the cache. The siginfo_t 
structure is 128 bytes large, that means 8 lines of cache on powerpc 8xx.

But maybe it is acceptable to do that only for the compat case. Let me 
think about it, it might be quite easy.

Christophe

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 13/09/2021 à 18:21, Eric W. Biederman a écrit :
> ebiederm@xmission.com (Eric W. Biederman) writes:
> 
>> 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.
> 
> If you can't make function calls from an unsafe macro there is another
> way to handle this that doesn't require everything to be inline.
> 
>  From a safety perspective it is probably even a better approach.

Yes but that's exactly what I wanted to avoid for the native ppc32 case: 
this double hop means useless pressure on the cache. The siginfo_t 
structure is 128 bytes large, that means 8 lines of cache on powerpc 8xx.

But maybe it is acceptable to do that only for the compat case. Let me 
think about it, it might be quite easy.

Christophe

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

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

> Le 13/09/2021 à 18:21, Eric W. Biederman a écrit :
>> ebiederm@xmission.com (Eric W. Biederman) writes:
>>
>>> 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.
>>
>> If you can't make function calls from an unsafe macro there is another
>> way to handle this that doesn't require everything to be inline.
>>
>>  From a safety perspective it is probably even a better approach.
>
> Yes but that's exactly what I wanted to avoid for the native ppc32 case: this
> double hop means useless pressure on the cache. The siginfo_t structure is 128
> bytes large, that means 8 lines of cache on powerpc 8xx.
>
> But maybe it is acceptable to do that only for the compat case. Let me think
> about it, it might be quite easy.

The places get_signal is called tend to be well known.  So I think we
are safe from a capacity standpoint.

I am not certain it makes a difference in capacity as there is a high
probability that the stack was deeper recently than it is now which
suggests the cache blocks might already be in the cache.

My sense it is worth benchmarking before optimizing out the extra copy
like that.

On the extreme side there is simply building the entire sigframe on the
stack and then just calling it copy_to_user.  As the stack cache lines
are likely to be hot, and copy_to_user is quite well optimized
there is a real possibility that it is faster to build everything
on the kernel stack, and then copy it to the user space stack.

It is also possible that I am wrong and we may want to figure out how
far up we can push the conversion to the 32bit siginfo format.

If could move the work into collect_signal we could guarantee there
would be no extra work.  That would require adjusting the sigframe
generation code on all of the architectures.

There is a lot we can do but we need benchmarking to tell if it is
worth it.

Eric

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Eric W. Biederman]

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

> Le 13/09/2021 à 18:21, Eric W. Biederman a écrit :
>> ebiederm@xmission.com (Eric W. Biederman) writes:
>>
>>> 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.
>>
>> If you can't make function calls from an unsafe macro there is another
>> way to handle this that doesn't require everything to be inline.
>>
>>  From a safety perspective it is probably even a better approach.
>
> Yes but that's exactly what I wanted to avoid for the native ppc32 case: this
> double hop means useless pressure on the cache. The siginfo_t structure is 128
> bytes large, that means 8 lines of cache on powerpc 8xx.
>
> But maybe it is acceptable to do that only for the compat case. Let me think
> about it, it might be quite easy.

The places get_signal is called tend to be well known.  So I think we
are safe from a capacity standpoint.

I am not certain it makes a difference in capacity as there is a high
probability that the stack was deeper recently than it is now which
suggests the cache blocks might already be in the cache.

My sense it is worth benchmarking before optimizing out the extra copy
like that.

On the extreme side there is simply building the entire sigframe on the
stack and then just calling it copy_to_user.  As the stack cache lines
are likely to be hot, and copy_to_user is quite well optimized
there is a real possibility that it is faster to build everything
on the kernel stack, and then copy it to the user space stack.

It is also possible that I am wrong and we may want to figure out how
far up we can push the conversion to the 32bit siginfo format.

If could move the work into collect_signal we could guarantee there
would be no extra work.  That would require adjusting the sigframe
generation code on all of the architectures.

There is a lot we can do but we need benchmarking to tell if it is
worth it.

Eric

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 13/09/2021 à 21:11, Eric W. Biederman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> Le 13/09/2021 à 18:21, Eric W. Biederman a écrit :
>>> ebiederm@xmission.com (Eric W. Biederman) writes:
>>>
>>>> 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.
>>>
>>> If you can't make function calls from an unsafe macro there is another
>>> way to handle this that doesn't require everything to be inline.
>>>
>>>   From a safety perspective it is probably even a better approach.
>>
>> Yes but that's exactly what I wanted to avoid for the native ppc32 case: this
>> double hop means useless pressure on the cache. The siginfo_t structure is 128
>> bytes large, that means 8 lines of cache on powerpc 8xx.
>>
>> But maybe it is acceptable to do that only for the compat case. Let me think
>> about it, it might be quite easy.
> 
> The places get_signal is called tend to be well known.  So I think we
> are safe from a capacity standpoint.
> 
> I am not certain it makes a difference in capacity as there is a high
> probability that the stack was deeper recently than it is now which
> suggests the cache blocks might already be in the cache.
> 
> My sense it is worth benchmarking before optimizing out the extra copy
> like that.
> 
> On the extreme side there is simply building the entire sigframe on the
> stack and then just calling it copy_to_user.  As the stack cache lines
> are likely to be hot, and copy_to_user is quite well optimized
> there is a real possibility that it is faster to build everything
> on the kernel stack, and then copy it to the user space stack.
> 
> It is also possible that I am wrong and we may want to figure out how
> far up we can push the conversion to the 32bit siginfo format.
> 
> If could move the work into collect_signal we could guarantee there
> would be no extra work.  That would require adjusting the sigframe
> generation code on all of the architectures.
> 
> There is a lot we can do but we need benchmarking to tell if it is
> worth it.
> 


Sure, I'm benchmarking all the work I have been doing on signal code 
with the following simple app that I run with 'perf stat':

#include <stdlib.h>
#include <signal.h>

void sigusr1(int sig) { }

int main(int argc, char **argv)
{
	int i = 100000;

	signal(SIGUSR1, sigusr1);
	for (;i--;)
	raise(SIGUSR1);
	exit(0);
}


On an mpc8321 a 32 bits powerpc with KUAP enabled (KUAP is equivalent of 
x86 SMAP)

Before changing copy_siginfo_to_user() to unsafe_copy_siginfo_to_user(), 
'perf stat' reports 1983 msec (task-clock)

After my change I get 1900 msec.

With your approach I get 1930 msec, so we are loosing 36% of the benefit 
of converting to the 'unsafe_' alternative.

So I think it is worth it.

Christophe

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 13/09/2021 à 21:11, Eric W. Biederman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> Le 13/09/2021 à 18:21, Eric W. Biederman a écrit :
>>> ebiederm@xmission.com (Eric W. Biederman) writes:
>>>
>>>> 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.
>>>
>>> If you can't make function calls from an unsafe macro there is another
>>> way to handle this that doesn't require everything to be inline.
>>>
>>>   From a safety perspective it is probably even a better approach.
>>
>> Yes but that's exactly what I wanted to avoid for the native ppc32 case: this
>> double hop means useless pressure on the cache. The siginfo_t structure is 128
>> bytes large, that means 8 lines of cache on powerpc 8xx.
>>
>> But maybe it is acceptable to do that only for the compat case. Let me think
>> about it, it might be quite easy.
> 
> The places get_signal is called tend to be well known.  So I think we
> are safe from a capacity standpoint.
> 
> I am not certain it makes a difference in capacity as there is a high
> probability that the stack was deeper recently than it is now which
> suggests the cache blocks might already be in the cache.
> 
> My sense it is worth benchmarking before optimizing out the extra copy
> like that.
> 
> On the extreme side there is simply building the entire sigframe on the
> stack and then just calling it copy_to_user.  As the stack cache lines
> are likely to be hot, and copy_to_user is quite well optimized
> there is a real possibility that it is faster to build everything
> on the kernel stack, and then copy it to the user space stack.
> 
> It is also possible that I am wrong and we may want to figure out how
> far up we can push the conversion to the 32bit siginfo format.
> 
> If could move the work into collect_signal we could guarantee there
> would be no extra work.  That would require adjusting the sigframe
> generation code on all of the architectures.
> 
> There is a lot we can do but we need benchmarking to tell if it is
> worth it.
> 


Sure, I'm benchmarking all the work I have been doing on signal code 
with the following simple app that I run with 'perf stat':

#include <stdlib.h>
#include <signal.h>

void sigusr1(int sig) { }

int main(int argc, char **argv)
{
	int i = 100000;

	signal(SIGUSR1, sigusr1);
	for (;i--;)
	raise(SIGUSR1);
	exit(0);
}


On an mpc8321 a 32 bits powerpc with KUAP enabled (KUAP is equivalent of 
x86 SMAP)

Before changing copy_siginfo_to_user() to unsafe_copy_siginfo_to_user(), 
'perf stat' reports 1983 msec (task-clock)

After my change I get 1900 msec.

With your approach I get 1930 msec, so we are loosing 36% of the benefit 
of converting to the 'unsafe_' alternative.

So I think it is worth it.

Christophe

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 13/09/2021 à 17:57, 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.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v3: Don't leave compat aside, use the new unsafe_copy_siginfo_to_user32()
>> ---
>>   arch/powerpc/kernel/signal_32.c | 8 +++-----
>>   arch/powerpc/kernel/signal_64.c | 5 +----
>>   2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>> index ff101e2b3bab..3a2db8af2d65 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -710,9 +710,9 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
>>   }
>>   #endif
>>   
>> -#ifdef CONFIG_PPC64
>> +#ifndef CONFIG_PPC64
>>   
>> -#define copy_siginfo_to_user	copy_siginfo_to_user32
>> +#define unsafe_copy_siginfo_to_user32	unsafe_copy_siginfo_to_user
>>   
>>   #endif /* CONFIG_PPC64 */
> 
> Any particular reason to reverse the sense of this #ifdef?

Yes I had double definition of unsafe_copy_siginfo_to_user(), I could 
have ifdefed out unsafe_copy_siginfo_to_user() in signal.h, but I 
prefered to ifdef out copy_siginfo_to_user32() in compat.h

> 
> Otherwise this change looks much cleaner.

Thanks
Christophe

Re: [PATCH RESEND v3 6/6] powerpc/signal: Use unsafe_copy_siginfo_to_user()

[Christophe Leroy]

Le 13/09/2021 à 17:57, 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.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v3: Don't leave compat aside, use the new unsafe_copy_siginfo_to_user32()
>> ---
>>   arch/powerpc/kernel/signal_32.c | 8 +++-----
>>   arch/powerpc/kernel/signal_64.c | 5 +----
>>   2 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>> index ff101e2b3bab..3a2db8af2d65 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -710,9 +710,9 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *s
>>   }
>>   #endif
>>   
>> -#ifdef CONFIG_PPC64
>> +#ifndef CONFIG_PPC64
>>   
>> -#define copy_siginfo_to_user	copy_siginfo_to_user32
>> +#define unsafe_copy_siginfo_to_user32	unsafe_copy_siginfo_to_user
>>   
>>   #endif /* CONFIG_PPC64 */
> 
> Any particular reason to reverse the sense of this #ifdef?

Yes I had double definition of unsafe_copy_siginfo_to_user(), I could 
have ifdefed out unsafe_copy_siginfo_to_user() in signal.h, but I 
prefered to ifdef out copy_siginfo_to_user32() in compat.h

> 
> Otherwise this change looks much cleaner.

Thanks
Christophe