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

Groups > linux.kernel > #1461491 > unrolled thread

[PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps

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

  [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps robert.foss@collabora.com - 2016-08-13 00:10 +0200
    Re: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps Jann Horn <jann@thejh.net> - 2016-08-13 16:50 +0200
      Re: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps Robert Foss <robert.foss@collabora.com> - 2016-08-15 16:20 +0200
        Re: [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps Robert Foss <robert.foss@collabora.com> - 2016-08-15 22:20 +0200

#1461491 — [PACTH v2 1/3] mm, proc: Implement /proc/<pid>/totmaps

From: Robert Foss <robert.foss@collabora.com>

This is based on earlier work by Thiago Goncales. It implements a new
per process proc file which summarizes the contents of the smaps file
but doesn't display any addresses.  It gives more detailed information
than statm like the PSS (proprotional set size).  It differs from the
original implementation in that it doesn't use the full blown set of
seq operations, uses a different termination condition, and doesn't
displayed "Locked" as that was broken on the original implemenation.

This new proc file provides information faster than parsing the potentially
huge smaps file.

Tested-by: Robert Foss <robert.foss@collabora.com>
Signed-off-by: Robert Foss <robert.foss@collabora.com>

Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
---
 fs/proc/base.c     |   1 +
 fs/proc/internal.h |   3 ++
 fs/proc/task_mmu.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index a11eb71..de3acdf 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2855,6 +2855,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
 	REG("smaps",      S_IRUGO, proc_pid_smaps_operations),
 	REG("pagemap",    S_IRUSR, proc_pagemap_operations),
+	REG("totmaps",    S_IRUGO, proc_totmaps_operations),
 #endif
 #ifdef CONFIG_SECURITY
 	DIR("attr",       S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index aa27810..c55e1fe 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -281,6 +281,7 @@ struct proc_maps_private {
 	struct mm_struct *mm;
 #ifdef CONFIG_MMU
 	struct vm_area_struct *tail_vma;
+	struct mem_size_stats *mss;
 #endif
 #ifdef CONFIG_NUMA
 	struct mempolicy *task_mempolicy;
@@ -297,6 +298,8 @@ extern const struct file_operations proc_pid_smaps_operations;
 extern const struct file_operations proc_tid_smaps_operations;
 extern const struct file_operations proc_clear_refs_operations;
 extern const struct file_operations proc_pagemap_operations;
+extern const struct file_operations proc_totmaps_operations;
+
 
 extern unsigned long task_vsize(struct mm_struct *);
 extern unsigned long task_statm(struct mm_struct *,
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 4648c7f..b7612e9 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -246,6 +246,9 @@ static int proc_map_release(struct inode *inode, struct file *file)
 	struct seq_file *seq = file->private_data;
 	struct proc_maps_private *priv = seq->private;
 
+	if (!priv)
+		return 0;
+
 	if (priv->mm)
 		mmdrop(priv->mm);
 
@@ -802,6 +805,75 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 	return 0;
 }
 
+static void add_smaps_sum(struct mem_size_stats *mss,
+		struct mem_size_stats *mss_sum)
+{
+	mss_sum->resident += mss->resident;
+	mss_sum->pss += mss->pss;
+	mss_sum->shared_clean += mss->shared_clean;
+	mss_sum->shared_dirty += mss->shared_dirty;
+	mss_sum->private_clean += mss->private_clean;
+	mss_sum->private_dirty += mss->private_dirty;
+	mss_sum->referenced += mss->referenced;
+	mss_sum->anonymous += mss->anonymous;
+	mss_sum->anonymous_thp += mss->anonymous_thp;
+	mss_sum->swap += mss->swap;
+}
+
+static int totmaps_proc_show(struct seq_file *m, void *data)
+{
+	struct proc_maps_private *priv = m->private;
+	struct mm_struct *mm = priv->mm;
+	struct vm_area_struct *vma;
+	struct mem_size_stats mss_sum;
+
+	memset(&mss_sum, 0, sizeof(mss_sum));
+	down_read(&mm->mmap_sem);
+	hold_task_mempolicy(priv);
+
+	for (vma = mm->mmap; vma != priv->tail_vma; vma = vma->vm_next) {
+		struct mem_size_stats mss;
+		struct mm_walk smaps_walk = {
+			.pmd_entry = smaps_pte_range,
+			.mm = vma->vm_mm,
+			.private = &mss,
+		};
+
+		if (vma->vm_mm && !is_vm_hugetlb_page(vma)) {
+			memset(&mss, 0, sizeof(mss));
+			walk_page_vma(vma, &smaps_walk);
+			add_smaps_sum(&mss, &mss_sum);
+		}
+	}
+
+	seq_printf(m,
+		   "Rss:            %8lu kB\n"
+		   "Pss:            %8lu kB\n"
+		   "Shared_Clean:   %8lu kB\n"
+		   "Shared_Dirty:   %8lu kB\n"
+		   "Private_Clean:  %8lu kB\n"
+		   "Private_Dirty:  %8lu kB\n"
+		   "Referenced:     %8lu kB\n"
+		   "Anonymous:      %8lu kB\n"
+		   "AnonHugePages:  %8lu kB\n"
+		   "Swap:           %8lu kB\n",
+		   mss_sum.resident >> 10,
+		   (unsigned long)(mss_sum.pss >> (10 + PSS_SHIFT)),
+		   mss_sum.shared_clean  >> 10,
+		   mss_sum.shared_dirty  >> 10,
+		   mss_sum.private_clean >> 10,
+		   mss_sum.private_dirty >> 10,
+		   mss_sum.referenced >> 10,
+		   mss_sum.anonymous >> 10,
+		   mss_sum.anonymous_thp >> 10,
+		   mss_sum.swap >> 10);
+
+	release_task_mempolicy(priv);
+	up_read(&mm->mmap_sem);
+
+	return 0;
+}
+
 static int show_pid_smap(struct seq_file *m, void *v)
 {
 	return show_smap(m, v, 1);
@@ -812,6 +884,28 @@ static int show_tid_smap(struct seq_file *m, void *v)
 	return show_smap(m, v, 0);
 }
 
+static void *m_totmaps_start(struct seq_file *p, loff_t *pos)
+{
+	return NULL + (*pos == 0);
+}
+
+static void *m_totmaps_next(struct seq_file *p, void *v, loff_t *pos)
+{
+	++*pos;
+	return NULL;
+}
+
+static void m_totmaps_stop(struct seq_file *p, void *v)
+{
+}
+
+static const struct seq_operations proc_totmaps_op = {
+	.start	= m_totmaps_start,
+	.next	= m_totmaps_next,
+	.stop	= m_totmaps_stop,
+	.show	= totmaps_proc_show
+};
+
 static const struct seq_operations proc_pid_smaps_op = {
 	.start	= m_start,
 	.next	= m_next,
@@ -836,6 +930,39 @@ static int tid_smaps_open(struct inode *inode, struct file *file)
 	return do_maps_open(inode, file, &proc_tid_smaps_op);
 }
 
+static int totmaps_open(struct inode *inode, struct file *file)
+{
+	struct proc_maps_private *priv = NULL;
+	struct seq_file *seq;
+	int ret;
+
+	ret = do_maps_open(inode, file, &proc_totmaps_op);
+	if (ret)
+		goto error;
+
+	seq = file->private_data;
+	priv = seq->private;
+
+	/*
+	 * We need to grab references to the task_struct
+	 * at open time, because there's a potential information
+	 * leak where the totmaps file is opened and held open
+	 * while the underlying pid to task mapping changes
+	 * underneath it
+	 */
+	priv->task = get_proc_task(inode);
+	if (!priv->task) {
+		ret = -ESRCH;
+		goto error;
+	}
+
+	return 0;
+
+error:
+	proc_map_release(inode, file);
+	return ret;
+}
+
 const struct file_operations proc_pid_smaps_operations = {
 	.open		= pid_smaps_open,
 	.read		= seq_read,
@@ -850,6 +977,13 @@ const struct file_operations proc_tid_smaps_operations = {
 	.release	= proc_map_release,
 };
 
+const struct file_operations proc_totmaps_operations = {
+	.open		= totmaps_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= proc_map_release,
+};
+
 enum clear_refs_types {
 	CLEAR_REFS_ALL = 1,
 	CLEAR_REFS_ANON,
-- 
2.7.4

[toc] | [next] | [standalone]

#1461628

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 12, 2016 at 06:04:20PM -0400, robert.foss@collabora.com wrote:
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index aa27810..c55e1fe 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -281,6 +281,7 @@ struct proc_maps_private {
>  	struct mm_struct *mm;
>  #ifdef CONFIG_MMU
>  	struct vm_area_struct *tail_vma;
> +	struct mem_size_stats *mss;

This is unused now, right?


> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 4648c7f..b7612e9 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -246,6 +246,9 @@ static int proc_map_release(struct inode *inode, struct file *file)
>  	struct seq_file *seq = file->private_data;
>  	struct proc_maps_private *priv = seq->private;
>  
> +	if (!priv)
> +		return 0;
> +

You might want to get rid of this, see below.


> +static int totmaps_open(struct inode *inode, struct file *file)
> +{
> +	struct proc_maps_private *priv = NULL;
> +	struct seq_file *seq;
> +	int ret;
> +
> +	ret = do_maps_open(inode, file, &proc_totmaps_op);
> +	if (ret)
> +		goto error;
[...]
> +error:
> +	proc_map_release(inode, file);
> +	return ret;

I don't think this is correct. Have a look at the other callers of
do_maps_open() - none of them do any cleanup steps on error, they
just return. I think the "goto error" here should be a return
instead.

Have a look at the error cases that can cause do_maps_open() to
fail: do_maps_open() just calls proc_maps_open(). If the
__seq_open_private() call fails because of memory pressure,
file->private_data is still NULL, and your newly added NULL check
in proc_map_release() causes proc_map_release() to be a no-op
there. But if proc_maps_open() fails later on, things get nasty:
If, for example, proc_mem_open() fails because of a ptrace
permission denial, __seq_open_file -> seq_open has already set
file->private_data to a struct seq_file *, and then
proc_maps_open(), prior to passing on the error code, calls
seq_release_private -> seq_release, which frees that
struct seq_file * without NULLing the private_data pointer.
As far as I can tell, proc_map_release() would then run into
a use-after-free scenario.


> +	priv->task = get_proc_task(inode);
> +	if (!priv->task) {
> +		ret = -ESRCH;
> +		goto error;
> +	}

You're not actually using ->task anywhere in the current version,
right? Can this be deleted?


> +const struct file_operations proc_totmaps_operations = {
[...]
> +	.release	= proc_map_release,

This won't release priv->task, causing a memory leak (exploitable
through a reference counter overflow of the task_struct usage
counter).

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

#1462813

On 2016-08-13 10:39 AM, Jann Horn wrote:
> On Fri, Aug 12, 2016 at 06:04:20PM -0400, robert.foss@collabora.com wrote:
>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>> index aa27810..c55e1fe 100644
>> --- a/fs/proc/internal.h
>> +++ b/fs/proc/internal.h
>> @@ -281,6 +281,7 @@ struct proc_maps_private {
>>  	struct mm_struct *mm;
>>  #ifdef CONFIG_MMU
>>  	struct vm_area_struct *tail_vma;
>> +	struct mem_size_stats *mss;
>
> This is unused now, right?

Fixing it in v3.

>
>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index 4648c7f..b7612e9 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -246,6 +246,9 @@ static int proc_map_release(struct inode *inode, struct file *file)
>>  	struct seq_file *seq = file->private_data;
>>  	struct proc_maps_private *priv = seq->private;
>>
>> +	if (!priv)
>> +		return 0;
>> +
>
> You might want to get rid of this, see below.

Fixing it in v3.

>
>
>> +static int totmaps_open(struct inode *inode, struct file *file)
>> +{
>> +	struct proc_maps_private *priv = NULL;
>> +	struct seq_file *seq;
>> +	int ret;
>> +
>> +	ret = do_maps_open(inode, file, &proc_totmaps_op);
>> +	if (ret)
>> +		goto error;
> [...]
>> +error:
>> +	proc_map_release(inode, file);
>> +	return ret;
>
> I don't think this is correct. Have a look at the other callers of
> do_maps_open() - none of them do any cleanup steps on error, they
> just return. I think the "goto error" here should be a return
> instead.
>
> Have a look at the error cases that can cause do_maps_open() to
> fail: do_maps_open() just calls proc_maps_open(). If the
> __seq_open_private() call fails because of memory pressure,
> file->private_data is still NULL, and your newly added NULL check
> in proc_map_release() causes proc_map_release() to be a no-op
> there. But if proc_maps_open() fails later on, things get nasty:
> If, for example, proc_mem_open() fails because of a ptrace
> permission denial, __seq_open_file -> seq_open has already set
> file->private_data to a struct seq_file *, and then
> proc_maps_open(), prior to passing on the error code, calls
> seq_release_private -> seq_release, which frees that
> struct seq_file * without NULLing the private_data pointer.
> As far as I can tell, proc_map_release() would then run into
> a use-after-free scenario.
>
>
>> +	priv->task = get_proc_task(inode);
>> +	if (!priv->task) {
>> +		ret = -ESRCH;
>> +		goto error;
>> +	}
>
> You're not actually using ->task anywhere in the current version,
> right? Can this be deleted?
>
>
>> +const struct file_operations proc_totmaps_operations = {
> [...]
>> +	.release	= proc_map_release,
>
> This won't release priv->task, causing a memory leak (exploitable
> through a reference counter overflow of the task_struct usage
> counter).
>

Thanks for the thorough walkthrough, it is much appreciated.

priv->task does not appear to be used any more, and can be removed.
When "priv->task = get_proc_task(inode)" is removed, totmaps_open()
starts to look just like the other XXX_open functions.

I'll send out v3 as soon as testing has been done.

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

#1463152

On 2016-08-15 09:57 AM, Robert Foss wrote:
>
>
> On 2016-08-13 10:39 AM, Jann Horn wrote:
>> On Fri, Aug 12, 2016 at 06:04:20PM -0400, robert.foss@collabora.com
>> wrote:
>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>> index aa27810..c55e1fe 100644
>>> --- a/fs/proc/internal.h
>>> +++ b/fs/proc/internal.h
>>> @@ -281,6 +281,7 @@ struct proc_maps_private {
>>>      struct mm_struct *mm;
>>>  #ifdef CONFIG_MMU
>>>      struct vm_area_struct *tail_vma;
>>> +    struct mem_size_stats *mss;
>>
>> This is unused now, right?
>
> Fixing it in v3.
>
>>
>>
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index 4648c7f..b7612e9 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -246,6 +246,9 @@ static int proc_map_release(struct inode *inode,
>>> struct file *file)
>>>      struct seq_file *seq = file->private_data;
>>>      struct proc_maps_private *priv = seq->private;
>>>
>>> +    if (!priv)
>>> +        return 0;
>>> +
>>
>> You might want to get rid of this, see below.
>
> Fixing it in v3.
>
>>
>>
>>> +static int totmaps_open(struct inode *inode, struct file *file)
>>> +{
>>> +    struct proc_maps_private *priv = NULL;
>>> +    struct seq_file *seq;
>>> +    int ret;
>>> +
>>> +    ret = do_maps_open(inode, file, &proc_totmaps_op);
>>> +    if (ret)
>>> +        goto error;
>> [...]
>>> +error:
>>> +    proc_map_release(inode, file);
>>> +    return ret;
>>
>> I don't think this is correct. Have a look at the other callers of
>> do_maps_open() - none of them do any cleanup steps on error, they
>> just return. I think the "goto error" here should be a return
>> instead.
>>
>> Have a look at the error cases that can cause do_maps_open() to
>> fail: do_maps_open() just calls proc_maps_open(). If the
>> __seq_open_private() call fails because of memory pressure,
>> file->private_data is still NULL, and your newly added NULL check
>> in proc_map_release() causes proc_map_release() to be a no-op
>> there. But if proc_maps_open() fails later on, things get nasty:
>> If, for example, proc_mem_open() fails because of a ptrace
>> permission denial, __seq_open_file -> seq_open has already set
>> file->private_data to a struct seq_file *, and then
>> proc_maps_open(), prior to passing on the error code, calls
>> seq_release_private -> seq_release, which frees that
>> struct seq_file * without NULLing the private_data pointer.
>> As far as I can tell, proc_map_release() would then run into
>> a use-after-free scenario.
>>
>>
>>> +    priv->task = get_proc_task(inode);
>>> +    if (!priv->task) {
>>> +        ret = -ESRCH;
>>> +        goto error;
>>> +    }
>>
>> You're not actually using ->task anywhere in the current version,
>> right? Can this be deleted?

Actually, priv->task is used by hold_task_mempolicy() in 
totmaps_proc_show(), which as far as I understand it is needed due to 
the "vma = mm->mmap" looping we do.


>>
>>
>>> +const struct file_operations proc_totmaps_operations = {
>> [...]
>>> +    .release    = proc_map_release,
>>
>> This won't release priv->task, causing a memory leak (exploitable
>> through a reference counter overflow of the task_struct usage
>> counter).
>>
>
> Thanks for the thorough walkthrough, it is much appreciated.
>
> priv->task does not appear to be used any more, and can be removed.
> When "priv->task = get_proc_task(inode)" is removed, totmaps_open()
> starts to look just like the other XXX_open functions.
>
> I'll send out v3 as soon as testing has been done.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web