* [PATCH v2 0/2] Introduce simple pidfd to task helper @ 2021-10-11 13:32 Christian Brauner 2021-10-11 13:32 ` [PATCH v2 1/2] pid: add pidfd_get_task() helper Christian Brauner 2021-10-11 13:32 ` [PATCH v2 2/2] mm: use pidfd_get_task() Christian Brauner 0 siblings, 2 replies; 6+ messages in thread From: Christian Brauner @ 2021-10-11 13:32 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Vlastimil Babka, Suren Baghdasaryan, Matthew Bobrowski, Alexander Duyck, David Hildenbrand, Jan Kara, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] pid: add pidfd_get_task() helper 2021-10-11 13:32 [PATCH v2 0/2] Introduce simple pidfd to task helper Christian Brauner @ 2021-10-11 13:32 ` Christian Brauner 2021-10-12 14:11 ` David Hildenbrand 2021-10-11 13:32 ` [PATCH v2 2/2] mm: use pidfd_get_task() Christian Brauner 1 sibling, 1 reply; 6+ messages in thread From: Christian Brauner @ 2021-10-11 13:32 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Vlastimil Babka, Suren Baghdasaryan, Matthew Bobrowski, Alexander Duyck, David Hildenbrand, Jan Kara, Christian Brauner, Minchan Kim 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] pid: add pidfd_get_task() helper 2021-10-11 13:32 ` [PATCH v2 1/2] pid: add pidfd_get_task() helper Christian Brauner @ 2021-10-12 14:11 ` David Hildenbrand 0 siblings, 0 replies; 6+ messages in thread From: David Hildenbrand @ 2021-10-12 14:11 UTC (permalink / raw) To: Christian Brauner, linux-kernel, linux-mm Cc: Vlastimil Babka, Suren Baghdasaryan, Matthew Bobrowski, Alexander Duyck, Jan Kara, Christian Brauner, Minchan Kim 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] mm: use pidfd_get_task() 2021-10-11 13:32 [PATCH v2 0/2] Introduce simple pidfd to task helper Christian Brauner 2021-10-11 13:32 ` [PATCH v2 1/2] pid: add pidfd_get_task() helper Christian Brauner @ 2021-10-11 13:32 ` Christian Brauner 2021-10-12 14:13 ` David Hildenbrand 1 sibling, 1 reply; 6+ messages in thread From: Christian Brauner @ 2021-10-11 13:32 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Vlastimil Babka, Suren Baghdasaryan, Matthew Bobrowski, Alexander Duyck, David Hildenbrand, Jan Kara, Christian Brauner, Minchan Kim 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] mm: use pidfd_get_task() 2021-10-11 13:32 ` [PATCH v2 2/2] mm: use pidfd_get_task() Christian Brauner @ 2021-10-12 14:13 ` David Hildenbrand 2021-10-13 12:12 ` Christian Brauner 0 siblings, 1 reply; 6+ messages in thread From: David Hildenbrand @ 2021-10-12 14:13 UTC (permalink / raw) To: Christian Brauner, linux-kernel, linux-mm Cc: Vlastimil Babka, Suren Baghdasaryan, Matthew Bobrowski, Alexander Duyck, Jan Kara, Christian Brauner, Minchan Kim 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] mm: use pidfd_get_task() 2021-10-12 14:13 ` David Hildenbrand @ 2021-10-13 12:12 ` Christian Brauner 0 siblings, 0 replies; 6+ messages in thread From: Christian Brauner @ 2021-10-13 12:12 UTC (permalink / raw) To: David Hildenbrand Cc: Christian Brauner, linux-kernel, linux-mm, Vlastimil Babka, Suren Baghdasaryan, Matthew Bobrowski, Alexander Duyck, Jan Kara, Minchan Kim 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-13 12:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-11 13:32 [PATCH v2 0/2] Introduce simple pidfd to task helper Christian Brauner 2021-10-11 13:32 ` [PATCH v2 1/2] pid: add pidfd_get_task() helper Christian Brauner 2021-10-12 14:11 ` David Hildenbrand 2021-10-11 13:32 ` [PATCH v2 2/2] mm: use pidfd_get_task() Christian Brauner 2021-10-12 14:13 ` David Hildenbrand 2021-10-13 12:12 ` Christian Brauner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox