It appears that /proc/PID/environ only returns the first 4096 bytes of a
processes' environment.
Is there any other way via userland to get the whole environment for a
process?
Thanks
James Pearson
On Wed, 24 May 2006, James Pearson wrote:
> It appears that /proc/PID/environ only returns the first 4096 bytes of a
> processes' environment.
>
> Is there any other way via userland to get the whole environment for a
> process?
>
> Thanks
>
> James Pearson
I think that /proc/PID/environ just returns the environment that
existed when the process was created, irrespective of size. You
can check this as:
#include <stdio.h>
main()
{
setenv("FOO=", "1234", 1);
printf("%d\n", getpid());
pause();
}
Variable "FOO" will not appear in /proc. It you set the environment
in non-standard ways, overwriting the original, you can see it in
/proc.
Cheers,
Dick Johnson
Penguin : Linux version 2.6.16.4 on an i686 machine (5592.89 BogoMips).
New book: http://www.AbominableFirebug.com/
_
****************************************************************
The information transmitted in this message is confidential and may be privileged. Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited. If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to [email protected] - and destroy all copies of this information, including any attachments, without reading or disclosing them.
Thank you.
linux-os (Dick Johnson) wrote:
> On Wed, 24 May 2006, James Pearson wrote:
>
>
>>It appears that /proc/PID/environ only returns the first 4096 bytes of a
>>processes' environment.
>>
>>Is there any other way via userland to get the whole environment for a
>>process?
>>
>>Thanks
>>
>>James Pearson
>
>
>
> I think that /proc/PID/environ just returns the environment that
> existed when the process was created, irrespective of size. You
> can check this as:
>
> #include <stdio.h>
>
> main()
> {
> setenv("FOO=", "1234", 1);
> printf("%d\n", getpid());
> pause();
> }
>
> Variable "FOO" will not appear in /proc. It you set the environment
> in non-standard ways, overwriting the original, you can see it in
> /proc.
I'm not worried about that - more the fact that when I do:
% cat /proc/$$/environ | wc -c
4096
% env | wc -c
7329
/proc/PID/environ is truncated ...
James Pearson
Followup to: <[email protected]>
By author: James Pearson <[email protected]>
In newsgroup: linux.dev.kernel
>
> I'm not worried about that - more the fact that when I do:
>
> % cat /proc/$$/environ | wc -c
> 4096
> % env | wc -c
> 7329
>
> /proc/PID/environ is truncated ...
>
Funny enough, I was looking at this yesterday. I think there is a
pretty clean solution for it, I just haven't had a chance to attack it
yet.
-hpa
H. Peter Anvin wrote:
>>
>> I'm not worried about that - more the fact that when I do:
>>
>> % cat /proc/$$/environ | wc -c
>> 4096
>> % env | wc -c
>> 7329
>>
>> /proc/PID/environ is truncated ...
>>
>
> Funny enough, I was looking at this yesterday. I think there is a
> pretty clean solution for it, I just haven't had a chance to attack it
> yet.
Having a poke about in fs/proc/, I came up with this - probably isn't
pretty or clean, but it works ...
James Pearson
--- ./include/linux/proc_fs.h.dist 2006-05-11 02:56:24.000000000 +0100
+++ ./include/linux/proc_fs.h 2006-05-24 13:43:55.964159897 +0100
@@ -250,7 +250,7 @@
int type;
union {
int (*proc_get_link)(struct inode *, struct dentry **,
struct vfsmount **);
- int (*proc_read)(struct task_struct *task, char *page);
+ int (*proc_read)(struct task_struct *task, char *page,
loff_t *pos);
} op;
struct proc_dir_entry *pde;
struct inode vfs_inode;
--- ./fs/proc/base.c.dist 2006-05-11 02:56:24.000000000 +0100
+++ ./fs/proc/base.c 2006-05-24 13:54:26.370965292 +0100
@@ -409,15 +409,27 @@
(task->state == TASK_STOPPED || task->state == TASK_TRACED) && \
security_ptrace(current,task) == 0))
-static int proc_pid_environ(struct task_struct *task, char * buffer)
+static int proc_pid_environ(struct task_struct *task, char * buffer,
loff_t *pos)
{
int res = 0;
+ int p = *pos;
struct mm_struct *mm = get_task_mm(task);
+
+ /* proc_pid_environ is a 'special case' - the required data could
+ be larger than a page, so we read sequential chunks of the
+ environment data into the buffer using the supplied offset */
+ if (p < 0)
+ return -EINVAL;
+
if (mm) {
- unsigned int len = mm->env_end - mm->env_start;
+ unsigned int len = mm->env_end - (mm->env_start + p);
if (len > PAGE_SIZE)
len = PAGE_SIZE;
- res = access_process_vm(task, mm->env_start, buffer,
len, 0);
+ res = access_process_vm(task, (mm->env_start + p),
buffer, len, 0);
+ /* the calling routine (proc_info_read) needs to know we've
+ used the offset to read the pid data */
+ *pos += res;
+
if (!ptrace_may_attach(task))
res = -ESRCH;
mmput(mm);
@@ -425,7 +437,7 @@
return res;
}
-static int proc_pid_cmdline(struct task_struct *task, char * buffer)
+static int proc_pid_cmdline(struct task_struct *task, char * buffer,
loff_t *pos)
{
int res = 0;
unsigned int len;
@@ -462,7 +474,7 @@
return res;
}
-static int proc_pid_auxv(struct task_struct *task, char *buffer)
+static int proc_pid_auxv(struct task_struct *task, char *buffer, loff_t
*pos)
{
int res = 0;
struct mm_struct *mm = get_task_mm(task);
@@ -486,7 +498,7 @@
* Provides a wchan file via kallsyms in a proper one-value-per-file
format.
* Returns the resolved symbol. If that fails, simply return the address.
*/
-static int proc_pid_wchan(struct task_struct *task, char *buffer)
+static int proc_pid_wchan(struct task_struct *task, char *buffer,
loff_t *pos)
{
char *modname;
const char *sym_name;
@@ -506,7 +518,7 @@
/*
* Provides /proc/PID/schedstat
*/
-static int proc_pid_schedstat(struct task_struct *task, char *buffer)
+static int proc_pid_schedstat(struct task_struct *task, char *buffer,
loff_t *pos)
{
return sprintf(buffer, "%lu %lu %lu\n",
task->sched_info.cpu_time,
@@ -517,7 +529,7 @@
/* The badness from the OOM killer */
unsigned long badness(struct task_struct *p, unsigned long uptime);
-static int proc_oom_score(struct task_struct *task, char *buffer)
+static int proc_oom_score(struct task_struct *task, char *buffer,
loff_t *pos)
{
unsigned long points;
struct timespec uptime;
@@ -745,16 +757,30 @@
unsigned long page;
ssize_t length;
struct task_struct *task = proc_task(inode);
+ loff_t pos = *ppos;
if (count > PROC_BLOCK_SIZE)
count = PROC_BLOCK_SIZE;
if (!(page = __get_free_page(GFP_KERNEL)))
return -ENOMEM;
- length = PROC_I(inode)->op.proc_read(task, (char*)page);
+ length = PROC_I(inode)->op.proc_read(task, (char*)page, &pos);
- if (length >= 0)
- length = simple_read_from_buffer(buf, count, ppos, (char
*)page, length);
+ if (length >= 0) {
+ if (pos != *ppos) {
+ /* we are using the buffer to store subsequent
sections
+ of the proc_pid data - so the offset into the
buffer
+ will always be 0 - the offset is used to get the
+ original data */
+ pos = 0;
+ length = simple_read_from_buffer(buf, count,
&pos, (char *)page, length);
+ /* need to update the 'real' offset into the data we
+ are reading ... */
+ *ppos += length;
+ }
+ else
+ length = simple_read_from_buffer(buf, count,
ppos, (char *)page, length);
+ }
free_page(page);
return length;
}
--- ./fs/proc/internal.h.dist 2006-05-11 02:56:24.000000000 +0100
+++ ./fs/proc/internal.h 2006-05-24 13:52:53.568391757 +0100
@@ -32,10 +32,10 @@
extern void create_seq_entry(char *name, mode_t mode, struct
file_operations *f);
extern int proc_exe_link(struct inode *, struct dentry **, struct
vfsmount **);
-extern int proc_tid_stat(struct task_struct *, char *);
-extern int proc_tgid_stat(struct task_struct *, char *);
-extern int proc_pid_status(struct task_struct *, char *);
-extern int proc_pid_statm(struct task_struct *, char *);
+extern int proc_tid_stat(struct task_struct *, char *, loff_t *);
+extern int proc_tgid_stat(struct task_struct *, char *, loff_t *);
+extern int proc_pid_status(struct task_struct *, char *, loff_t *);
+extern int proc_pid_statm(struct task_struct *, char *, loff_t *);
void free_proc_entry(struct proc_dir_entry *de);
--- ./fs/proc/array.c.dist 2006-05-11 02:56:24.000000000 +0100
+++ ./fs/proc/array.c 2006-05-24 13:43:09.327905203 +0100
@@ -293,7 +293,7 @@
cap_t(p->cap_effective));
}
-int proc_pid_status(struct task_struct *task, char * buffer)
+int proc_pid_status(struct task_struct *task, char * buffer, loff_t *pos)
{
char * orig = buffer;
struct mm_struct *mm = get_task_mm(task);
@@ -465,17 +465,17 @@
return res;
}
-int proc_tid_stat(struct task_struct *task, char * buffer)
+int proc_tid_stat(struct task_struct *task, char * buffer, loff_t *pos)
{
return do_task_stat(task, buffer, 0);
}
-int proc_tgid_stat(struct task_struct *task, char * buffer)
+int proc_tgid_stat(struct task_struct *task, char * buffer, loff_t *pos)
{
return do_task_stat(task, buffer, 1);
}
-int proc_pid_statm(struct task_struct *task, char *buffer)
+int proc_pid_statm(struct task_struct *task, char *buffer, loff_t *pos)
{
int size = 0, resident = 0, shared = 0, text = 0, lib = 0, data
= 0;
struct mm_struct *mm = get_task_mm(task);
I think this is the wrong approach.
Many of these should probably be converted to seq_file, but in the
particular case of environ, the right approach is to observe the fact
that reading environ is just like reading /proc/PID/mem, except:
a. the access restrictions are less strict, and
b. there is a range restriction, which needs to be enforced, and
c. there is an offset.
Pretty much, take the guts from /proc/PID/mem and generalize it
slightly, and you have the code that can run either /proc/PID/mem or
/proc/PID/environ.
-hpa
On 6/1/06, James Pearson <[email protected]> wrote:
> H. Peter Anvin wrote:
> > I think this is the wrong approach.
> >
> > Many of these should probably be converted to seq_file, but in the
> > particular case of environ, the right approach is to observe the fact
> > that reading environ is just like reading /proc/PID/mem, except:
> >
> > a. the access restrictions are less strict, and
> > b. there is a range restriction, which needs to be enforced, and
> > c. there is an offset.
> >
> > Pretty much, take the guts from /proc/PID/mem and generalize it
> > slightly, and you have the code that can run either /proc/PID/mem or
> > /proc/PID/environ.
>
> The following patch is based on the /proc/PID/mem code appears to work fine.
>
> James Pearson
>
>
> --- ./fs/proc/base.c.dist 2006-05-11 02:56:24.000000000 +0100
> +++ ./fs/proc/base.c 2006-06-01 13:40:50.865851007 +0100
> @@ -409,22 +409,6 @@
> (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 = mm->env_end - mm->env_start;
> - if (len > PAGE_SIZE)
> - len = PAGE_SIZE;
> - res = access_process_vm(task, mm->env_start, buffer,
> len, 0);
> - if (!ptrace_may_attach(task))
> - res = -ESRCH;
> - mmput(mm);
> - }
> - return res;
> -}
> -
> static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> {
> int res = 0;
> @@ -897,6 +881,80 @@
> .open = mem_open,
> };
>
> +static ssize_t env_read(struct file * file, char __user * buf,
> + size_t count, loff_t *ppos)
> +{
> + struct task_struct *task = 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 (!ptrace_may_attach(task))
> + goto out;
> +
> + ret = -ENOMEM;
> + page = (char *)__get_free_page(GFP_USER);
> + if (!page)
> + goto out;
> +
> + ret = 0;
> +
> + mm = get_task_mm(task);
> + if (!mm)
> + goto out_free;
> +
> + ret = 0;
> + 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 (!ptrace_may_attach(task)) {
> + ret = -ESRCH;
> + break;
> + }
> +
> + 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:
> + return ret;
> +}
> +
> +static struct file_operations proc_env_operations = {
> + .read = env_read,
> +};
> +
> static ssize_t oom_adjust_read(struct file *file, char __user *buf,
> size_t count, loff_t *ppos)
> {
> @@ -1675,11 +1733,6 @@
> inode->i_op = &proc_pid_link_inode_operations;
> ei->op.proc_get_link = proc_root_link;
> break;
> - case PROC_TID_ENVIRON:
> - case PROC_TGID_ENVIRON:
> - inode->i_fop = &proc_info_file_operations;
> - ei->op.proc_read = proc_pid_environ;
> - break;
> case PROC_TID_AUXV:
> case PROC_TGID_AUXV:
> inode->i_fop = &proc_info_file_operations;
> @@ -1723,6 +1776,10 @@
> inode->i_op = &proc_mem_inode_operations;
> inode->i_fop = &proc_mem_operations;
> break;
> + case PROC_TID_ENVIRON:
> + case PROC_TGID_ENVIRON:
> + inode->i_fop = &proc_env_operations;
> + break;
> #ifdef CONFIG_SECCOMP
> case PROC_TID_SECCOMP:
> case PROC_TGID_SECCOMP:
> -
This thread has gone stale. The PAGE_SIZE limit still exists. Is this
solution acceptable?
thanks,
--Guy Streeter
Guy Streeter wrote:
> On 6/1/06, James Pearson <[email protected]> wrote:
>> H. Peter Anvin wrote:
>>> I think this is the wrong approach.
>>>
>>> Many of these should probably be converted to seq_file, but in the
>>> particular case of environ, the right approach is to observe the fact
>>> that reading environ is just like reading /proc/PID/mem, except:
>>>
>>> a. the access restrictions are less strict, and
>>> b. there is a range restriction, which needs to be enforced, and
>>> c. there is an offset.
>>>
>>> Pretty much, take the guts from /proc/PID/mem and generalize it
>>> slightly, and you have the code that can run either /proc/PID/mem or
>>> /proc/PID/environ.
>> The following patch is based on the /proc/PID/mem code appears to work fine.
>>
>
> This thread has gone stale. The PAGE_SIZE limit still exists. Is this
> solution acceptable?
>
Can we avoid the code duplication?
-hpa
On 8/15/07, H. Peter Anvin <[email protected]> wrote:
> Guy Streeter wrote:
> > On 6/1/06, James Pearson <[email protected]> wrote:
> >> H. Peter Anvin wrote:
> >>> I think this is the wrong approach.
> >>>
> >>> Many of these should probably be converted to seq_file, but in the
> >>> particular case of environ, the right approach is to observe the fact
> >>> that reading environ is just like reading /proc/PID/mem, except:
> >>>
> >>> a. the access restrictions are less strict, and
> >>> b. there is a range restriction, which needs to be enforced, and
> >>> c. there is an offset.
> >>>
> >>> Pretty much, take the guts from /proc/PID/mem and generalize it
> >>> slightly, and you have the code that can run either /proc/PID/mem or
> >>> /proc/PID/environ.
> >> The following patch is based on the /proc/PID/mem code appears to work fine.
> >>
> >
> > This thread has gone stale. The PAGE_SIZE limit still exists. Is this
> > solution acceptable?
> >
>
> Can we avoid the code duplication?
>
>
I hope you're not asking me. I don't know the proc fs code well enough
to re-write this.
--Guy
H. Peter Anvin wrote:
> Guy Streeter wrote:
>
>>On 6/1/06, James Pearson <[email protected]> wrote:
>>
>>>H. Peter Anvin wrote:
>>>
>>>>I think this is the wrong approach.
>>>>
>>>>Many of these should probably be converted to seq_file, but in the
>>>>particular case of environ, the right approach is to observe the fact
>>>>that reading environ is just like reading /proc/PID/mem, except:
>>>>
>>>> a. the access restrictions are less strict, and
>>>> b. there is a range restriction, which needs to be enforced, and
>>>> c. there is an offset.
>>>>
>>>>Pretty much, take the guts from /proc/PID/mem and generalize it
>>>>slightly, and you have the code that can run either /proc/PID/mem or
>>>>/proc/PID/environ.
>>>
>>>The following patch is based on the /proc/PID/mem code appears to work fine.
>>>
>>
>>This thread has gone stale. The PAGE_SIZE limit still exists. Is this
>>solution acceptable?
>>
>
>
> Can we avoid the code duplication?
There isn't that much that is duplicated - and there are also bits of
the /proc/PID/mem code that are not needed in this case, so I'm not
really sure if it is worth doing.
I did submit a patch a few months ago - see:
<http://marc.info/?l=linux-kernel&m=117862109623007&w=2>
James Pearson
Hey guys, the future of this patch is important for me. What do you think, has this patch any chances to be committed to upstream?
James Pearson <[email protected]> writes:
> H. Peter Anvin wrote:
> There isn't that much that is duplicated - and there are also bits of
> the /proc/PID/mem code that are not needed in this case, so I'm not
> really sure if it is worth doing.
>
> I did submit a patch a few months ago - see:
>
> <http://marc.info/?l=linux-kernel&m=117862109623007&w=2>
--
Anton Arapov, <[email protected]>
Anton Arapov wrote:
> Hey guys, the future of this patch is important for me. What do you think, has this patch any chances to be committed to upstream?
>
> James Pearson <[email protected]> writes:
>> H. Peter Anvin wrote:
>> There isn't that much that is duplicated - and there are also bits of
>> the /proc/PID/mem code that are not needed in this case, so I'm not
>> really sure if it is worth doing.
>>
>> I did submit a patch a few months ago - see:
>>
>> <http://marc.info/?l=linux-kernel&m=117862109623007&w=2>
>
Looks reasonable to me, except for the one overlong line.
-hpa
H. Peter Anvin wrote:
> Anton Arapov wrote:
>
>> Hey guys, the future of this patch is important for me. What do you
>> think, has this patch any chances to be committed to upstream?
>>
>> James Pearson <[email protected]> writes:
>>
>>> H. Peter Anvin wrote:
>>> There isn't that much that is duplicated - and there are also bits of
>>> the /proc/PID/mem code that are not needed in this case, so I'm not
>>> really sure if it is worth doing.
>>>
>>> I did submit a patch a few months ago - see:
>>>
>>> <http://marc.info/?l=linux-kernel&m=117862109623007&w=2>
>>
>>
>
> Looks reasonable to me, except for the one overlong line.
>
OK, here is the patch (without the long line) against 2.6.23-rc5 - what
else needs to be done to get it committed?
James Pearson
--- ./fs/proc/base.c.dist 2007-09-01 07:08:24.000000000 +0100
+++ ./fs/proc/base.c 2007-09-05 14:08:15.762518000 +0100
@@ -199,27 +199,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;
@@ -658,6 +637,85 @@ 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_USER);
+ 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 (!ptrace_may_attach(task)) {
+ ret = -ESRCH;
+ break;
+ }
+
+ 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)
{
@@ -2048,7 +2106,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),
#ifdef CONFIG_SCHED_DEBUG
@@ -2335,7 +2393,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),
#ifdef CONFIG_SCHED_DEBUG
On Wed, 05 Sep 2007 18:00:57 +0100 James Pearson wrote:
> H. Peter Anvin wrote:
> > Anton Arapov wrote:
> >
> >> Hey guys, the future of this patch is important for me. What do you
> >> think, has this patch any chances to be committed to upstream?
> >>
> >> James Pearson <[email protected]> writes:
> >>
> >>> H. Peter Anvin wrote:
> >>> There isn't that much that is duplicated - and there are also bits of
> >>> the /proc/PID/mem code that are not needed in this case, so I'm not
> >>> really sure if it is worth doing.
> >>>
> >>> I did submit a patch a few months ago - see:
> >>>
> >>> <http://marc.info/?l=linux-kernel&m=117862109623007&w=2>
> >>
> >>
> >
> > Looks reasonable to me, except for the one overlong line.
> >
>
> OK, here is the patch (without the long line) against 2.6.23-rc5 - what
> else needs to be done to get it committed?
Hi,
a. It needs a changelog that describes the problem and the patch.
b. It needs to apply cleanly to a current kernel.
(It does not apply cleanly now due to some odd line breaks [see #1
below.)
c. It needs to use tabs instead of spaces. That will probably help
on item b as well.
linux-2.6.23-rc5> dryrun < ~/fs-proc-read-sizes.patch
4 out of 4 hunks FAILED -- saving rejects to file fs/proc/base.c.rej
Even using patch -l (--ignore-whitespace), it fails with:
linux-2.6.23-rc5> patch -p1 -bl --dry-run --verbose < ~/fs-proc-read-sizes.patch
Hmm... Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- ./fs/proc/base.c.dist 2007-09-01 07:08:24.000000000 +0100
|+++ ./fs/proc/base.c 2007-09-05 14:08:15.762518000 +0100
--------------------------
Patching file fs/proc/base.c using Plan A...
Hunk #1 FAILED at 199.
Hunk #2 FAILED at 637.
Hunk #3 succeeded at 2106 with fuzz 1.
Hunk #4 succeeded at 2393 with fuzz 1.
2 out of 4 hunks FAILED -- saving rejects to file fs/proc/base.c.rej
done
> --- ./fs/proc/base.c.dist 2007-09-01 07:08:24.000000000 +0100
> +++ ./fs/proc/base.c 2007-09-05 14:08:15.762518000 +0100
> @@ -199,27 +199,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);
#1 ^^^^^^^
> -out:
> - mmput(mm);
> - }
> - return res;
> -}
> -
> static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> {
> int res = 0;
> @@ -658,6 +637,85 @@ 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_USER);
> + 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 (!ptrace_may_attach(task)) {
> + ret = -ESRCH;
> + break;
> + }
> +
> + 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)
> {
> @@ -2048,7 +2106,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),
> #ifdef CONFIG_SCHED_DEBUG
> @@ -2335,7 +2393,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),
> #ifdef CONFIG_SCHED_DEBUG
> -
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
On Wed, Sep 05, 2007 at 06:00:57PM +0100, James Pearson wrote:
> H. Peter Anvin wrote:
> >Anton Arapov wrote:
> >
> >> Hey guys, the future of this patch is important for me. What do you
> >>think, has this patch any chances to be committed to upstream?
> >>
> >>James Pearson <[email protected]> writes:
> >>
> >>>H. Peter Anvin wrote:
> >>>There isn't that much that is duplicated - and there are also bits of
> >>>the /proc/PID/mem code that are not needed in this case, so I'm not
> >>>really sure if it is worth doing.
> >>>
> >>>I did submit a patch a few months ago - see:
> >>>
> >>><http://marc.info/?l=linux-kernel&m=117862109623007&w=2>
> >>
> >>
> >
> >Looks reasonable to me, except for the one overlong line.
> >
>
> OK, here is the patch (without the long line) against 2.6.23-rc5 - what
> else needs to be done to get it committed?
Remove duplicate ptrace_may_attach() checks, unecessary (), {} and
spaces before pointer names -- char *buf.
Randy Dunlap wrote:
>>
>>OK, here is the patch (without the long line) against 2.6.23-rc5 - what
>>else needs to be done to get it committed?
>
>
> Hi,
>
> a. It needs a changelog that describes the problem and the patch.
> b. It needs to apply cleanly to a current kernel.
> (It does not apply cleanly now due to some odd line breaks [see #1
> below.)
> c. It needs to use tabs instead of spaces. That will probably help
> on item b as well.
>
> linux-2.6.23-rc5> dryrun < ~/fs-proc-read-sizes.patch
> 4 out of 4 hunks FAILED -- saving rejects to file fs/proc/base.c.rej
I think a 'cut-n-paste' to my mail app mangled the patch - I'll
re-submit it 'cleanly' ...
Thanks
James Pearson
Alexey Dobriyan wrote:
> On Wed, Sep 05, 2007 at 06:00:57PM +0100, James Pearson wrote:
>
>>H. Peter Anvin wrote:
>>
>>>Anton Arapov wrote:
>>>
>>>
>>>> Hey guys, the future of this patch is important for me. What do you
>>>>think, has this patch any chances to be committed to upstream?
>>>>
>>>>James Pearson <[email protected]> writes:
>>>>
>>>>
>>>>>H. Peter Anvin wrote:
>>>>>There isn't that much that is duplicated - and there are also bits of
>>>>>the /proc/PID/mem code that are not needed in this case, so I'm not
>>>>>really sure if it is worth doing.
>>>>>
>>>>>I did submit a patch a few months ago - see:
>>>>>
>>>>><http://marc.info/?l=linux-kernel&m=117862109623007&w=2>
>>>>
>>>>
>>>Looks reasonable to me, except for the one overlong line.
>>>
>>
>>OK, here is the patch (without the long line) against 2.6.23-rc5 - what
>>else needs to be done to get it committed?
>
>
> Remove duplicate ptrace_may_attach() checks, unecessary (), {} and
> spaces before pointer names -- char *buf.
environ_read() in the patch uses ptrace_may_attach() in a similar way as
does mem_read(). Given that environ_read() is based on mem_read(), does
this mean that duplicate ptrace_may_attach() checks need to be removed
from mem_read() as well? Which ptrace_may_attach() needs to be removed?
Thanks
James Pearson
On Sep 5 2007 18:00, James Pearson wrote:
>
> OK, here is the patch (without the long line) against 2.6.23-rc5 - what else
> needs to be done to get it committed?
Someone has to point out CodingStyle, so I'll ruin everyone's day now :)
> +static ssize_t environ_read(struct file * file, char __user * buf,
^ ^
be gone these spaces.
> + while (count > 0) {
> + int this_len, retval;
> +
> + this_len = mm->env_end - (mm->env_start + src);
> +
> + if (this_len <= 0) {
> + break;
> + }
Be gone, braces.
Jan Engelhardt wrote:
> On Sep 5 2007 18:00, James Pearson wrote:
>> OK, here is the patch (without the long line) against 2.6.23-rc5 - what else
>> needs to be done to get it committed?
>
> Someone has to point out CodingStyle, so I'll ruin everyone's day now :)
>
>> +static ssize_t environ_read(struct file * file, char __user * buf,
> ^ ^
> be gone these spaces.
>
>> + while (count > 0) {
>> + int this_len, retval;
>> +
>> + this_len = mm->env_end - (mm->env_start + src);
>> +
>> + if (this_len <= 0) {
>> + break;
>> + }
>
> Be gone, braces.
>
Right, also please use use checkpatch.pl.
-hpa
Jan Engelhardt <[email protected]> writes:
> On Sep 5 2007 18:00, James Pearson wrote:
>>
>> OK, here is the patch (without the long line) against 2.6.23-rc5 - what else
>> needs to be done to get it committed?
>
> Someone has to point out CodingStyle, so I'll ruin everyone's day now :)
>
Already pointed out! :) By Alexey Dobriyan.
>> +static ssize_t environ_read(struct file * file, char __user * buf,
> ^ ^
> be gone these spaces.
>
>> + while (count > 0) {
>> + int this_len, retval;
>> +
>> + this_len = mm->env_end - (mm->env_start + src);
>> +
>> + if (this_len <= 0) {
>> + break;
>> + }
>
> Be gone, braces.
>
>
>
--
Anton Arapov, <[email protected]>
H. Peter Anvin wrote:
>
> Right, also please use use checkpatch.pl.
>
OK - how about:
/proc/PID/environ currently truncates at 4096 characters, patch based on
the /proc/PID/mem code.
Patch against 2.6.23-rc5
Signed-off-by: James Pearson <[email protected]>
--- ./fs/proc/base.c.dist 2007-09-01 07:08:24.000000000 +0100
+++ ./fs/proc/base.c 2007-09-06 14:29:46.413680554 +0100
@@ -199,27 +199,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;
@@ -658,6 +637,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_USER);
+ 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)
{
@@ -2048,7 +2100,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),
#ifdef CONFIG_SCHED_DEBUG
@@ -2335,7 +2387,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),
#ifdef CONFIG_SCHED_DEBUG
Hi fellas! What we can do for this patch? It looks like ignored at
all? I want reinitiate this thread because I want to have the
answer!
What do you think?
"James Pearson" <[email protected]> writes:
> H. Peter Anvin wrote:
>> Right, also please use use checkpatch.pl.
> OK - how about:
>
> /proc/PID/environ currently truncates at 4096 characters, patch based on
> the /proc/PID/mem code.
>
> Patch against 2.6.23-rc5
>
> Signed-off-by: James Pearson <[email protected]>
[...patch_skipped...]
--
Anton Arapov, <[email protected]>
Anton Arapov wrote:
> Hi fellas! What we can do for this patch? It looks like ignored at
> all? I want reinitiate this thread because I want to have the
> answer!
>
> What do you think?
Clean up the style, then we can put it into -mm.
It should be in -mm for about a full kernel cycle.
-hpa