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@xxxxxxxxxx>

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