Re: [PATCH v2 2/2] signal: make send_signal_locked() take const siginfo

[Oleg Nesterov <oleg@redhat.com>]

On 06/22, Bradley Morgan wrote:
>
> send_signal_locked() should not change the caller's siginfo. Make that
> part of the type and keep the local rewrite on its copy.
>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>

Ah, sorry... I only suggested to change the signature of send_signal_locked()
and thus has_si_pid_and_uid(). Perhaps a broader change makes sense too, but
this conflicts with another (under discussion) series:

	PATCH v2 3/3] signal: fix evasion of SA_IMMUTABLE signals
	https://lore.kernel.org/all/ajVD6ZmiSQLxjj57@redhat.com/

Now let me take another look at 1/2 ...

Oleg.

> Signed-off-by: Bradley Morgan <include@grrlz.net>
> ---
> Changes since v1:
> - New patch from Oleg's suggestion.
> - Link to Oleg's suggestion:
>   https://lore.kernel.org/all/0873AC4A-3CB2-4F7B-BFE6-75D855AD22DC@grrlz.net/T/#m5f8a2d54928efff41de539969b68149e1ec5fca4
> 
>  include/linux/signal.h        |  2 +-
>  include/trace/events/signal.h |  4 ++--
>  kernel/signal.c               | 20 +++++++++++---------
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/signal.h b/include/linux/signal.h
> index f19816832f05..a1ba8c5973c6 100644
> --- a/include/linux/signal.h
> +++ b/include/linux/signal.h
> @@ -283,7 +283,7 @@ extern int do_send_sig_info(int sig, struct kernel_siginfo *info,
>  				struct task_struct *p, enum pid_type type);
>  extern int group_send_sig_info(int sig, struct kernel_siginfo *info,
>  			       struct task_struct *p, enum pid_type type);
> -extern int send_signal_locked(int sig, struct kernel_siginfo *info,
> +extern int send_signal_locked(int sig, const struct kernel_siginfo *info,
>  			      struct task_struct *p, enum pid_type type);
>  extern int sigprocmask(int, sigset_t *, sigset_t *);
>  extern void set_current_blocked(sigset_t *);
> diff --git a/include/trace/events/signal.h b/include/trace/events/signal.h
> index 1db7e4b07c01..05a46135ee34 100644
> --- a/include/trace/events/signal.h
> +++ b/include/trace/events/signal.h
> @@ -49,8 +49,8 @@ enum {
>   */
>  TRACE_EVENT(signal_generate,
>  
> -	TP_PROTO(int sig, struct kernel_siginfo *info, struct task_struct *task,
> -			int group, int result),
> +	TP_PROTO(int sig, const struct kernel_siginfo *info,
> +		 struct task_struct *task, int group, int result),
>  
>  	TP_ARGS(sig, info, task, group, result),
>  
> diff --git a/kernel/signal.c b/kernel/signal.c
> index d72d9be3a992..26e8b8e1d03c 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1037,7 +1037,7 @@ static inline bool legacy_queue(struct sigpending *signals, int sig)
>  	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
>  }
>  
> -static int __send_signal_locked(int sig, struct kernel_siginfo *info,
> +static int __send_signal_locked(int sig, const struct kernel_siginfo *info,
>  				struct task_struct *t, enum pid_type type, bool force)
>  {
>  	struct sigpending *pending;
> @@ -1154,7 +1154,7 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
>  	return ret;
>  }
>  
> -static inline bool has_si_pid_and_uid(struct kernel_siginfo *info)
> +static inline bool has_si_pid_and_uid(const struct kernel_siginfo *info)
>  {
>  	bool ret = false;
>  	switch (siginfo_layout(info->si_signo, info->si_code)) {
> @@ -1178,10 +1178,11 @@ static inline bool has_si_pid_and_uid(struct kernel_siginfo *info)
>  	return ret;
>  }
>  
> -int send_signal_locked(int sig, struct kernel_siginfo *info,
> +int send_signal_locked(int sig, const struct kernel_siginfo *info,
>  		       struct task_struct *t, enum pid_type type)
>  {
>  	struct kernel_siginfo rewritten;
> +	const struct kernel_siginfo *send_info = info;
>  	/* Should SIGKILL or SIGSTOP be received by a pid namespace init? */
>  	bool force = false;
>  
> @@ -1196,26 +1197,27 @@ int send_signal_locked(int sig, struct kernel_siginfo *info,
>  		struct user_namespace *t_user_ns;
>  
>  		rewritten = *info;
> -		info = &rewritten;
> +		send_info = &rewritten;
>  
>  		rcu_read_lock();
>  		t_user_ns = task_cred_xxx(t, user_ns);
>  		if (current_user_ns() != t_user_ns) {
> -			kuid_t uid = make_kuid(current_user_ns(), info->si_uid);
> -			info->si_uid = from_kuid_munged(t_user_ns, uid);
> +			kuid_t uid = make_kuid(current_user_ns(), rewritten.si_uid);
> +
> +			rewritten.si_uid = from_kuid_munged(t_user_ns, uid);
>  		}
>  		rcu_read_unlock();
>  
>  		/* A kernel generated signal? */
> -		force = (info->si_code == SI_KERNEL);
> +		force = (rewritten.si_code == SI_KERNEL);
>  
>  		/* From an ancestor pid namespace? */
>  		if (!task_pid_nr_ns(current, task_active_pid_ns(t))) {
> -			info->si_pid = 0;
> +			rewritten.si_pid = 0;
>  			force = true;
>  		}
>  	}
> -	return __send_signal_locked(sig, info, t, type, force);
> +	return __send_signal_locked(sig, send_info, t, type, force);
>  }
>  
>  static void print_fatal_signal(int signr)
> -- 
> 2.53.0
>