linux-kernel - Re: [PATCH UPDATED AGAIN 03/10] threadgroup: extend threadgroup

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: <20111209234246.GC27173@somewhere>
Date:	Sat, 10 Dec 2011 00:42:53 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	paul@...lmenage.org, rjw@...k.pl, lizf@...fujitsu.com,
	linux-pm@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
	containers@...ts.linux-foundation.org, matthltc@...ibm.com,
	akpm@...ux-foundation.org, oleg@...hat.com,
	kamezawa.hiroyu@...fujitsu.com
Subject: Re: [PATCH UPDATED AGAIN 03/10] threadgroup: extend
 threadgroup_lock() to cover exit and exec

On Thu, Dec 08, 2011 at 12:50:55PM -0800, Tejun Heo wrote:
> (Note for Linus at the bottom)
> 
> threadgroup_lock() protected only protected against new addition to
> the threadgroup, which was inherently somewhat incomplete and
> problematic for its only user cgroup.  On-going migration could race
> against exec and exit leading to interesting problems - the symmetry
> between various attach methods, task exiting during method execution,
> ->exit() racing against attach methods, migrating task switching basic
> properties during exec and so on.
> 
> This patch extends threadgroup_lock() such that it protects against
> all three threadgroup altering operations - fork, exit and exec.  For
> exit, threadgroup_change_begin/end() calls are added to exit_signals
> around assertion of PF_EXITING.  For exec, threadgroup_[un]lock() are
> updated to also grab and release cred_guard_mutex.
> 
> With this change, threadgroup_lock() guarantees that the target
> threadgroup will remain stable - no new task will be added, no new
> PF_EXITING will be set and exec won't happen.
> 
> The next patch will update cgroup so that it can take full advantage
> of this change.
> 
> -v2: beefed up comment as suggested by Frederic.
> 
> -v3: narrowed scope of protection in exit path as suggested by
>      Frederic.
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> Acked-by: Li Zefan <lizf@...fujitsu.com>
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Paul Menage <paul@...lmenage.org>
> Cc: Frederic Weisbecker <fweisbec@...il.com>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> ---
> Okay, narrowed exit path protection down to setting of PF_EXITING
> itself.  ->exit() on dangling tasks is a bit weird but I don't think
> it's too bad.  Frederic, are you okay with this version?

Yeah that new scheme that only protects PF_EXITING may look a bit
strange. But I think we are fine. With rcu list traversal, it should
be safe even if a group member is concurrently dropped from the list (in that
case all we check if its PF_EXITING then we give up). And we may
have a concurrent ->exit() but that should be fine too.

Thanks!

Acked-by: Frederic Weisbecker <fweisbec@...il.com>

> 
> Linus, if Frederic is okay with it, I'm gonna rebase the series on top
> of freezer changes in pm tree to avoid conflicts in cgroup_freezer,
> which sits between cgroup and freezer, both of which are going through
> non-trivial changes, push the branch to linux-next and put pending
> cgroup patches on top.  Please scream if you're mighty unhappy with it
> or have a better idea.
> 
> Thank you.
--
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/