RE: [PATCH 1/2v6] procfs: show hierarchy of pid namespace

[Chen, Hanxiao]

> -----Original Message-----
> From: Mateusz Guzik [mailto:mguzik@xxxxxxxxxx]
> Sent: Wednesday, November 05, 2014 7:54 PM
> To: Chen, Hanxiao/é æé
> Cc: Eric W. Biederman; Serge Hallyn; Oleg Nesterov;
> containers@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; David
> Howells; Richard Weinberger; Pavel Emelyanov; Vasiliy Kulikov
> Subject: Re: [PATCH 1/2v6] procfs: show hierarchy of pid namespace
> 
> On Wed, Nov 05, 2014 at 06:41:54PM +0800, Chen Hanxiao wrote:
> > +static void free_pidns_list(struct list_head *head)
> > +{
> > +	struct pidns_list *tmp, *pos;
> > +
> > +	list_for_each_entry_safe(pos, tmp, head, list) {
> > +		list_del(&pos->list);
> 
> Any need for this one? stuff is freed anyway...
> 
Yes, they're all freed.
But is that ok to leave a list full of freed stuffs..

[snip]
> > +
> > +		if (flag == 0) {
> > +			rcu_read_lock();
> > +			get_pid(pos->pid);
> > +			rcu_read_unlock();
> 
> At this point you should have a valid reference for pid, so rcu should
> not matter.
> 
Right.

> 
> > +			rc = pidns_list_add(pos->pid, pidns_pid_tree);
> > +			if (rc) {
> > +				put_pid(pos->pid);
> > +				goto out;
> > +                        }
> 
> '}' is misindented. Also 'out' is not a good label if it used solely for
> cleanup on error. 'out_err', 'fail' or something woud be better.

something wrong with my vimrc...
'out' is definitely not a good label.
I think 'cleanup' is a better one.

> 
[snip]
> > +static int proc_pidns_list_refresh(struct pid_namespace *curr_ns,
> > +		struct list_head *pidns_pid_list,
> > +		struct list_head *pidns_pid_tree)
> > +{
> > +	struct pid *pid;
> > +	int new_nr, nr = 0;
> > +	int rc;
> > +
> > +	/* collect pids in current namespace */
> > +	while (nr < PID_MAX_LIMIT) {
> > +		rcu_read_lock();
> > +		pid = find_ge_pid(nr, curr_ns);
> > +		if (pid) {
> > +			new_nr = pid_vnr(pid);
> > +			if (!is_child_reaper(pid)) {
> > +				nr = new_nr + 1;
> > +				rcu_read_unlock();
> > +				continue;
> > +			}
> > +			get_pid(pid);
> > +			rcu_read_unlock();
> > +			rc = pidns_list_add(pid, pidns_pid_list);
> > +			if (rc) {
> > +				put_pid(pid);
> > +				goto out;
> > +                        }
> > +		} else {
> > +			rcu_read_unlock();
> > +			break;
> > +		}
> > +		nr = new_nr + 1;
> > +	}
> > +
> 
> Would be beneficial to reorganize this loop. Handle shorter case (!pid)
> first.

OK.

> 
> I consulted Dr. Grep and it told me about delayed_put_pid, so I guess
> pid itself is not going to be freed in the meantime, but this still
> seems fishy.

I found 
call_rcu(&pid->rcu, delayed_put_pid) in free_pid


Thanks,
- Chen

> 
> > +	/*
> > +	 * Only one pid found as the child reaper,
> > +	 * so current pid namespace do not have sub-namespace,
> > +	 * return 0 directly.
> > +	 */
> > +	if (list_is_singular(pidns_pid_list)) {
> > +		rc = 0;
> > +		goto out;
> > +	}
> > +
> > +	/*
> > +	 * screen duplicate pids from pidns_pid_list
> > +	 * and form a new list pidns_pid_tree.
> > +	 */
> > +	rc = pidns_list_filter(pidns_pid_list, pidns_pid_tree);
> > +	if (rc)
> > +		goto out;
> > +
> > +	return 0;
> > +
> > +out:
> > +	free_pidns_list(pidns_pid_list);
> > +	return rc;
> > +}
> > +
> > +static int nslist_proc_show(struct seq_file *m, void *v)
> > +{
> > +	struct pidns_list *pos;
> > +	struct pid_namespace *ns, *curr_ns;
> > +	struct pid *pid;
> > +	char pid_buf[16];
> > +	int i, rc;
> > +
> > +	LIST_HEAD(pidns_pid_list);
> > +	LIST_HEAD(pidns_pid_tree);
> > +
> > +	curr_ns = task_active_pid_ns(current);
> > +
> > +	rc = proc_pidns_list_refresh(curr_ns, &pidns_pid_list, &pidns_pid_tree);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/* print pid namespace's hierarchy */
> > +	list_for_each_entry(pos, &pidns_pid_tree, list) {
> > +		pid = pos->pid;
> > +		for (i = curr_ns->level + 1; i <= pid->level; i++) {
> > +			ns = pid->numbers[i].ns;
> > +			/* show PID '1' in specific pid ns */
> > +			snprintf(pid_buf, 16, "%u",
> > +				pid_vnr(find_pid_ns(1, ns)));
> > +			seq_printf(m, "%s ", pid_buf);
> > +		}
> > +
> > +		seq_putc(m, '\n');
> > +	}
> > +
> > +	free_pidns_list(&pidns_pid_tree);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nslist_proc_open(struct inode *inode, struct file *file)
> > +{
> > +	return single_open(file, nslist_proc_show, NULL);
> > +}
> > +
> > +static const struct file_operations proc_nspid_nslist_fops = {
> > +	.open		= nslist_proc_open,
> > +	.read		= seq_read,
> > +	.llseek		= seq_lseek,
> > +	.release	= single_release,
> > +};
> > +
> > +static int __init pidns_hierarchy_init(void)
> > +{
> > +	proc_create(NS_HIERARCHY, S_IWUGO,
> > +		NULL, &proc_nspid_nslist_fops);
> > +
> > +	return 0;
> > +}
> > +fs_initcall(pidns_hierarchy_init);
> > --
> > 1.9.3
> >
> 
> --
> Mateusz Guzik