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

[ebiederm@xmission.com (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