linux-kernel - Re: [RFC] fs, proc: Introduce /proc/<pid>/task/<tid>/children entry v6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 17 Jan 2012 18:40:49 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Cyrill Gorcunov <gorcunov@...il.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Pavel Emelyanov <xemul@...allels.com>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Tejun Heo <tj@...nel.org>, Andrew Vagin <avagin@...nvz.org>,
	Vasiliy Kulikov <segoon@...nwall.com>
Subject: Re: [RFC] fs, proc: Introduce /proc/<pid>/task/<tid>/children
	entry v6

On 01/16, Cyrill Gorcunov wrote:
>
> On Mon, Jan 16, 2012 at 05:11:14PM +0100, Oleg Nesterov wrote:
> > On 01/16, Cyrill Gorcunov wrote:
> > >
> > > +get_children_pid(struct proc_pid_children_iter *iter, struct pid *pid_prev, loff_t pos)
> > > +{
> > > +	struct task_struct *start, *task;
> > > +	struct pid *pid = NULL;
> > > +
> > > +	read_lock(&tasklist_lock);
> > > +
> > > +	start = pid_task(iter->pid_start, PIDTYPE_PID);
> > > +	if (!start)
> > > +		goto out;
> > > +
> > > +	/*
> > > +	 * Lets try to continue searching first, this gives
> > > +	 * us significant speedup on children-rich processes.
> > > +	 */
> > > +	if (pid_prev) {
> > > +		task = pid_task(pid_prev, PIDTYPE_PID);
> > > +		if (task && task->real_parent == start &&
> > > +		    !(list_empty(&task->sibling))) {
> >
> > Damn. No, this is wrong.
> >
> > Damn! Yes, it was we who told you to check list_empty(sibling) ;)
> >
> > But this is not enough. exit_ptrace() can do list_move() without
> > changing ->real_parent.
> >
> > I'll try to think. At first glance we can rely on EXIT_DEAD, but
> > I'd like to avoid this, I think EXIT_DEAD should die.
>
> Ouch! Thanks for catching this Oleg. I'll try to come with something
> to show as well.

Do you see another approach? I don't, so I'd suggest to check
"task->exit_state != EXIT_DEAD" instead of !list_empty().

Just in case, we can also check "start->exit_state == 0" instead
of "task->real_parent == start" with the same effect, up to you.

It would be nice to add the comment explaining these checks...

And I forgot to mention, the comment below

	> +			/*
	> +			 * We might miss some freshly created children
	> +			 * here, but it was never promised to be
	> +			 * accurate.
	> +			 */
	> +			if (list_is_last(&task->sibling, &start->children))
	> +				goto out;

looks misleading. Contrary to the slow path, we can't miss the
freshly forked child here, copy_process() does list_add_tail().

But the slow path obviously can skip much more than needed and
miss children (freshly forked or not), probably it would be better
to move the comment down and remove the "freshly created" part.

What do you think?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/