[v2,1/2] signal: add the "int si_code" arg to prepare_kill_siginfo()

  So that do_tkill() can use this helper too. This also simplifies
the next patch.

TODO: perhaps we can kill prepare_kill_siginfo() and change the
callers to use SEND_SIG_NOINFO,  but this needs some changes in
__send_signal_locked() and TP_STORE_SIGINFO().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
  

On Fri, Feb 09, 2024 at 02:06:20PM +0100, Oleg Nesterov wrote:
> So that do_tkill() can use this helper too. This also simplifies
> the next patch.
> 
> TODO: perhaps we can kill prepare_kill_siginfo() and change the
> callers to use SEND_SIG_NOINFO,  but this needs some changes in
> __send_signal_locked() and TP_STORE_SIGINFO().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>
  

On Fri, 09 Feb 2024 14:06:20 +0100, Oleg Nesterov wrote:
> So that do_tkill() can use this helper too. This also simplifies
> the next patch.
> 
> TODO: perhaps we can kill prepare_kill_siginfo() and change the
> callers to use SEND_SIG_NOINFO,  but this needs some changes in
> __send_signal_locked() and TP_STORE_SIGINFO().
> 
> [...]

Applied to the vfs.pidfd branch of the vfs/vfs.git tree.
Patches in the vfs.pidfd branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.pidfd

[1/2] signal: add the "int si_code" arg to prepare_kill_siginfo()
      https://git.kernel.org/vfs/vfs/c/3a363602809c
[2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
      https://git.kernel.org/vfs/vfs/c/2885a4b3358d
  

Oleg Nesterov <oleg@redhat.com> writes:

> So that do_tkill() can use this helper too. This also simplifies
> the next patch.
>
> TODO: perhaps we can kill prepare_kill_siginfo() and change the
> callers to use SEND_SIG_NOINFO,  but this needs some changes in
> __send_signal_locked() and TP_STORE_SIGINFO().

Could you can pass in the destination type instead of the si_code?
Something like I have shown below?

That allows the knowledge of the siginfo details to be isolated to
prepare_kill_siginfo, making it easy to verify that a the union members
match the union decode for the signal/si_code combination?

It is all too easy to fill in siginfo improperly.

Looking at siginfo_layout() SI_USER paired with any signal results in
SIL_KILL whereas SI_TKILL paired with any signal results in SIL_RT.  A
superset of the fields of SIL_KILL.

We use clear_siginfo() so si_sigval is set to 0 for SI_TKILL which seems
correct.  But we do allow userspace if it specifies SI_TKILL to provide
si_sigval.  So the current do_tkill code is very close to being wrong.

Likewise you are filling in the details to match what the existing
code is doing, so you are fine.  Still it is a loaded footgun
to allow passing in an arbitrary si_code.

Eric

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/signal.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c3fac06937e2..a8199fda0d61 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3793,12 +3793,12 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait_time32, compat_sigset_t __user *, uthese,
>  #endif
>  #endif
>  
> -static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> +static void prepare_kill_siginfo(int sig, struct kernel_siginfo *info, int si_code)
>  {
>  	clear_siginfo(info);
>  	info->si_signo = sig;
>  	info->si_errno = 0;
> -	info->si_code = SI_USER;
> +	info->si_code = si_code;
	info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;
>  	info->si_pid = task_tgid_vnr(current);
>  	info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
>  }
> @@ -3812,7 +3812,7 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
>  {
>  	struct kernel_siginfo info;
>  
> -	prepare_kill_siginfo(sig, &info);
> +	prepare_kill_siginfo(sig, &info, SI_USER);
>  
>  	return kill_something_info(sig, &info, pid);
>  }
> @@ -3925,7 +3925,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
>  			goto err;
>  	} else {
> -		prepare_kill_siginfo(sig, &kinfo);
> +		prepare_kill_siginfo(sig, &kinfo, SI_USER);
>  	}
>  
>  	/* TODO: respect PIDFD_THREAD */
> @@ -3970,12 +3970,7 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
>  {
>  	struct kernel_siginfo info;
>  
> -	clear_siginfo(&info);
> -	info.si_signo = sig;
> -	info.si_errno = 0;
> -	info.si_code = SI_TKILL;
> -	info.si_pid = task_tgid_vnr(current);
> -	info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> +	prepare_kill_siginfo(sig, &info, SI_TKILL);
>  
>  	return do_send_specific(tgid, pid, sig, &info);
>  }
  

On 02/09, Eric W. Biederman wrote:
>
> Could you can pass in the destination type instead of the si_code?
> Something like I have shown below?

..

> 	info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;

Yes, I considered this option too.

OK, will send V3 tomorrow.

Oleg.
  

On Fri, Feb 09, 2024 at 05:39:14PM +0100, Oleg Nesterov wrote:
> On 02/09, Eric W. Biederman wrote:
> >
> > Could you can pass in the destination type instead of the si_code?
> > Something like I have shown below?
> 
> ...
> 
> > 	info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;
> 
> Yes, I considered this option too.
> 
> OK, will send V3 tomorrow.

Hm, I don't think that's necessary if you're happy to have me just fix
that up in tree. Here's the two patches updated. It was straightforward
but I have a baby on my lap so double check, please:

From 05ffda39f6f5c887cae319274366cbf856c88fe5 Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Fri, 9 Feb 2024 14:06:20 +0100
Subject: [PATCH 1/2] signal: fill in si_code in prepare_kill_siginfo()

So that do_tkill() can use this helper too. This also simplifies
the next patch.

TODO: perhaps we can kill prepare_kill_siginfo() and change the
callers to use SEND_SIG_NOINFO,  but this needs some changes in
__send_signal_locked() and TP_STORE_SIGINFO().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Link: https://lore.kernel.org/r/20240209130620.GA8039@redhat.com
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 kernel/signal.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index c3fac06937e2..1450689302d9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3793,12 +3793,13 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait_time32, compat_sigset_t __user *, uthese,
 #endif
 #endif
 
-static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
+static void prepare_kill_siginfo(int sig, struct kernel_siginfo *info,
+				 enum pid_type type)
 {
 	clear_siginfo(info);
 	info->si_signo = sig;
 	info->si_errno = 0;
-	info->si_code = SI_USER;
+	info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;
 	info->si_pid = task_tgid_vnr(current);
 	info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
 }
@@ -3812,7 +3813,7 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
 {
 	struct kernel_siginfo info;
 
-	prepare_kill_siginfo(sig, &info);
+	prepare_kill_siginfo(sig, &info, PIDTYPE_TGID);
 
 	return kill_something_info(sig, &info, pid);
 }
@@ -3925,7 +3926,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
 		    (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
 			goto err;
 	} else {
-		prepare_kill_siginfo(sig, &kinfo);
+		prepare_kill_siginfo(sig, &kinfo, PIDTYPE_TGID);
 	}
 
 	/* TODO: respect PIDFD_THREAD */
@@ -3970,12 +3971,7 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig)
 {
 	struct kernel_siginfo info;
 
-	clear_siginfo(&info);
-	info.si_signo = sig;
-	info.si_errno = 0;
-	info.si_code = SI_TKILL;
-	info.si_pid = task_tgid_vnr(current);
-	info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+	prepare_kill_siginfo(sig, &info, PIDTYPE_PID);
 
 	return do_send_specific(tgid, pid, sig, &info);
 }
  

On 02/09, Christian Brauner wrote:
>
> On Fri, Feb 09, 2024 at 05:39:14PM +0100, Oleg Nesterov wrote:
> > On 02/09, Eric W. Biederman wrote:
> > >
> > > Could you can pass in the destination type instead of the si_code?
> > > Something like I have shown below?
> > 
> > ...
> > 
> > > 	info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;
> > 
> > Yes, I considered this option too.
> > 
> > OK, will send V3 tomorrow.
> 
> Hm, I don't think that's necessary if you're happy to have me just fix
> that up in tree.

Thank you!!!

looks obviously correct, but again, I will double check tomorrow just
in case.

> but I have a baby on my lap so double check, please:

;) I'm familiar with this.

Oleg.
  

On Fri, Feb 09, 2024 at 08:36:24PM +0100, Christian Brauner wrote:
> On Fri, Feb 09, 2024 at 05:39:14PM +0100, Oleg Nesterov wrote:
> > On 02/09, Eric W. Biederman wrote:
> > >
> > > Could you can pass in the destination type instead of the si_code?
> > > Something like I have shown below?
> > 
> > ...
> > 
> > > 	info->si_code = (type == PIDTYPE_PID) ? SI_TKILL : SI_USER;
> > 
> > Yes, I considered this option too.
> > 
> > OK, will send V3 tomorrow.
> 
> Hm, I don't think that's necessary if you're happy to have me just fix
> that up in tree. Here's the two patches updated. It was straightforward
> but I have a baby on my lap so double check, please:
> 
> From 05ffda39f6f5c887cae319274366cbf856c88fe5 Mon Sep 17 00:00:00 2001
> From: Oleg Nesterov <oleg@redhat.com>
> Date: Fri, 9 Feb 2024 14:06:20 +0100
> Subject: [PATCH 1/2] signal: fill in si_code in prepare_kill_siginfo()
> 
> So that do_tkill() can use this helper too. This also simplifies
> the next patch.
> 
> TODO: perhaps we can kill prepare_kill_siginfo() and change the
> callers to use SEND_SIG_NOINFO,  but this needs some changes in
> __send_signal_locked() and TP_STORE_SIGINFO().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Link: https://lore.kernel.org/r/20240209130620.GA8039@redhat.com
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me as well,

Reviewed-by: Tycho Andersen <tandersen@netflix.com>