Properly share process and session keyrings with CLONE_THREAD
Subsystems (6)
David Howells
maintainerPATCH 1/1 Properly share process and session keyrings with CLONE_THREAD
The attached patch causes process and session keyrings to be shared properly when CLONE_THREAD is in force. It does this by moving the keyring pointers into struct signal_struct[*].
[*] I have a patch to rename this to struct thread_group that I'll revisit after the advent of 2.6.11.
Furthermore, once this patch is applied, process keyrings will no longer be allocated at fork, but will instead only be allocated when needed. Allocating them at fork was a way of half getting around the sharing across threads problem, but that's no longer necessary.
---
warthog>diffstat -p1 keys-task-2611rc4.diff
include/linux/key.h | 5 + include/linux/sched.h | 12 ++- init/main.c | 2 kernel/fork.c | 8 ++ kernel/signal.c | 1 security/keys/process_keys.c | 157 +++++++++++++++++++++++++------------------ security/keys/request_key.c | 45 +++++++++--- 7 files changed, 150 insertions(+), 80 deletions(-)
include/linux/key.h +5 -0
@@ -58,6 +58,7 @@struct seq_file ;struct user_struct ; +struct signal_struct ;struct key_type ;struct key_owner ;
@@ -258,7 +259,9 @@extern int alloc_uid_keyring (struct user_struct *user);extern void switch_uid_keyring (struct user_struct *new_user);extern int copy_keys (unsigned long clone_flags,struct task_struct *tsk); +extern int copy_thread_group_keys (struct task_struct *tsk);extern void exit_keys (struct task_struct *tsk); +extern void exit_thread_group_keys (struct signal_struct *tg);extern int suid_keys (struct task_struct *tsk);extern int exec_keys (struct task_struct *tsk);extern void key_fsuid_changed (struct task_struct *tsk);
@@ -274,7 +277,9 @@#define alloc_uid_keyring (u) 0#define switch_uid_keyring (u) do { } while(0)#define copy_keys (f,t) 0 +#define copy_thread_group_keys (t) 0#define exit_keys (t) do { } while(0) +#define exit_thread_group_keys (tg) do { } while(0)#define suid_keys (t) do { } while(0)#define exec_keys (t) do { } while(0)#define key_fsuid_changed (t) do { } while(0)
include/linux/sched.h +10 -2
@@ -329,8 +329,18 @@* have no need to disable irqs. */ struct rlimit rlim[RLIM_NLIMITS ]; + +/* keep the process-shared keyrings here so that they do the right +* thing in threads created with CLONE_THREAD */ +#ifdef CONFIG_KEYS +struct key *session_keyring;/* keyring inherited over fork */ +struct key *process_keyring;/* keyring private to this process */ +#endif }; +#define thread_group_lock (tsk, flags) spin_lock_irqsave(&(tsk)->sighand->siglock, flags) +#define thread_group_unlock (tsk, flags) spin_unlock_irqrestore(&(tsk)->sighand->siglock, flags); +/* * Bits in flags field of signal_struct. */
@@ -610,8 +620,6 @@ unsigned keep_capabilities:1;struct user_struct *user;#ifdef CONFIG_KEYS -struct key *session_keyring;/* keyring inherited over fork */ -struct key *process_keyring;/* keyring private to this process (CLONE_THREAD) */ struct key *thread_keyring;/* keyring private to this thread */ #endif int oomkilladj;/* OOM kill score adjustment (bit shift). */
init/main.c +1 -1
@@ -497,6 +497,7 @@proc_caches_init ();buffer_init ();unnamed_dev_init (); +key_init ();security_init ();vfs_caches_init (num_physpages);radix_tree_init ();
@@ -574,7 +575,6 @@/* drivers will send hotplug events */ init_workqueues ();usermodehelper_init (); -key_init ();driver_init ();#ifdef CONFIG_SYSCTL
kernel/fork.c +8 -0
@@ -719,6 +719,7 @@static inline int copy_signal (unsigned long clone_flags,struct task_struct * tsk) {struct signal_struct *sig; +int ret;if (clone_flags &CLONE_THREAD ) {atomic_inc (¤t->signal->count);
@@ -729,6 +730,13 @@ tsk->signal = sig;if (!sig)return -ENOMEM ; + + ret =copy_thread_group_keys (tsk); +if (ret <0 ) { +kmem_cache_free (signal_cachep, sig); +return ret; + } +atomic_set (&sig->count,1 );atomic_set (&sig->live,1 );init_waitqueue_head (&sig->wait_chldexit);
kernel/signal.c +1 -0
@@ -402,6 +402,7 @@* signals are constrained to threads inside the group. */ exit_itimers (sig); +exit_thread_group_keys (sig);kmem_cache_free (signal_cachep, sig); } }
security/keys/process_keys.c +92 -65
294 lines not on this page · raw message
security/keys/request_key.c +33 -12
71 lines not on this page · raw message
David Howells
maintainerPATCH 1/1 Keys: Doc update for properly sharing process keyrings patch
The attached patch updates the documentation on the kernel keys to reflect the fact that process keyrings will no longer change ownership when the UID and GID of a thread change; process's don't have UIDs and GIDs in Linux, only threads do.
This patch is contingent on the keyring sharing patch submitted a few minutes ago.
---
warthog>diffstat -p1 keys-task-doc-2611rc4.diff
Documentation/keys.txt | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
Documentation/keys.txt +2 -2
@@ -151,8 +151,8 @@ by using PR_JOIN_SESSION_KEYRING. It is permitted to request an anonymous new one, or to attempt to create or join one of a specific name. - The ownership of the thread and process-specific keyrings changes when - the real UID and GID of the thread changes. + The ownership of the thread keyring changes when the real UID and GID of + the thread changes. (*) Each user ID resident in the system holds two special keyrings: a user specific keyring and a default user session keyring. The default session
Quoted 4 lines · David Howells
The attached patch causes process and session keyrings to be shared properly when
CLONE_THREADis in force. It does this by moving the keyring pointers into struct signal_struct[*].
I do not see the point of associating keys with signal state.
And it _is_ signal state, even if some people mistakenly think that it's about "processes". Linux still hasn't fallen into the trap of believing that POSIX threads are somehow magical and the only way to do things.
The kernel very much believes in various shared resources. Signal state (tty, resource limits, and actual signals handlers) is one such shared thing, but so is VM state, file table state etc etc. Why would keys logically be associated with signals, and not with the file table, for example? Or the VM? Or just per-thread?
In other words, what does this buy us, if anything? From a logical standpoint, I'd have said that it makes more sense to associate this with the filesystem information, since keys would tend to mostly go together with the notion of permissions on file descriptors.
Making keys be associated with signals just doesn't seem to make any sense.
Linus
David Howells
maintainerQuoted 5 lines · Linus Torvalds
I do not see the point of associating keys with signal state.
And it _is_ signal state, even if some people mistakenly think that it's about "processes". Linux still hasn't fallen into the trap of believing that POSIX threads are somehow magical and the only way to do things.
Erm... Even if it's you who suggested it? To quote:
| Also, if it's shared on CLONE_THREAD, then logically the structure already | exists: it is "tsk->signal". The _naming_ may be historical and slightly | confusing, but the fact is, that's the structure that gets shared on | CLONE_THREAD, and there's no technical point to creating a new one. | | If the name really bothers you, I'd be more willing to rename | "tsk->signal", although the pain of that makes me strongly suspect it's | not worth it.
Well, I've come up with a patch to do the renaming; whilst it is quite invasive, if it's applied as soon as possible after 2.6.11 is out, it shouldn't cause too much of a problem. I can even make the changes in stages to have less impact on the drivers.
Also, _why_ can't signal_struct be changed into something associated with a thread group? So it contains the controls for thread-group-wide signalling? But that's okay - the struct would then be for thread-group-wide concepts, and so it would still fit.
Quoted 5 lines
The kernel very much believes in various shared resources. Signal state (tty, resource limits, and actual signals handlers) is one such shared thing, but so is VM state, file table state etc etc. Why would keys logically be associated with signals, and not with the file table, for example? Or the VM? Or just per-thread?
Because I want to have the session- and process-keyrings associated with each process to be shared amongst all the threads that process contains, such that if one thread joins a new session this affects all its sibling threads too. And in this context, I think 'threads' have to be defined by CLONE_THREAD. Why else have such a clone flag? Or does CLONE_THREAD not "distinguish" threads and processes as far as /proc, exec and exit are concerned?
There's a per-thread keyring available too; and that is strictly per thread.
Quoted 4 lines
In other words, what does this buy us, if anything? From a logical standpoint, I'd have said that it makes more sense to associate this with the filesystem information, since keys would tend to mostly go together with the notion of permissions on file descriptors.
Just like uids, gids and group lists, eh? I think keys are more comparable to those than to permissions masks.
Besides, from a logical standpoint, I generally think of a "process" as being the concept to which threads, fds, VMs, etc can be attached. Obviously Linux is more flexible than that, but I nonetheless find it easier to explain the semantics to people in terms of processes and threads.
Quoted 2 lines
Making keys be associated with signals just doesn't seem to make any sense.
Then redefine the signal_struct structure. There's no particular reason it can't be made into the "anchor" block for a thread group, and I can see a number of advantages from doing so.
David
Quoted 2 lines · David Howells
There's a per-thread keyring available too; and that is strictly per thread.
Ahh, I'd forgotten about that. Yeah, as long as the per-thread thing is still there, I guess I'm ok with it (what I _really_ don't want to lose is the ability to have independent threads - threaded servers using per-request keys etc, and that's my main gripe with the stupid POSIX threading model: it takes away per-thread data).
Can you not do the "thread_group_lock()" thing, though? I hate hiding locks in "helper functions". You search for the lock, and now you've hidden that "siglock" usage away and made it very different from all the other siglock usage. You've also hidden the fact that it accesses "current->signal" (while not hiding it would mean that the code would likely be more cleanly done as
struct sighand *grp = current->sighand; spin_lock_irq(grp->siglock); .. access group keys .. spin_unlock_irq(grp->siglock);
Also, you have "current->thread_group" in there:
+ key_check(current->thread_group->session_keyring);
+ key_check(current->thread_group->process_keyring);
which seems to have gotten through only because your key debug stuff is disabled..
Linus
David Howells
maintainerPATCH 1/1 Properly share process and session keyrings with CLONE_THREAD [try #2]
In reply to David HowellsThe attached patch causes process and session keyrings to be shared properly when CLONE_THREAD is in force. It does this by moving the keyring pointers into struct signal_struct[*].
[*] I have a patch to rename this to struct thread_group that I'll revisit after the advent of 2.6.11.
Furthermore, once this patch is applied, process keyrings will no longer be allocated at fork, but will instead only be allocated when needed. Allocating them at fork was a way of half getting around the sharing across threads problem, but that's no longer necessary.
This revision of the patch has the documentation changes patch rolled into it and no longer abstracts the locking for signal_struct into a pair of macros.
---
warthog>diffstat -p1 keys-task-2611rc4-2.diff
Documentation/keys.txt | 4 include/linux/key.h | 5 + include/linux/sched.h | 9 +- init/main.c | 2 kernel/fork.c | 8 + kernel/signal.c | 1 security/keys/process_keys.c | 175 ++++++++++++++++++++++++------------------- security/keys/request_key.c | 46 ++++++++--- 8 files changed, 158 insertions(+), 92 deletions(-)
Documentation/keys.txt +2 -2
@@ -151,8 +151,8 @@ by using PR_JOIN_SESSION_KEYRING. It is permitted to request an anonymous new one, or to attempt to create or join one of a specific name. - The ownership of the thread and process-specific keyrings changes when - the real UID and GID of the thread changes. + The ownership of the thread keyring changes when the real UID and GID of + the thread changes. (*) Each user ID resident in the system holds two special keyrings: a user specific keyring and a default user session keyring. The default session
include/linux/key.h +5 -0
@@ -58,6 +58,7 @@struct seq_file ;struct user_struct ; +struct signal_struct ;struct key_type ;struct key_owner ;
@@ -258,7 +259,9 @@extern int alloc_uid_keyring (struct user_struct *user);extern void switch_uid_keyring (struct user_struct *new_user);extern int copy_keys (unsigned long clone_flags,struct task_struct *tsk); +extern int copy_thread_group_keys (struct task_struct *tsk);extern void exit_keys (struct task_struct *tsk); +extern void exit_thread_group_keys (struct signal_struct *tg);extern int suid_keys (struct task_struct *tsk);extern int exec_keys (struct task_struct *tsk);extern void key_fsuid_changed (struct task_struct *tsk);
@@ -274,7 +277,9 @@#define alloc_uid_keyring (u) 0#define switch_uid_keyring (u) do { } while(0)#define copy_keys (f,t) 0 +#define copy_thread_group_keys (t) 0#define exit_keys (t) do { } while(0) +#define exit_thread_group_keys (tg) do { } while(0)#define suid_keys (t) do { } while(0)#define exec_keys (t) do { } while(0)#define key_fsuid_changed (t) do { } while(0)
include/linux/sched.h +7 -2
@@ -329,6 +329,13 @@* have no need to disable irqs. */ struct rlimit rlim[RLIM_NLIMITS ]; + +/* keep the process-shared keyrings here so that they do the right +* thing in threads created with CLONE_THREAD */ +#ifdef CONFIG_KEYS +struct key *session_keyring;/* keyring inherited over fork */ +struct key *process_keyring;/* keyring private to this process */ +#endif };/*
@@ -610,8 +617,6 @@ unsigned keep_capabilities:1;struct user_struct *user;#ifdef CONFIG_KEYS -struct key *session_keyring;/* keyring inherited over fork */ -struct key *process_keyring;/* keyring private to this process (CLONE_THREAD) */ struct key *thread_keyring;/* keyring private to this thread */ #endif int oomkilladj;/* OOM kill score adjustment (bit shift). */
init/main.c +1 -1
@@ -497,6 +497,7 @@proc_caches_init ();buffer_init ();unnamed_dev_init (); +key_init ();security_init ();vfs_caches_init (num_physpages);radix_tree_init ();
@@ -574,7 +575,6 @@/* drivers will send hotplug events */ init_workqueues ();usermodehelper_init (); -key_init ();driver_init ();#ifdef CONFIG_SYSCTL
kernel/fork.c +8 -0
@@ -719,6 +719,7 @@static inline int copy_signal (unsigned long clone_flags,struct task_struct * tsk) {struct signal_struct *sig; +int ret;if (clone_flags &CLONE_THREAD ) {atomic_inc (¤t->signal->count);
@@ -729,6 +730,13 @@ tsk->signal = sig;if (!sig)return -ENOMEM ; + + ret =copy_thread_group_keys (tsk); +if (ret <0 ) { +kmem_cache_free (signal_cachep, sig); +return ret; + } +atomic_set (&sig->count,1 );atomic_set (&sig->live,1 );init_waitqueue_head (&sig->wait_chldexit);
diff -uNr linux-2.6.11-rc4/kernel/signal.c linux-2.6.11-rc4-keys-task/kernel/signal.c
kernel/signal.c +1 -0
@@ -402,6 +402,7 @@* signals are constrained to threads inside the group. */ exit_itimers (sig); +exit_thread_group_keys (sig);kmem_cache_free (signal_cachep, sig); } }
security/keys/process_keys.c +100 -75
316 lines not on this page · raw message
security/keys/request_key.c +34 -12
72 lines not on this page · raw message