From: Christian Brauner <[email protected]>
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
From: Christian Brauner <[email protected]>
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/[email protected]
Cc: Vlastimil Babka <[email protected]>
Cc: Suren Baghdasaryan <[email protected]>
Cc: Matthew Bobrowski <[email protected]>
Cc: Alexander Duyck <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Minchan Kim <[email protected]>
Reviewed-by: Matthew Bobrowski <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>
---
/* 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
On 11.10.21 15:32, Christian Brauner wrote:
> From: Christian Brauner <[email protected]>
>
> 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/[email protected]
> Cc: Vlastimil Babka <[email protected]>
> Cc: Suren Baghdasaryan <[email protected]>
> Cc: Matthew Bobrowski <[email protected]>
> Cc: Alexander Duyck <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Reviewed-by: Matthew Bobrowski <[email protected]>
> Signed-off-by: Christian Brauner <[email protected]>
> ---
> /* v2 */
> unchanged
Acked-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
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 <[email protected]>
> >
> > 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/[email protected]
> > Cc: Vlastimil Babka <[email protected]>
> > Cc: Suren Baghdasaryan <[email protected]>
> > Cc: Matthew Bobrowski <[email protected]>
> > Cc: Alexander Duyck <[email protected]>
> > Cc: David Hildenbrand <[email protected]>
> > Cc: Jan Kara <[email protected]>
> > Cc: Minchan Kim <[email protected]>
> > Reviewed-by: Matthew Bobrowski <[email protected]>
> > Signed-off-by: Christian Brauner <[email protected]>
> > ---
> > /* v2 */
> > unchanged
>
> Acked-by: David Hildenbrand <[email protected]>
Thanks!
Christian