[PATCH v2 0/2] Introduce simple pidfd to task helper

[PATCH v2 0/2] Introduce simple pidfd to task helper

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

Hey everyone,

This adds a simple helper to get rid of some code duplication introduced
with the addition of two new pidfd-based syscalls in mm. We should've
probably added the helper right away and I think I mentioned this during
in the review on one of the revisions but we probably just lost track of
it. If this looks ok to you, I'll queue this up for next.

/* v2 */
Add a note to the kernel doc what the caller is expected to clean up.
No semantical changes.

Thanks!
Christian

Christian Brauner (2):
  pid: add pidfd_get_task() helper
  mm: use pidfd_get_task()

 include/linux/pid.h |  1 +
 kernel/pid.c        | 36 ++++++++++++++++++++++++++++++++++++
 mm/madvise.c        | 15 +++------------
 mm/oom_kill.c       | 15 +++------------
 4 files changed, 43 insertions(+), 24 deletions(-)


base-commit: 9e1ff307c779ce1f0f810c7ecce3d95bbae40896
-- 
2.30.2

[PATCH v2 1/2] pid: add pidfd_get_task() helper

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

The number of system calls making use of pidfds is constantly
increasing. Some of those new system calls duplicate the code to turn a
pidfd into task_struct it refers to. Give them a simple helper for this.

Link: https://lore.kernel.org/r/20211004125050.1153693-2-christian.brauner@ubuntu.com
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Matthew Bobrowski <repnop@google.com>
Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Reviewed-by: Matthew Bobrowski <repnop@google.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- David Hildenbrand <david@redhat.com>:
  - Also document that the caller is expected to decrease the reference
    count on the returned task.
---
 include/linux/pid.h |  1 +
 kernel/pid.c        | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index af308e15f174..343abf22092e 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -78,6 +78,7 @@ struct file;
 
 extern struct pid *pidfd_pid(const struct file *file);
 struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
+struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
 int pidfd_create(struct pid *pid, unsigned int flags);
 
 static inline struct pid *get_pid(struct pid *pid)
diff --git a/kernel/pid.c b/kernel/pid.c
index efe87db44683..2fc0a16ec77b 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -539,6 +539,42 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
 	return pid;
 }
 
+/**
+ * pidfd_get_task() - Get the task associated with a pidfd
+ *
+ * @pidfd: pidfd for which to get the task
+ * @flags: flags associated with this pidfd
+ *
+ * Return the task associated with @pidfd. The function takes a reference on
+ * the returned task. The caller is responsible for releasing that reference.
+ *
+ * Currently, the process identified by @pidfd is always a thread-group leader.
+ * This restriction currently exists for all aspects of pidfds including pidfd
+ * creation (CLONE_PIDFD cannot be used with CLONE_THREAD) and pidfd polling
+ * (only supports thread group leaders).
+ *
+ * Return: On success, the task_struct associated with the pidfd.
+ *	   On error, a negative errno number will be returned.
+ */
+struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags)
+{
+	unsigned int f_flags;
+	struct pid *pid;
+	struct task_struct *task;
+
+	pid = pidfd_get_pid(pidfd, &f_flags);
+	if (IS_ERR(pid))
+		return ERR_CAST(pid);
+
+	task = get_pid_task(pid, PIDTYPE_TGID);
+	put_pid(pid);
+	if (!task)
+		return ERR_PTR(-ESRCH);
+
+	*flags = f_flags;
+	return task;
+}
+
 /**
  * pidfd_create() - Create a new pid file descriptor.
  *
-- 
2.30.2

[PATCH v2 2/2] mm: use pidfd_get_task()

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

Instead of duplicating the same code in two places use the newly added
pidfd_get_task() helper. This fixes an (unimportant for now) bug where
PIDTYPE_PID is used whereas PIDTYPE_TGID should have been used.

Link: https://lore.kernel.org/r/20211004125050.1153693-3-christian.brauner@ubuntu.com
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Matthew Bobrowski <repnop@google.com>
Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Reviewed-by: Matthew Bobrowski <repnop@google.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
unchanged
---
 mm/madvise.c  | 15 +++------------
 mm/oom_kill.c | 15 +++------------
 2 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 0734db8d53a7..8c927202bbe6 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1235,7 +1235,6 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
 	struct iovec iovstack[UIO_FASTIOV], iovec;
 	struct iovec *iov = iovstack;
 	struct iov_iter iter;
-	struct pid *pid;
 	struct task_struct *task;
 	struct mm_struct *mm;
 	size_t total_len;
@@ -1250,18 +1249,12 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
 	if (ret < 0)
 		goto out;
 
-	pid = pidfd_get_pid(pidfd, &f_flags);
-	if (IS_ERR(pid)) {
-		ret = PTR_ERR(pid);
+	task = pidfd_get_task(pidfd, &f_flags);
+	if (IS_ERR(task)) {
+		ret = PTR_ERR(task);
 		goto free_iov;
 	}
 
-	task = get_pid_task(pid, PIDTYPE_PID);
-	if (!task) {
-		ret = -ESRCH;
-		goto put_pid;
-	}
-
 	if (!process_madvise_behavior_valid(behavior)) {
 		ret = -EINVAL;
 		goto release_task;
@@ -1301,8 +1294,6 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
 	mmput(mm);
 release_task:
 	put_task_struct(task);
-put_pid:
-	put_pid(pid);
 free_iov:
 	kfree(iov);
 out:
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 831340e7ad8b..70d399d5817e 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -1151,21 +1151,14 @@ SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
 	struct task_struct *p;
 	unsigned int f_flags;
 	bool reap = true;
-	struct pid *pid;
 	long ret = 0;
 
 	if (flags)
 		return -EINVAL;
 
-	pid = pidfd_get_pid(pidfd, &f_flags);
-	if (IS_ERR(pid))
-		return PTR_ERR(pid);
-
-	task = get_pid_task(pid, PIDTYPE_TGID);
-	if (!task) {
-		ret = -ESRCH;
-		goto put_pid;
-	}
+	task = pidfd_get_task(pidfd, &f_flags);
+	if (IS_ERR(task))
+		return PTR_ERR(task);
 
 	/*
 	 * Make sure to choose a thread which still has a reference to mm
@@ -1204,8 +1197,6 @@ SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
 	mmdrop(mm);
 put_task:
 	put_task_struct(task);
-put_pid:
-	put_pid(pid);
 	return ret;
 #else
 	return -ENOSYS;
-- 
2.30.2

Re: [PATCH v2 1/2] pid: add pidfd_get_task() helper

[David Hildenbrand]

On 11.10.21 15:32, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> The number of system calls making use of pidfds is constantly
> increasing. Some of those new system calls duplicate the code to turn a
> pidfd into task_struct it refers to. Give them a simple helper for this.
> 
> Link: https://lore.kernel.org/r/20211004125050.1153693-2-christian.brauner@ubuntu.com
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Matthew Bobrowski <repnop@google.com>
> Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Reviewed-by: Matthew Bobrowski <repnop@google.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> /* v2 */
> - David Hildenbrand <david@redhat.com>:
>    - Also document that the caller is expected to decrease the reference
>      count on the returned task.
> ---
>   include/linux/pid.h |  1 +
>   kernel/pid.c        | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 37 insertions(+)
> 
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index af308e15f174..343abf22092e 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -78,6 +78,7 @@ struct file;
>   
>   extern struct pid *pidfd_pid(const struct file *file);
>   struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> +struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
>   int pidfd_create(struct pid *pid, unsigned int flags);
>   
>   static inline struct pid *get_pid(struct pid *pid)
> diff --git a/kernel/pid.c b/kernel/pid.c
> index efe87db44683..2fc0a16ec77b 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -539,6 +539,42 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
>   	return pid;
>   }
>   
> +/**
> + * pidfd_get_task() - Get the task associated with a pidfd
> + *
> + * @pidfd: pidfd for which to get the task
> + * @flags: flags associated with this pidfd
> + *
> + * Return the task associated with @pidfd. The function takes a reference on
> + * the returned task. The caller is responsible for releasing that reference.
> + *
> + * Currently, the process identified by @pidfd is always a thread-group leader.
> + * This restriction currently exists for all aspects of pidfds including pidfd
> + * creation (CLONE_PIDFD cannot be used with CLONE_THREAD) and pidfd polling
> + * (only supports thread group leaders).
> + *
> + * Return: On success, the task_struct associated with the pidfd.
> + *	   On error, a negative errno number will be returned.
> + */
> +struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags)
> +{
> +	unsigned int f_flags;
> +	struct pid *pid;
> +	struct task_struct *task;
> +
> +	pid = pidfd_get_pid(pidfd, &f_flags);
> +	if (IS_ERR(pid))
> +		return ERR_CAST(pid);
> +
> +	task = get_pid_task(pid, PIDTYPE_TGID);
> +	put_pid(pid);
> +	if (!task)
> +		return ERR_PTR(-ESRCH);
> +
> +	*flags = f_flags;
> +	return task;
> +}
> +
>   /**
>    * pidfd_create() - Create a new pid file descriptor.
>    *
> 

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/2] mm: use pidfd_get_task()

[David Hildenbrand]

On 11.10.21 15:32, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Instead of duplicating the same code in two places use the newly added
> pidfd_get_task() helper. This fixes an (unimportant for now) bug where
> PIDTYPE_PID is used whereas PIDTYPE_TGID should have been used.

What would have been the effect of the BUG? Is it worth Fixes: or better 
even separating out the fix?

> 
> Link: https://lore.kernel.org/r/20211004125050.1153693-3-christian.brauner@ubuntu.com
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Matthew Bobrowski <repnop@google.com>
> Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Reviewed-by: Matthew Bobrowski <repnop@google.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> /* v2 */
> unchanged

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/2] mm: use pidfd_get_task()

[Christian Brauner]

On Tue, Oct 12, 2021 at 04:13:31PM +0200, David Hildenbrand wrote:
> On 11.10.21 15:32, Christian Brauner wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > Instead of duplicating the same code in two places use the newly added
> > pidfd_get_task() helper. This fixes an (unimportant for now) bug where
> > PIDTYPE_PID is used whereas PIDTYPE_TGID should have been used.
> 
> What would have been the effect of the BUG? Is it worth Fixes: or better

Right now, there's no issue. I hope my "unimportant for now" gets that
across.
Retrieving it via PIDTYPE_PID or PIDTYPE_TGID doesn't matter right now
because at pidfd creation time we ensure that:
- the pid used with pidfd_open()
- the task created via clone{3}()'s CLONE_PIDFD
are used as PIDTYPE_TGID, i.e. the struct pid the pidfd references is
used as PIDTYPE_TGID, i.e. is a thread-group leader.
The concern is for the future were we may want to enable pidfds to refer
to individual threads. Once that happens the passed in pidfd to e.g.
process_mrelease() or process_madvise() can refer to a struct pid that
is only used as PIDTYPE_PID and not as PIDTYPE_TGID, i.e. it might be a
pidfd refering to a non-threadgroup leader. Once that happens we want to
make sure that all users of pidfds are ok working with non-threadgroup
leaders. If we have on central helper that becomes a relatively simple
exercise in grepping and we're sure that all current callers use
PIDTYPE_TGID as they're using the helper. If we let places use
PIDTYPE_PID or PIDTYPE_TGID interchangeably this becomes a more arduous
task. So in a sense it's a bug-in-the-making. It's arguably fixes the
addition of process_mrelease() since I mentioned this pretty early on
and requested the addition of a helper as part of the patchset. I think
it just got lost in the reviews though.

> even separating out the fix?
> 
> > 
> > Link: https://lore.kernel.org/r/20211004125050.1153693-3-christian.brauner@ubuntu.com
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: Matthew Bobrowski <repnop@google.com>
> > Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Reviewed-by: Matthew Bobrowski <repnop@google.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > /* v2 */
> > unchanged
> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks!
Christian