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

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

[Christian Brauner]

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.

Thanks!
Christian

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

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


base-commit: 9e1ff307c779ce1f0f810c7ecce3d95bbae40896
-- 
2.30.2

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

[Christian Brauner]

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.

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: Jan Kara <jack@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/linux/pid.h |  1 +
 kernel/pid.c        | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 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..2ffbb87b2ce8 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -539,6 +539,40 @@ 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 the given pidfd.
+ * 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

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

[David Hildenbrand]

On 04.10.21 14:50, Christian Brauner wrote:
> 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.
> 
> 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: Jan Kara <jack@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>   include/linux/pid.h |  1 +
>   kernel/pid.c        | 34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 35 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..2ffbb87b2ce8 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -539,6 +539,40 @@ 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 the given pidfd.
> + * 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.

Nice doc.

You might want to document what callers of this function are expected to 
do to clean up.


> + */
> +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);

The code to be replaced always does the put_pid() after the 
put_task_struct(). Is this new ordering safe? (didn't check)

> +	if (!task)
> +		return ERR_PTR(-ESRCH);
> +
> +	*flags = f_flags;
> +	return task;
> +}
> +
>   /**
>    * pidfd_create() - Create a new pid file descriptor.
>    *
> 

I'd have squashed this into the second patch, makes it a lot easier to 
review and it's only a MM cleanup at this point.

-- 
Thanks,

David / dhildenb

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

[Christian Brauner]

On Fri, Oct 08, 2021 at 10:47:36AM +0200, David Hildenbrand wrote:
> On 04.10.21 14:50, Christian Brauner wrote:
> > 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.
> > 
> > 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: Jan Kara <jack@suse.cz>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> >   include/linux/pid.h |  1 +
> >   kernel/pid.c        | 34 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 35 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..2ffbb87b2ce8 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -539,6 +539,40 @@ 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 the given pidfd.
> > + * 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.
> 
> Nice doc.
> 
> You might want to document what callers of this function are expected to do
> to clean up.

That's a good idea! Let me add that.

> 
> 
> > + */
> > +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);
> 
> The code to be replaced always does the put_pid() after the
> put_task_struct(). Is this new ordering safe? (didn't check)

I at least see no obvious problems and so do think this is safe. 
The lifetimes of struct pid and struct task_struct are independent of
each other. They don't mess with each others refcounts. And the caller's
aren't going back from struct task_struct to struct pid anywhere.

> 
> > +	if (!task)
> > +		return ERR_PTR(-ESRCH);
> > +
> > +	*flags = f_flags;
> > +	return task;
> > +}
> > +
> >   /**
> >    * pidfd_create() - Create a new pid file descriptor.
> >    *
> > 
> 
> I'd have squashed this into the second patch, makes it a lot easier to
> review and it's only a MM cleanup at this point.

Hm, I prefer the split between introducing a helper and making use of a
helper. I find that nicer than mixing up the two steps. I only tend to
do both if it would introduce breakage.

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

[Christian Brauner]

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.

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: Jan Kara <jack@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 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 0/2] Introduce simple pidfd to task helper

[Matthew Bobrowski]

On Mon, Oct 04, 2021 at 02:50:48PM +0200, Christian Brauner wrote:
> 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.

I went through the series. Seems like a very sensible thing to do, so thanks for
adding the helper and updating the call sites accordingly.

Reviewed-by: Matthew Bobrowski <repnop@google.com>

> Christian Brauner (2):
>   pid: add pidfd_get_task() helper
>   mm: use pidfd_get_task()
> 
>  include/linux/pid.h |  1 +
>  kernel/pid.c        | 34 ++++++++++++++++++++++++++++++++++
>  mm/madvise.c        | 15 +++------------
>  mm/oom_kill.c       | 15 +++------------
>  4 files changed, 41 insertions(+), 24 deletions(-)
> 
> 
> base-commit: 9e1ff307c779ce1f0f810c7ecce3d95bbae40896
> -- 
> 2.30.2
> 
/M