| Message ID | 20191016153606.2326-1-christian.brauner@ubuntu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] pidfd: verify task is alive when printing fdinfo | expand |
On 10/16, Christian Brauner wrote: > > +static inline bool task_alive(struct pid *pid, enum pid_type type) > +{ > + return !hlist_empty(&pid->tasks[type]); > +} So you decided to add a helper ;) OK, but note that its name is very confusing and misleading. Even more than pid_alive() we already have. What does "alive" actually mean? Say, task_alive(pid, PIDTYPE_SID) == F after fork(). Then it becomes T if this task does setsid(). And why task_ if it accepts pid+pid_type? May be pid_has_task() or something like this... OK, since I can't suggest a better name I won't really argue. Feel free to add my reviewed-by to this series. Oleg.
On Wed, Oct 16, 2019 at 06:24:09PM +0200, Oleg Nesterov wrote: > On 10/16, Christian Brauner wrote: > > > > +static inline bool task_alive(struct pid *pid, enum pid_type type) > > +{ > > + return !hlist_empty(&pid->tasks[type]); > > +} > > So you decided to add a helper ;) OK, but note that its name is very > confusing and misleading. Even more than pid_alive() we already have. That's why I chose that name. This is the second time I get bitten by taking inspiration from prior naming examples. :) > > What does "alive" actually mean? Say, task_alive(pid, PIDTYPE_SID) == F > after fork(). Then it becomes T if this task does setsid(). Yes, that annoyed me to. If you think about pidfd_open() you have a similar problem. The question we are asking in pidfd_open() is not task_alive() but rather was-this-pid-used-as. > > And why task_ if it accepts pid+pid_type? May be pid_has_task() or > something like this... Given what I said above that might be a decent name. > > OK, since I can't suggest a better name I won't really argue. Feel free > to add my reviewed-by to this series. No, naming is important. Thanks for being picky about that too and I'll happily resend. :) Thanks! Christian
On 10/16, Christian Brauner wrote: > > On Wed, Oct 16, 2019 at 06:24:09PM +0200, Oleg Nesterov wrote: > > > > And why task_ if it accepts pid+pid_type? May be pid_has_task() or > > something like this... > > Given what I said above that might be a decent name. > > > > > OK, since I can't suggest a better name I won't really argue. Feel free > > to add my reviewed-by to this series. > > No, naming is important. Thanks for being picky about that too and I'll > happily resend. :) Thanks ;) May be pid_in_use() ? Up to you, anything which starts with pid_ looks better to me than task_alive(). Oleg.
diff --git a/include/linux/pid.h b/include/linux/pid.h index 9645b1194c98..5f1c8ce10b71 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -85,6 +85,10 @@ static inline struct pid *get_pid(struct pid *pid) extern void put_pid(struct pid *pid); extern struct task_struct *pid_task(struct pid *pid, enum pid_type); +static inline bool task_alive(struct pid *pid, enum pid_type type) +{ + return !hlist_empty(&pid->tasks[type]); +} extern struct task_struct *get_pid_task(struct pid *pid, enum pid_type); extern struct pid *get_task_pid(struct task_struct *task, enum pid_type type); diff --git a/kernel/fork.c b/kernel/fork.c index 782986962d47..ef9a9d661079 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1732,15 +1732,20 @@ static int pidfd_release(struct inode *inode, struct file *file) */ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) { - struct pid_namespace *ns = proc_pid_ns(file_inode(m->file)); struct pid *pid = f->private_data; - pid_t nr = pid_nr_ns(pid, ns); + struct pid_namespace *ns; + pid_t nr = -1; - seq_put_decimal_ull(m, "Pid:\t", nr); + if (likely(task_alive(pid, PIDTYPE_PID))) { + ns = proc_pid_ns(file_inode(m->file)); + nr = pid_nr_ns(pid, ns); + } + + seq_put_decimal_ll(m, "Pid:\t", nr); #ifdef CONFIG_PID_NS - seq_put_decimal_ull(m, "\nNSpid:\t", nr); - if (nr) { + seq_put_decimal_ll(m, "\nNSpid:\t", nr); + if (nr > 0) { int i; /* If nr is non-zero it means that 'pid' is valid and that @@ -1749,7 +1754,7 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f) * Start at one below the already printed level. */ for (i = ns->level + 1; i <= pid->level; i++) - seq_put_decimal_ull(m, "\t", pid->numbers[i].nr); + seq_put_decimal_ll(m, "\t", pid->numbers[i].nr); } #endif seq_putc(m, '\n');
Currently, when a task is dead we still print the pid it used to use in the fdinfo files of its pidfds. This doesn't make much sense since the pid may have already been reused. So verify that the task is still alive by introducing the task_alive() helper which will be used by other callers in follow-up patches. If the task is not alive anymore, we will print -1. This allows us to differentiate between a task not being present in a given pid namespace - in which case we already print 0 - and a task having been reaped. Note that this uses PIDTYPE_PID for the check. Technically, we could've checked PIDTYPE_TGID since pidfds currently only refer to thread-group leaders but if they won't anymore in the future then this check becomes problematic without it being immediately obvious to non-experts imho. If a thread is created via clone(CLONE_THREAD) than struct pid has a single non-empty list pid->tasks[PIDTYPE_PID] and this pid can't be used as a PIDTYPE_TGID meaning pid->tasks[PIDTYPE_TGID] will return NULL even though the thread-group leader might still be very much alive. So checking PIDTYPE_PID is fine and is easier to maintain should we ever allow pidfds to refer to threads. Cc: Jann Horn <jannh@google.com> Cc: Christian Kellner <christian@kellner.me> Cc: Oleg Nesterov <oleg@redhat.com> Cc: linux-api@vger.kernel.org Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- /* v1 */ Link: https://lore.kernel.org/r/20191015141332.4055-1-christian.brauner@ubuntu.com /* v2 */ - Oleg Nesterov <oleg@redhat.com>: - simplify check whether task is still alive to hlist_empty() - optionally introduce generic helper to replace open coded hlist_emtpy() checks whether or not a task is alive - Christian Brauner <christian.brauner@ubuntu.com>: - introduce task_alive() helper and use in pidfd_show_fdinfo() --- include/linux/pid.h | 4 ++++ kernel/fork.c | 17 +++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)