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 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