linux-kernel - Re: For review: pidfd_send

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] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 25 Sep 2019 15:53:09 +0200
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     "Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
Cc:     Florian Weimer <fw@...eb.enyo.de>, Oleg Nesterov <oleg@...hat.com>,
        Jann Horn <jannh@...gle.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Daniel Colascione <dancol@...gle.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        linux-man <linux-man@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: For review: pidfd_send_signal(2) manual page

On Wed, Sep 25, 2019 at 03:46:26PM +0200, Michael Kerrisk (man-pages) wrote:
> On 9/24/19 11:53 PM, Christian Brauner wrote:
> > On Tue, Sep 24, 2019 at 11:00:03PM +0200, Michael Kerrisk (man-pages) wrote:
> >> Hello Christian,
> >>
> >>>>> If you're the parent of the process you can do this without CLONE_PIDFD:
> >>>>> pid = fork();
> >>>>> pidfd = pidfd_open();
> >>>>> ret = pidfd_send_signal(pidfd, 0, NULL, 0);
> >>>>> if (ret < 0 && errno == ESRCH)
> >>>>> 	/* pidfd refers to another, recycled process */
> >>>>
> >>>> Although there is still the race between the fork() and the
> >>>> pidfd_open(), right?
> >>>
> >>> Actually no and my code is even too complex.
> >>> If you are the parent, and this is really a sequence that obeys the
> >>> ordering pidfd_open() before waiting:
> >>>
> >>> pid = fork();
> >>> if (pid == 0)
> >>> 	exit(EXIT_SUCCESS);
> >>> pidfd = pidfd_open(pid, 0);
> >>> waitid(pid, ...);
> >>>
> >>> Then you are guaranteed that pidfd will refer to pid. No recycling can
> >>> happen since the process has not been waited upon yet (That is,
> >>
> >> D'oh! Yes, of course. 
> >>
> >>> excluding special cases such as where you have a mainloop where a
> >>> callback reacts to a SIGCHLD event and waits on the child behind your
> >>> back and your next callback in the mainloop calls pidfd_open() while the
> >>> pid has been recycled etc.).
> >>> A race could only appear in sequences where waiting happens before
> >>> pidfd_open():
> >>>
> >>> pid = fork();
> >>> if (pid == 0)
> >>> 	exit(EXIT_SUCCESS);
> >>> waitid(pid, ...);
> >>> pidfd = pidfd_open(pid, 0);
> >>>
> >>> which honestly simply doesn't make any sense. So if you're the parent
> >>> and you combine fork() + pidfd_open() correctly things should be fine
> >>> without even having to verify via pidfd_send_signal() (I missed that in
> >>> my first mail.).
> >>
> >> Thanks for the additional detail.
> > 
> > You're very welcome.
> > 
> >>
> >> I added the following to the pidfd_open() page, to
> >> prevent people making the same thinko as me:
> >>
> >>        The following code sequence can be used to obtain a file  descrip‐
> >>        tor for the child of fork(2):
> >>
> >>            pid = fork();
> >>            if (pid > 0) {     /* If parent */
> >>                pidfd = pidfd_open(pid, 0);
> >>                ...
> >>            }
> >>
> >>        Even  if  the  child process has already terminated by the time of
> >>        the pidfd_open() call, the returned file descriptor is  guaranteed
> >>        to refer to the child because the parent has not yet waited on the
> >>        child (and therefore, the child's ID has not been recycled).
> > 
> > Thanks! I'm fine with the example. The code illustrates the basics. If
> > you want to go overboard, you can mention my callback example and put my
> > SIG_IGN code snippet from my earlier mails (cf. [1] and [2]) in there.
> > But imho, that'll complicate the manpage and I'm not sure it's worth it.
> 
> I agree that we should not complicate this discussion with more code,
> but how about we refine the text as follows:
> 
>        The following code sequence can be used to obtain a file  descrip‐
>        tor for the child of fork(2):
> 
>            pid = fork();
>            if (pid > 0) {     /* If parent */
>                pidfd = pidfd_open(pid, 0);
>                ...
>            }
> 
>        Even  if  the  child  has  already  terminated  by the time of the
>        pidfd_open() call, its PID will not have  been  recycled  and  the
>        returned  file  descriptor  will  refer  to  the  resulting zombie
>        process.  Note, however, that this is guaranteed only if the  fol‐
>        lowing conditions hold true:
> 
>        *  the  disposition  of  SIGCHLD  has  not  been explicitly set to
>           SIG_IGN (see sigaction(2)); and

Ugh, I forgot a third one. There's also SA_NOCLDWAIT. When set and
the SIGCHLD handler is set to SIG_DFL then no zombie processes are
created and no SIGCHLD signal is sent. When an explicit handler for
SIGCHLD is set then a SIGCHLD signal is generated but the process will
still not be turned into a zombie...

> 
>        *  the zombie process was not  reaped  elsewhere  in  the  program
>           (e.g.,  either  by an asynchronously executed signal handler or
>           by wait(2) or similar in another thread).
> 
>        If these conditions don't hold true, then the child process should

"If any of these conditions does not hold, the child process..."

That might be clearer. But I leave the call on that to you. :)

Christian