Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

James Pearson wrote:
> Arvin Moezzi wrote:
> 
>> I think that's not true. 'count' is changing through the iteration.
>> The difference in the mem_read():
>>
>> * while (count > 0) {
>> * int this_len, retval;
>> *
>> *  this_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>> *  retval = access_process_vm(task, src, page, this_len, 0);
>> *
>> *  ...
>> * }
>>
>> is the fact, that this_len = min(PAGE_SIZE, count) is in the
>> iteration block, hence retval <= this_len <= count in each iteration
>> step. So this is ok. But IMHO in your code 'retval' may be bigger than
>> 'count' in the last iteration of the block, because 'max_len' is fix
>> through your iteration but 'count' is changing. Or am i missing
>> something?
> 
> 
> Yes, you are correct ...

Here is a new patch that fixes the above issue ...

However, I'm not sure if I should be using GFP_TEMPORARY, GFP_KERNEL or
GFP_USER ?

Thanks

James Pearson


Patch against 2.6.23-rc6-mm1

---

/proc/PID/environ currently truncates at 4096 characters, patch based on 
the /proc/PID/mem code.

Signed-off-by: James Pearson <[EMAIL PROTECTED]>

--- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c2007-09-25 12:40:53.194497911 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
 (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
 security_ptrace(current,task) == 0))
 
-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
-   int res = 0;
-   struct mm_struct *mm = get_task_mm(task);
-   if (mm) {
-   unsigned int len;
-
-   res = -ESRCH;
-   if (!ptrace_may_attach(task))
-   goto out;
-
-   len  = mm->env_end - mm->env_start;
-   if (len > PAGE_SIZE)
-   len = PAGE_SIZE;
-   res = access_process_vm(task, mm->env_start, buffer, len, 0);
-out:
-   mmput(mm);
-   }
-   return res;
-}
-
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 {
int res = 0;
@@ -740,6 +719,76 @@ static const struct file_operations proc
.open   = mem_open,
 };
 
+static ssize_t environ_read(struct file *file, char __user *buf,
+   size_t count, loff_t *ppos)
+{
+   struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
+   char *page;
+   unsigned long src = *ppos;
+   int ret = -ESRCH;
+   struct mm_struct *mm;
+
+   if (!task)
+   goto out_no_task;
+
+   if (!ptrace_may_attach(task))
+   goto out;
+
+   ret = -ENOMEM;
+   page = (char *)__get_free_page(GFP_TEMPORARY);
+   if (!page)
+   goto out;
+
+   ret = 0;
+
+   mm = get_task_mm(task);
+   if (!mm)
+   goto out_free;
+
+   while (count > 0) {
+   int this_len, retval, max_len;
+
+   this_len = mm->env_end - (mm->env_start + src);
+
+   if (this_len <= 0)
+   break;
+
+   max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
+   this_len = (this_len > max_len) ? max_len : this_len;
+
+   retval = access_process_vm(task, (mm->env_start + src),
+   page, this_len, 0);
+
+   if (retval <= 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {
+   ret = -EFAULT;
+   break;
+   }
+
+   ret += retval;
+   src += retval;
+   buf += retval;
+   count -= retval;
+   }
+   *ppos = src;
+
+   mmput(mm);
+out_free:
+   free_page((unsigned long) page);
+out:
+   put_task_struct(task);
+out_no_task:
+   return ret;
+}
+
+static const struct file_operations proc_environ_operations = {
+   .read   = environ_read,
+};
+
 static ssize_t oom_adjust_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
 {
@@ -2092,7 +2141,7 @@ static const struct pid_entry tgid_base_
DIR("task",   S_IRUGO|S_IXUGO, task),
DIR("fd", S_IRUSR|S_IXUSR, fd),
DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
-   INF("environ",S_IRUSR, pid_environ),
+   REG("environ",S_IRUSR, environ),
INF("auxv",   S_IRUSR, pid_auxv),
INF("status", S_IRUGO, pid_status),
INF("limits", S_IRUSR, pid_limits),
@@ -2421,7 +2470,7 @@ out_no_task:
 static const struct pid_entry tid_base_stuff[] = {
DIR("fd",S_IRUSR|S_IXUSR, fd),
DIR("fdinfo",S_IRUSR|S_IXUSR, fdinfo),
-   INF("environ",   S_IRUSR, pid_environ),
+   REG("environ",   S_IRUSR, environ),
INF("auxv",  S_IRUSR, pid_auxv),
INF("status",S_IRUGO, pid_status),
INF("limits",

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

James Pearson wrote:
 Arvin Moezzi wrote:
 
 I think that's not true. 'count' is changing through the iteration.
 The difference in the mem_read():

 * while (count  0) {
 * int this_len, retval;
 *
 *  this_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
 *  retval = access_process_vm(task, src, page, this_len, 0);
 *
 *  ...
 * }

 is the fact, that this_len = min(PAGE_SIZE, count) is in the
 iteration block, hence retval = this_len = count in each iteration
 step. So this is ok. But IMHO in your code 'retval' may be bigger than
 'count' in the last iteration of the block, because 'max_len' is fix
 through your iteration but 'count' is changing. Or am i missing
 something?
 
 
 Yes, you are correct ...

Here is a new patch that fixes the above issue ...

However, I'm not sure if I should be using GFP_TEMPORARY, GFP_KERNEL or
GFP_USER ?

Thanks

James Pearson


Patch against 2.6.23-rc6-mm1

---

/proc/PID/environ currently truncates at 4096 characters, patch based on 
the /proc/PID/mem code.

Signed-off-by: James Pearson [EMAIL PROTECTED]

--- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c2007-09-25 12:40:53.194497911 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
 (task-state == TASK_STOPPED || task-state == TASK_TRACED)  \
 security_ptrace(current,task) == 0))
 
-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
-   int res = 0;
-   struct mm_struct *mm = get_task_mm(task);
-   if (mm) {
-   unsigned int len;
-
-   res = -ESRCH;
-   if (!ptrace_may_attach(task))
-   goto out;
-
-   len  = mm-env_end - mm-env_start;
-   if (len  PAGE_SIZE)
-   len = PAGE_SIZE;
-   res = access_process_vm(task, mm-env_start, buffer, len, 0);
-out:
-   mmput(mm);
-   }
-   return res;
-}
-
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 {
int res = 0;
@@ -740,6 +719,76 @@ static const struct file_operations proc
.open   = mem_open,
 };
 
+static ssize_t environ_read(struct file *file, char __user *buf,
+   size_t count, loff_t *ppos)
+{
+   struct task_struct *task = get_proc_task(file-f_dentry-d_inode);
+   char *page;
+   unsigned long src = *ppos;
+   int ret = -ESRCH;
+   struct mm_struct *mm;
+
+   if (!task)
+   goto out_no_task;
+
+   if (!ptrace_may_attach(task))
+   goto out;
+
+   ret = -ENOMEM;
+   page = (char *)__get_free_page(GFP_TEMPORARY);
+   if (!page)
+   goto out;
+
+   ret = 0;
+
+   mm = get_task_mm(task);
+   if (!mm)
+   goto out_free;
+
+   while (count  0) {
+   int this_len, retval, max_len;
+
+   this_len = mm-env_end - (mm-env_start + src);
+
+   if (this_len = 0)
+   break;
+
+   max_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
+   this_len = (this_len  max_len) ? max_len : this_len;
+
+   retval = access_process_vm(task, (mm-env_start + src),
+   page, this_len, 0);
+
+   if (retval = 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {
+   ret = -EFAULT;
+   break;
+   }
+
+   ret += retval;
+   src += retval;
+   buf += retval;
+   count -= retval;
+   }
+   *ppos = src;
+
+   mmput(mm);
+out_free:
+   free_page((unsigned long) page);
+out:
+   put_task_struct(task);
+out_no_task:
+   return ret;
+}
+
+static const struct file_operations proc_environ_operations = {
+   .read   = environ_read,
+};
+
 static ssize_t oom_adjust_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
 {
@@ -2092,7 +2141,7 @@ static const struct pid_entry tgid_base_
DIR(task,   S_IRUGO|S_IXUGO, task),
DIR(fd, S_IRUSR|S_IXUSR, fd),
DIR(fdinfo, S_IRUSR|S_IXUSR, fdinfo),
-   INF(environ,S_IRUSR, pid_environ),
+   REG(environ,S_IRUSR, environ),
INF(auxv,   S_IRUSR, pid_auxv),
INF(status, S_IRUGO, pid_status),
INF(limits, S_IRUSR, pid_limits),
@@ -2421,7 +2470,7 @@ out_no_task:
 static const struct pid_entry tid_base_stuff[] = {
DIR(fd,S_IRUSR|S_IXUSR, fd),
DIR(fdinfo,S_IRUSR|S_IXUSR, fdinfo),
-   INF(environ,   S_IRUSR, pid_environ),
+   REG(environ,   S_IRUSR, environ),
INF(auxv,  S_IRUSR, pid_auxv),
INF(status,S_IRUGO, pid_status),
INF(limits,S_IRUSR, pid_limits),
-
To unsubscribe from this list: send the line unsubscribe linux-kernel 

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Arvin Moezzi wrote:


I think that's not true. 'count' is changing through the iteration.
The difference in the mem_read():

* while (count > 0) {
* int this_len, retval;
*
*  this_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
*  retval = access_process_vm(task, src, page, this_len, 0);
*
*  ...
* }

is the fact, that this_len = min(PAGE_SIZE, count) is in the
iteration block, hence retval <= this_len <= count in each iteration
step. So this is ok. But IMHO in your code 'retval' may be bigger than
'count' in the last iteration of the block, because 'max_len' is fix
through your iteration but 'count' is changing. Or am i missing
something?


Yes, you are correct ...

Thanks

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Arvin Moezzi]

> >>+
> >>+   if (copy_to_user(buf, page, retval)) {
> >
> > 
> > shouldn't you only copy min(count,retval) bytes? otherwise you could
> > write beyond the users buffer "buf", right?
>
> AFAIK, 'retval' can never be greater than 'this_len', which can never be
> greater than 'max_len', which can never be greater than 'count'

I think that's not true. 'count' is changing through the iteration.
The difference in the mem_read():

* while (count > 0) {
* int this_len, retval;
*
*  this_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
*  retval = access_process_vm(task, src, page, this_len, 0);
*
*  ...
* }

is the fact, that this_len = min(PAGE_SIZE, count) is in the
iteration block, hence retval <= this_len <= count in each iteration
step. So this is ok. But IMHO in your code 'retval' may be bigger than
'count' in the last iteration of the block, because 'max_len' is fix
through your iteration but 'count' is changing. Or am i missing
something?

> James Pearson

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mel Gorman]

On (20/09/07 16:46), Andrew Morton didst pronounce:
> On Wed, 19 Sep 2007 14:35:29 +0100
> "James Pearson" <[EMAIL PROTECTED]> wrote:
> 
> > 
> > From: James Pearson <[EMAIL PROTECTED]>
> > 
> > /proc/PID/environ currently truncates at 4096 characters, patch based on 
> > the /proc/PID/mem code.
> 
> patch needs to be carefully reviewed from the security POV (ie: permissions)
> as well as for correctness.  Does anyone have time to do that?
> 
> > Signed-off-by: James Pearson <[EMAIL PROTECTED]>
> > 
> > --- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
> > +++ ./fs/proc/base.c2007-09-19 12:36:18.155648760 +0100
> > @@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
> >  (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
> >  security_ptrace(current,task) == 0))
> >  
> > -static int proc_pid_environ(struct task_struct *task, char * buffer)
> > -{
> > -   int res = 0;
> > -   struct mm_struct *mm = get_task_mm(task);
> > -   if (mm) {
> > -   unsigned int len;
> > -
> > -   res = -ESRCH;
> > -   if (!ptrace_may_attach(task))
> > -   goto out;
> > -
> > -   len  = mm->env_end - mm->env_start;
> > -   if (len > PAGE_SIZE)
> > -   len = PAGE_SIZE;
> > -   res = access_process_vm(task, mm->env_start, buffer, len, 0);
> > -out:
> > -   mmput(mm);
> > -   }
> > -   return res;
> > -}
> > -
> >  static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> >  {
> > int res = 0;
> > @@ -740,6 +719,79 @@ static const struct file_operations proc
> > .open   = mem_open,
> >  };
> >  
> > +static ssize_t environ_read(struct file *file, char __user *buf,
> > +   size_t count, loff_t *ppos)
> > +{
> > +   struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
> > +   char *page;
> > +   unsigned long src = *ppos;
> > +   int ret = -ESRCH;
> > +   struct mm_struct *mm;
> > +   size_t max_len;
> > +
> > +   if (!task)
> > +   goto out_no_task;
> > +
> > +   if (!ptrace_may_attach(task))
> > +   goto out;
> > +
> > +   ret = -ENOMEM;
> > +   page = (char *)__get_free_page(GFP_TEMPORARY);
> 
> Now I wonder what inspired you to reach for GFP_TEMPORARY?  Perhaps the
> fact that it is crappily named and undocumented.
> 

Because it's a very short lived allocation? He allocates it here and
frees it at the end of the function again.

> This should be GFP_KERNEL - the page you're allocating here is not
> reclaimable by the VM.
> 

The patch leader documented how this works although you're right in that the
code doesn't explain it adequately. Temporary and short-lived allocations
are grouped with kernel-reclaimable pages such as inode and dcache pages
intentionally.

> > +   if (!page)
> > +   goto out;
> > +
> > +   ret = 0;
> > +
> > +   mm = get_task_mm(task);
> > +   if (!mm)
> > +   goto out_free;
> > +
> > +   max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> > +
> > +   while (count > 0) {
> > +   int this_len, retval;
> > +
> > +   this_len = mm->env_end - (mm->env_start + src);
> > +
> > +   if (this_len <= 0)
> > +   break;
> > +
> > +   if (this_len > max_len)
> > +   this_len = max_len;
> > +
> > +   retval = access_process_vm(task, (mm->env_start + src),
> > +   page, this_len, 0);
> > +
> > +   if (retval <= 0) {
> > +   ret = retval;
> > +   break;
> > +   }
> > +
> > +   if (copy_to_user(buf, page, retval)) {
> > +   ret = -EFAULT;
> > +   break;
> > +   }
> > +
> > +   ret += retval;
> > +   src += retval;
> > +   buf += retval;
> > +   count -= retval;
> > +   }
> 
> Now that's a funky loop.  Someone please convince me that there is no way
> in which `count - retval' can ever go negative (ie: huge positive).
> 
> 
> > +   *ppos = src;
> > +
> > +   mmput(mm);
> > +out_free:
> > +   free_page((unsigned long) page);
> > +out:
> > +   put_task_struct(task);
> > +out_no_task:
> > +   return ret;
> > +}
> > +
> > +static const struct file_operations proc_environ_operations = {
> > +   .read   = environ_read,
> > +};
> > +
> >  static ssize_t oom_adjust_read(struct file *file, char __user *buf,
> > size_t count, loff_t *ppos)
> >  {
> > @@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
> > DIR("task",   S_IRUGO|S_IXUGO, task),
> > DIR("fd", S_IRUSR|S_IXUSR, fd),
> > DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
> > -   INF("environ",S_IRUSR, pid_environ),
> > +   REG("environ",S_IRUSR, environ),
> > INF("auxv",   S_IRUSR, pid_auxv),
> > INF("status", S_IRUGO, pid_status),
> > INF("limits", S_IRUSR, pid_limits),
> > @@ -2421,7 +2473,7 @@ out_no_task:
> >  static const struct 

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Arvin Moezzi wrote:

2007/9/19, James Pearson <[EMAIL PROTECTED]>:


+   while (count > 0) {
+   int this_len, retval;
+
+   this_len = mm->env_end - (mm->env_start + src);
+
+   if (this_len <= 0)
+   break;
+
+   if (this_len > max_len)
+   this_len = max_len;
+
+   retval = access_process_vm(task, (mm->env_start + src),
+   page, this_len, 0);
+
+   if (retval <= 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {



shouldn't you only copy min(count,retval) bytes? otherwise you could
write beyond the users buffer "buf", right?


AFAIK, 'retval' can never be greater than 'this_len', which can never be 
greater than 'max_len', which can never be greater than 'count'


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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Andrew Morton wrote:

On Wed, 19 Sep 2007 14:35:29 +0100
"James Pearson" <[EMAIL PROTECTED]> wrote:



From: James Pearson <[EMAIL PROTECTED]>

/proc/PID/environ currently truncates at 4096 characters, patch based on 
the /proc/PID/mem code.



patch needs to be carefully reviewed from the security POV (ie: permissions)
as well as for correctness.  Does anyone have time to do that?



Signed-off-by: James Pearson <[EMAIL PROTECTED]>

--- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c2007-09-19 12:36:18.155648760 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
 (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
 security_ptrace(current,task) == 0))

-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
-   int res = 0;
-   struct mm_struct *mm = get_task_mm(task);
-   if (mm) {
-   unsigned int len;
-
-   res = -ESRCH;
-   if (!ptrace_may_attach(task))
-   goto out;
-
-   len  = mm->env_end - mm->env_start;
-   if (len > PAGE_SIZE)
-   len = PAGE_SIZE;
-   res = access_process_vm(task, mm->env_start, buffer, len, 0);
-out:
-   mmput(mm);
-   }
-   return res;
-}
-
static int proc_pid_cmdline(struct task_struct *task, char * buffer)
{
int res = 0;
@@ -740,6 +719,79 @@ static const struct file_operations proc
.open   = mem_open,
};

+static ssize_t environ_read(struct file *file, char __user *buf,
+   size_t count, loff_t *ppos)
+{
+   struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
+   char *page;
+   unsigned long src = *ppos;
+   int ret = -ESRCH;
+   struct mm_struct *mm;
+   size_t max_len;
+
+   if (!task)
+   goto out_no_task;
+
+   if (!ptrace_may_attach(task))
+   goto out;
+
+   ret = -ENOMEM;
+   page = (char *)__get_free_page(GFP_TEMPORARY);



Now I wonder what inspired you to reach for GFP_TEMPORARY?  Perhaps the
fact that it is crappily named and undocumented.

This should be GFP_KERNEL - the page you're allocating here is not
reclaimable by the VM.


The code is based on mem_read() - and that is what mem_read() does in 
2.6.23rc6-mm1 - my previous patch for 2.6.23rc5 used GFP_USER, as that 
is what mem_read() does in 2.6.23rc5.



+   if (!page)
+   goto out;
+
+   ret = 0;
+
+   mm = get_task_mm(task);
+   if (!mm)
+   goto out_free;
+
+   max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
+
+   while (count > 0) {
+   int this_len, retval;
+
+   this_len = mm->env_end - (mm->env_start + src);
+
+   if (this_len <= 0)
+   break;
+
+   if (this_len > max_len)
+   this_len = max_len;
+
+   retval = access_process_vm(task, (mm->env_start + src),
+   page, this_len, 0);
+
+   if (retval <= 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {
+   ret = -EFAULT;
+   break;
+   }
+
+   ret += retval;
+   src += retval;
+   buf += retval;
+   count -= retval;
+   }



Now that's a funky loop.  Someone please convince me that there is no way
in which `count - retval' can ever go negative (ie: huge positive).


Again, this is exactly the same as in mem_read()

James Pearson

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Arvin Moezzi wrote:

2007/9/19, James Pearson [EMAIL PROTECTED]:


+   while (count  0) {
+   int this_len, retval;
+
+   this_len = mm-env_end - (mm-env_start + src);
+
+   if (this_len = 0)
+   break;
+
+   if (this_len  max_len)
+   this_len = max_len;
+
+   retval = access_process_vm(task, (mm-env_start + src),
+   page, this_len, 0);
+
+   if (retval = 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {



shouldn't you only copy min(count,retval) bytes? otherwise you could
write beyond the users buffer buf, right?


AFAIK, 'retval' can never be greater than 'this_len', which can never be 
greater than 'max_len', which can never be greater than 'count'


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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Andrew Morton wrote:

On Wed, 19 Sep 2007 14:35:29 +0100
James Pearson [EMAIL PROTECTED] wrote:



From: James Pearson [EMAIL PROTECTED]

/proc/PID/environ currently truncates at 4096 characters, patch based on 
the /proc/PID/mem code.



patch needs to be carefully reviewed from the security POV (ie: permissions)
as well as for correctness.  Does anyone have time to do that?



Signed-off-by: James Pearson [EMAIL PROTECTED]

--- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c2007-09-19 12:36:18.155648760 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
 (task-state == TASK_STOPPED || task-state == TASK_TRACED)  \
 security_ptrace(current,task) == 0))

-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
-   int res = 0;
-   struct mm_struct *mm = get_task_mm(task);
-   if (mm) {
-   unsigned int len;
-
-   res = -ESRCH;
-   if (!ptrace_may_attach(task))
-   goto out;
-
-   len  = mm-env_end - mm-env_start;
-   if (len  PAGE_SIZE)
-   len = PAGE_SIZE;
-   res = access_process_vm(task, mm-env_start, buffer, len, 0);
-out:
-   mmput(mm);
-   }
-   return res;
-}
-
static int proc_pid_cmdline(struct task_struct *task, char * buffer)
{
int res = 0;
@@ -740,6 +719,79 @@ static const struct file_operations proc
.open   = mem_open,
};

+static ssize_t environ_read(struct file *file, char __user *buf,
+   size_t count, loff_t *ppos)
+{
+   struct task_struct *task = get_proc_task(file-f_dentry-d_inode);
+   char *page;
+   unsigned long src = *ppos;
+   int ret = -ESRCH;
+   struct mm_struct *mm;
+   size_t max_len;
+
+   if (!task)
+   goto out_no_task;
+
+   if (!ptrace_may_attach(task))
+   goto out;
+
+   ret = -ENOMEM;
+   page = (char *)__get_free_page(GFP_TEMPORARY);



Now I wonder what inspired you to reach for GFP_TEMPORARY?  Perhaps the
fact that it is crappily named and undocumented.

This should be GFP_KERNEL - the page you're allocating here is not
reclaimable by the VM.


The code is based on mem_read() - and that is what mem_read() does in 
2.6.23rc6-mm1 - my previous patch for 2.6.23rc5 used GFP_USER, as that 
is what mem_read() does in 2.6.23rc5.



+   if (!page)
+   goto out;
+
+   ret = 0;
+
+   mm = get_task_mm(task);
+   if (!mm)
+   goto out_free;
+
+   max_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
+
+   while (count  0) {
+   int this_len, retval;
+
+   this_len = mm-env_end - (mm-env_start + src);
+
+   if (this_len = 0)
+   break;
+
+   if (this_len  max_len)
+   this_len = max_len;
+
+   retval = access_process_vm(task, (mm-env_start + src),
+   page, this_len, 0);
+
+   if (retval = 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {
+   ret = -EFAULT;
+   break;
+   }
+
+   ret += retval;
+   src += retval;
+   buf += retval;
+   count -= retval;
+   }



Now that's a funky loop.  Someone please convince me that there is no way
in which `count - retval' can ever go negative (ie: huge positive).


Again, this is exactly the same as in mem_read()

James Pearson

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mel Gorman]

On (20/09/07 16:46), Andrew Morton didst pronounce:
 On Wed, 19 Sep 2007 14:35:29 +0100
 James Pearson [EMAIL PROTECTED] wrote:
 
  
  From: James Pearson [EMAIL PROTECTED]
  
  /proc/PID/environ currently truncates at 4096 characters, patch based on 
  the /proc/PID/mem code.
 
 patch needs to be carefully reviewed from the security POV (ie: permissions)
 as well as for correctness.  Does anyone have time to do that?
 
  Signed-off-by: James Pearson [EMAIL PROTECTED]
  
  --- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
  +++ ./fs/proc/base.c2007-09-19 12:36:18.155648760 +0100
  @@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
   (task-state == TASK_STOPPED || task-state == TASK_TRACED)  \
   security_ptrace(current,task) == 0))
   
  -static int proc_pid_environ(struct task_struct *task, char * buffer)
  -{
  -   int res = 0;
  -   struct mm_struct *mm = get_task_mm(task);
  -   if (mm) {
  -   unsigned int len;
  -
  -   res = -ESRCH;
  -   if (!ptrace_may_attach(task))
  -   goto out;
  -
  -   len  = mm-env_end - mm-env_start;
  -   if (len  PAGE_SIZE)
  -   len = PAGE_SIZE;
  -   res = access_process_vm(task, mm-env_start, buffer, len, 0);
  -out:
  -   mmput(mm);
  -   }
  -   return res;
  -}
  -
   static int proc_pid_cmdline(struct task_struct *task, char * buffer)
   {
  int res = 0;
  @@ -740,6 +719,79 @@ static const struct file_operations proc
  .open   = mem_open,
   };
   
  +static ssize_t environ_read(struct file *file, char __user *buf,
  +   size_t count, loff_t *ppos)
  +{
  +   struct task_struct *task = get_proc_task(file-f_dentry-d_inode);
  +   char *page;
  +   unsigned long src = *ppos;
  +   int ret = -ESRCH;
  +   struct mm_struct *mm;
  +   size_t max_len;
  +
  +   if (!task)
  +   goto out_no_task;
  +
  +   if (!ptrace_may_attach(task))
  +   goto out;
  +
  +   ret = -ENOMEM;
  +   page = (char *)__get_free_page(GFP_TEMPORARY);
 
 Now I wonder what inspired you to reach for GFP_TEMPORARY?  Perhaps the
 fact that it is crappily named and undocumented.
 

Because it's a very short lived allocation? He allocates it here and
frees it at the end of the function again.

 This should be GFP_KERNEL - the page you're allocating here is not
 reclaimable by the VM.
 

The patch leader documented how this works although you're right in that the
code doesn't explain it adequately. Temporary and short-lived allocations
are grouped with kernel-reclaimable pages such as inode and dcache pages
intentionally.

  +   if (!page)
  +   goto out;
  +
  +   ret = 0;
  +
  +   mm = get_task_mm(task);
  +   if (!mm)
  +   goto out_free;
  +
  +   max_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
  +
  +   while (count  0) {
  +   int this_len, retval;
  +
  +   this_len = mm-env_end - (mm-env_start + src);
  +
  +   if (this_len = 0)
  +   break;
  +
  +   if (this_len  max_len)
  +   this_len = max_len;
  +
  +   retval = access_process_vm(task, (mm-env_start + src),
  +   page, this_len, 0);
  +
  +   if (retval = 0) {
  +   ret = retval;
  +   break;
  +   }
  +
  +   if (copy_to_user(buf, page, retval)) {
  +   ret = -EFAULT;
  +   break;
  +   }
  +
  +   ret += retval;
  +   src += retval;
  +   buf += retval;
  +   count -= retval;
  +   }
 
 Now that's a funky loop.  Someone please convince me that there is no way
 in which `count - retval' can ever go negative (ie: huge positive).
 
 
  +   *ppos = src;
  +
  +   mmput(mm);
  +out_free:
  +   free_page((unsigned long) page);
  +out:
  +   put_task_struct(task);
  +out_no_task:
  +   return ret;
  +}
  +
  +static const struct file_operations proc_environ_operations = {
  +   .read   = environ_read,
  +};
  +
   static ssize_t oom_adjust_read(struct file *file, char __user *buf,
  size_t count, loff_t *ppos)
   {
  @@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
  DIR(task,   S_IRUGO|S_IXUGO, task),
  DIR(fd, S_IRUSR|S_IXUSR, fd),
  DIR(fdinfo, S_IRUSR|S_IXUSR, fdinfo),
  -   INF(environ,S_IRUSR, pid_environ),
  +   REG(environ,S_IRUSR, environ),
  INF(auxv,   S_IRUSR, pid_auxv),
  INF(status, S_IRUGO, pid_status),
  INF(limits, S_IRUSR, pid_limits),
  @@ -2421,7 +2473,7 @@ out_no_task:
   static const struct pid_entry tid_base_stuff[] = {
  DIR(fd,S_IRUSR|S_IXUSR, fd),
  DIR(fdinfo,S_IRUSR|S_IXUSR, fdinfo),
  -   INF(environ,   S_IRUSR, pid_environ),
  +   REG(environ,   S_IRUSR, environ),
  INF(auxv,  S_IRUSR, pid_auxv),
  INF(status,S_IRUGO, pid_status),
  INF(limits,S_IRUSR, 

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Arvin Moezzi]

 +
 +   if (copy_to_user(buf, page, retval)) {
 
  
  shouldn't you only copy min(count,retval) bytes? otherwise you could
  write beyond the users buffer buf, right?

 AFAIK, 'retval' can never be greater than 'this_len', which can never be
 greater than 'max_len', which can never be greater than 'count'

I think that's not true. 'count' is changing through the iteration.
The difference in the mem_read():

* while (count  0) {
* int this_len, retval;
*
*  this_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
*  retval = access_process_vm(task, src, page, this_len, 0);
*
*  ...
* }

is the fact, that this_len = min(PAGE_SIZE, count) is in the
iteration block, hence retval = this_len = count in each iteration
step. So this is ok. But IMHO in your code 'retval' may be bigger than
'count' in the last iteration of the block, because 'max_len' is fix
through your iteration but 'count' is changing. Or am i missing
something?

 James Pearson

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

Arvin Moezzi wrote:


I think that's not true. 'count' is changing through the iteration.
The difference in the mem_read():

* while (count  0) {
* int this_len, retval;
*
*  this_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
*  retval = access_process_vm(task, src, page, this_len, 0);
*
*  ...
* }

is the fact, that this_len = min(PAGE_SIZE, count) is in the
iteration block, hence retval = this_len = count in each iteration
step. So this is ok. But IMHO in your code 'retval' may be bigger than
'count' in the last iteration of the block, because 'max_len' is fix
through your iteration but 'count' is changing. Or am i missing
something?


Yes, you are correct ...

Thanks

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Arvin Moezzi]

2007/9/19, James Pearson <[EMAIL PROTECTED]>:
> +   while (count > 0) {
> +   int this_len, retval;
> +
> +   this_len = mm->env_end - (mm->env_start + src);
> +
> +   if (this_len <= 0)
> +   break;
> +
> +   if (this_len > max_len)
> +   this_len = max_len;
> +
> +   retval = access_process_vm(task, (mm->env_start + src),
> +   page, this_len, 0);
> +
> +   if (retval <= 0) {
> +   ret = retval;
> +   break;
> +   }
> +
> +   if (copy_to_user(buf, page, retval)) {

shouldn't you only copy min(count,retval) bytes? otherwise you could
write beyond the users buffer "buf", right?

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Andrew Morton]

On Wed, 19 Sep 2007 14:35:29 +0100
"James Pearson" <[EMAIL PROTECTED]> wrote:

> 
> From: James Pearson <[EMAIL PROTECTED]>
> 
> /proc/PID/environ currently truncates at 4096 characters, patch based on 
> the /proc/PID/mem code.

patch needs to be carefully reviewed from the security POV (ie: permissions)
as well as for correctness.  Does anyone have time to do that?

> Signed-off-by: James Pearson <[EMAIL PROTECTED]>
> 
> --- ./fs/proc/base.c.dist 2007-09-19 12:29:46.244929651 +0100
> +++ ./fs/proc/base.c  2007-09-19 12:36:18.155648760 +0100
> @@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
>(task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
>security_ptrace(current,task) == 0))
>  
> -static int proc_pid_environ(struct task_struct *task, char * buffer)
> -{
> - int res = 0;
> - struct mm_struct *mm = get_task_mm(task);
> - if (mm) {
> - unsigned int len;
> -
> - res = -ESRCH;
> - if (!ptrace_may_attach(task))
> - goto out;
> -
> - len  = mm->env_end - mm->env_start;
> - if (len > PAGE_SIZE)
> - len = PAGE_SIZE;
> - res = access_process_vm(task, mm->env_start, buffer, len, 0);
> -out:
> - mmput(mm);
> - }
> - return res;
> -}
> -
>  static int proc_pid_cmdline(struct task_struct *task, char * buffer)
>  {
>   int res = 0;
> @@ -740,6 +719,79 @@ static const struct file_operations proc
>   .open   = mem_open,
>  };
>  
> +static ssize_t environ_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
> + char *page;
> + unsigned long src = *ppos;
> + int ret = -ESRCH;
> + struct mm_struct *mm;
> + size_t max_len;
> +
> + if (!task)
> + goto out_no_task;
> +
> + if (!ptrace_may_attach(task))
> + goto out;
> +
> + ret = -ENOMEM;
> + page = (char *)__get_free_page(GFP_TEMPORARY);

Now I wonder what inspired you to reach for GFP_TEMPORARY?  Perhaps the
fact that it is crappily named and undocumented.

This should be GFP_KERNEL - the page you're allocating here is not
reclaimable by the VM.

> + if (!page)
> + goto out;
> +
> + ret = 0;
> +
> + mm = get_task_mm(task);
> + if (!mm)
> + goto out_free;
> +
> + max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> +
> + while (count > 0) {
> + int this_len, retval;
> +
> + this_len = mm->env_end - (mm->env_start + src);
> +
> + if (this_len <= 0)
> + break;
> +
> + if (this_len > max_len)
> + this_len = max_len;
> +
> + retval = access_process_vm(task, (mm->env_start + src),
> + page, this_len, 0);
> +
> + if (retval <= 0) {
> + ret = retval;
> + break;
> + }
> +
> + if (copy_to_user(buf, page, retval)) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + ret += retval;
> + src += retval;
> + buf += retval;
> + count -= retval;
> + }

Now that's a funky loop.  Someone please convince me that there is no way
in which `count - retval' can ever go negative (ie: huge positive).


> + *ppos = src;
> +
> + mmput(mm);
> +out_free:
> + free_page((unsigned long) page);
> +out:
> + put_task_struct(task);
> +out_no_task:
> + return ret;
> +}
> +
> +static const struct file_operations proc_environ_operations = {
> + .read   = environ_read,
> +};
> +
>  static ssize_t oom_adjust_read(struct file *file, char __user *buf,
>   size_t count, loff_t *ppos)
>  {
> @@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
>   DIR("task",   S_IRUGO|S_IXUGO, task),
>   DIR("fd", S_IRUSR|S_IXUSR, fd),
>   DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
> - INF("environ",S_IRUSR, pid_environ),
> + REG("environ",S_IRUSR, environ),
>   INF("auxv",   S_IRUSR, pid_auxv),
>   INF("status", S_IRUGO, pid_status),
>   INF("limits", S_IRUSR, pid_limits),
> @@ -2421,7 +2473,7 @@ out_no_task:
>  static const struct pid_entry tid_base_stuff[] = {
>   DIR("fd",S_IRUSR|S_IXUSR, fd),
>   DIR("fdinfo",S_IRUSR|S_IXUSR, fdinfo),
> - INF("environ",   S_IRUSR, pid_environ),
> + REG("environ",   S_IRUSR, environ),
>   INF("auxv",  S_IRUSR, pid_auxv),
>   INF("status",S_IRUGO, pid_status),
>   INF("limits",S_IRUSR, pid_limits),
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read 

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Andrew Morton]

On Wed, 19 Sep 2007 14:35:29 +0100
James Pearson [EMAIL PROTECTED] wrote:

 
 From: James Pearson [EMAIL PROTECTED]
 
 /proc/PID/environ currently truncates at 4096 characters, patch based on 
 the /proc/PID/mem code.

patch needs to be carefully reviewed from the security POV (ie: permissions)
as well as for correctness.  Does anyone have time to do that?

 Signed-off-by: James Pearson [EMAIL PROTECTED]
 
 --- ./fs/proc/base.c.dist 2007-09-19 12:29:46.244929651 +0100
 +++ ./fs/proc/base.c  2007-09-19 12:36:18.155648760 +0100
 @@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
(task-state == TASK_STOPPED || task-state == TASK_TRACED)  \
security_ptrace(current,task) == 0))
  
 -static int proc_pid_environ(struct task_struct *task, char * buffer)
 -{
 - int res = 0;
 - struct mm_struct *mm = get_task_mm(task);
 - if (mm) {
 - unsigned int len;
 -
 - res = -ESRCH;
 - if (!ptrace_may_attach(task))
 - goto out;
 -
 - len  = mm-env_end - mm-env_start;
 - if (len  PAGE_SIZE)
 - len = PAGE_SIZE;
 - res = access_process_vm(task, mm-env_start, buffer, len, 0);
 -out:
 - mmput(mm);
 - }
 - return res;
 -}
 -
  static int proc_pid_cmdline(struct task_struct *task, char * buffer)
  {
   int res = 0;
 @@ -740,6 +719,79 @@ static const struct file_operations proc
   .open   = mem_open,
  };
  
 +static ssize_t environ_read(struct file *file, char __user *buf,
 + size_t count, loff_t *ppos)
 +{
 + struct task_struct *task = get_proc_task(file-f_dentry-d_inode);
 + char *page;
 + unsigned long src = *ppos;
 + int ret = -ESRCH;
 + struct mm_struct *mm;
 + size_t max_len;
 +
 + if (!task)
 + goto out_no_task;
 +
 + if (!ptrace_may_attach(task))
 + goto out;
 +
 + ret = -ENOMEM;
 + page = (char *)__get_free_page(GFP_TEMPORARY);

Now I wonder what inspired you to reach for GFP_TEMPORARY?  Perhaps the
fact that it is crappily named and undocumented.

This should be GFP_KERNEL - the page you're allocating here is not
reclaimable by the VM.

 + if (!page)
 + goto out;
 +
 + ret = 0;
 +
 + mm = get_task_mm(task);
 + if (!mm)
 + goto out_free;
 +
 + max_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
 +
 + while (count  0) {
 + int this_len, retval;
 +
 + this_len = mm-env_end - (mm-env_start + src);
 +
 + if (this_len = 0)
 + break;
 +
 + if (this_len  max_len)
 + this_len = max_len;
 +
 + retval = access_process_vm(task, (mm-env_start + src),
 + page, this_len, 0);
 +
 + if (retval = 0) {
 + ret = retval;
 + break;
 + }
 +
 + if (copy_to_user(buf, page, retval)) {
 + ret = -EFAULT;
 + break;
 + }
 +
 + ret += retval;
 + src += retval;
 + buf += retval;
 + count -= retval;
 + }

Now that's a funky loop.  Someone please convince me that there is no way
in which `count - retval' can ever go negative (ie: huge positive).


 + *ppos = src;
 +
 + mmput(mm);
 +out_free:
 + free_page((unsigned long) page);
 +out:
 + put_task_struct(task);
 +out_no_task:
 + return ret;
 +}
 +
 +static const struct file_operations proc_environ_operations = {
 + .read   = environ_read,
 +};
 +
  static ssize_t oom_adjust_read(struct file *file, char __user *buf,
   size_t count, loff_t *ppos)
  {
 @@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
   DIR(task,   S_IRUGO|S_IXUGO, task),
   DIR(fd, S_IRUSR|S_IXUSR, fd),
   DIR(fdinfo, S_IRUSR|S_IXUSR, fdinfo),
 - INF(environ,S_IRUSR, pid_environ),
 + REG(environ,S_IRUSR, environ),
   INF(auxv,   S_IRUSR, pid_auxv),
   INF(status, S_IRUGO, pid_status),
   INF(limits, S_IRUSR, pid_limits),
 @@ -2421,7 +2473,7 @@ out_no_task:
  static const struct pid_entry tid_base_stuff[] = {
   DIR(fd,S_IRUSR|S_IXUSR, fd),
   DIR(fdinfo,S_IRUSR|S_IXUSR, fdinfo),
 - INF(environ,   S_IRUSR, pid_environ),
 + REG(environ,   S_IRUSR, environ),
   INF(auxv,  S_IRUSR, pid_auxv),
   INF(status,S_IRUGO, pid_status),
   INF(limits,S_IRUSR, pid_limits),
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Arvin Moezzi]

2007/9/19, James Pearson [EMAIL PROTECTED]:
 +   while (count  0) {
 +   int this_len, retval;
 +
 +   this_len = mm-env_end - (mm-env_start + src);
 +
 +   if (this_len = 0)
 +   break;
 +
 +   if (this_len  max_len)
 +   this_len = max_len;
 +
 +   retval = access_process_vm(task, (mm-env_start + src),
 +   page, this_len, 0);
 +
 +   if (retval = 0) {
 +   ret = retval;
 +   break;
 +   }
 +
 +   if (copy_to_user(buf, page, retval)) {

shouldn't you only copy min(count,retval) bytes? otherwise you could
write beyond the users buffer buf, right?

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Hugh Dickins]

On Wed, 19 Sep 2007, Mikael Pettersson wrote:
> H. Peter Anvin writes:
>  > Mikael Pettersson wrote:
>  > > Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > 
> /dev/null'
>  > > for a known good PID, the first read() from /proc/PID/mem fails with 
> ESRCH,
>  > 
>  > Of course it does.  Address zero isn't typically mapped.
> 
> Indeed. My bad :-(

No, not quite.  Peter explains why "cat /proc/self/mem" gets EIO,
but you were seeing "cat /proc/other/mem" get ESRCH: that's from
the stringent !MAY_PTRACE || !ptrace_may_attach checks.

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mikael Pettersson]

H. Peter Anvin writes:
 > Mikael Pettersson wrote:
 > > On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
 > >> /proc/PID/environ currently truncates at 4096 characters, patch based on 
 > >> the /proc/PID/mem code.
 > > 
 > > Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > 
 > > /dev/null'
 > > for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
 > 
 > Of course it does.  Address zero isn't typically mapped.

Indeed. My bad :-(
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[H. Peter Anvin]

Mikael Pettersson wrote:
> On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
>> /proc/PID/environ currently truncates at 4096 characters, patch based on 
>> the /proc/PID/mem code.
> 
> Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > /dev/null'
> for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,

Of course it does.  Address zero isn't typically mapped.

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mikael Pettersson]

On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
> /proc/PID/environ currently truncates at 4096 characters, patch based on 
> the /proc/PID/mem code.

Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem > /dev/null'
for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
which is bullocks. cat /proc/PID/environ does work. I'm seeing this on
2.6.22 vanilla, 2.6.20 FC6, and 2.6.18 RHEL5 kernels.

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

[PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

From: James Pearson <[EMAIL PROTECTED]>

/proc/PID/environ currently truncates at 4096 characters, patch based on 
the /proc/PID/mem code.

Signed-off-by: James Pearson <[EMAIL PROTECTED]>

---
Patch against 2.6.23-rc6-mm1

--- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c2007-09-19 12:36:18.155648760 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
 (task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
 security_ptrace(current,task) == 0))
 
-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
-   int res = 0;
-   struct mm_struct *mm = get_task_mm(task);
-   if (mm) {
-   unsigned int len;
-
-   res = -ESRCH;
-   if (!ptrace_may_attach(task))
-   goto out;
-
-   len  = mm->env_end - mm->env_start;
-   if (len > PAGE_SIZE)
-   len = PAGE_SIZE;
-   res = access_process_vm(task, mm->env_start, buffer, len, 0);
-out:
-   mmput(mm);
-   }
-   return res;
-}
-
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 {
int res = 0;
@@ -740,6 +719,79 @@ static const struct file_operations proc
.open   = mem_open,
 };
 
+static ssize_t environ_read(struct file *file, char __user *buf,
+   size_t count, loff_t *ppos)
+{
+   struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
+   char *page;
+   unsigned long src = *ppos;
+   int ret = -ESRCH;
+   struct mm_struct *mm;
+   size_t max_len;
+
+   if (!task)
+   goto out_no_task;
+
+   if (!ptrace_may_attach(task))
+   goto out;
+
+   ret = -ENOMEM;
+   page = (char *)__get_free_page(GFP_TEMPORARY);
+   if (!page)
+   goto out;
+
+   ret = 0;
+
+   mm = get_task_mm(task);
+   if (!mm)
+   goto out_free;
+
+   max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
+
+   while (count > 0) {
+   int this_len, retval;
+
+   this_len = mm->env_end - (mm->env_start + src);
+
+   if (this_len <= 0)
+   break;
+
+   if (this_len > max_len)
+   this_len = max_len;
+
+   retval = access_process_vm(task, (mm->env_start + src),
+   page, this_len, 0);
+
+   if (retval <= 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {
+   ret = -EFAULT;
+   break;
+   }
+
+   ret += retval;
+   src += retval;
+   buf += retval;
+   count -= retval;
+   }
+   *ppos = src;
+
+   mmput(mm);
+out_free:
+   free_page((unsigned long) page);
+out:
+   put_task_struct(task);
+out_no_task:
+   return ret;
+}
+
+static const struct file_operations proc_environ_operations = {
+   .read   = environ_read,
+};
+
 static ssize_t oom_adjust_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
 {
@@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
DIR("task",   S_IRUGO|S_IXUGO, task),
DIR("fd", S_IRUSR|S_IXUSR, fd),
DIR("fdinfo", S_IRUSR|S_IXUSR, fdinfo),
-   INF("environ",S_IRUSR, pid_environ),
+   REG("environ",S_IRUSR, environ),
INF("auxv",   S_IRUSR, pid_auxv),
INF("status", S_IRUGO, pid_status),
INF("limits", S_IRUSR, pid_limits),
@@ -2421,7 +2473,7 @@ out_no_task:
 static const struct pid_entry tid_base_stuff[] = {
DIR("fd",S_IRUSR|S_IXUSR, fd),
DIR("fdinfo",S_IRUSR|S_IXUSR, fdinfo),
-   INF("environ",   S_IRUSR, pid_environ),
+   REG("environ",   S_IRUSR, environ),
INF("auxv",  S_IRUSR, pid_auxv),
INF("status",S_IRUGO, pid_status),
INF("limits",S_IRUSR, pid_limits),
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[James Pearson]

From: James Pearson [EMAIL PROTECTED]

/proc/PID/environ currently truncates at 4096 characters, patch based on 
the /proc/PID/mem code.

Signed-off-by: James Pearson [EMAIL PROTECTED]

---
Patch against 2.6.23-rc6-mm1

--- ./fs/proc/base.c.dist   2007-09-19 12:29:46.244929651 +0100
+++ ./fs/proc/base.c2007-09-19 12:36:18.155648760 +0100
@@ -202,27 +202,6 @@ static int proc_root_link(struct inode *
 (task-state == TASK_STOPPED || task-state == TASK_TRACED)  \
 security_ptrace(current,task) == 0))
 
-static int proc_pid_environ(struct task_struct *task, char * buffer)
-{
-   int res = 0;
-   struct mm_struct *mm = get_task_mm(task);
-   if (mm) {
-   unsigned int len;
-
-   res = -ESRCH;
-   if (!ptrace_may_attach(task))
-   goto out;
-
-   len  = mm-env_end - mm-env_start;
-   if (len  PAGE_SIZE)
-   len = PAGE_SIZE;
-   res = access_process_vm(task, mm-env_start, buffer, len, 0);
-out:
-   mmput(mm);
-   }
-   return res;
-}
-
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 {
int res = 0;
@@ -740,6 +719,79 @@ static const struct file_operations proc
.open   = mem_open,
 };
 
+static ssize_t environ_read(struct file *file, char __user *buf,
+   size_t count, loff_t *ppos)
+{
+   struct task_struct *task = get_proc_task(file-f_dentry-d_inode);
+   char *page;
+   unsigned long src = *ppos;
+   int ret = -ESRCH;
+   struct mm_struct *mm;
+   size_t max_len;
+
+   if (!task)
+   goto out_no_task;
+
+   if (!ptrace_may_attach(task))
+   goto out;
+
+   ret = -ENOMEM;
+   page = (char *)__get_free_page(GFP_TEMPORARY);
+   if (!page)
+   goto out;
+
+   ret = 0;
+
+   mm = get_task_mm(task);
+   if (!mm)
+   goto out_free;
+
+   max_len = (count  PAGE_SIZE) ? PAGE_SIZE : count;
+
+   while (count  0) {
+   int this_len, retval;
+
+   this_len = mm-env_end - (mm-env_start + src);
+
+   if (this_len = 0)
+   break;
+
+   if (this_len  max_len)
+   this_len = max_len;
+
+   retval = access_process_vm(task, (mm-env_start + src),
+   page, this_len, 0);
+
+   if (retval = 0) {
+   ret = retval;
+   break;
+   }
+
+   if (copy_to_user(buf, page, retval)) {
+   ret = -EFAULT;
+   break;
+   }
+
+   ret += retval;
+   src += retval;
+   buf += retval;
+   count -= retval;
+   }
+   *ppos = src;
+
+   mmput(mm);
+out_free:
+   free_page((unsigned long) page);
+out:
+   put_task_struct(task);
+out_no_task:
+   return ret;
+}
+
+static const struct file_operations proc_environ_operations = {
+   .read   = environ_read,
+};
+
 static ssize_t oom_adjust_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
 {
@@ -2092,7 +2144,7 @@ static const struct pid_entry tgid_base_
DIR(task,   S_IRUGO|S_IXUGO, task),
DIR(fd, S_IRUSR|S_IXUSR, fd),
DIR(fdinfo, S_IRUSR|S_IXUSR, fdinfo),
-   INF(environ,S_IRUSR, pid_environ),
+   REG(environ,S_IRUSR, environ),
INF(auxv,   S_IRUSR, pid_auxv),
INF(status, S_IRUGO, pid_status),
INF(limits, S_IRUSR, pid_limits),
@@ -2421,7 +2473,7 @@ out_no_task:
 static const struct pid_entry tid_base_stuff[] = {
DIR(fd,S_IRUSR|S_IXUSR, fd),
DIR(fdinfo,S_IRUSR|S_IXUSR, fdinfo),
-   INF(environ,   S_IRUSR, pid_environ),
+   REG(environ,   S_IRUSR, environ),
INF(auxv,  S_IRUSR, pid_auxv),
INF(status,S_IRUGO, pid_status),
INF(limits,S_IRUSR, pid_limits),
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mikael Pettersson]

On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
 /proc/PID/environ currently truncates at 4096 characters, patch based on 
 the /proc/PID/mem code.

Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem  /dev/null'
for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
which is bullocks. cat /proc/PID/environ does work. I'm seeing this on
2.6.22 vanilla, 2.6.20 FC6, and 2.6.18 RHEL5 kernels.

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[H. Peter Anvin]

Mikael Pettersson wrote:
 On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
 /proc/PID/environ currently truncates at 4096 characters, patch based on 
 the /proc/PID/mem code.
 
 Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem  /dev/null'
 for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,

Of course it does.  Address zero isn't typically mapped.

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

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Mikael Pettersson]

H. Peter Anvin writes:
  Mikael Pettersson wrote:
   On Wed, 19 Sep 2007 14:35:29 +0100, James Pearson wrote:
   /proc/PID/environ currently truncates at 4096 characters, patch based on 
   the /proc/PID/mem code.
   
   Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem  
   /dev/null'
   for a known good PID, the first read() from /proc/PID/mem fails with ESRCH,
  
  Of course it does.  Address zero isn't typically mapped.

Indeed. My bad :-(
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -mm] Don't truncate /proc/PID/environ at 4096 characters

[Hugh Dickins]

On Wed, 19 Sep 2007, Mikael Pettersson wrote:
 H. Peter Anvin writes:
   Mikael Pettersson wrote:
Does /proc/PID/mem even work? If I do `strace cat /proc/PID/mem  
 /dev/null'
for a known good PID, the first read() from /proc/PID/mem fails with 
 ESRCH,
   
   Of course it does.  Address zero isn't typically mapped.
 
 Indeed. My bad :-(

No, not quite.  Peter explains why cat /proc/self/mem gets EIO,
but you were seeing cat /proc/other/mem get ESRCH: that's from
the stringent !MAY_PTRACE || !ptrace_may_attach checks.

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