| Message ID | 20211004125050.1153693-2-christian.brauner@ubuntu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce simple pidfd to task helper | expand |
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.
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.
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. *
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(+)