Re: [PATCH 05/23] proc: Simplify the ownership rules for /proc

Re: [PATCH 05/23] proc: Simplify the ownership rules for /proc

[Albert Cahalan]

Eric W. Biederman writes:

> Currently in /proc if the task is dumpable all of files are owned by
> the tasks effective users.  Otherwise the files are owned by root.
> Unless it is the /proc/tgid>/ or /proc/tgid>/task/pid> directory
> in that case we always make the directory owned by the effective user.
>
> However the special case for directories is pointless except as a way
> to read the effective user, because the permissions on both of those
> directories are world readable, and executable.

Well, that's exactly how "top" gets the EUID. The code:

p->euid = sb.st_uid;       /* need a way to get real uid */
p->egid = sb.st_gid;       /* need a way to get real gid */

I sure hope this patch didn't slip by me somehow. Big proc changes
ought to get review by the maintainers of procps, gtop, gdb, and
probably a good number of packages that don't come to mind right now.
I'm lucky I spotted this while reading over old lwn.net stories.

> /proc/tgid>/status provides a much better way to read a processes
> effecitve userid, so it is silly to try to provide that on the directory.

The stat() call is cheap.

The status file is kind of nasty:

open()
read()
close()
parse vague ill-defined ASCII text using evil speed hacks

The procps code uses stat() for selection by EUID in some
cases, and for everything whenever the status file is not
needed for some other reason. The "top" program is quite
good about not opening the status file. Lots of profiling
showed that there would be a noticable performance difference.

Re: [PATCH 05/23] proc: Simplify the ownership rules for /proc

[Eric W. Biederman]

"Albert Cahalan" <acahalan@gmail.com> writes:

> Eric W. Biederman writes:
>
>> Currently in /proc if the task is dumpable all of files are owned by
>> the tasks effective users.  Otherwise the files are owned by root.
>> Unless it is the /proc/<tgid>/ or /proc/<tgid>/task/<pid> directory
>> in that case we always make the directory owned by the effective user.
>>
>> However the special case for directories is pointless except as a way
>> to read the effective user, because the permissions on both of those
>> directories are world readable, and executable.
>
> Well, that's exactly how "top" gets the EUID. The code:
>
> p->euid = sb.st_uid;       /* need a way to get real uid */
> p->egid = sb.st_gid;       /* need a way to get real gid */
>
> I sure hope this patch didn't slip by me somehow. 

It is still in -mm so there is sufficient time to comment.  My apologies
for not cc'ing you.

> Big proc changes
> ought to get review by the maintainers of procps, gtop, gdb, and
> probably a good number of packages that don't come to mind right now.
> I'm lucky I spotted this while reading over old lwn.net stories.

Well it is a bunch of cleanups to the implementation of /proc not
really a big user visible change.  The problem is that the implementation
is a maintenance nightmare.

There are some significant changes on my todo list to cope
with multiple processes having the same pid but those have not
happened yet.

>> /proc/<tgid>/status provides a much better way to read a processes
>> effecitve userid, so it is silly to try to provide that on the directory.
>
> The stat() call is cheap.

So I did not break the fact that stat() works.  But now
stat does not give you the euid on if the task is not dumpable.

> The status file is kind of nasty:
Agreed.

> The procps code uses stat() for selection by EUID in some
> cases, and for everything whenever the status file is not
> needed for some other reason. The "top" program is quite
> good about not opening the status file. Lots of profiling
> showed that there would be a noticable performance difference.

All of which sounds sane.  Although I wonder if the kernel side
implementation of the status file was improved if that could
help things.

Looking at 2.4 and 2.2 this case does seem to be consistently
maintained, although I'm not at all certain if the application
changed it's euid that the change would be reflected in /proc,
until the version of revalidate in 2.6.

My real problem with the implementation is the hard coded magic
inode numbers.  That does really ugly things to the implementation
of /proc.

If instead of special case /proc/<pid>/ would it be ok if
this applied to any directory that is world readable and executable?

ie.

#define S_ISDIR_RXUGO(m) \
	(((m) & (S_IFMT|S_IRUGO|S_IXUGO)) == (S_IFDIR|S_IRUGO|S_IXUGO))

if (S_ISDIR_RXUGO(inode->i_mode) || task_dumpable(task)) {
	inode->i_uid = task->euid;
	inode->i_gid = task->egid;
} else {
	inode->i_uid = 0;
	inode->i_gid = 0;
}
	
Eric

Re: [PATCH 05/23] proc: Simplify the ownership rules for /proc

[Albert Cahalan]

On 3/20/06, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Albert Cahalan" <acahalan@gmail.com> writes:

> > Well, that's exactly how "top" gets the EUID. The code:
> >
> > p->euid = sb.st_uid;       /* need a way to get real uid */
> > p->egid = sb.st_gid;       /* need a way to get real gid */
> >
> > I sure hope this patch didn't slip by me somehow.
>
> It is still in -mm so there is sufficient time to comment.  My apologies
> for not cc'ing you.
>
> > Big proc changes
> > ought to get review by the maintainers of procps, gtop, gdb, and
> > probably a good number of packages that don't come to mind right now.
> > I'm lucky I spotted this while reading over old lwn.net stories.
>
> Well it is a bunch of cleanups to the implementation of /proc not
> really a big user visible change.  The problem is that the implementation
> is a maintenance nightmare.

Use of the info returned by stat() goes way back in history, to a
time before the /proc/*/status files even existed.

If you want to rip out something, pick a recent and nasty feature.
The /proc/*/smaps file would be a prime example.

It would be very good to have a set of "deprecated" flags for
the files in /proc. The files could be present but not seen in
directory listings, and could log (rate limited) warnings if used.

> There are some significant changes on my todo list to cope
> with multiple processes having the same pid but those have not
> happened yet.

How in Hell is procps supposed to deal with that?
(and gdb, and gtop, and pstools...) I'd rather not see
the complexity. It seems I'll need multiple /proc mount
points, many extra command options, etc.

This is a lot of work for a feature that seems to be
taken care of by Xen and SE Linux.

> >> /proc/<tgid>/status provides a much better way to read a processes
> >> effecitve userid, so it is silly to try to provide that on the directory.
> >
> > The stat() call is cheap.
>
> So I did not break the fact that stat() works.  But now
> stat does not give you the euid on if the task is not dumpable.

That counts as breaking it.

> > The status file is kind of nasty:
> Agreed.
>
> > The procps code uses stat() for selection by EUID in some
> > cases, and for everything whenever the status file is not
> > needed for some other reason. The "top" program is quite
> > good about not opening the status file. Lots of profiling
> > showed that there would be a noticable performance difference.
>
> All of which sounds sane.  Although I wonder if the kernel side
> implementation of the status file was improved if that could
> help things.

The concept is only well-suited to toy sysadmin tool hacks.

> Looking at 2.4 and 2.2 this case does seem to be consistently
> maintained, although I'm not at all certain if the application
> changed it's euid that the change would be reflected in /proc,
> until the version of revalidate in 2.6.

I'm 100% sure this goes back to the 1.2.xx kernels.
I'm 99% sure it goes back to the 1.0.x kernels. So that
is over a decade of active use.

Remember, there were no /proc/*/status files. There are no UID
values in the /proc/*/stat files. The procps code worked fine.

> My real problem with the implementation is the hard coded magic
> inode numbers.  That does really ugly things to the implementation
> of /proc.

You could probably set all the inode numbers to 42 and
not have anything break.

I notice that you made a comment about being annoyed that
the Alpha has a 32-bit ino_t, which thus can't hold a pointer.

There won't be any more Alpha systems, so memory sizes
won't be getting any bigger for them, so...

cookie = ((unsigned long)ptr-PAGE_OFFSET)/sizeof(struct foo)

BTW, the max pid is limited by the pid allocator and the futex
code, so you only need 7 decimal digits.

> If instead of special case /proc/<pid>/ would it be ok if
> this applied to any directory that is world readable and executable?
>
> ie.
>
> #define S_ISDIR_RXUGO(m) \
>         (((m) & (S_IFMT|S_IRUGO|S_IXUGO)) == (S_IFDIR|S_IRUGO|S_IXUGO))

This kind of S_IRUGORXXWUOG stuff is quite unreadable.
Octal is way easier to deal with, especially once you get
to ORing the values together to make up for not having 512
defines for the permissions.

> if (S_ISDIR_RXUGO(inode->i_mode) || task_dumpable(task)) {
>         inode->i_uid = task->euid;
>         inode->i_gid = task->egid;
> } else {
>         inode->i_uid = 0;
>         inode->i_gid = 0;
> }

That seems OK, depending on how /proc/*/fd/* works.

Given the nature of /proc, checking at read() time is probably
a better idea. Any checking at open() should just be a bit of
politeness. It's not always OK to keep using a file descriptor
after the app went through a setuid exec.

The /proc/*/mem files are broken right now. They should be
readable and writable to anybody who could use ptrace, even
if not currently attached. (as is now, they are just a less-bad
way for debuggers to read memory)

It would be nice to ensure that a PID doesn't get reused while
a /proc file is open. Then, just by keeping the directory open,
apps would avoid inconsistencies.

Re: [PATCH 05/23] proc: Simplify the ownership rules for /proc

[Eric W. Biederman]

"Albert Cahalan" <acahalan@gmail.com> writes:

> Use of the info returned by stat() goes way back in history, to a
> time before the /proc/*/status files even existed.

Ok.  That make sense.  Definitely worth keeping then.

Like too many pieces of the /proc implementation the functionality
was kept with no memory of why it was they way it is.

Knowing this I just realized the current implementation is actually
broken with respect to fstat.

> If you want to rip out something, pick a recent and nasty feature.
> The /proc/*/smaps file would be a prime example.
>
> It would be very good to have a set of "deprecated" flags for
> the files in /proc. The files could be present but not seen in
> directory listings, and could log (rate limited) warnings if used.

That and we can drop a note in
Documentation/feature-removal-schedule.txt

I am much more likely to go after the non processes local stuff
in /proc.  Although perversely some of the things like /proc/sysvipc
and /proc/net are likely to become process local and stay.

>> There are some significant changes on my todo list to cope
>> with multiple processes having the same pid but those have not
>> happened yet.
>
> How in Hell is procps supposed to deal with that?
> (and gdb, and gtop, and pstools...) I'd rather not see
> the complexity. It seems I'll need multiple /proc mount
> points, many extra command options, etc.

In current draft I make a child pid space show up in it's parent like
a threaded process.  All of the statistics are under one /proc/pid
directory.  So most things work without modification.

Beyond that for the cluster case I really would like to see
a version of tools that can handle multiple mounts of /proc.
That way I can use 9fs, nfs or similar and mount a remote
copy of /proc and see what is going on.

> This is a lot of work for a feature that seems to be
> taken care of by Xen and SE Linux.

The hard part is actually the code cleanups and api review
and the discussions to get it included.  The actual implementation
is pretty simple.

>> >> /proc/<tgid>/status provides a much better way to read a processes
>> >> effecitve userid, so it is silly to try to provide that on the directory.
>> >
>> > The stat() call is cheap.
>>
>> So I did not break the fact that stat() works.  But now
>> stat does not give you the euid on if the task is not dumpable.
>
> That counts as breaking it.

Agreed.  But unfortunately not enough for it to be immediately visible.

>> > The status file is kind of nasty:
>> Agreed.
>>
>> > The procps code uses stat() for selection by EUID in some
>> > cases, and for everything whenever the status file is not
>> > needed for some other reason. The "top" program is quite
>> > good about not opening the status file. Lots of profiling
>> > showed that there would be a noticable performance difference.
>>
>> All of which sounds sane.  Although I wonder if the kernel side
>> implementation of the status file was improved if that could
>> help things.
>
> The concept is only well-suited to toy sysadmin tool hacks.
>
>> Looking at 2.4 and 2.2 this case does seem to be consistently
>> maintained, although I'm not at all certain if the application
>> changed it's euid that the change would be reflected in /proc,
>> until the version of revalidate in 2.6.
>
> I'm 100% sure this goes back to the 1.2.xx kernels.
> I'm 99% sure it goes back to the 1.0.x kernels. So that
> is over a decade of active use.

Yep.  1.2 is interesting to look at.  It didn't report
the euid unless the uid equaled the euid.  But the implementation
looks surprisingly similar to the current proc.  2.2 looks a lot
more different.

> Remember, there were no /proc/*/status files. There are no UID
> values in the /proc/*/stat files. The procps code worked fine.

The lack of uid values in /proc/*/stat files I hadn't realized.

>> My real problem with the implementation is the hard coded magic
>> inode numbers.  That does really ugly things to the implementation
>> of /proc.
>
> You could probably set all the inode numbers to 42 and
> not have anything break.

find in /proc breaks if I do that :)

> I notice that you made a comment about being annoyed that
> the Alpha has a 32-bit ino_t, which thus can't hold a pointer.
>
> There won't be any more Alpha systems, so memory sizes
> won't be getting any bigger for them, so...
>
> cookie = ((unsigned long)ptr-PAGE_OFFSET)/sizeof(struct foo)

Yes.  I have been thinking about that.  That looks like
a good implementation to put into fs/inode.c:new_inode().

Alpha has a 40bit physical address space so I need a structure
that is at least 256 bytes for that to work properly.  Unfortunately
struct inode easily qualifies.  

A related question is do you know if there is a way to tell
if two processes share a the filesystem mount namespace?

> BTW, the max pid is limited by the pid allocator and the futex
> code, so you only need 7 decimal digits.

Yes the pid allocator limits the current pid value to that range.
I'm not at all certain I like the futex code caring.

>> If instead of special case /proc/<pid>/ would it be ok if
>> this applied to any directory that is world readable and executable?
>>
>> ie.
>>
>> #define S_ISDIR_RXUGO(m) \
>>         (((m) & (S_IFMT|S_IRUGO|S_IXUGO)) == (S_IFDIR|S_IRUGO|S_IXUGO))
>
> This kind of S_IRUGORXXWUOG stuff is quite unreadable.
> Octal is way easier to deal with, especially once you get
> to ORing the values together to make up for not having 512
> defines for the permissions.
>

I am almost convinced.  Things like reading the type don't
work quiet as well.

Well the check wound up being:
	if ((inode->i_mode == S_IFDIR|S_IRUGO|S_IXUGO) ||

Which is a little less magic than I proposed, and I don't think
spotting the type of a file is at easy to do in octal.

>> if (S_ISDIR_RXUGO(inode->i_mode) || task_dumpable(task)) {
>>         inode->i_uid = task->euid;
>>         inode->i_gid = task->egid;
>> } else {
>>         inode->i_uid = 0;
>>         inode->i_gid = 0;
>> }
>
> That seems OK, depending on how /proc/*/fd/* works.

I deliberately made the check so it doesn't select
the /proc/*/fd and fd/* files.  So we should be ok there.

That is the one big change I have made.  The /proc/*/fd/*
files no require you to be able to ptrace the process to
use them.  For anyone outside a chroot who isn't root
the change should be totally invisible.

> Given the nature of /proc, checking at read() time is probably
> a better idea. Any checking at open() should just be a bit of
> politeness. It's not always OK to keep using a file descriptor
> after the app went through a setuid exec.

Hmm.  Good point.  All of the serious permission checking in
/proc already does that but this bit in revalidate needs a bit
of reexamination.

> The /proc/*/mem files are broken right now. They should be
> readable and writable to anybody who could use ptrace, even
> if not currently attached. (as is now, they are just a less-bad
> way for debuggers to read memory)

Makes sense.  Currently the mem_write is totally disabled.
Which is probably worse.

> It would be nice to ensure that a PID doesn't get reused while
> a /proc file is open. Then, just by keeping the directory open,
> apps would avoid inconsistencies.

The problem there is that unless we make the pid space a lot bigger
it becomes trivial to exhaust the pid allocator.  I think I could
do that with about 32 processes each with a 1000 open directories.
Something within most systems rlimit values.  The real pain
is that except for my tree in /proc that keeps task_struct pinned
so you can burn a lot of low memory doing that.

A variant of that does work.  You can open the pid directory and then
check the directory to see if the process is still alive.  It's
not quite as good but it does give you a way to detect pid wrap
around.  Unfortunately that isn't completely race free.

Hmm.  It just occurred to me pid files that include the process
start time would be much more robust.  Unfortunately that falls down
because you can't get the start time portably, or easily.

Eric

[PATCH 00/23] proc cleanup.

[Eric W. Biederman]

When working on pid namespaces I keep tripping over /proc.
It's hard coded inode numbers and the amount of cruft
accumulated over the years makes it hard to deal with.

So to put /proc out of my misery here is a series of patches that
removes the worst of the warts.

The first patch which introduces task_refs is used later to address
one of the worst faults how much low kernel memory it allows
an unprivileged process to pin.  There are other patches to cleanup
the permission checking, to cleanup how /proc interacts with the rest
of the kernel, and to patches to simply clean /proc up.

At least some of the cleans up go back to cruft that was introduced
in 2.2.  That was a challenge to track down and understand the
thinking at the time because even the historic git archive I have
doesn't go back that far :(

Ultimately the biggest cleanup is that this patchset removes 
the hard coded inode numbers from /proc.  There are still a few
theoretical issues about non-unique inode numbers but the /proc code
doesn't care, and it is no worse than the current situation with the
file descriptor inode numbers.  I would have loved to have made the
inode number the address of the inode data structure in the kernel but
I can't because on alpha __kernel_ino_t is an unsigned int!  Oh well,
the current situation keeps the inode numbers small and readable, and
32bit.

Eric

[PATCH 01/23] tref: Implement task references.

[Eric W. Biederman]

Holding a reference to a task_struct pins about 10K of low memory even
after that task has exited.  Which seems to be at 1 or 2 orders of
mangnitude more memory than any other data structure in the kernel.
Not holding a reference to a task_struct and you risk problems with
pid wrap around.

Even worse because we allow session and process group leaders to exit
there is no task_struct you can hold onto to prevent pid wrap around
problems for those kinds of structures.

The task_ref is an small intermediate data structure that other
structures can point, that solves these problems.  A task_ref will
always point at the first user of a pid value or contain a NULL
pointer if there are no longer any users of that pid.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 include/linux/pid.h      |    4 +
 include/linux/task_ref.h |   69 ++++++++++++++++++++++++
 kernel/Makefile          |    2 -
 kernel/fork.c            |    7 ++
 kernel/pid.c             |   12 ++++
 kernel/task_ref.c        |  131 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 224 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/task_ref.h
 create mode 100644 kernel/task_ref.c

8622b332e1e3c5ca2e451828f127e91729ae497f
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 099e70e..2849b7d 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -1,6 +1,8 @@
 #ifndef _LINUX_PID_H
 #define _LINUX_PID_H
 
+struct task_ref;
+
 enum pid_type
 {
 	PIDTYPE_PID,
@@ -17,6 +19,8 @@ struct pid
 	struct hlist_node pid_chain;
 	/* list of pids with the same nr, only one of them is in the hash */
 	struct list_head pid_list;
+	/* Does a weak reference of this type exist to the task struct? */
+	struct task_ref *ref;
 };
 
 #define pid_task(elem, type) \
diff --git a/include/linux/task_ref.h b/include/linux/task_ref.h
new file mode 100644
index 0000000..e8446bd
--- /dev/null
+++ b/include/linux/task_ref.h
@@ -0,0 +1,69 @@
+#ifndef _LINUX_TASK_REF_H
+#define _LINUX_TASK_REF_H
+
+/* What is a task_ref?
+ *
+ * A task_ref is a structure that holds a pointer to a task_struct, but
+ * instead of holding a reference count to the task_struct a backwards
+ * pointer from the task_struct to the task_ref is maintained.  When
+ * the task exits that references is broken and the task_struct
+ * pointer in the task_ref is cleared to NULL.
+ *
+ * This allows tracking a task_struct without pinning it in memory.  A
+ * task_struct plus a stack consumes around 10K of low kernel memory.
+ * More precisely this is THREAD_SIZE + sizeof(struct task_struct).
+ * By comparision a task_ref is between 16 and 20 bytes.
+ *
+ * The task_ref allows tracking not individual pids but also any pid_type.
+ * This means we can stop using individual pids in kernel data
+ * structures and directly track the processes those pids refer to.
+ * This advantage is that this allows the kernel to avoid pid wrap
+ * problems with it's internal references.
+ *
+ *
+ * Using a pointer to a pointer can be awkward, especially if you
+ * always must test to see if that pointer is NULL before using it.
+ *
+ * I simply things by including having the init_tref member
+ * and the tref_init, tref_set, tref_reset, and tref_fini functions
+ * for manipulating a task_ref pointer.  They take care of reference
+ * counting and ensuring that a task_ref pointer will point to
+ * init_task_ref if it does not have something useful to point to.
+ *
+ */
+
+struct task_struct;
+enum pid_type;
+
+struct task_ref
+{
+	atomic_t count;
+	enum pid_type type;
+	pid_t pid;
+	struct task_struct *task;
+};
+
+/* Note to read a usable value task value from struct task_ref
+ * the tasklist_lock must be held.  The atomic property of single
+ * word reads will keep any value you read consistent but it doesn't
+ * protect you from the race of the task exiting on another cpu and
+ * having it's task_struct freed or reused.  Holding the tasklist_lock
+ * prevents the task from going away as you dereference the task pointer.
+ */
+
+extern struct task_ref init_tref;
+
+extern void tref_put(struct task_ref *ref);
+extern struct task_ref *tref_get(struct task_ref *ref);
+extern struct task_ref *tref_get_by_task(task_t *task, enum pid_type type);
+extern struct task_ref *tref_get_by_pid(int pid, enum pid_type type);
+
+extern void tref_init(struct task_ref **dst);
+extern void tref_set(struct task_ref **dst, struct task_ref *ref);
+extern void tref_reset(struct task_ref **dst);
+extern void tref_fini(struct task_ref **dst);
+
+extern struct task_struct *get_tref_task(const struct task_ref *tref);
+
+
+#endif /* _LINUX_TASK_REF_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index 4ae0fbd..d8c0970 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -5,7 +5,7 @@
 obj-y     = sched.o fork.o exec_domain.o panic.o printk.o profile.o \
 	    exit.o itimer.o time.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o timer.o user.o \
-	    signal.o sys.o kmod.o workqueue.o pid.o \
+	    signal.o sys.o kmod.o workqueue.o pid.o task_ref.o \
 	    rcupdate.o extable.o params.o posix-timers.o \
 	    kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \
 	    hrtimer.o
diff --git a/kernel/fork.c b/kernel/fork.c
index fbea12d..3f56d5a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -157,6 +157,7 @@ void __init fork_init(unsigned long memp
 
 static struct task_struct *dup_task_struct(struct task_struct *orig)
 {
+	int type;
 	struct task_struct *tsk;
 	struct thread_info *ti;
 
@@ -179,6 +180,12 @@ static struct task_struct *dup_task_stru
 	/* One for us, one for whoever does the "release_task()" (usually parent) */
 	atomic_set(&tsk->usage,2);
 	atomic_set(&tsk->fs_excl, 0);
+
+	/* Initially there are no weak references to this task */
+	for (type = 0; type < PIDTYPE_MAX; type++) {
+		tsk->pids[type].nr = 0;
+		tsk->pids[type].ref = NULL;
+	}
 	return tsk;
 }
 
diff --git a/kernel/pid.c b/kernel/pid.c
index 7781d99..f365dbb 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -26,6 +26,7 @@
 #include <linux/init.h>
 #include <linux/bootmem.h>
 #include <linux/hash.h>
+#include <linux/task_ref.h>
 
 #define pid_hashfn(nr) hash_long((unsigned long)nr, pidhash_shift)
 static struct hlist_head *pid_hash[PIDTYPE_MAX];
@@ -151,6 +152,7 @@ int fastcall attach_pid(task_t *task, en
 	task_pid = &task->pids[type];
 	pid = find_pid(type, nr);
 	task_pid->nr = nr;
+	task_pid->ref = NULL;
 	if (pid == NULL) {
 		INIT_LIST_HEAD(&task_pid->pid_list);
 		hlist_add_head_rcu(&task_pid->pid_chain,
@@ -165,18 +167,28 @@ int fastcall attach_pid(task_t *task, en
 
 static fastcall int __detach_pid(task_t *task, enum pid_type type)
 {
+	task_t *task_next;
 	struct pid *pid, *pid_next;
+	struct task_ref *ref;
 	int nr = 0;
 
 	pid = &task->pids[type];
+	ref = pid->ref;
 	if (!hlist_unhashed(&pid->pid_chain)) {
 
 		if (list_empty(&pid->pid_list)) {
+			if (ref)
+				ref->task = NULL;
 			nr = pid->nr;
 			hlist_del_rcu(&pid->pid_chain);
 		} else {
+			task_next = pid_task(pid->pid_list.next, type);
 			pid_next = list_entry(pid->pid_list.next,
 						struct pid, pid_list);
+			/* Update the reference to point at the next task */
+			if (ref)
+				ref->task = task_next;
+			pid_next->ref = ref;
 			/* insert next pid from pid_list to hash */
 			hlist_replace_rcu(&pid->pid_chain,
 					  &pid_next->pid_chain);
diff --git a/kernel/task_ref.c b/kernel/task_ref.c
new file mode 100644
index 0000000..2f0a880
--- /dev/null
+++ b/kernel/task_ref.c
@@ -0,0 +1,131 @@
+#include <linux/sched.h>
+#include <linux/task_ref.h>
+
+struct task_ref init_tref = {
+	.count = ATOMIC_INIT(1),
+	.type  = PIDTYPE_PID,
+	.pid   = 0,
+	.task  = NULL,
+};
+
+void tref_put(struct task_ref *ref)
+{
+	might_sleep();
+	if (atomic_dec_and_test(&ref->count)) {
+		struct task_struct *task;
+		BUG_ON(ref == &init_tref);
+		/* Carefully serialize against __detach_pid and tref_get_by_pid */
+		write_lock_irq(&tasklist_lock);
+		task = ref->task;
+		if (task)
+			task->pids[ref->type].ref = NULL;
+		write_unlock_irq(&tasklist_lock);
+		kfree(ref);
+	}
+}
+
+struct task_ref *tref_get(struct task_ref *ref)
+{
+	atomic_inc(&ref->count);
+	return ref;
+}
+
+struct task_ref *tref_get_by_task(struct task_struct *task, enum pid_type type)
+{
+	struct task_ref *new_ref, *ref = NULL;
+	struct pid *pid;
+	might_sleep();
+	
+	/* Get the pid hash table entry */
+	pid = &task->pids[type];
+
+	/* Safely get the an existing reference */
+	read_lock(&tasklist_lock);
+	ref = pid->ref;
+	if (ref)
+		tref_get(ref);
+	read_unlock(&tasklist_lock);
+	if (ref)
+		goto out;
+
+	/* There was not an existing task ref so allocate one */
+	new_ref = kmalloc(sizeof(*new_ref), GFP_KERNEL);
+	if (new_ref) {
+		/* Carefully serialize against __detach_pid and tref_put */
+		write_lock_irq(&tasklist_lock);
+		ref = pid->ref;
+		if (ref)
+			tref_get(ref);
+		else if (pid->nr) {
+			atomic_set(&new_ref->count, 1);
+			new_ref->type = type;
+			new_ref->pid  = pid->nr;
+			new_ref->task = task;
+			pid->ref = ref = new_ref;
+		}
+		write_unlock_irq(&tasklist_lock);
+		if (ref != new_ref)
+			kfree(new_ref);
+	}
+out:
+	if (!ref)
+		ref = tref_get(&init_tref);
+	return ref;
+}
+
+struct task_ref *tref_get_by_pid(int pid, enum pid_type type)
+{
+	struct task_struct *task;
+	struct task_ref *tref;
+
+	/* Lookup the and pin the task */
+	read_lock(&tasklist_lock);
+	task = find_task_by_pid_type(type, pid);
+	if (task)
+		get_task_struct(task);
+	read_unlock(&tasklist_lock);
+
+	/* Now get the tref */
+	if (task) {
+		tref = tref_get_by_task(task, type);
+		put_task_struct(task);
+	}
+	else
+		tref = tref_get(&init_tref);
+	return tref;
+}
+
+void tref_init(struct task_ref **dst)
+{
+	*dst = tref_get(&init_tref);
+}
+
+void tref_set(struct task_ref **dst, struct task_ref *ref)
+{
+	tref_put(*dst);
+	*dst = ref;
+}
+
+void tref_reset(struct task_ref **dst)
+{
+	tref_put(*dst);
+	*dst = tref_get(&init_tref);
+}
+
+void tref_fini(struct task_ref **dst)
+{
+	tref_put(*dst);
+	*dst = NULL;
+}
+
+
+struct task_struct *get_tref_task(const struct task_ref *tref)
+{
+	struct task_struct *task;
+	read_lock(&tasklist_lock);
+	task = tref->task;
+	if (task)
+		get_task_struct(task);
+	read_unlock(&tasklist_lock);
+	return task;
+}
-- 
1.2.2.g709a

[PATCH 02/23] proc: Fix the .. inode number on /proc/<pid>/fd

[Eric W. Biederman]

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/proc/base.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

c901696b26aa347532930dc5ab12ecb54e473722
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 20feb75..4cbbd2d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1149,7 +1149,8 @@ static struct inode_operations proc_pid_
 
 static int proc_readfd(struct file * filp, void * dirent, filldir_t filldir)
 {
-	struct inode *inode = filp->f_dentry->d_inode;
+	struct dentry *dentry = filp->f_dentry;
+	struct inode *inode = dentry->d_inode;
 	struct task_struct *p = proc_task(inode);
 	unsigned int fd, tid, ino;
 	int retval;
@@ -1170,7 +1171,7 @@ static int proc_readfd(struct file * fil
 				goto out;
 			filp->f_pos++;
 		case 1:
-			ino = fake_ino(tid, PROC_TID_INO);
+			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
 				goto out;
 			filp->f_pos++;
-- 
1.2.2.g709a

[PATCH 03/23] proc: Remove useless BKL in proc_pid_readlink.

[Eric W. Biederman]

We already call everything except do_proc_readlink
outside of the BKL in proc_pid_followlink, and there
appears to be nothing in do_proc_readlink that needs
any special protection.

So remove this leftover from one of the BKL cleanup
efforts.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/proc/base.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

da9fe7b5227340bea1f4bd1e246af4a921ce765a
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 4cbbd2d..24a3526 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1120,7 +1120,6 @@ static int proc_pid_readlink(struct dent
 	struct dentry *de;
 	struct vfsmount *mnt = NULL;
 
-	lock_kernel();
 
 	if (current->fsuid != inode->i_uid && !capable(CAP_DAC_OVERRIDE))
 		goto out;
@@ -1136,7 +1135,6 @@ static int proc_pid_readlink(struct dent
 	dput(de);
 	mntput(mnt);
 out:
-	unlock_kernel();
 	return error;
 }
 
-- 
1.2.2.g709a

[PATCH 04/23] proc: Remove unnecessary and misleading assignments from proc_pid_make_inode.

[Eric W. Biederman]

The removed fields are already set by proc_alloc_inode.  
Initializing them in proc_pid_make_inode implies they need
to be set.  At least ei->pde was not set on all paths making
it look like proc_pid_make_inode was buggy.  So just remove 
the redundant assignments.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/proc/base.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

2b0fa5317e60458090cfa528e9421ecd3de38f6b
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 24a3526..56ca519 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1310,7 +1310,6 @@ static struct inode *proc_pid_make_inode
 
 	/* Common stuff */
 	ei = PROC_I(inode);
-	ei->task = NULL;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->i_ino = fake_ino(task->pid, ino);
 
@@ -1335,7 +1334,6 @@ out:
 	return inode;
 
 out_unlock:
-	ei->pde = NULL;
 	iput(inode);
 	return NULL;
 }
-- 
1.2.2.g709a

[PATCH 05/23] proc: Simplify the ownership rules for /proc

[Eric W. Biederman]

Currently in /proc if the task is dumpable all of files are owned by
the tasks effective users.  Otherwise the files are owned by root.
Unless it is the /proc/<tgid>/ or /proc/<tgid>/task/<pid> directory
in that case we always make the directory owned by the effective user.

However the special case for directories is pointless except as a way
to read the effective user, because the permissions on both of those
directories are world readable, and executable.

/proc/<tgid>/status provides a much better way to read a processes effecitve
userid, so it is silly to try to provide that on the directory.

So this patch simplifies the code by removing a pointless special case and
gets us one step closer to being able to remove the hard coded /proc inode
numbers.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/proc/base.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

453d43f2b9e9fee71c23007f1cfe5dbedd9d3790
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 56ca519..c35f340 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1324,7 +1324,7 @@ static struct inode *proc_pid_make_inode
 	ei->type = ino;
 	inode->i_uid = 0;
 	inode->i_gid = 0;
-	if (ino == PROC_TGID_INO || ino == PROC_TID_INO || task_dumpable(task)) {
+	if (task_dumpable(task)) {
 		inode->i_uid = task->euid;
 		inode->i_gid = task->egid;
 	}
@@ -1353,7 +1353,7 @@ static int pid_revalidate(struct dentry 
 	struct inode *inode = dentry->d_inode;
 	struct task_struct *task = proc_task(inode);
 	if (pid_alive(task)) {
-		if (proc_type(inode) == PROC_TGID_INO || proc_type(inode) == PROC_TID_INO || task_dumpable(task)) {
+		if (task_dumpable(task)) {
 			inode->i_uid = task->euid;
 			inode->i_gid = task->egid;
 		} else {
-- 
1.2.2.g709a

[PATCH 05/23] proc: Simplify the ownership rules for /proc

[Eric W. Biederman]

Currently in /proc if the task is dumpable all of files are owned by
the tasks effective users.  Otherwise the files are owned by root.
Unless it is the /proc/<tgid>/ or /proc/<tgid>/task/<pid> directory
in that case we always make the directory owned by the effective user.

However the special case for directories is pointless except as a way
to read the effective user, because the permissions on both of those
directories are world readable, and executable.

/proc/<tgid>/status provides a much better way to read a processes effecitve
userid, so it is silly to try to provide that on the directory.

So this patch simplifies the code by removing a pointless special case and
gets us one step closer to being able to remove the hard coded /proc inode
numbers.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 fs/proc/base.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

453d43f2b9e9fee71c23007f1cfe5dbedd9d3790
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 56ca519..c35f340 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1324,7 +1324,7 @@ static struct inode *proc_pid_make_inode
 	ei->type = ino;
 	inode->i_uid = 0;
 	inode->i_gid = 0;
-	if (ino == PROC_TGID_INO || ino == PROC_TID_INO || task_dumpable(task)) {
+	if (task_dumpable(task)) {
 		inode->i_uid = task->euid;
 		inode->i_gid = task->egid;
 	}
@@ -1353,7 +1353,7 @@ static int pid_revalidate(struct dentry 
 	struct inode *inode = dentry->d_inode;
 	struct task_struct *task = proc_task(inode);
 	if (pid_alive(task)) {
-		if (proc_type(inode) == PROC_TGID_INO || proc_type(inode) == PROC_TID_INO || task_dumpable(task)) {
+		if (task_dumpable(task)) {
 			inode->i_uid = task->euid;
 			inode->i_gid = task->egid;
 		} else {
-- 
1.2.2.g709a