public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Luca Boccassi <bluca@debian.org>
To: libc-alpha@sourceware.org
Subject: Re: [PATCH v8 0/7] Add pidfd and cgroupv2 support for process creation
Date: Mon, 28 Aug 2023 13:52:42 +0100	[thread overview]
Message-ID: <a63c53f221eb68d9e7153fe2679763d3d4f667d0.camel@debian.org> (raw)
In-Reply-To: <401e2f03-08e0-6edf-74d2-1be94e95a79e@linaro.org>

[-- Attachment #1: Type: text/plain, Size: 2754 bytes --]

> On 18/08/23 14:51, Rich Felker wrote:
> > On Fri, Aug 18, 2023 at 11:06:35AM -0300, Adhemerval Zanella via
> Libc-alpha wrote:
> >> The glibc 2.36 added wrappers for Linux syscall pidfd_open,
> pidfd_getfd,
> >> and pidfd_send_signal, and exported the P_PIDFD to use along with
> >> waitid. The pidfd is a race-free interface, however, the
> pidfd_open is
> >> subject to TOCTOU if the file descriptor is not obtained directly
> from
> >> the clone or clone3 syscall (there is still a small window between
> the
> >> clone return and the pidfd_getfd where the process can be reaped
> and the
> >> process ID reused).
> > 
> > Unless I'm missing something, that window is purely programmer
> error.
> > The pid belongs to the parent process, that called fork,
> posix_spawn,
> > clone, or whatever, and is responsible for not freeing it until
> it's
> > done using it.
> > 
> > Yes this can happen if you install a SIGCHLD handler that reaps
> > anything it sees, or if you're calling wait without a pid. This is
> > programming error. If you're stuck with code outside your control
> that
> > makes that mistake, you can already avoid it with clone by setting
> the
> > child exit signal to 0 rather than SIGCHLD. But it's best just not
> to
> > do that.
> > 
> 
> Yes, this is the issue GNOME is having with their code base [1] and
> that
> motivated this new interface.  Systemd also seems to be interested in
> these interface, although I am not sure if it is also subject to same
> issue.
> 
> I don't have a strong opinion whether this should be considered a
> solid
> reason to provide a new API, another option would to close BZ#30349
> [2] 
> as wontfix with this rationale.  However, this does not really
> provide 
> an workaround, and worse it will pass the idea that to fully resolve
> it 
> you will need either to allow the racy condition or issue clone
> directly.

These are real race conditions, that cannot be solved otherwise,
characterizing them as 'programming errors' is very misleading and
wrong.

We very much need both of those interfaces in systemd, and fully intend
to use them as soon as they are available. We are slowly moving towards
using pidfds everywhere to be able to do end-to-end race-free process
tracking and management, and these are fundamental pieces for this
effort. From what I can read the GNOME developers feel the same way,
and I wouldn't be surprised if QT followed suit too given what you
mentioned in the cover letter.

Surely implementing useful, core functionality for the direct and
immediate benefit of 3 major Linux projects is a reason as solid as you
could ever find to add a new interface.

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2023-08-28 12:52 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-18 14:06 Adhemerval Zanella
2023-08-18 14:06 ` [PATCH v8 1/7] arm: Add the clone3 wrapper Adhemerval Zanella
2023-08-18 14:06 ` [PATCH v8 2/7] mips: " Adhemerval Zanella
2023-08-18 14:06 ` [PATCH v8 3/7] linux: Define __ASSUME_CLONE3 to 0 for alpha, ia64, nios2, sh, and sparc Adhemerval Zanella
2023-08-24  6:06   ` Florian Weimer
2023-08-18 14:06 ` [PATCH v8 4/7] linux: Add posix_spawnattr_{get,set}cgroup_np (BZ 26731) Adhemerval Zanella
2023-08-24  7:00   ` Florian Weimer
2023-08-18 14:06 ` [PATCH v8 5/7] posix: Add pidfd_spawn and pidfd_spawnp (BZ 30349) Adhemerval Zanella
2023-08-24  7:13   ` Florian Weimer
2023-08-24 15:43     ` Adhemerval Zanella Netto
2023-08-24 17:00       ` Florian Weimer
2023-08-24 17:10         ` Adhemerval Zanella Netto
2023-08-24 18:18           ` Florian Weimer
2023-08-24 18:22             ` Adhemerval Zanella Netto
2023-08-25 10:38               ` Florian Weimer
2023-08-25 16:37                 ` Adhemerval Zanella Netto
2023-08-18 14:06 ` [PATCH v8 6/7] posix: Add fork_np (BZ 26371) Adhemerval Zanella
2023-08-24  6:07   ` Florian Weimer
2023-08-18 14:06 ` [PATCH v8 7/7] linux: Add pidfd_getpid Adhemerval Zanella
2023-08-24  7:53   ` Florian Weimer
2023-08-18 17:51 ` [PATCH v8 0/7] Add pidfd and cgroupv2 support for process creation Rich Felker
2023-08-18 18:34   ` Adhemerval Zanella Netto
2023-08-28 12:52     ` Luca Boccassi [this message]
2023-08-28 13:21       ` Florian Weimer
2023-08-28 13:50         ` Luca Boccassi
2023-08-21  6:53   ` Florian Weimer
2023-08-21 13:55     ` Rich Felker
2023-08-24  7:25       ` Florian Weimer
2023-08-24 12:21         ` Rich Felker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a63c53f221eb68d9e7153fe2679763d3d4f667d0.camel@debian.org \
    --to=bluca@debian.org \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).