Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1160953 > unrolled thread

[PATCH v6] procfs: Always expose /proc/<pid>/map_files/ and make it readable

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6] procfs: Always expose /proc/<pid>/map_files/ and make it readable Calvin Owens <calvinowens@fb.com> - 2015-06-09 05:50 +0200
    Re: [PATCH v6] procfs: Always expose /proc/<pid>/map_files/ and make  it readable Calvin Owens <calvinowens@fb.com> - 2015-06-10 03:50 +0200
      Re: [PATCH v6] procfs: Always expose /proc/<pid>/map_files/ and make  it readable Alexey Dobriyan <adobriyan@gmail.com> - 2015-06-11 13:20 +0200

#1160953 — [PATCH v6] procfs: Always expose /proc/<pid>/map_files/ and make it readable

Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and
is only exposed if CONFIG_CHECKPOINT_RESTORE is set.

This interface very useful because it allows userspace to stat()
deleted files that are still mapped by some process, which enables a
much quicker and more accurate answer to the question "How much disk
space is being consumed by files that are deleted but still mapped?"
than is currently possible.

This patch moves map_files/ out from behind CONFIG_CHECKPOINT_RESTORE,
and adjusts the permissions enforced on it as follows:

* proc_map_files_lookup()
* proc_map_files_readdir()
* map_files_d_revalidate()

	Remove the CAP_SYS_ADMIN restriction, leaving only the current
	restriction requiring PTRACE_MODE_READ.

	In earlier versions of this patch, I changed the ptrace checks
	in the functions above to enforce MODE_ATTACH instead of
	MODE_READ. That was an oversight: all the information exposed
	by the above three functions is already available with
	MODE_READ from /proc/PID/maps. I was only being asked to
	strengthen the protection around functionality provided by
	follow_link(), not the above.

	So, I've left the checks for MODE_READ as-is, since AFAICS all
	objections raised so far are addressed by the new CAP_SYS_ADMIN
	check in follow_link(), explained below.

* proc_map_files_follow_link()

	This stub has been added, and requires that the user have
	CAP_SYS_ADMIN in order to follow the links in map_files/,
	since there was concern on LKML both about the potential for
	bypassing permissions on ancestor directories in the path to
	files pointed to, and about what happens with more exotic
	memory mappings created by some drivers (ie dma-buf).

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Joe Perches <joe@perches.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Calvin Owens <calvinowens@fb.com>
---
Changes in v6:	Require CAP_SYS_ADMIN for follow_link(). Leave other
		PTRACE_MODE_READ checks as-is, since CAP_SYS_ADMIN
		alone addresses all concerns raised AFAICS.

Changes in v5:	s/dentry->d_inode/d_inode(dentry)/g

Changes in v4:	Return -ESRCH from follow_link() when get_proc_task()
		returns NULL.

Changes in v3:	Changed permission checks to use PTRACE_MODE_ATTACH
		instead of PTRACE_MODE_READ, and added a stub to
		enforce MODE_ATTACH on follow_link() as well.

Changes in v2:	Removed the follow_link() stub that returned -EPERM if
		the caller didn't have CAP_SYS_ADMIN, since the caller
		in my chroot() scenario gets -EACCES anyway.

 fs/proc/base.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 093ca14..0270191 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1641,8 +1641,6 @@ end_instantiate:
 	return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-
 /*
  * dname_to_vma_addr - maps a dentry name into two unsigned longs
  * which represent vma start and end addresses.
@@ -1669,11 +1667,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
 
-	if (!capable(CAP_SYS_ADMIN)) {
-		status = -EPERM;
-		goto out_notask;
-	}
-
 	inode = d_inode(dentry);
 	task = get_proc_task(inode);
 	if (!task)
@@ -1762,6 +1755,28 @@ struct map_files_info {
 	unsigned char	name[4*sizeof(long)+2]; /* max: %lx-%lx\0 */
 };
 
+/*
+ * Only allow CAP_SYS_ADMIN to follow the links, due to concerns about how the
+ * symlinks may be used to bypass permissions on ancestor directories in the
+ * path to the file in question.
+ */
+static void *proc_map_files_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+	if (!capable(CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
+	return proc_pid_follow_link(dentry, nd);
+}
+
+/*
+ * Identical to proc_pid_link_inode_operations except for follow_link()
+ */
+static const struct inode_operations proc_map_files_link_inode_operations = {
+	.readlink	= proc_pid_readlink,
+	.follow_link	= proc_map_files_follow_link,
+	.setattr	= proc_setattr,
+};
+
 static int
 proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
 			   struct task_struct *task, const void *ptr)
@@ -1777,7 +1792,7 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
 	ei = PROC_I(inode);
 	ei->op.proc_get_link = proc_map_files_get_link;
 
-	inode->i_op = &proc_pid_link_inode_operations;
+	inode->i_op = &proc_map_files_link_inode_operations;
 	inode->i_size = 64;
 	inode->i_mode = S_IFLNK;
 
@@ -1801,10 +1816,6 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 	int result;
 	struct mm_struct *mm;
 
-	result = -EPERM;
-	if (!capable(CAP_SYS_ADMIN))
-		goto out;
-
 	result = -ENOENT;
 	task = get_proc_task(dir);
 	if (!task)
@@ -1858,10 +1869,6 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
 	struct map_files_info *p;
 	int ret;
 
-	ret = -EPERM;
-	if (!capable(CAP_SYS_ADMIN))
-		goto out;
-
 	ret = -ENOENT;
 	task = get_proc_task(file_inode(file));
 	if (!task)
@@ -2050,7 +2057,6 @@ static const struct file_operations proc_timers_operations = {
 	.llseek		= seq_lseek,
 	.release	= seq_release_private,
 };
-#endif /* CONFIG_CHECKPOINT_RESTORE */
 
 static int proc_pident_instantiate(struct inode *dir,
 	struct dentry *dentry, struct task_struct *task, const void *ptr)
@@ -2549,9 +2555,7 @@ static const struct inode_operations proc_task_inode_operations;
 static const struct pid_entry tgid_base_stuff[] = {
 	DIR("task",       S_IRUGO|S_IXUGO, proc_task_inode_operations, proc_task_operations),
 	DIR("fd",         S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
-#ifdef CONFIG_CHECKPOINT_RESTORE
 	DIR("map_files",  S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations),
-#endif
 	DIR("fdinfo",     S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
 	DIR("ns",	  S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
 #ifdef CONFIG_NET
-- 
1.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1161899 — Re: [PATCH v6] procfs: Always expose /proc/<pid>/map_files/ and make it readable

On Tuesday 06/09 at 14:13 -0700, Andrew Morton wrote:
> On Mon, 8 Jun 2015 20:39:33 -0700 Calvin Owens <calvinowens@fb.com> wrote:
> 
> > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and
> > is only exposed if CONFIG_CHECKPOINT_RESTORE is set.
> > 
> > This interface very useful because it allows userspace to stat()
> > deleted files that are still mapped by some process, which enables a
> > much quicker and more accurate answer to the question "How much disk
> > space is being consumed by files that are deleted but still mapped?"
> > than is currently possible.
> 
> Why is that information useful?
> 
> I could perhaps think of some use for "How much disk space is being
> consumed by files that are deleted but still open", but to count the
> mmapped-then-unlinked files while excluding the opened-then-unlinked
> files seems damned peculiar.

Let's phrase the question a bit more generically:

"How much disk space is being consumed by files that have been
unlinked, but are still referenced by some process?"

There are two pieces to this problem:
	1) Unlinked files that are still open (whether mapped or not)
	2) Unlinked files that are not open, but are still mapped

You can track down everything in (1) using /proc/<pid>/fd/*, and you
can use stat() to figure out how much space they're using.

But directly measuring how much space (2) consumes is actually not
currently possible from userspace: there's no way to stat() the files.
You can get the inode number from /proc/<pid>/maps, but that still
doesn't get you anywhere because it's been unlinked from the
filesystem.

So I'm not looking to measure (2) and exclude (1): I'm looking to have
a way to directly measure (2) at all.

The reason I say "directly", and I say "quicker and more accurate" in
the original message, is that there is a very ugly way to answer this
question right now: you sum up the number of blocks used by every file
on the disk and subtract it from what statfs() tells you. This
obviously stinks, and becomes untenable once your filesystem is large
enough.
 
> IOW, this changelog failed to explain the value of the patch.  Bad
> changelog!  Please sell it to us.  Preferably with real-world use
> cases.

The real-world use case is catching long-lived processes that leak
references to temporary files and waste space on the disk. When such
processes leak file-backed mappings, this wasted space is especially
difficult to detect until it gets out of hand. The map_files/
interface eliminates this difficulty.

I've included a little test program at the end of this file to illustrate
what I'm getting at here. It creates a file at /tmp/DELETEDFILE:

	calvinowens@Haydn:~$ gcc test.c 
	calvinowens@Haydn:~$ ./a.out &
	[1] 5832
	Holding mapping at 0x7fe74d1ea000
	calvinowens@Haydn:~$ lsof -p `pgrep a.out`
	COMMAND  PID        USER   FD   TYPE DEVICE SIZE/OFF    NODE NAME
	a.out   5832 calvinowens  cwd    DIR  254,1     4096 3413033 /home/calvinowens
	a.out   5832 calvinowens  rtd    DIR  254,1     4096       2 /
	a.out   5832 calvinowens  txt    REG  254,1     7512 3408268 /home/calvinowens/a.out
	a.out   5832 calvinowens  mem    REG  254,1  1729984 4456767 /lib/x86_64-linux-gnu/libc-2.19.so
	a.out   5832 calvinowens  mem    REG  254,1   140928 4456619 /lib/x86_64-linux-gnu/ld-2.19.so
	a.out   5832 calvinowens  mem    REG   0,32    32768  184946 /tmp/DELETEDFILE
	a.out   5832 calvinowens    0u   CHR  136,2      0t0       5 /dev/pts/2
	a.out   5832 calvinowens    1u   CHR  136,2      0t0       5 /dev/pts/2
	a.out   5832 calvinowens    2u   CHR  136,2      0t0       5 /dev/pts/2
	calvinowens@Haydn:~$ killall a.out
	[1]+  Terminated              ./a.out
	calvinowens@Haydn:~$ gcc -DDO_UNLINK test.c 
	calvinowens@Haydn:~$ ./a.out &
	[1] 5842
	Holding mapping at 0x7fec8ae63000
	calvinowens@Haydn:~$ lsof -p `pgrep a.out`
	COMMAND  PID        USER   FD   TYPE DEVICE SIZE/OFF    NODE NAME
	a.out   5842 calvinowens  cwd    DIR  254,1     4096 3413033 /home/calvinowens
	a.out   5842 calvinowens  rtd    DIR  254,1     4096       2 /
	a.out   5842 calvinowens  txt    REG  254,1     7640 3408268 /home/calvinowens/a.out
	a.out   5842 calvinowens  mem    REG  254,1  1729984 4456767 /lib/x86_64-linux-gnu/libc-2.19.so
	a.out   5842 calvinowens  mem    REG  254,1   140928 4456619 /lib/x86_64-linux-gnu/ld-2.19.so
	a.out   5842 calvinowens  DEL    REG   0,32           184946 /tmp/DELETEDFILE
	a.out   5842 calvinowens    0u   CHR  136,2      0t0       5 /dev/pts/2
	a.out   5842 calvinowens    1u   CHR  136,2      0t0       5 /dev/pts/2
	a.out   5842 calvinowens    2u   CHR  136,2      0t0       5 /dev/pts/2

Notice the gap under "SIZE/OFF" in the 2nd output? This is because lsof
has no possible way to actually determine the leaked file's size.
That's the functionality "hole" I'm trying to fill with this patch.

Does that all seem sensible?

Thanks,
Calvin

--
#include <stdlib.h>
#include <stdio.h>
#include <time.h>
#include <limits.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>

int main(void)
{
	int ret, fd;
	void *map;

	fd = open("/tmp/DELETEDFILE", O_CREAT|O_TRUNC|O_RDWR, 0777);
	if (fd == -1)
		return -1;

	ret = ftruncate(fd, 32768);
	if (ret == -1)
		return -1;

	map = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_POPULATE,
			fd, 0);
	if (map == MAP_FAILED)
		return -1;

	close(fd);
	#ifdef DO_UNLINK
	unlink("/tmp/DELETEDFILE");
	#endif

	printf("Holding mapping at %p\n", map);
	while (1)
		sleep(UINT_MAX);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1163156 — Re: [PATCH v6] procfs: Always expose /proc/<pid>/map_files/ and make it readable

On Wed, Jun 10, 2015 at 11:58 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 9 Jun 2015 18:39:02 -0700 Calvin Owens <calvinowens@fb.com> wrote:
>
>> On Tuesday 06/09 at 14:13 -0700, Andrew Morton wrote:
>> > On Mon, 8 Jun 2015 20:39:33 -0700 Calvin Owens <calvinowens@fb.com> wrote:
>> >
>> > > Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and
>> > > is only exposed if CONFIG_CHECKPOINT_RESTORE is set.
>> > >
>> > > This interface very useful because it allows userspace to stat()
>> > > deleted files that are still mapped by some process, which enables a
>> > > much quicker and more accurate answer to the question "How much disk
>> > > space is being consumed by files that are deleted but still mapped?"
>> > > than is currently possible.
>> >
>> > Why is that information useful?
>> >
>> > I could perhaps think of some use for "How much disk space is being
>> > consumed by files that are deleted but still open", but to count the
>> > mmapped-then-unlinked files while excluding the opened-then-unlinked
>> > files seems damned peculiar.
>>
>> Let's phrase the question a bit more generically:
>>
>> "How much disk space is being consumed by files that have been
>> unlinked, but are still referenced by some process?"
>>
>> There are two pieces to this problem:
>>       1) Unlinked files that are still open (whether mapped or not)
>>       2) Unlinked files that are not open, but are still mapped
>>
>> You can track down everything in (1) using /proc/<pid>/fd/*, and you
>> can use stat() to figure out how much space they're using.
>
> This doesn't work if the mapped file has been unlinked?  What does the
> /proc/pid/map_files listing look like for these?

It says "(deleted)" like /proc/*/exe or any other symlink.

>> Does that all seem sensible?
>
> Spose so.  Please capture all this info in the changelog.
>
>
> It all seems a bit awkward though.  If we want to know "how much disk
> space is this process using" (or similar) then I wonder what a syscall
> (or prctl mode?) which does this would look like.

I believe something like this is needed for checkpointing,
otherwise mmaped but unlinked files could not be restored fully
(how do you reach them?).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web