2011-03-01 16:25:01

by Aaro Koskinen

[permalink] [raw]
Subject: [PATCH] procfs: fix /proc/<pid>/maps heap check

The current check looks wrong and prints "[heap]" only if the mapping
matches exactly the heap. However, the heap may be merged with some
other mappings, and there may be also be multiple mappings.

Signed-off-by: Aaro Koskinen <[email protected]>
Cc: [email protected]
---
fs/proc/task_mmu.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 60b9148..f269ee6 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -249,8 +249,8 @@ static void show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
const char *name = arch_vma_name(vma);
if (!name) {
if (mm) {
- if (vma->vm_start <= mm->start_brk &&
- vma->vm_end >= mm->brk) {
+ if (vma->vm_start <= mm->brk &&
+ vma->vm_end >= mm->start_brk) {
name = "[heap]";
} else if (vma->vm_start <= mm->start_stack &&
vma->vm_end >= mm->start_stack) {
--
1.5.6.5


2011-03-02 12:59:13

by Aaro Koskinen

[permalink] [raw]
Subject: Re: [PATCH] procfs: fix /proc/<pid>/maps heap check

Hi,

On Tue, 1 Mar 2011, Aaro Koskinen wrote:
> The current check looks wrong and prints "[heap]" only if the mapping
> matches exactly the heap. However, the heap may be merged with some
> other mappings, and there may be also be multiple mappings.
>
> Signed-off-by: Aaro Koskinen <[email protected]>
> Cc: [email protected]

Below is a test program and an example output showing the problem,
and the correct output with the patch:

Without the patch:

# ./a.out &
# cat /proc/$!/maps | head -4
00008000-00009000 r-xp 00000000 01:00 9224 /a.out
00010000-00011000 rw-p 00000000 01:00 9224 /a.out
00011000-00012000 rw-p 00000000 00:00 0
00012000-00013000 rw-p 00000000 00:00 0

With the patch:

# ./a.out &
# cat /proc/$!/maps | head -4
00008000-00009000 r-xp 00000000 01:00 9228 /a.out
00010000-00011000 rw-p 00000000 01:00 9228 /a.out
00011000-00012000 rw-p 00000000 00:00 0 [heap]
00012000-00013000 rw-p 00000000 00:00 0 [heap]

The test program:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/mman.h>

int main (void)
{
if (sbrk(4096) == (void *)-1) {
perror("first sbrk(): ");
return EXIT_FAILURE;
}

if (mlockall(MCL_FUTURE)) {
perror("mlockall(): ");
return EXIT_FAILURE;
}

if (sbrk(4096) == (void *)-1) {
perror("second sbrk(): ");
return EXIT_FAILURE;
}

while (1)
sleep(1);
}

2011-03-03 01:30:24

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH] procfs: fix /proc/<pid>/maps heap check

> Hi,
>
> On Tue, 1 Mar 2011, Aaro Koskinen wrote:
> > The current check looks wrong and prints "[heap]" only if the mapping
> > matches exactly the heap. However, the heap may be merged with some
> > other mappings, and there may be also be multiple mappings.
> >
> > Signed-off-by: Aaro Koskinen <[email protected]>
> > Cc: [email protected]
>
> Below is a test program and an example output showing the problem,
> and the correct output with the patch:
>
> Without the patch:
>
> # ./a.out &
> # cat /proc/$!/maps | head -4
> 00008000-00009000 r-xp 00000000 01:00 9224 /a.out
> 00010000-00011000 rw-p 00000000 01:00 9224 /a.out
> 00011000-00012000 rw-p 00000000 00:00 0
> 00012000-00013000 rw-p 00000000 00:00 0
>
> With the patch:
>
> # ./a.out &
> # cat /proc/$!/maps | head -4
> 00008000-00009000 r-xp 00000000 01:00 9228 /a.out
> 00010000-00011000 rw-p 00000000 01:00 9228 /a.out
> 00011000-00012000 rw-p 00000000 00:00 0 [heap]
> 00012000-00013000 rw-p 00000000 00:00 0 [heap]
>
> The test program:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/mman.h>
>
> int main (void)
> {
> if (sbrk(4096) == (void *)-1) {
> perror("first sbrk(): ");
> return EXIT_FAILURE;
> }
>
> if (mlockall(MCL_FUTURE)) {
> perror("mlockall(): ");
> return EXIT_FAILURE;
> }
>
> if (sbrk(4096) == (void *)-1) {
> perror("second sbrk(): ");
> return EXIT_FAILURE;
> }

Your description said,
the heap may be merged with some other mappings,
^^^^^^
but your example is splitting case. not merge. In other words, your
patch care splitting case but break merge case.

Ok, we have no obvious correct behavior. This is debatable. So,
Why do you think vma splitting case is important than merge?

2011-03-03 11:41:07

by Aaro Koskinen

[permalink] [raw]
Subject: Re: [PATCH] procfs: fix /proc/<pid>/maps heap check

Hi,

On Thu, 3 Mar 2011, KOSAKI Motohiro wrote:
>> On Tue, 1 Mar 2011, Aaro Koskinen wrote:
>>> The current check looks wrong and prints "[heap]" only if the mapping
>>> matches exactly the heap. However, the heap may be merged with some
>>> other mappings, and there may be also be multiple mappings.
>>>
>>> Signed-off-by: Aaro Koskinen <[email protected]>
>>> Cc: [email protected]

[...]

> Your description said,
> the heap may be merged with some other mappings,
> ^^^^^^
> but your example is splitting case. not merge. In other words, your
> patch care splitting case but break merge case.
>
> Ok, we have no obvious correct behavior. This is debatable. So,
> Why do you think vma splitting case is important than merge?

Sorry, I was unclear.

The current behaviour is wrong for both merged and split cases, and I
think the patch fixes both.

And yes, the example program is for the split case. I'll see if I can
make a test program for the merged case...

A.

2011-03-03 12:24:51

by Aaro Koskinen

[permalink] [raw]
Subject: Re: [PATCH] procfs: fix /proc/<pid>/maps heap check

Hi,

On Thu, 3 Mar 2011, Aaro Koskinen wrote:
> On Thu, 3 Mar 2011, KOSAKI Motohiro wrote:
>>> On Tue, 1 Mar 2011, Aaro Koskinen wrote:
>>>> The current check looks wrong and prints "[heap]" only if the mapping
>>>> matches exactly the heap. However, the heap may be merged with some
>>>> other mappings, and there may be also be multiple mappings.
>>>>
>>>> Signed-off-by: Aaro Koskinen <[email protected]>
>>>> Cc: [email protected]
>
> [...]
>
>> Your description said,
>> the heap may be merged with some other mappings,
>> ^^^^^^
>> but your example is splitting case. not merge. In other words, your
>> patch care splitting case but break merge case.
>>
>> Ok, we have no obvious correct behavior. This is debatable. So,
>> Why do you think vma splitting case is important than merge?
>
> Sorry, I was unclear.
>
> The current behaviour is wrong for both merged and split cases, and I
> think the patch fixes both.

Argh, this is confusing. The current check:

vma->vm_start <= mm->start_brk && vma->vm_end >= mm->brk

obviously works with the merged case. The patch changes this to:

vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk

This works with the split case, but it does not break the merged case
(or do I miss something still?).

So the current behaviour is broken only with the splitting case. I will
correct the patch description and resend it.

A.