[PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Cyrill Gorcunov]

It helps to dump and restore this mm_struct members.

v2:
 - Kees and Alexey pointed out that "1" hack is unnecessary,
   so make it to have plain (mm && permitted) ? mm->member : 0
   form.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
---
 fs/proc/array.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -464,7 +464,7 @@ static int do_task_stat(struct seq_file
 
 	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
-%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
+%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
 		pid_nr_ns(pid, ns),
 		tcomm,
 		state,
@@ -511,7 +511,10 @@ static int do_task_stat(struct seq_file
 		task->policy,
 		(unsigned long long)delayacct_blkio_ticks(task),
 		cputime_to_clock_t(gtime),
-		cputime_to_clock_t(cgtime));
+		cputime_to_clock_t(cgtime),
+		(mm && permitted) ? mm->start_data : 0,
+		(mm && permitted) ? mm->end_data : 0,
+		(mm && permitted) ? mm->start_brk : 0);
 	if (mm)
 		mmput(mm);
 	return 0;

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Andrew Morton]

On Mon, 5 Dec 2011 22:20:11 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> It helps to dump and restore this mm_struct members.

Sorry, I'm not going to apply a patch with a changelog like that.

It needs a full description of how this will be used and why it is
needed.  So that others can understand and evaluate the proposal, and
any possible alternatives.

Such a description would also let us see whether this is just some
"hey, maybe we'll use this sometime" thing, in which case it should not
yet be applied.

>  fs/proc/array.c |    7 +++++--

Also, every change to any procfs interface should have an associated
change to procfs documentation.  If no relevant document exists then
heck, write one.  No exceptions.

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Cyrill Gorcunov]

On Tue, Dec 06, 2011 at 02:08:33PM -0800, Andrew Morton wrote:
> On Mon, 5 Dec 2011 22:20:11 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > It helps to dump and restore this mm_struct members.
> 
> Sorry, I'm not going to apply a patch with a changelog like that.
>

OK, I'll update it. The problem is that while we "read" this entries
from procfs, the restoration is supposed to be done via prctl interface
(the patch is in fly, and I'm trying to improve it at moment).

> It needs a full description of how this will be used and why it is
> needed.  So that others can understand and evaluate the proposal, and
> any possible alternatives.
> 
> Such a description would also let us see whether this is just some
> "hey, maybe we'll use this sometime" thing, in which case it should not
> yet be applied.

I understand.

> 
> >  fs/proc/array.c |    7 +++++--
> 
> Also, every change to any procfs interface should have an associated
> change to procfs documentation.  If no relevant document exists then
> heck, write one.  No exceptions.
>

ok, thanks.

	Cyrill

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Cyrill Gorcunov]

On Wed, Dec 07, 2011 at 02:15:15AM +0400, Cyrill Gorcunov wrote:
> On Tue, Dec 06, 2011 at 02:08:33PM -0800, Andrew Morton wrote:
> > On Mon, 5 Dec 2011 22:20:11 +0400
> > Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > 
> > > It helps to dump and restore this mm_struct members.
> > 
> > Sorry, I'm not going to apply a patch with a changelog like that.
> >
> 
> OK, I'll update it. The problem is that while we "read" this entries
> from procfs, the restoration is supposed to be done via prctl interface
> (the patch is in fly, and I'm trying to improve it at moment).
> 
> > It needs a full description of how this will be used and why it is
> > needed.  So that others can understand and evaluate the proposal, and
> > any possible alternatives.
> > 
> > Such a description would also let us see whether this is just some
> > "hey, maybe we'll use this sometime" thing, in which case it should not
> > yet be applied.
> 
> I understand.
> 
> > 
> > >  fs/proc/array.c |    7 +++++--
> > 
> > Also, every change to any procfs interface should have an associated
> > change to procfs documentation.  If no relevant document exists then
> > heck, write one.  No exceptions.
> >
> 
> ok, thanks.
> 
> 	Cyrill

What about such changelog? Does it look better?

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v3

These members are involved into calculation of program
data segment size (which might be seen in /proc/<pid>/statm)
and brk() call, so in a sake of restore we need
these entries to be set to exactly same values
a process had at dumpimg time.

While setting these entries back to a running task
is supposed to be implemented via prctl interface (which is
not addressed in this patch) the export of these values
are done via /proc/$pid/stat, which btw already has
start_code, end_code and start_stack entries.

v2:
 - Kees and Alexey pointed out that "1" hack is unnecessary,
   so make it to have plain (mm && permitted) ? mm->member : 0
   form.

v3:
 - Andrew pointed that documentation update is missed.
   Chagelog is updated as well.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
---
 Documentation/filesystems/proc.txt |    3 +++
 fs/proc/array.c                    |    7 +++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -305,6 +305,9 @@ Table 1-4: Contents of the stat files (a
   blkio_ticks   time spent waiting for block IO
   gtime         guest time of the task in jiffies
   cgtime        guest time of the task children in jiffies
+  start_data    address above which program data is placed
+  end_data      address above which program data is placed
+  start_brk     address above which program data can be expanded with brk() call
 ..............................................................................
 
 The /proc/PID/maps file containing the currently mapped memory regions and
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -464,7 +464,7 @@ static int do_task_stat(struct seq_file
 
 	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
-%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
+%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
 		pid_nr_ns(pid, ns),
 		tcomm,
 		state,
@@ -511,7 +511,10 @@ static int do_task_stat(struct seq_file
 		task->policy,
 		(unsigned long long)delayacct_blkio_ticks(task),
 		cputime_to_clock_t(gtime),
-		cputime_to_clock_t(cgtime));
+		cputime_to_clock_t(cgtime),
+		(mm && permitted) ? mm->start_data : 0,
+		(mm && permitted) ? mm->end_data : 0,
+		(mm && permitted) ? mm->start_brk : 0);
 	if (mm)
 		mmput(mm);
 	return 0;

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Kees Cook]

On Tue, Dec 6, 2011 at 2:53 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> +  start_data    address above which program data is placed
> +  end_data      address above which program data is placed

Should the latter be "address below which program data is placed"?

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[KAMEZAWA Hiroyuki]

On Wed, 7 Dec 2011 02:53:15 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:
 
> Index: linux-2.6.git/Documentation/filesystems/proc.txt
> ===================================================================
> --- linux-2.6.git.orig/Documentation/filesystems/proc.txt
> +++ linux-2.6.git/Documentation/filesystems/proc.txt
> @@ -305,6 +305,9 @@ Table 1-4: Contents of the stat files (a
>    blkio_ticks   time spent waiting for block IO
>    gtime         guest time of the task in jiffies
>    cgtime        guest time of the task children in jiffies
> +  start_data    address above which program data is placed
> +  end_data      address above which program data is placed
> +  start_brk     address above which program data can be expanded with brk() call
>  ..............................................................................
> 
nitpick.

After reading Wikipedia, http://en.wikipedia.org/wiki/Data_segment

I prefer

start_data   address above program data+bss is placed.
end_data     address below program data+bss is placed
start_break  address above which program heap can be expaned with brk() call

rather than just 'data'. 

Thanks,
-Kame

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Cyrill Gorcunov]

On Tue, Dec 06, 2011 at 03:22:03PM -0800, Kees Cook wrote:
> On Tue, Dec 6, 2011 at 2:53 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > +  start_data    address above which program data is placed
> > +  end_data      address above which program data is placed
> 
> Should the latter be "address below which program data is placed"?
> 

Yeah, copy-paste typo, thanks!

	Cyrill

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Cyrill Gorcunov]

On Wed, Dec 07, 2011 at 09:02:59AM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 7 Dec 2011 02:53:15 +0400
> Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>  
> > Index: linux-2.6.git/Documentation/filesystems/proc.txt
> > ===================================================================
> > --- linux-2.6.git.orig/Documentation/filesystems/proc.txt
> > +++ linux-2.6.git/Documentation/filesystems/proc.txt
> > @@ -305,6 +305,9 @@ Table 1-4: Contents of the stat files (a
> >    blkio_ticks   time spent waiting for block IO
> >    gtime         guest time of the task in jiffies
> >    cgtime        guest time of the task children in jiffies
> > +  start_data    address above which program data is placed
> > +  end_data      address above which program data is placed
> > +  start_brk     address above which program data can be expanded with brk() call
> >  ..............................................................................
> > 
> nitpick.
> 
> After reading Wikipedia, http://en.wikipedia.org/wiki/Data_segment
> 
> I prefer
> 
> start_data   address above program data+bss is placed.
> end_data     address below program data+bss is placed
> start_break  address above which program heap can be expaned with brk() call
> 
> rather than just 'data'. 
> 

No problem, I'll update (These segments are filled by binfmt handlers).

	Cyrill

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Cyrill Gorcunov]

On Wed, Dec 07, 2011 at 10:33:08AM +0400, Cyrill Gorcunov wrote:
> 
> No problem, I'll update (These segments are filled by binfmt handlers).
> 

This one should fit.

	Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v4

These members are involved into calculation of program
data segment (which might be seen in /proc/<pid>/statm)
and into brk() call, so in a sake of restore we need
these entries to be set exactly to the same value the
process had at dumpimg time.

While setting these entries back to a running task
is supposed to be done via prctl interface (which is
not addressed in this patch) the export of these values
are done via /proc/$pid/stat, which already have start_code,
end_code and start_stack entries.

v2:
 - Kees and Alexey pointed out that "1" hack is unnecessary,
   so make it to have plain (mm && permitted) ? mm->member : 0
   form.

v3:
 - Andrew pointed that documentation update is missed.
   Chagelog is updated as well.

v4:
 - Update proc.txt to address concerns from Kees and Kame.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
---
 Documentation/filesystems/proc.txt |    3 +++
 fs/proc/array.c                    |    7 +++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -305,6 +305,9 @@ Table 1-4: Contents of the stat files (a
   blkio_ticks   time spent waiting for block IO
   gtime         guest time of the task in jiffies
   cgtime        guest time of the task children in jiffies
+  start_data    address above which program data+bss is placed
+  end_data      address below which program data+bss is placed
+  start_brk     address above which program heap can be expaned with brk() call
 ..............................................................................
 
 The /proc/PID/maps file containing the currently mapped memory regions and
Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -464,7 +464,7 @@ static int do_task_stat(struct seq_file
 
 	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
 %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
-%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
+%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
 		pid_nr_ns(pid, ns),
 		tcomm,
 		state,
@@ -511,7 +511,10 @@ static int do_task_stat(struct seq_file
 		task->policy,
 		(unsigned long long)delayacct_blkio_ticks(task),
 		cputime_to_clock_t(gtime),
-		cputime_to_clock_t(cgtime));
+		cputime_to_clock_t(cgtime),
+		(mm && permitted) ? mm->start_data : 0,
+		(mm && permitted) ? mm->end_data : 0,
+		(mm && permitted) ? mm->start_brk : 0);
 	if (mm)
 		mmput(mm);
 	return 0;

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Eric W. Biederman]

Cyrill Gorcunov <gorcunov@gmail.com> writes:

> On Wed, Dec 07, 2011 at 10:33:08AM +0400, Cyrill Gorcunov wrote:
>> 
>> No problem, I'll update (These segments are filled by binfmt handlers).
>> 
>
> This one should fit.

How extensively have you tested this.  I remember a few other occasions
where adding elements to a line in a proc file have resulted in breaking
readers.

I expect it is much safer to add this to /proc/<pid>/status rather than
/proc/<pid>/stat.

Eric


>
> 	Cyrill
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v4
>
> These members are involved into calculation of program
> data segment (which might be seen in /proc/<pid>/statm)
> and into brk() call, so in a sake of restore we need
> these entries to be set exactly to the same value the
> process had at dumpimg time.
>
> While setting these entries back to a running task
> is supposed to be done via prctl interface (which is
> not addressed in this patch) the export of these values
> are done via /proc/$pid/stat, which already have start_code,
> end_code and start_stack entries.
>
> v2:
>  - Kees and Alexey pointed out that "1" hack is unnecessary,
>    so make it to have plain (mm && permitted) ? mm->member : 0
>    form.
>
> v3:
>  - Andrew pointed that documentation update is missed.
>    Chagelog is updated as well.
>
> v4:
>  - Update proc.txt to address concerns from Kees and Kame.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>  Documentation/filesystems/proc.txt |    3 +++
>  fs/proc/array.c                    |    7 +++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> Index: linux-2.6.git/Documentation/filesystems/proc.txt
> ===================================================================
> --- linux-2.6.git.orig/Documentation/filesystems/proc.txt
> +++ linux-2.6.git/Documentation/filesystems/proc.txt
> @@ -305,6 +305,9 @@ Table 1-4: Contents of the stat files (a
>    blkio_ticks   time spent waiting for block IO
>    gtime         guest time of the task in jiffies
>    cgtime        guest time of the task children in jiffies
> +  start_data    address above which program data+bss is placed
> +  end_data      address below which program data+bss is placed
> +  start_brk     address above which program heap can be expaned with brk() call
>  ..............................................................................
>  
>  The /proc/PID/maps file containing the currently mapped memory regions and
> Index: linux-2.6.git/fs/proc/array.c
> ===================================================================
> --- linux-2.6.git.orig/fs/proc/array.c
> +++ linux-2.6.git/fs/proc/array.c
> @@ -464,7 +464,7 @@ static int do_task_stat(struct seq_file
>  
>  	seq_printf(m, "%d (%s) %c %d %d %d %d %d %u %lu \
>  %lu %lu %lu %lu %lu %ld %ld %ld %ld %d 0 %llu %lu %ld %lu %lu %lu %lu %lu \
> -%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld\n",
> +%lu %lu %lu %lu %lu %lu %lu %lu %d %d %u %u %llu %lu %ld %lu %lu %lu\n",
>  		pid_nr_ns(pid, ns),
>  		tcomm,
>  		state,
> @@ -511,7 +511,10 @@ static int do_task_stat(struct seq_file
>  		task->policy,
>  		(unsigned long long)delayacct_blkio_ticks(task),
>  		cputime_to_clock_t(gtime),
> -		cputime_to_clock_t(cgtime));
> +		cputime_to_clock_t(cgtime),
> +		(mm && permitted) ? mm->start_data : 0,
> +		(mm && permitted) ? mm->end_data : 0,
> +		(mm && permitted) ? mm->start_brk : 0);
>  	if (mm)
>  		mmput(mm);
>  	return 0;
> --
> 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/

Re: [PATCH] fs, proc: Add start_data, end_data, start_brk members to /proc/$pid/stat v2

[Cyrill Gorcunov]

On Sun, Dec 11, 2011 at 11:30:21PM -0800, Eric W. Biederman wrote:
> Cyrill Gorcunov <gorcunov@gmail.com> writes:
> 
> > On Wed, Dec 07, 2011 at 10:33:08AM +0400, Cyrill Gorcunov wrote:
> >> 
> >> No problem, I'll update (These segments are filled by binfmt handlers).
> >> 
> >
> > This one should fit.
> 
> How extensively have you tested this.  I remember a few other occasions
> where adding elements to a line in a proc file have resulted in breaking
> readers.
> 

Hmm. Tested it pretty heavy on centos 5 series. I'll dig some more.

> I expect it is much safer to add this to /proc/<pid>/status rather than
> /proc/<pid>/stat.
> 

Eric, I'm fine with any file. I thought about /.../stat only because it
already has start_code/end_code, so to have all pieces in one place this
file was choosen.

	Cyrill