| Message ID | 20191015141332.4055-1-christian.brauner@ubuntu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] pidfd: verify task is alive when printing fdinfo | expand |
On 10/15, Christian Brauner wrote: > > +static inline bool task_alive(struct pid *pid) > +{ > + bool alive = true; > + > + rcu_read_lock(); > + if (!pid_task(pid, PIDTYPE_PID)) > + alive = false; > + rcu_read_unlock(); > + > + return alive; > +} Well, the usage of rcu_read_lock/unlock looks confusing to me... I mean, this helper does not need rcu lock at all. Except rcu_dereference_check() will complain. static inline bool task_alive(struct pid *pid) { bool alive; /* shut up rcu_dereference_check() */ rcu_lock_acquire(&rcu_lock_map); alive = !!pid_task(pid, PIDTYPE_PID)); rcu_lock_release(&rcu_lock_map); return alive; } looks more clear imo. But in fact I'd suggest to simply use !hlist_empty(&pid->tasks[PIDTYPE_PID]) in pidfd_show_fdinfo() and do not add a new helper. Oleg.
On Tue, Oct 15, 2019 at 04:43:57PM +0200, Oleg Nesterov wrote: > On 10/15, Christian Brauner wrote: > > > > +static inline bool task_alive(struct pid *pid) > > +{ > > + bool alive = true; > > + > > + rcu_read_lock(); > > + if (!pid_task(pid, PIDTYPE_PID)) > > + alive = false; > > + rcu_read_unlock(); > > + > > + return alive; > > +} > > Well, the usage of rcu_read_lock/unlock looks confusing to me... > > I mean, this helper does not need rcu lock at all. Except > rcu_dereference_check() will complain. Yep, I think we have another codepath were the rcu locks might be purely cosmetic so I thought it's not a big deal (see below). > > static inline bool task_alive(struct pid *pid) > { > bool alive; > > /* shut up rcu_dereference_check() */ > rcu_lock_acquire(&rcu_lock_map); > alive = !!pid_task(pid, PIDTYPE_PID)); > rcu_lock_release(&rcu_lock_map); > > return alive; > } > > looks more clear imo. > > But in fact I'd suggest to simply use !hlist_empty(&pid->tasks[PIDTYPE_PID]) > in pidfd_show_fdinfo() and do not add a new helper. Sounds good to me. But can't we then just do something similar just with !hlist_empty(&pid->tasks[PIDTYPE_TGID]) in v5.4-rc3:kernel/pid.c:pidfd_open():514-517 ? or would this be problematic because of de_thread()? Thanks! Christian
On 10/15, Christian Brauner wrote: > > On Tue, Oct 15, 2019 at 04:43:57PM +0200, Oleg Nesterov wrote: > > But in fact I'd suggest to simply use !hlist_empty(&pid->tasks[PIDTYPE_PID]) > > in pidfd_show_fdinfo() and do not add a new helper. > > Sounds good to me. But can't we then just do something similar just with > !hlist_empty(&pid->tasks[PIDTYPE_TGID]) > > in v5.4-rc3:kernel/pid.c:pidfd_open():514-517 ? Agreed. Actually, it seems to me I suggested to use rcu_lock_acquire() rather than rcu_read_lock() in pidfd_open() too. But hlist_empty(pid->tasks[type]) looks even better. If you decide to add a new helper, you can also change do_wait() which checks hlist_empty(&wo->wo_pid->tasks[wo->wo_type]). May be even __change_pid(). Oleg.
diff --git a/kernel/fork.c b/kernel/fork.c index 782986962d47..a67944a5e542 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1695,6 +1695,18 @@ static int pidfd_release(struct inode *inode, struct file *file) } #ifdef CONFIG_PROC_FS +static inline bool task_alive(struct pid *pid) +{ + bool alive = true; + + rcu_read_lock(); + if (!pid_task(pid, PIDTYPE_PID)) + alive = false; + rcu_read_unlock(); + + return alive; +} + /** * pidfd_show_fdinfo - print information about a pidfd * @m: proc fdinfo file @@ -1732,15 +1744,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; + + if (likely(task_alive(pid))) { + ns = proc_pid_ns(file_inode(m->file)); + nr = pid_nr_ns(pid, ns); + } - seq_put_decimal_ull(m, "Pid:\t", nr); + 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 +1766,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. 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. We could be more complicated and do something like: bool alive = false; rcu_read_lock(); struct task_struct *tsk = pid_task(pid, PIDTYPE_PID); if (tsk && task_tgid(tsk)) alive = true; rcu_read_unlock(); but it's really not worth it. We already have created a pidfd and we thus know it refers to a thread-group leader. 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> --- kernel/fork.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)