Message ID | 20240129112313.GA11635@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-42610-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:2087:b0:106:209c:c626 with SMTP id gs7csp497622dyb; Mon, 29 Jan 2024 03:25:10 -0800 (PST) X-Google-Smtp-Source: AGHT+IE9S9yyB/nUrnhcrd7BNIX1nTbtWAQA4y2H498obT0/Tv5BaDf1zGv1Rz/WPEh+8vd6iVHq X-Received: by 2002:a05:6402:1657:b0:55f:2958:7b26 with SMTP id s23-20020a056402165700b0055f29587b26mr41242edx.8.1706527509843; Mon, 29 Jan 2024 03:25:09 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1706527509; cv=pass; d=google.com; s=arc-20160816; b=pmBpmjz3Ood/MnsnDSba1eBFzMj6YmeTHcJhLcT8FVL8bxTpJa6ax4pkecmFF4qHhd 2ZeFeu9JyNeLyWSMgpnnwKuZhRjU1ntOyWG26Q8LmRHlZKQFicVwEMGpAKPTl3Oj5pBq BnJhngtF2nACgcvRuZ0CV9Ce+3Tn8wk0BFPIE1JE7G1uVQvwcXYaGxvMuYPoTjO65erE 8vtciWS7uXmQJraqQq8/7STqgMwdWwNv1/Cu/9uU/W+ibw8swq5lBEtsqI35yhpOda0H BYigTnEHtu5jsN5KPsURA8fKBZC9nTrGBCtG11Va3Ccm4Qd7Bolm44bnsJsiSGfGTWqw tiIg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version :list-unsubscribe:list-subscribe:list-id:precedence:references :message-id:subject:cc:to:from:date:dkim-signature; bh=JNer/v8iFw3iF09xSTRkhGhoZsEuyh12Q2EV6DHDyCc=; fh=5x8jKdugBbn2PDPPoX0J3BkHEP5mHA4Sg8E/8psICO4=; b=P3HWEH6g2AaRoopdpoJ/91DtCKzH7I7aj0QwdYOgNwNFSWMgjcOvKIMFKkNEzCG61m U6WpdvCygYFMPHdHp6TXJvY49PXCAqWUDoLa0I7G+8T0NB05G35AiqrEn8ZvuSADdmpD /qoeNGRIH1JLlXc1s+JcEdtJN0sB7NOpBkVfr0d2swo+gP5CJ7jdk3BDHO1NEyXHEStr InjEZzr2piljtlMuZCTdcYm2LTaieCqSmLtB2YffVeieiauApA79w9BsZ51zp+EEFBWs 1XU1G8s4+wH4piYnLQnSWegbRUwRqjE1de94vJadJFvgvqGaa6BiAJtRgHaN8unb2xPz sCuQ== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eTbSexiS; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-42610-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-42610-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id ew11-20020a056402538b00b0055c92372da6si3549000edb.483.2024.01.29.03.25.09 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Jan 2024 03:25:09 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-42610-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eTbSexiS; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-42610-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-42610-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 4A8CB1F23924 for <ouuuleilei@gmail.com>; Mon, 29 Jan 2024 11:25:09 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id AD4CB5DF3D; Mon, 29 Jan 2024 11:24:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="eTbSexiS" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A72AB5C5E0 for <linux-kernel@vger.kernel.org>; Mon, 29 Jan 2024 11:24:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706527481; cv=none; b=cygUu92FXOQgtyvRG9yOEnBX/SgyLqW7uSleblzMmxwPwMN5fRDHe9sO/dEXNG4mc47gfi9S90IEGVd/peMG8KE3+rDW4jTnxqvgiESyrtXNGERxDrfamDWcB6tFFIiSNPM5kKLNgw2btUYgNXbAYgkEoLuordNyZytlkn0O8UQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706527481; c=relaxed/simple; bh=M1eZFkZenuGschG9LJXhONAKU2yn51nzZ4is/X0qaLA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Wslvh4UuvzLB5ohlSsn7Xx+Ds8Lj8b26C8k7jB74SixicJ7SfMnGPfx8TMyDQquPyNzr69Nox6/OlSPhC809X+pgs9Qn4+afWsq0BGVXnMJzNW27OXGtMyuJKITzGpyBujKHNJNJZYEHpp9rCul7kKGRaCJoGFmKV3fXjqQliJA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=eTbSexiS; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1706527478; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=JNer/v8iFw3iF09xSTRkhGhoZsEuyh12Q2EV6DHDyCc=; b=eTbSexiSrAvOctlVTVm5cb9NEvWJ0N8+TxKa568yj3jhWDwwk75xcA6RfoOZRdsUcsviUc roim5TJZFPLVb9f09YaO0HnqPt9FLtrMmChx+oQfKCrgHPHHOPHS7+h/VbUMIoAcq3ROnN 5QzDWNtFvLgU8ody9ueCnRuMgQPrGEU= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-620-Phvqwf0aNS-LDx8ZAaLthQ-1; Mon, 29 Jan 2024 06:24:32 -0500 X-MC-Unique: Phvqwf0aNS-LDx8ZAaLthQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3CB933806635; Mon, 29 Jan 2024 11:24:32 +0000 (UTC) Received: from dhcp-27-174.brq.redhat.com (unknown [10.45.225.247]) by smtp.corp.redhat.com (Postfix) with SMTP id A7502152E; Mon, 29 Jan 2024 11:24:30 +0000 (UTC) Received: by dhcp-27-174.brq.redhat.com (nbSMTP-1.00) for uid 1000 oleg@redhat.com; Mon, 29 Jan 2024 12:23:17 +0100 (CET) Date: Mon, 29 Jan 2024 12:23:15 +0100 From: Oleg Nesterov <oleg@redhat.com> To: Tycho Andersen <tycho@tycho.pizza> Cc: Christian Brauner <brauner@kernel.org>, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, Tycho Andersen <tandersen@netflix.com>, "Eric W. Biederman" <ebiederm@xmission.com> Subject: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open() Message-ID: <20240129112313.GA11635@redhat.com> References: <ZbArN3EYRfhrNs3o@tycho.pizza> <20240125140830.GA5513@redhat.com> <ZbQpPknTTCyiyxrP@tycho.pizza> <20240127105410.GA13787@redhat.com> <ZbUngjQMg+YUBAME@tycho.pizza> <20240127163117.GB13787@redhat.com> <ZbU7d0dpTY08JgIl@tycho.pizza> <20240127193127.GC13787@redhat.com> <ZbVrRgIvudX242ZU@tycho.pizza> <20240127210634.GE13787@redhat.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240127210634.GE13787@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1789423789994056335 X-GMAIL-MSGID: 1789423789994056335 |
Series |
[RFC] pidfd: implement PIDFD_THREAD flag for pidfd_open()
|
|
Commit Message
Oleg Nesterov
Jan. 29, 2024, 11:23 a.m. UTC
On 01/27, Oleg Nesterov wrote: > > I'll (hopefully) send v2 on top of > > pidfd: cleanup the usage of __pidfd_prepare's flags > pidfd: don't do_notify_pidfd() if !thread_group_empty() > > on Monday Sorry, I don't have time to finish v2 today, I need to update the comments and write the changelog. But the patch itself is ready, I am sending it for review. Tycho, Christian, any comments? Oleg. From c31780f6c1136a72048d24701ac6d8401fc1afda Mon Sep 17 00:00:00 2001 From: Oleg Nesterov <oleg@redhat.com> Date: Sat, 27 Jan 2024 16:59:18 +0100 Subject: [PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open() --- include/uapi/linux/pidfd.h | 3 ++- kernel/exit.c | 7 +++++++ kernel/fork.c | 29 +++++++++++++++++++++++++++-- kernel/pid.c | 2 +- kernel/signal.c | 4 +++- 5 files changed, 40 insertions(+), 5 deletions(-)
Comments
On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote: > On 01/27, Oleg Nesterov wrote: > > > > I'll (hopefully) send v2 on top of > > > > pidfd: cleanup the usage of __pidfd_prepare's flags > > pidfd: don't do_notify_pidfd() if !thread_group_empty() > > > > on Monday > > Sorry, I don't have time to finish v2 today, I need to update the comments > and write the changelog. > > But the patch itself is ready, I am sending it for review. > > Tycho, Christian, any comments? > > Oleg. > > > From c31780f6c1136a72048d24701ac6d8401fc1afda Mon Sep 17 00:00:00 2001 > From: Oleg Nesterov <oleg@redhat.com> > Date: Sat, 27 Jan 2024 16:59:18 +0100 > Subject: [PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open() > > --- > include/uapi/linux/pidfd.h | 3 ++- > kernel/exit.c | 7 +++++++ > kernel/fork.c | 29 +++++++++++++++++++++++++++-- > kernel/pid.c | 2 +- > kernel/signal.c | 4 +++- > 5 files changed, 40 insertions(+), 5 deletions(-) > > diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h > index 5406fbc13074..2e6461459877 100644 > --- a/include/uapi/linux/pidfd.h > +++ b/include/uapi/linux/pidfd.h > @@ -7,6 +7,7 @@ > #include <linux/fcntl.h> > > /* Flags for pidfd_open(). */ > -#define PIDFD_NONBLOCK O_NONBLOCK > +#define PIDFD_NONBLOCK O_NONBLOCK > +#define PIDFD_THREAD O_EXCL > > #endif /* _UAPI_LINUX_PIDFD_H */ > diff --git a/kernel/exit.c b/kernel/exit.c > index dfb963d2f862..74fe6bfb9577 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -739,6 +739,13 @@ static void exit_notify(struct task_struct *tsk, int group_dead) > kill_orphaned_pgrp(tsk->group_leader, NULL); > > tsk->exit_state = EXIT_ZOMBIE; > + /* > + * sub-thread or delay_group_leader(), wake up the PIDFD_THREAD > + * waiters. > + */ > + if (!thread_group_empty(tsk)) > + do_notify_pidfd(tsk); > + > if (unlikely(tsk->ptrace)) { > int sig = thread_group_leader(tsk) && > thread_group_empty(tsk) && > diff --git a/kernel/fork.c b/kernel/fork.c > index 347641398f9d..977b58c0eac6 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -101,6 +101,7 @@ > #include <linux/user_events.h> > #include <linux/iommu.h> > #include <linux/rseq.h> > +#include <uapi/linux/pidfd.h> > > #include <asm/pgalloc.h> > #include <linux/uaccess.h> > @@ -2050,6 +2051,8 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) > > seq_put_decimal_ll(m, "Pid:\t", nr); > > + /* TODO: report PIDFD_THREAD */ Ah yes, very good point. We should give userspace an indicator whether something is thread pidfd or not. > + > #ifdef CONFIG_PID_NS > seq_put_decimal_ll(m, "\nNSpid:\t", nr); > if (nr > 0) { > @@ -2068,12 +2071,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) > } > #endif > > +static bool pidfd_task_exited(struct pid *pid, bool thread) > +{ > + struct task_struct *task; > + bool exited; > + > + rcu_read_lock(); > + task = pid_task(pid, PIDTYPE_PID); > + exited = !task || > + (READ_ONCE(task->exit_state) && (thread || thread_group_empty(task))); > + rcu_read_unlock(); > + > + return exited; > +} > + > /* > * Poll support for process exit notification. > */ > static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts) > { > struct pid *pid = file->private_data; > + bool thread = file->f_flags & PIDFD_THREAD; > __poll_t poll_flags = 0; > > poll_wait(file, &pid->wait_pidfd, pts); > @@ -2083,7 +2101,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts) > * If the thread group leader exits before all other threads in the > * group, then poll(2) should block, similar to the wait(2) family. > */ > - if (thread_group_exited(pid)) > + if (pidfd_task_exited(pid, thread)) > poll_flags = EPOLLIN | EPOLLRDNORM; > > return poll_flags; > @@ -2141,6 +2159,11 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re > return PTR_ERR(pidfd_file); > } > get_pid(pid); /* held by pidfd_file now */ > + /* > + * anon_inode_getfile() ignores everything outside of the > + * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually. > + */ > + pidfd_file->f_flags |= (flags & PIDFD_THREAD); > *ret = pidfd_file; > return pidfd; > } > @@ -2173,7 +2196,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re > */ > int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret) > { > - if (!pid || !pid_has_task(pid, PIDTYPE_TGID)) > + bool thread = flags & PIDFD_THREAD; > + > + if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID)); > return -EINVAL; > > return __pidfd_prepare(pid, flags, ret); > diff --git a/kernel/pid.c b/kernel/pid.c > index c7a3e359f8f5..04bdd5ecf183 100644 > --- a/kernel/pid.c > +++ b/kernel/pid.c > @@ -629,7 +629,7 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags) > int fd; > struct pid *p; > > - if (flags & ~PIDFD_NONBLOCK) > + if (flags & ~(PIDFD_NONBLOCK | PIDFD_THREAD)) > return -EINVAL; > > if (pid <= 0) > diff --git a/kernel/signal.c b/kernel/signal.c > index 9561a3962ca6..919cd33a0405 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) > WARN_ON_ONCE(!tsk->ptrace && > (tsk->group_leader != tsk || !thread_group_empty(tsk))); > /* > - * tsk is a group leader and has no threads, wake up the pidfd waiters. > + * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD > + * waiters. > */ > if (thread_group_empty(tsk)) > do_notify_pidfd(tsk); > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > prepare_kill_siginfo(sig, &kinfo); > } > > + /* TODO: respect PIDFD_THREAD */ So I've been thinking about this at the end of last week. Do we need to give userspace a way to send a thread-group wide signal even when a PIDFD_THREAD pidfd is passed? Or should we just not worry about this right now and wait until someone needs this? > ret = kill_pid_info(sig, &kinfo, pid); Otherwise this looks good to me!
On Mon, Jan 29, 2024 at 02:41:11PM +0100, Christian Brauner wrote: > On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote: > > --- a/kernel/signal.c > > +++ b/kernel/signal.c > > @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) > > WARN_ON_ONCE(!tsk->ptrace && > > (tsk->group_leader != tsk || !thread_group_empty(tsk))); > > /* > > - * tsk is a group leader and has no threads, wake up the pidfd waiters. > > + * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD > > + * waiters. > > */ > > if (thread_group_empty(tsk)) > > do_notify_pidfd(tsk); > > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > prepare_kill_siginfo(sig, &kinfo); > > } > > > > + /* TODO: respect PIDFD_THREAD */ > > So I've been thinking about this at the end of last week. Do we need to > give userspace a way to send a thread-group wide signal even when a > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > right now and wait until someone needs this? I don't need it currently, but it would have been handy for some of the tests I wrote. Should I fix those up and send them too on top of Oleg's v2? Tycho
On Mon, Jan 29, 2024 at 07:31:35AM -0700, Tycho Andersen wrote: > On Mon, Jan 29, 2024 at 02:41:11PM +0100, Christian Brauner wrote: > > On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote: > > > --- a/kernel/signal.c > > > +++ b/kernel/signal.c > > > @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) > > > WARN_ON_ONCE(!tsk->ptrace && > > > (tsk->group_leader != tsk || !thread_group_empty(tsk))); > > > /* > > > - * tsk is a group leader and has no threads, wake up the pidfd waiters. > > > + * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD > > > + * waiters. > > > */ > > > if (thread_group_empty(tsk)) > > > do_notify_pidfd(tsk); > > > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > > prepare_kill_siginfo(sig, &kinfo); > > > } > > > > > > + /* TODO: respect PIDFD_THREAD */ > > > > So I've been thinking about this at the end of last week. Do we need to > > give userspace a way to send a thread-group wide signal even when a > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > right now and wait until someone needs this? > > I don't need it currently, but it would have been handy for some of > the tests I wrote. > > Should I fix those up and send them too on top of Oleg's v2? Sure, I don't mind.
On 01/29, Christian Brauner wrote: > > On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote: > > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > > prepare_kill_siginfo(sig, &kinfo); > > } > > > > + /* TODO: respect PIDFD_THREAD */ > > So I've been thinking about this at the end of last week. Do we need to > give userspace a way to send a thread-group wide signal even when a > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > right now and wait until someone needs this? I don't know. I am fine either way, but I think this needs a separate patch and another discussion in any case. Anyway should be trivial, pidfd_send_signal() has the "flags" argument. On a related note, should copy_process(CLONE_PIDFD | CLONE_THREAD) add PIDFD_THREAD flag "automatically" depending on CLONE_THREAD? Or do we want another CLONE_PIDFD_THREAD flag so that PIDFD_THREAD can be used without CLONE_THREAD? Again, I do not know, needs another discussion. > Otherwise this looks good to me! OK, thanks, I'll send v2 in a minute. The patch is the same, I only updated the comments. Oleg.
On Mon, Jan 29, 2024 at 3:24 AM Oleg Nesterov <oleg@redhat.com> wrote: > > On 01/27, Oleg Nesterov wrote: > > > > I'll (hopefully) send v2 on top of > > > > pidfd: cleanup the usage of __pidfd_prepare's flags > > pidfd: don't do_notify_pidfd() if !thread_group_empty() > > > > on Monday > > Sorry, I don't have time to finish v2 today, I need to update the comments > and write the changelog. > > But the patch itself is ready, I am sending it for review. > > Tycho, Christian, any comments? Right now, pidfd_send_signal() sends signals to processes, like so: * The syscall currently only signals via PIDTYPE_PID which covers * kill(<positive-pid>, <signal>. It does not signal threads or process * groups. This patch adds PIDFD_THREAD which, potentially confusingly, doesn't change this (AFAICS). So at least that should be documented loudly and clearly, IMO. But I actually just bumped in to this limitation in pidfd_send_signal(), like so: https://github.com/systemd/systemd/issues/31093 Specifically, systemd can't properly emulate Ctrl-C using pidfd_send_signal(). I don't know whether implementing the other signal types belongs as part of this patch, but they're at least thematically related. --Andy
On 01/31, Andy Lutomirski wrote: > > Right now, pidfd_send_signal() sends signals to processes, like so: > > * The syscall currently only signals via PIDTYPE_PID which covers > * kill(<positive-pid>, <signal>. It does not signal threads or process > * groups. > > This patch adds PIDFD_THREAD which, potentially confusingly, doesn't > change this (AFAICS). Yes, > So at least that should be documented loudly > and clearly, IMO. Please note /* TODO: respect PIDFD_THREAD */ this patch adds into pidfd_send_signal(). See also this part of discussion > > + /* TODO: respect PIDFD_THREAD */ > > So I've been thinking about this at the end of last week. Do we need to > give userspace a way to send a thread-group wide signal even when a > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > right now and wait until someone needs this? I don't know. I am fine either way, but I think this needs a separate patch and another discussion in any case. Anyway should be trivial, pidfd_send_signal() has the "flags" argument. with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ Or did I misunderstand you? Oleg.
Forgot to mention... And I agree that pidfd_send_signal(flags => PGID/SID) can make some sense too. But this a) doesn't depend on PIDFD_THREAD, and b) needs another patch/discussion. But again, I am not sure I understood you correctly. On 01/31, Oleg Nesterov wrote: > > On 01/31, Andy Lutomirski wrote: > > > > Right now, pidfd_send_signal() sends signals to processes, like so: > > > > * The syscall currently only signals via PIDTYPE_PID which covers > > * kill(<positive-pid>, <signal>. It does not signal threads or process > > * groups. > > > > This patch adds PIDFD_THREAD which, potentially confusingly, doesn't > > change this (AFAICS). > > Yes, > > > So at least that should be documented loudly > > and clearly, IMO. > > Please note > > /* TODO: respect PIDFD_THREAD */ > > this patch adds into pidfd_send_signal(). > > See also this part of discussion > > > > + /* TODO: respect PIDFD_THREAD */ > > > > So I've been thinking about this at the end of last week. Do we need to > > give userspace a way to send a thread-group wide signal even when a > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > right now and wait until someone needs this? > > I don't know. I am fine either way, but I think this needs a separate > patch and another discussion in any case. Anyway should be trivial, > pidfd_send_signal() has the "flags" argument. > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ > > Or did I misunderstand you? > > Oleg.
> On 01/31, Oleg Nesterov wrote: > > > > On 01/31, Andy Lutomirski wrote: > > Please note > > > > /* TODO: respect PIDFD_THREAD */ > > > > this patch adds into pidfd_send_signal(). > > > > See also this part of discussion > > > > > > + /* TODO: respect PIDFD_THREAD */ > > > > > > So I've been thinking about this at the end of last week. Do we need to > > > give userspace a way to send a thread-group wide signal even when a > > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > > right now and wait until someone needs this? > > > > I don't know. I am fine either way, but I think this needs a separate > > patch and another discussion in any case. Anyway should be trivial, > > pidfd_send_signal() has the "flags" argument. > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ I missed that. Whoops. On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote: > > Forgot to mention... > > And I agree that pidfd_send_signal(flags => PGID/SID) can make > some sense too. > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another > patch/discussion. > > But again, I am not sure I understood you correctly. > Hmm. When one works with regular (non-fd) pids / pgids etc, one specifies the signal domain at the time that one sends the signal. I don't know what pidfds should do. It seems a bit inefficient for anything that wants a pidfd and might send a signal in a different mode in the future to have to hold on to multiple pidfds, so it probably should be a pidfd_send_signal flag. Which leaves the question of what the default should be. Should pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process or the thread? I guess there are two reasonable solutions: 1. flags = 0 always means process. And maybe there's a special flag to send a signal that matches the pidfd type, or maybe not. 2. flags = 0 does what the pidfd seems to imply, and a new PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the pidfd is PIDFD_THREAD. Do any of you have actual use cases in mind where one choice is clearly better than the other choice? --Andy
On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote: > > On 01/31, Oleg Nesterov wrote: > > > > > > On 01/31, Andy Lutomirski wrote: > > > Please note > > > > > > /* TODO: respect PIDFD_THREAD */ > > > > > > this patch adds into pidfd_send_signal(). > > > > > > See also this part of discussion > > > > > > > > + /* TODO: respect PIDFD_THREAD */ > > > > > > > > So I've been thinking about this at the end of last week. Do we need to > > > > give userspace a way to send a thread-group wide signal even when a > > > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > > > right now and wait until someone needs this? > > > > > > I don't know. I am fine either way, but I think this needs a separate > > > patch and another discussion in any case. Anyway should be trivial, > > > pidfd_send_signal() has the "flags" argument. > > > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ > > I missed that. Whoops. > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > Forgot to mention... > > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make > > some sense too. > > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another > > patch/discussion. > > > > But again, I am not sure I understood you correctly. > > > > Hmm. > > When one works with regular (non-fd) pids / pgids etc, one specifies > the signal domain at the time that one sends the signal. I don't know > what pidfds should do. It seems a bit inefficient for anything that > wants a pidfd and might send a signal in a different mode in the > future to have to hold on to multiple pidfds, so it probably should be > a pidfd_send_signal flag. > > Which leaves the question of what the default should be. Should > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process > or the thread? I guess there are two reasonable solutions: > > 1. flags = 0 always means process. And maybe there's a special flag > to send a signal that matches the pidfd type, or maybe not. > > 2. flags = 0 does what the pidfd seems to imply, and a new > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the > pidfd is PIDFD_THREAD. > > Do any of you have actual use cases in mind where one choice is > clearly better than the other choice? So conceptually I think having the type of pidfd dictate the default scope of the signal is the most elegant approach. And then very likely we should just have: PIDFD_SIGNAL_THREAD PIDFD_SIGNAL_THREAD_GROUP PIDFD_SIGNAL_PROCESS_GROUP I think for userspace it doesn't really matter as long as we clearly document what's going on. Thoughts?
On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote: > > On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote: > > > On 01/31, Oleg Nesterov wrote: > > > > > > > > On 01/31, Andy Lutomirski wrote: > > > > Please note > > > > > > > > /* TODO: respect PIDFD_THREAD */ > > > > > > > > this patch adds into pidfd_send_signal(). > > > > > > > > See also this part of discussion > > > > > > > > > > + /* TODO: respect PIDFD_THREAD */ > > > > > > > > > > So I've been thinking about this at the end of last week. Do we need to > > > > > give userspace a way to send a thread-group wide signal even when a > > > > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > > > > right now and wait until someone needs this? > > > > > > > > I don't know. I am fine either way, but I think this needs a separate > > > > patch and another discussion in any case. Anyway should be trivial, > > > > pidfd_send_signal() has the "flags" argument. > > > > > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ > > > > I missed that. Whoops. > > > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > Forgot to mention... > > > > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make > > > some sense too. > > > > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another > > > patch/discussion. > > > > > > But again, I am not sure I understood you correctly. > > > > > > > Hmm. > > > > When one works with regular (non-fd) pids / pgids etc, one specifies > > the signal domain at the time that one sends the signal. I don't know > > what pidfds should do. It seems a bit inefficient for anything that > > wants a pidfd and might send a signal in a different mode in the > > future to have to hold on to multiple pidfds, so it probably should be > > a pidfd_send_signal flag. > > > > Which leaves the question of what the default should be. Should > > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process > > or the thread? I guess there are two reasonable solutions: > > > > 1. flags = 0 always means process. And maybe there's a special flag > > to send a signal that matches the pidfd type, or maybe not. > > > > 2. flags = 0 does what the pidfd seems to imply, and a new > > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the > > pidfd is PIDFD_THREAD. > > > > Do any of you have actual use cases in mind where one choice is > > clearly better than the other choice? > > So conceptually I think having the type of pidfd dictate the default > scope of the signal is the most elegant approach. And then very likely > we should just have: > > PIDFD_SIGNAL_THREAD > PIDFD_SIGNAL_THREAD_GROUP > PIDFD_SIGNAL_PROCESS_GROUP > > I think for userspace it doesn't really matter as long as we clearly > document what's going on. > This seems reasonable unless we're likely to end up with a pidfd mode that doesn't actually make sense in a send_signal context. But I'm not immediately seeing any reason that that would happen. --Andy
On Wed, Jan 31, 2024 at 11:50:23AM -0800, Andy Lutomirski wrote: > On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote: > > > > On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote: > > > > On 01/31, Oleg Nesterov wrote: > > > > > > > > > > On 01/31, Andy Lutomirski wrote: > > > > > Please note > > > > > > > > > > /* TODO: respect PIDFD_THREAD */ > > > > > > > > > > this patch adds into pidfd_send_signal(). > > > > > > > > > > See also this part of discussion > > > > > > > > > > > > + /* TODO: respect PIDFD_THREAD */ > > > > > > > > > > > > So I've been thinking about this at the end of last week. Do we need to > > > > > > give userspace a way to send a thread-group wide signal even when a > > > > > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > > > > > right now and wait until someone needs this? > > > > > > > > > > I don't know. I am fine either way, but I think this needs a separate > > > > > patch and another discussion in any case. Anyway should be trivial, > > > > > pidfd_send_signal() has the "flags" argument. > > > > > > > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ > > > > > > I missed that. Whoops. > > > > > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > Forgot to mention... > > > > > > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make > > > > some sense too. > > > > > > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another > > > > patch/discussion. > > > > > > > > But again, I am not sure I understood you correctly. > > > > > > > > > > Hmm. > > > > > > When one works with regular (non-fd) pids / pgids etc, one specifies > > > the signal domain at the time that one sends the signal. I don't know > > > what pidfds should do. It seems a bit inefficient for anything that > > > wants a pidfd and might send a signal in a different mode in the > > > future to have to hold on to multiple pidfds, so it probably should be > > > a pidfd_send_signal flag. > > > > > > Which leaves the question of what the default should be. Should > > > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process > > > or the thread? I guess there are two reasonable solutions: > > > > > > 1. flags = 0 always means process. And maybe there's a special flag > > > to send a signal that matches the pidfd type, or maybe not. > > > > > > 2. flags = 0 does what the pidfd seems to imply, and a new > > > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the > > > pidfd is PIDFD_THREAD. > > > > > > Do any of you have actual use cases in mind where one choice is > > > clearly better than the other choice? > > > > So conceptually I think having the type of pidfd dictate the default > > scope of the signal is the most elegant approach. And then very likely > > we should just have: > > > > PIDFD_SIGNAL_THREAD > > PIDFD_SIGNAL_THREAD_GROUP > > PIDFD_SIGNAL_PROCESS_GROUP > > > > I think for userspace it doesn't really matter as long as we clearly > > document what's going on. > > > > This seems reasonable unless we're likely to end up with a pidfd mode > that doesn't actually make sense in a send_signal context. But I'm > not immediately seeing any reason that that would happen. Yeah, I think that's very unlikely and we could reject it obased on api design considerations.
On Thu, Feb 01, 2024 at 02:30:46PM +0100, Christian Brauner wrote: > On Wed, Jan 31, 2024 at 11:50:23AM -0800, Andy Lutomirski wrote: > > On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote: > > > > > On 01/31, Oleg Nesterov wrote: > > > > > > > > > > > > On 01/31, Andy Lutomirski wrote: > > > > > > Please note > > > > > > > > > > > > /* TODO: respect PIDFD_THREAD */ > > > > > > > > > > > > this patch adds into pidfd_send_signal(). > > > > > > > > > > > > See also this part of discussion > > > > > > > > > > > > > > + /* TODO: respect PIDFD_THREAD */ > > > > > > > > > > > > > > So I've been thinking about this at the end of last week. Do we need to > > > > > > > give userspace a way to send a thread-group wide signal even when a > > > > > > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > > > > > > right now and wait until someone needs this? > > > > > > > > > > > > I don't know. I am fine either way, but I think this needs a separate > > > > > > patch and another discussion in any case. Anyway should be trivial, > > > > > > pidfd_send_signal() has the "flags" argument. > > > > > > > > > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ > > > > > > > > I missed that. Whoops. > > > > > > > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > > > Forgot to mention... > > > > > > > > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make > > > > > some sense too. > > > > > > > > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another > > > > > patch/discussion. > > > > > > > > > > But again, I am not sure I understood you correctly. > > > > > > > > > > > > > Hmm. > > > > > > > > When one works with regular (non-fd) pids / pgids etc, one specifies > > > > the signal domain at the time that one sends the signal. I don't know > > > > what pidfds should do. It seems a bit inefficient for anything that > > > > wants a pidfd and might send a signal in a different mode in the > > > > future to have to hold on to multiple pidfds, so it probably should be > > > > a pidfd_send_signal flag. > > > > > > > > Which leaves the question of what the default should be. Should > > > > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process > > > > or the thread? I guess there are two reasonable solutions: > > > > > > > > 1. flags = 0 always means process. And maybe there's a special flag > > > > to send a signal that matches the pidfd type, or maybe not. > > > > > > > > 2. flags = 0 does what the pidfd seems to imply, and a new > > > > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the > > > > pidfd is PIDFD_THREAD. > > > > > > > > Do any of you have actual use cases in mind where one choice is > > > > clearly better than the other choice? > > > > > > So conceptually I think having the type of pidfd dictate the default > > > scope of the signal is the most elegant approach. And then very likely > > > we should just have: > > > > > > PIDFD_SIGNAL_THREAD > > > PIDFD_SIGNAL_THREAD_GROUP > > > PIDFD_SIGNAL_PROCESS_GROUP > > > > > > I think for userspace it doesn't really matter as long as we clearly > > > document what's going on. > > > > > > > This seems reasonable unless we're likely to end up with a pidfd mode > > that doesn't actually make sense in a send_signal context. But I'm > > not immediately seeing any reason that that would happen. > > Yeah, I think that's very unlikely and we could reject it obased on api > design considerations. Ah, forgot to ask. Did you intend to send a patch for this?
On Thu, Feb 1, 2024 at 5:39 AM Christian Brauner <brauner@kernel.org> wrote: > > On Thu, Feb 01, 2024 at 02:30:46PM +0100, Christian Brauner wrote: > > On Wed, Jan 31, 2024 at 11:50:23AM -0800, Andy Lutomirski wrote: > > > On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote: > > > > > > On 01/31, Oleg Nesterov wrote: > > > > > > > > > > > > > > On 01/31, Andy Lutomirski wrote: > > > > > > > Please note > > > > > > > > > > > > > > /* TODO: respect PIDFD_THREAD */ > > > > > > > > > > > > > > this patch adds into pidfd_send_signal(). > > > > > > > > > > > > > > See also this part of discussion > > > > > > > > > > > > > > > > + /* TODO: respect PIDFD_THREAD */ > > > > > > > > > > > > > > > > So I've been thinking about this at the end of last week. Do we need to > > > > > > > > give userspace a way to send a thread-group wide signal even when a > > > > > > > > PIDFD_THREAD pidfd is passed? Or should we just not worry about this > > > > > > > > right now and wait until someone needs this? > > > > > > > > > > > > > > I don't know. I am fine either way, but I think this needs a separate > > > > > > > patch and another discussion in any case. Anyway should be trivial, > > > > > > > pidfd_send_signal() has the "flags" argument. > > > > > > > > > > > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ > > > > > > > > > > I missed that. Whoops. > > > > > > > > > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > > > > > Forgot to mention... > > > > > > > > > > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make > > > > > > some sense too. > > > > > > > > > > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another > > > > > > patch/discussion. > > > > > > > > > > > > But again, I am not sure I understood you correctly. > > > > > > > > > > > > > > > > Hmm. > > > > > > > > > > When one works with regular (non-fd) pids / pgids etc, one specifies > > > > > the signal domain at the time that one sends the signal. I don't know > > > > > what pidfds should do. It seems a bit inefficient for anything that > > > > > wants a pidfd and might send a signal in a different mode in the > > > > > future to have to hold on to multiple pidfds, so it probably should be > > > > > a pidfd_send_signal flag. > > > > > > > > > > Which leaves the question of what the default should be. Should > > > > > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process > > > > > or the thread? I guess there are two reasonable solutions: > > > > > > > > > > 1. flags = 0 always means process. And maybe there's a special flag > > > > > to send a signal that matches the pidfd type, or maybe not. > > > > > > > > > > 2. flags = 0 does what the pidfd seems to imply, and a new > > > > > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the > > > > > pidfd is PIDFD_THREAD. > > > > > > > > > > Do any of you have actual use cases in mind where one choice is > > > > > clearly better than the other choice? > > > > > > > > So conceptually I think having the type of pidfd dictate the default > > > > scope of the signal is the most elegant approach. And then very likely > > > > we should just have: > > > > > > > > PIDFD_SIGNAL_THREAD > > > > PIDFD_SIGNAL_THREAD_GROUP > > > > PIDFD_SIGNAL_PROCESS_GROUP > > > > > > > > I think for userspace it doesn't really matter as long as we clearly > > > > document what's going on. > > > > > > > > > > This seems reasonable unless we're likely to end up with a pidfd mode > > > that doesn't actually make sense in a send_signal context. But I'm > > > not immediately seeing any reason that that would happen. > > > > Yeah, I think that's very unlikely and we could reject it obased on api > > design considerations. > > Ah, forgot to ask. Did you intend to send a patch for this? I can try to get to it tomorrow. Currently trying to madly line up a whole bunch of stuff in time for a maintenance window.
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h index 5406fbc13074..2e6461459877 100644 --- a/include/uapi/linux/pidfd.h +++ b/include/uapi/linux/pidfd.h @@ -7,6 +7,7 @@ #include <linux/fcntl.h> /* Flags for pidfd_open(). */ -#define PIDFD_NONBLOCK O_NONBLOCK +#define PIDFD_NONBLOCK O_NONBLOCK +#define PIDFD_THREAD O_EXCL #endif /* _UAPI_LINUX_PIDFD_H */ diff --git a/kernel/exit.c b/kernel/exit.c index dfb963d2f862..74fe6bfb9577 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -739,6 +739,13 @@ static void exit_notify(struct task_struct *tsk, int group_dead) kill_orphaned_pgrp(tsk->group_leader, NULL); tsk->exit_state = EXIT_ZOMBIE; + /* + * sub-thread or delay_group_leader(), wake up the PIDFD_THREAD + * waiters. + */ + if (!thread_group_empty(tsk)) + do_notify_pidfd(tsk); + if (unlikely(tsk->ptrace)) { int sig = thread_group_leader(tsk) && thread_group_empty(tsk) && diff --git a/kernel/fork.c b/kernel/fork.c index 347641398f9d..977b58c0eac6 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -101,6 +101,7 @@ #include <linux/user_events.h> #include <linux/iommu.h> #include <linux/rseq.h> +#include <uapi/linux/pidfd.h> #include <asm/pgalloc.h> #include <linux/uaccess.h> @@ -2050,6 +2051,8 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) seq_put_decimal_ll(m, "Pid:\t", nr); + /* TODO: report PIDFD_THREAD */ + #ifdef CONFIG_PID_NS seq_put_decimal_ll(m, "\nNSpid:\t", nr); if (nr > 0) { @@ -2068,12 +2071,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) } #endif +static bool pidfd_task_exited(struct pid *pid, bool thread) +{ + struct task_struct *task; + bool exited; + + rcu_read_lock(); + task = pid_task(pid, PIDTYPE_PID); + exited = !task || + (READ_ONCE(task->exit_state) && (thread || thread_group_empty(task))); + rcu_read_unlock(); + + return exited; +} + /* * Poll support for process exit notification. */ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts) { struct pid *pid = file->private_data; + bool thread = file->f_flags & PIDFD_THREAD; __poll_t poll_flags = 0; poll_wait(file, &pid->wait_pidfd, pts); @@ -2083,7 +2101,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts) * If the thread group leader exits before all other threads in the * group, then poll(2) should block, similar to the wait(2) family. */ - if (thread_group_exited(pid)) + if (pidfd_task_exited(pid, thread)) poll_flags = EPOLLIN | EPOLLRDNORM; return poll_flags; @@ -2141,6 +2159,11 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re return PTR_ERR(pidfd_file); } get_pid(pid); /* held by pidfd_file now */ + /* + * anon_inode_getfile() ignores everything outside of the + * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually. + */ + pidfd_file->f_flags |= (flags & PIDFD_THREAD); *ret = pidfd_file; return pidfd; } @@ -2173,7 +2196,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re */ int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret) { - if (!pid || !pid_has_task(pid, PIDTYPE_TGID)) + bool thread = flags & PIDFD_THREAD; + + if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID)); return -EINVAL; return __pidfd_prepare(pid, flags, ret); diff --git a/kernel/pid.c b/kernel/pid.c index c7a3e359f8f5..04bdd5ecf183 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -629,7 +629,7 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags) int fd; struct pid *p; - if (flags & ~PIDFD_NONBLOCK) + if (flags & ~(PIDFD_NONBLOCK | PIDFD_THREAD)) return -EINVAL; if (pid <= 0) diff --git a/kernel/signal.c b/kernel/signal.c index 9561a3962ca6..919cd33a0405 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig) WARN_ON_ONCE(!tsk->ptrace && (tsk->group_leader != tsk || !thread_group_empty(tsk))); /* - * tsk is a group leader and has no threads, wake up the pidfd waiters. + * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD + * waiters. */ if (thread_group_empty(tsk)) do_notify_pidfd(tsk); @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, prepare_kill_siginfo(sig, &kinfo); } + /* TODO: respect PIDFD_THREAD */ ret = kill_pid_info(sig, &kinfo, pid); err: