From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2d.google.com (mail-oa1-x2d.google.com [IPv6:2001:4860:4864:20::2d]) by sourceware.org (Postfix) with ESMTPS id 04EDF3858C53 for ; Thu, 24 Aug 2023 15:43:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 04EDF3858C53 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oa1-x2d.google.com with SMTP id 586e51a60fabf-1a1fa977667so4610982fac.1 for ; Thu, 24 Aug 2023 08:43:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1692891800; x=1693496600; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=vj902B8vBUVYBs4BJwLNqKPs/Q9zwxeIZWQlCFsE6eA=; b=lkYjYC/xzEB86oQuf7SGJYFdkplyLFsJkM9qnc6dNB0CIrEEijOO33wOc8yI9dx+5j LNKnIAlcMUpgZWq3yKj6ymd2ZQdJZg5GdDNk3bK9UU6HwzyaaCF5D4SdXb+pasFGb8Cd fF4RYCXjnaAIAWIVhXivqOPBGAxTr1L9aMnoBcVAkVFI4kxx2pAPQ+kFS2I8zTQ+H9oW jy5vBWCdY8JHRaFUl5yTgZNINnwjNR+pLgYBD/fCG9BauzvxgOFmO2lTOqAYfucllcFt Fb9cBUvb1uqRAqrb+BWUhktyjCxCsavCvWDIphLP4VHGbt/4YXUotEGm+ioAOEMdmkMB 9vfQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692891800; x=1693496600; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=vj902B8vBUVYBs4BJwLNqKPs/Q9zwxeIZWQlCFsE6eA=; b=aMclyggqTLTcIGVRaWLAiV8ZbWXk23y7c3RNzslHR6fhHE1r6FZ4AwUwtm4QA0EDCv 4UY1iu4alXZPMn/lykmn8w7Kx+0ETb3c17W7dvsRWAIQlkofh40F4KYui2P+qpB92jqE 1cvneW5WJj0u/RW9YkVFNPDX0XleN5la/kGfEVFiKX4v03Bj3ZovBBHe4qPa1wR3nJun lyrYTxorFRw4cxpbg56FKpUao94yAcQbTGi8mPxhvG/JKJJbDWAxMDlhlt7gj8zhKIHD BuLWhhfT4GezLOWpIESsWhIcmts9XDaEFU1bj/FdQUVal4POuLHmAyapwckTBxOZimMc OvMg== X-Gm-Message-State: AOJu0YxniJOCpjQ2GHtSIuCnhWQShK5cvX4xoGbOKz1ZJLtMt6DqDn32 /bxaaqOCeKERK4h4V8+fE4Ric8zJwRCQGQ1mpl/8+w== X-Google-Smtp-Source: AGHT+IE3yuSshuc0sfIt4ekMQKVOIyGCKbFVIY62DIZpgr07czRfibUgIInX8ucGEmYY+rXvO/lWiQ== X-Received: by 2002:a05:6870:14d0:b0:1b0:293e:f8f3 with SMTP id l16-20020a05687014d000b001b0293ef8f3mr96554oab.53.1692891800268; Thu, 24 Aug 2023 08:43:20 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c2:c275:d8f:2562:4517:f8f5? ([2804:1b3:a7c2:c275:d8f:2562:4517:f8f5]) by smtp.gmail.com with ESMTPSA id v2-20020a056870e28200b001b3d67934e9sm8248039oad.26.2023.08.24.08.43.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 24 Aug 2023 08:43:19 -0700 (PDT) Message-ID: <095993ea-8773-fafd-7d0c-4750517c76e9@linaro.org> Date: Thu, 24 Aug 2023 12:43:17 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.14.0 Subject: Re: [PATCH v8 5/7] posix: Add pidfd_spawn and pidfd_spawnp (BZ 30349) Content-Language: en-US To: Florian Weimer Cc: libc-alpha@sourceware.org References: <20230818140642.1623571-1-adhemerval.zanella@linaro.org> <20230818140642.1623571-6-adhemerval.zanella@linaro.org> <875y5429r4.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <875y5429r4.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 24/08/23 04:13, Florian Weimer wrote: > * Adhemerval Zanella: > >> Returning a pidfd allows a process to keep a race-free handle for a >> child process, otherwise, the caller will need to either use pidfd_open >> (which still might be subject to TOCTOU) or keep the old racy interface >> base on pid_t. >> >> The implementation makes sure that kernel must support the complete >> pidfd interface, meaning that waitid (P_PIDFD) should be supported >> (added on Linux 5.4). It ensures that a non-racy workaround is required >> (such as reading procfs fdinfo pid to use along with wait interfaces). > > Sorry, I don't understand the second sentence. It is indeed confusing, I will change to: To correctly use pifd_spawn, the kernel must support not only returning the pidfd with clone/clone3 but also waitid (P_PIDFD) (added on Linux 5.4). If the kernel does not support the waitid, pidfd returns ENOSYS. It avoids the need for racy workarounds, such as reading the procfs fdinfo to get the pid to use along with other wait interfaces. > >> diff --git a/posix/tst-spawn3.c b/posix/tst-spawn3.c >> index e7ce0fb386..64052dc911 100644 >> --- a/posix/tst-spawn3.c >> +++ b/posix/tst-spawn3.c >> @@ -16,6 +16,7 @@ >> License along with the GNU C Library; if not, see >> . */ >> >> +#include > > Please use TEST_VERIFY_EXIT, see below. > >> @@ -75,75 +78,82 @@ do_test (void) >> FAIL_EXIT1 ("create_temp_file: %m"); >> break; >> } >> - files[nfiles++] = fd; >> + files[nfiles] = fd; >> } >> + assert (nfiles != 0); > > TEST_VERIFY_EXIT (nfiles != 0); Ack. > >> diff --git a/sysdeps/unix/sysv/linux/bits/spawn_ext.h b/sysdeps/unix/sysv/linux/bits/spawn_ext.h >> index a3aa020d5c..3254cfe9be 100644 >> --- a/sysdeps/unix/sysv/linux/bits/spawn_ext.h >> +++ b/sysdeps/unix/sysv/linux/bits/spawn_ext.h >> @@ -37,4 +37,35 @@ extern int posix_spawnattr_setcgroup_np (posix_spawnattr_t *__attr, >> >> #endif /* __USE_MISC */ >> >> +#ifdef __USE_GNU > > Please use __USE_MISC, so this is available with _DEFAULT_SOURCE (like > the cgroups functions). Ack. > >> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c >> index f0d4c62ae6..d4ff23d955 100644 >> --- a/sysdeps/unix/sysv/linux/spawni.c >> +++ b/sysdeps/unix/sysv/linux/spawni.c > >> internal_signal_block_all (&args.oldmask); >> @@ -386,13 +399,16 @@ __spawnix (pid_t * pid, const char *file, >> /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by >> __clone_internal_fallback. */ >> .flags = (set_cgroup ? CLONE_INTO_CGROUP : 0) >> + | (use_pidfd ? CLONE_PIDFD : 0) >> | CLONE_CLEAR_SIGHAND >> | CLONE_VM >> | CLONE_VFORK, >> .exit_signal = SIGCHLD, >> .stack = (uintptr_t) stack, >> .stack_size = stack_size, >> - .cgroup = (set_cgroup ? attrp->__cgroup : 0) >> + .cgroup = (set_cgroup ? attrp->__cgroup : 0), >> + .pidfd = use_pidfd ? (uintptr_t) &args.pidfd : 0, >> + .parent_tid = use_pidfd ? (uintptr_t) &args.pidfd : 0, > > The .parent_tid line looks wrong? It is required for clone (and that's why you can't use CLONE_PIDFD with CLONE_PARENT_SETTID). It could only set parent_tid on clone fallback, but I think this is simpler. I will add a comment.