[PATCH linux-next v2] signal: clarify __send_signal_locked comment in do_notify_parent

[PATCH linux-next v2] signal: clarify __send_signal_locked comment in do_notify_parent

[fan.yu9]

From: Fan Yu <fan.yu9@zte.com.cn>

The original comment (introduced in commit 61e713bdca36 ("signal: Avoid
corrupting si_pid and si_uid in do_notify_parent")) stated that
__send_signal should be used because si_pid/si_uid are in the parent's
namespace, but it did not explain why send_signal_locked() is unsafe here.

This became more ambiguous after
  commit 157cc18122b4 ("signal: Rename send_signal send_signal_locked")
without updating the comment.

Explicitly clarify that:
1. send_signal_locked() may incorrectly modify si_pid/si_uid when crossing
   PID/user namespaces (e.g., reset si_pid to 0 or translate si_uid).
2. __send_signal_locked() preserves the original siginfo values, which is
   critical since they are already in the parent's namespace.

Signed-off-by: Fan Yu <fan.yu9@zte.com.cn>
---
Changes in V2:
- Some fixes according to
https://lore.kernel.org/all/878qk8pdkd.ffs@tglx/
https://lore.kernel.org/all/20250728155815.GB25567@redhat.com/
- Clarify why __send_signal_locked must be used

 kernel/signal.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index e2c928de7d2c..047b22837a36 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2252,8 +2252,10 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
 			sig = 0;
 	}
 	/*
-	 * Send with __send_signal as si_pid and si_uid are in the
-	 * parent's namespaces.
+	 * Use __send_signal_locked() instead of send_signal_locked()
+	 * because si_pid and si_uid are already in the parent's
+	 * namespace. send_signal_locked() would incorrectly modify
+	 * them when crossing PID/user namespaces.
 	 */
 	if (valid_signal(sig) && sig)
 		__send_signal_locked(sig, &info, tsk->parent, PIDTYPE_TGID, false);
-- 
2.25.1

Re: [PATCH linux-next v2] signal: clarify __send_signal_locked comment in do_notify_parent

[Oleg Nesterov]

On 07/29, fan.yu9@zte.com.cn wrote:
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2252,8 +2252,10 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
>  			sig = 0;
>  	}
>  	/*
> -	 * Send with __send_signal as si_pid and si_uid are in the
> -	 * parent's namespaces.
> +	 * Use __send_signal_locked() instead of send_signal_locked()
> +	 * because si_pid and si_uid are already in the parent's
> +	 * namespace. send_signal_locked() would incorrectly modify
> +	 * them when crossing PID/user namespaces.
>  	 */

Somehow I'd still prefer the previous version which simply kills this comment,
but as I said I won't argue.

However. It seems to me that the new comment adds another confusion. I'll try
to recheck tomorrow, I am very busy today.

Oleg.

Re: [PATCH linux-next v2] signal: clarify __send_signal_locked comment in do_notify_parent

[fan.yu9]

> > -     * Send with __send_signal as si_pid and si_uid are in the
> > -     * parent's namespaces.
> > +     * Use __send_signal_locked() instead of send_signal_locked()
> > +     * because si_pid and si_uid are already in the parent's
> > +     * namespace. send_signal_locked() would incorrectly modify
> > +     * them when crossing PID/user namespaces.
> >       */
> 
> Somehow I'd still prefer the previous version which simply kills this comment,
> but as I said I won't argue.
> 
> However. It seems to me that the new comment adds another confusion. I'll try
> to recheck tomorrow, I am very busy today.
> 
> Oleg.

Hi Oleg,

Thanks for your feedback and time! I understand you're busy today, so no rush at all.

Regarding the comment change, I'm happy to adjust it if you think the new version
is confusing. If you have a moment, could you suggest a clearer way to explain the
rationale behind using `__send_signal_locked()`? 

Thanks again for your help!

Best regards,
Fan Yu

Re: [PATCH linux-next v2] signal: clarify __send_signal_locked comment in do_notify_parent

[Oleg Nesterov]

On 07/29, fan.yu9@zte.com.cn wrote:
>
> @@ -2252,8 +2252,10 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
>  			sig = 0;
>  	}
>  	/*
> -	 * Send with __send_signal as si_pid and si_uid are in the
> -	 * parent's namespaces.
> +	 * Use __send_signal_locked() instead of send_signal_locked()
> +	 * because si_pid and si_uid are already in the parent's
> +	 * namespace. send_signal_locked() would incorrectly modify
> +	 * them when crossing PID/user namespaces.
>  	 */

Well, Thomas doesn't like the idea to kill this comment, I won't argue.

However, this comment still looks confusing to me, and I don't know how to
make it more clear. Yes, send_signal_locked() may, say, clear info->si_pid
but not "because si_pid and si_uid are already in the parent's namespace".

There are several obvious reasons not to use send_signal_locked():

	1. do_notify_parent() has already correctly filled si_pid/si_uid,
	   the "has_si_pid_and_uid()" checks in send_signal_locked() are
	   pointless.

	   That is why I think this comment should simply die.

	2. send_signal_locked() assumes that different namespaces mean
	   "From an ancestor namespace", but in this case the child can
	   send a signal to the parent namespace while "from parent ns"
	   is not possible.

	3. send_signal_locked() assumes that "current" is a) the sender
	   and b) alive task. Both assumptions may be wrong if "current"
	   is the last exiting thread which calls do_notify_parent() from
	   release_task().

	   In this case task_pid_nr_ns(current, task_active_pid_ns(parent))
	   will return 0 because current->thread_pid is already NULL, and
	   send_signal_locked() will misinterpret this as "from parent ns"
	   and clear si_pid.

	   But imo, it is simply unsafe to use send_signal_locked() in this
	   case, even if currently nothing "really bad" can happen.

OTOH. This patch doesn't make the comment more confusing, plus it removes
the reference to __send_signal() which no longer exists, so let me ack
this patch and forget this surprisingly long discussion ;)

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH linux-next v2] signal: clarify __send_signal_locked comment in do_notify_parent

[fan.yu9]

> > -    * Send with __send_signal as si_pid and si_uid are in the
> > -    * parent's namespaces.
> > +    * Use __send_signal_locked() instead of send_signal_locked()
> > +    * because si_pid and si_uid are already in the parent's
> > +    * namespace. send_signal_locked() would incorrectly modify
> > +    * them when crossing PID/user namespaces.
> >      */
> 
> Well, Thomas doesn't like the idea to kill this comment, I won't argue.
> 
> However, this comment still looks confusing to me, and I don't know how to
> make it more clear. Yes, send_signal_locked() may, say, clear info->si_pid
> but not "because si_pid and si_uid are already in the parent's namespace".
> 
> There are several obvious reasons not to use send_signal_locked():
> 
>       1. do_notify_parent() has already correctly filled si_pid/si_uid,
>          the "has_si_pid_and_uid()" checks in send_signal_locked() are
>          pointless.
> 
>          That is why I think this comment should simply die.
> 
>       2. send_signal_locked() assumes that different namespaces mean
>          "From an ancestor namespace", but in this case the child can
>          send a signal to the parent namespace while "from parent ns"
>          is not possible.
> 
>       3. send_signal_locked() assumes that "current" is a) the sender
>          and b) alive task. Both assumptions may be wrong if "current"
>          is the last exiting thread which calls do_notify_parent() from
>          release_task().
> 
>          In this case task_pid_nr_ns(current, task_active_pid_ns(parent))
>          will return 0 because current->thread_pid is already NULL, and
>          send_signal_locked() will misinterpret this as "from parent ns"
>          and clear si_pid.
> 
>          But imo, it is simply unsafe to use send_signal_locked() in this
>          case, even if currently nothing "really bad" can happen.
> 
> OTOH. This patch doesn't make the comment more confusing, plus it removes
> the reference to __send_signal() which no longer exists, so let me ack
> this patch and forget this surprisingly long discussion ;)
> 
> Acked-by: Oleg Nesterov <oleg@redhat.com>

Hi Oleg,

Thank you sincerely for your thorough review. I truly appreciate you taking
the time to share your valuable insights on this subtle but important issue.

You raise an excellent point - while the original comment highlighted 'parent's
namespaces' as the key concern , your analysis reveals more fundamental
challenges at play: redundant validation, namespace direction mismatches,
and the inherent unreliability of 'current' references.

If we keep the comment, would this version work better?

        /*
         * Use __send_signal_locked() instead of send_signal_locked() because:
         * a) "current" may be zombie/unrelated (sender context invalid)
         * b) si_pid/si_uid are parent-namespace ready
         * c) child→parent direction breaks send_signal_locked()'s assumptions
         */

I'm happy to follow your preference here — please let me know if you'd
prefer to keep this.

Thanks again for your guidance.

Best regards,
Fan Yu