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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250731223914188BCyA6ps7dI6ffNSFD1K-V@zte.com.cn>
Date: Thu, 31 Jul 2025 22:39:14 +0800 (CST)
From: <fan.yu9@....com.cn>
To: <oleg@...hat.com>
Cc: <tglx@...utronix.de>, <frederic@...nel.org>, <peterz@...radead.org>,
        <brauner@...nel.org>, <iro@...iv.linux.org.uk>,
        <joel.granados@...nel.org>, <lorenzo.stoakes@...cle.com>,
        <akpm@...ux-foundation.org>, <linux-kernel@...r.kernel.org>,
        <xu.xin16@....com.cn>, <yang.yang29@....com.cn>
Subject: Re: [PATCH linux-next v2] signal: clarify __send_signal_locked comment in do_notify_parent

> > -    * 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@...hat.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