linux-kernel - Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach

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]

Message-ID: <20110907235931.GA22545@unix33.andrew.cmu.edu>
Date:	Wed, 7 Sep 2011 19:59:31 -0400
From:	Ben Blum <bblum@...rew.cmu.edu>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ben Blum <bblum@...rew.cmu.edu>, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, fweisbec@...il.com, neilb@...e.de,
	paul@...lmenage.org, paulmck@...ux.vnet.ibm.com
Subject: Re: +
 cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to
 -mm tree

On Fri, Sep 02, 2011 at 05:55:34PM +0200, Oleg Nesterov wrote:
> On 09/02, Ben Blum wrote:
> >
> > On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote:
> > > Forgot to mention, sorry...
> > >
> > > That said, I believe the patch is correct and should fix the problem.
> >
> > Thanks!
> >
> > But I don't think the check becomes pointless? If a sub-thread execs
> > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid
> > in attach_task_by_pid), that causes the case that the comment refers to.
> 
> How so? The comment says:
> 
> 	* a race with de_thread from another thread's exec() may strip
> 	* us of our leadership, making while_each_thread unsafe
> 
> This is not true.

Sorry, the comment is unclear. The reason I think this is necessary is
if de_thread happens, the leader would fall off the thread-group list:

de_thread()
=> release_task(leader)
=> __exit_signal(leader)
=> __unhash_process(leader, false)
=> list_del_rcu(&leader->thread_group)

which is the same list that while_each_thread() iterates over.

and this looks like an unconditionally taken path?

> 
> And. Given that ->group_leader can be changed right after we drop tasklist
> this check is pointless. Yes, it can detect the case when this task_struct
> has nothing to do with this process sometimes, but not in general. (This
> connects to other problems I mentioned).

I agree there is a problem later with the ss->attach(leader) calls.

If the above reasoning is right, though, it's necessary here, and also
guarantees that that the later iteration (in cgroup_attach_proc's "step
3") accurately reflects all threads in the group.

Thanks,
Ben

> 
> IOW, personally I think it would be better to update the patch. But I
> won't insist.
> 
> 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/