[PATCH] fs/proc: allow larger /proc/<pid>/cmdline output

[PATCH] fs/proc: allow larger /proc/<pid>/cmdline output

[Jarod Wilson]

There are people who run java. Sometimes, when it misbehaves, they try to
figure out what's going on by dumping /proc/<pid>/cmdline, but the length
of that output is currently capped by PAGE_SIZE (so x86_64's 4k, in most
cases), and sometimes, java command lines are longer than 4k characters.

This change allows the user to request a larger max length, up to 4x
PAGE_SIZE, but the default out-of-the-box setting should keep things the
same as they ever were. The 4x maximum is somewhat arbitrary, but seemed
like it should be more than enough, because really, if you have more than
16k characters on your command line, you're probably doing it wrong...

I've tested this lightly with non-java shell commands with really long
parameters, and things are perfectly stable after several hundred
iterations of exercising things on a system booted with both
proc_pid_maxlen=8192 and 16384. I wouldn't call my testing exhaustive,
and I may not have considered something that will blow up horribly here,
so comments and clues welcomed.

Using single_open_size() looked less messy than giving proc_pid_cmdline()
its own .start op that would allow multiple buffers.

Note: I've only added this extended sizing for /proc/<pid>/cmdline output,
rather than for all /proc/<pid>/foo elements, thinking that nothing else
should ever really be that long, but anything that is can simply switch
from using the ONE() macro to the ONE_SIZE() macro.

CC: Andrew Morton <akpm@linux-foundation.org>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: "Eric W. Biederman" <ebiederm@xmission.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Miklos Szeredi <miklos@szeredi.hu>
CC: Zefan Li <lizefan@huawei.com>
CC: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 fs/proc/base.c | 42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 3f3d7ae..bb466c1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -134,6 +134,30 @@ struct pid_entry {
 	NOD(NAME, (S_IFREG|(MODE)), 			\
 		NULL, &proc_single_file_operations,	\
 		{ .proc_show = show } )
+#define ONE_SIZE(NAME, MODE, show)				\
+	NOD(NAME, (S_IFREG|(MODE)), 				\
+		NULL, &proc_single_file_size_operations,	\
+		{ .proc_show = show } )
+
+/*
+ * Its hideous, but some java gunk winds up with a cmdline that is longer
+ * than PAGE_SIZE, and some people want to be able to see all of it for
+ * debugging purposes. Allocate at least PAGE_SIZE, and allow the user to
+ * ask for up to PAGE_SIZE << 2 (4x) to help with that situation.
+ */
+static unsigned long proc_pid_maxlen = PAGE_SIZE;
+static int set_proc_pid_maxlen(char *str)
+{
+	if (!str)
+		return 0;
+
+	proc_pid_maxlen = simple_strtoul(str, &str, 0);
+	proc_pid_maxlen = max(PAGE_SIZE, proc_pid_maxlen);
+	proc_pid_maxlen = min(PAGE_SIZE << 2, proc_pid_maxlen);
+
+	return 1;
+}
+__setup("proc_pid_maxlen=", set_proc_pid_maxlen);
 
 /*
  * Count the number of hardlinks for the pid_entry table, excluding the .
@@ -204,7 +228,7 @@ static int proc_pid_cmdline(struct seq_file *m, struct pid_namespace *ns,
 	 * per internal buffer allocation. See single_open(), traverse().
 	 */
 	BUG_ON(m->size < PAGE_SIZE);
-	m->count += get_cmdline(task, m->buf, PAGE_SIZE);
+	m->count += get_cmdline(task, m->buf, proc_pid_maxlen);
 	return 0;
 }
 
@@ -601,6 +625,18 @@ static const struct file_operations proc_single_file_operations = {
 	.release	= single_release,
 };
 
+static int proc_single_open_size(struct inode *inode, struct file *filp)
+{
+	return single_open_size(filp, proc_single_show, inode, proc_pid_maxlen);
+}
+
+static const struct file_operations proc_single_file_size_operations = {
+	.open		= proc_single_open_size,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 
 struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
 {
@@ -2560,7 +2596,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	ONE("syscall",    S_IRUSR, proc_pid_syscall),
 #endif
-	ONE("cmdline",    S_IRUGO, proc_pid_cmdline),
+	ONE_SIZE("cmdline", S_IRUGO, proc_pid_cmdline),
 	ONE("stat",       S_IRUGO, proc_tgid_stat),
 	ONE("statm",      S_IRUGO, proc_pid_statm),
 	REG("maps",       S_IRUGO, proc_pid_maps_operations),
@@ -2906,7 +2942,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	ONE("syscall",   S_IRUSR, proc_pid_syscall),
 #endif
-	ONE("cmdline",   S_IRUGO, proc_pid_cmdline),
+	ONE_SIZE("cmdline", S_IRUGO, proc_pid_cmdline),
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_tid_maps_operations),
-- 
1.8.3.1

Re: [PATCH] fs/proc: allow larger /proc/<pid>/cmdline output

[Andrew Morton]

On Thu,  9 Apr 2015 23:59:02 -0400 Jarod Wilson <jarod@redhat.com> wrote:

> There are people who run java. Sometimes, when it misbehaves, they try to
> figure out what's going on by dumping /proc/<pid>/cmdline, but the length
> of that output is currently capped by PAGE_SIZE (so x86_64's 4k, in most
> cases), and sometimes, java command lines are longer than 4k characters.
> 
> This change allows the user to request a larger max length, up to 4x
> PAGE_SIZE, but the default out-of-the-box setting should keep things the
> same as they ever were. The 4x maximum is somewhat arbitrary, but seemed
> like it should be more than enough, because really, if you have more than
> 16k characters on your command line, you're probably doing it wrong...
> 
> I've tested this lightly with non-java shell commands with really long
> parameters, and things are perfectly stable after several hundred
> iterations of exercising things on a system booted with both
> proc_pid_maxlen=8192 and 16384. I wouldn't call my testing exhaustive,
> and I may not have considered something that will blow up horribly here,
> so comments and clues welcomed.
> 
> Using single_open_size() looked less messy than giving proc_pid_cmdline()
> its own .start op that would allow multiple buffers.
> 
> Note: I've only added this extended sizing for /proc/<pid>/cmdline output,
> rather than for all /proc/<pid>/foo elements, thinking that nothing else
> should ever really be that long, but anything that is can simply switch
> from using the ONE() macro to the ONE_SIZE() macro.

Why have an upper limit at all?

> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -134,6 +134,30 @@ struct pid_entry {
>  	NOD(NAME, (S_IFREG|(MODE)), 			\
>  		NULL, &proc_single_file_operations,	\
>  		{ .proc_show = show } )
> +#define ONE_SIZE(NAME, MODE, show)				\
> +	NOD(NAME, (S_IFREG|(MODE)), 				\
> +		NULL, &proc_single_file_size_operations,	\
> +		{ .proc_show = show } )
> +
> +/*
> + * Its hideous, but some java gunk winds up with a cmdline that is longer
> + * than PAGE_SIZE, and some people want to be able to see all of it for
> + * debugging purposes. Allocate at least PAGE_SIZE, and allow the user to
> + * ask for up to PAGE_SIZE << 2 (4x) to help with that situation.
> + */
> +static unsigned long proc_pid_maxlen = PAGE_SIZE;
> +static int set_proc_pid_maxlen(char *str)
> +{
> +	if (!str)
> +		return 0;
> +
> +	proc_pid_maxlen = simple_strtoul(str, &str, 0);
> +	proc_pid_maxlen = max(PAGE_SIZE, proc_pid_maxlen);
> +	proc_pid_maxlen = min(PAGE_SIZE << 2, proc_pid_maxlen);
> +
> +	return 1;
> +}
> +__setup("proc_pid_maxlen=", set_proc_pid_maxlen);

This permits 4k-16k on x86 and 64k-256k on powerpc.  This makes the
kernel interface inconsistent across architectures, which is not good -
some applications will work OK on one arch but will fail when moved to
a different arch.

s/PAGE_SIZE/4096/g would fix that.

Re: [PATCH] fs/proc: allow larger /proc/<pid>/cmdline output

[Jarod Wilson]

On Thu, Apr 09, 2015 at 09:12:00PM -0700, Andrew Morton wrote:
> On Thu,  9 Apr 2015 23:59:02 -0400 Jarod Wilson <jarod@redhat.com> wrote:
> 
> > There are people who run java. Sometimes, when it misbehaves, they try to
> > figure out what's going on by dumping /proc/<pid>/cmdline, but the length
> > of that output is currently capped by PAGE_SIZE (so x86_64's 4k, in most
> > cases), and sometimes, java command lines are longer than 4k characters.
> > 
> > This change allows the user to request a larger max length, up to 4x
> > PAGE_SIZE, but the default out-of-the-box setting should keep things the
> > same as they ever were. The 4x maximum is somewhat arbitrary, but seemed
> > like it should be more than enough, because really, if you have more than
> > 16k characters on your command line, you're probably doing it wrong...
> > 
> > I've tested this lightly with non-java shell commands with really long
> > parameters, and things are perfectly stable after several hundred
> > iterations of exercising things on a system booted with both
> > proc_pid_maxlen=8192 and 16384. I wouldn't call my testing exhaustive,
> > and I may not have considered something that will blow up horribly here,
> > so comments and clues welcomed.
> > 
> > Using single_open_size() looked less messy than giving proc_pid_cmdline()
> > its own .start op that would allow multiple buffers.
> > 
> > Note: I've only added this extended sizing for /proc/<pid>/cmdline output,
> > rather than for all /proc/<pid>/foo elements, thinking that nothing else
> > should ever really be that long, but anything that is can simply switch
> > from using the ONE() macro to the ONE_SIZE() macro.
> 
> Why have an upper limit at all?

Just trying to be conservative and keep people from doing anything too
insane, but I didn't really have any particularly good reason beyond that
for capping it. I'll remove the upper bound next iteration.

> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -134,6 +134,30 @@ struct pid_entry {
> >  	NOD(NAME, (S_IFREG|(MODE)), 			\
> >  		NULL, &proc_single_file_operations,	\
> >  		{ .proc_show = show } )
> > +#define ONE_SIZE(NAME, MODE, show)				\
> > +	NOD(NAME, (S_IFREG|(MODE)), 				\
> > +		NULL, &proc_single_file_size_operations,	\
> > +		{ .proc_show = show } )
> > +
> > +/*
> > + * Its hideous, but some java gunk winds up with a cmdline that is longer
> > + * than PAGE_SIZE, and some people want to be able to see all of it for
> > + * debugging purposes. Allocate at least PAGE_SIZE, and allow the user to
> > + * ask for up to PAGE_SIZE << 2 (4x) to help with that situation.
> > + */
> > +static unsigned long proc_pid_maxlen = PAGE_SIZE;
> > +static int set_proc_pid_maxlen(char *str)
> > +{
> > +	if (!str)
> > +		return 0;
> > +
> > +	proc_pid_maxlen = simple_strtoul(str, &str, 0);
> > +	proc_pid_maxlen = max(PAGE_SIZE, proc_pid_maxlen);
> > +	proc_pid_maxlen = min(PAGE_SIZE << 2, proc_pid_maxlen);
> > +
> > +	return 1;
> > +}
> > +__setup("proc_pid_maxlen=", set_proc_pid_maxlen);
> 
> This permits 4k-16k on x86 and 64k-256k on powerpc.  This makes the
> kernel interface inconsistent across architectures, which is not good -
> some applications will work OK on one arch but will fail when moved to
> a different arch.
> 
> s/PAGE_SIZE/4096/g would fix that.

I was going for minimal disturbance to the status quo, which is that
everything is structured around PAGE_SIZE. proc_pid_cmdline() has a
BUG_ON(m->size < PAGE_SIZE) in it and the default initial allocation for
every seq_file using single_open() is PAGE_SIZE, making it a bit more
invasive to change it. Perhaps add a default size #define to seq_file.h
and use that for seq_file everywhere instead of PAGE_SIZE to get
consistency across all arches?

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] fs/proc: allow larger /proc/<pid>/cmdline output

[Alexey Dobriyan]

On Fri, Apr 10, 2015 at 08:18:38AM -0400, Jarod Wilson wrote:
> On Thu, Apr 09, 2015 at 09:12:00PM -0700, Andrew Morton wrote:
> > On Thu,  9 Apr 2015 23:59:02 -0400 Jarod Wilson <jarod@redhat.com> wrote:
> > 
> > > There are people who run java. Sometimes, when it misbehaves, they try to
> > > figure out what's going on by dumping /proc/<pid>/cmdline, but the length
> > > of that output is currently capped by PAGE_SIZE (so x86_64's 4k, in most
> > > cases), and sometimes, java command lines are longer than 4k characters.
> > > 
> > > This change allows the user to request a larger max length, up to 4x
> > > PAGE_SIZE, but the default out-of-the-box setting should keep things the
> > > same as they ever were. The 4x maximum is somewhat arbitrary, but seemed
> > > like it should be more than enough, because really, if you have more than
> > > 16k characters on your command line, you're probably doing it wrong...
> > > 
> > > I've tested this lightly with non-java shell commands with really long
> > > parameters, and things are perfectly stable after several hundred
> > > iterations of exercising things on a system booted with both
> > > proc_pid_maxlen=8192 and 16384. I wouldn't call my testing exhaustive,
> > > and I may not have considered something that will blow up horribly here,
> > > so comments and clues welcomed.
> > > 
> > > Using single_open_size() looked less messy than giving proc_pid_cmdline()
> > > its own .start op that would allow multiple buffers.
> > > 
> > > Note: I've only added this extended sizing for /proc/<pid>/cmdline output,
> > > rather than for all /proc/<pid>/foo elements, thinking that nothing else
> > > should ever really be that long, but anything that is can simply switch
> > > from using the ONE() macro to the ONE_SIZE() macro.
> > 
> > Why have an upper limit at all?
> 
> Just trying to be conservative and keep people from doing anything too
> insane, but I didn't really have any particularly good reason beyond that
> for capping it. I'll remove the upper bound next iteration.

There is no need for next iteration. Andrew has been ignoring real fix for more
than a month now!

	Alexey

[PATCH try #3] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline

[Alexey Dobriyan]

/proc/$PID/cmdline truncates output at PAGE_SIZE. It is easy to see with

	$ cat /proc/self/cmdline $(seq 1037) 2>/dev/null

However, command line size was never limited to PAGE_SIZE but to 128 KB and
relatively recently limitation was removed altogether.

People noticed and are asking questions:
http://stackoverflow.com/questions/199130/how-do-i-increase-the-proc-pid-cmdline-4096-byte-limit

seq file interface is not OK, because it kmalloc's for whole output and
open + read(, 1) + sleep will pin arbitrary amounts of kernel memory.
To not do that, limit must be imposed which is incompatible with
arbitrary sized command lines.

I apologize for hairy code, but this it direct consequence of command line
layout in memory and hacks to support things like "init [3]".

The loops are "unrolled" otherwise it is either macros which hide
control flow or functions with 7-8 arguments with equal line count.

There should be real setproctitle(2) or something.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/base.c |  203 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 194 insertions(+), 9 deletions(-)

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -196,18 +196,203 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
-static int proc_pid_cmdline(struct seq_file *m, struct pid_namespace *ns,
-			    struct pid *pid, struct task_struct *task)
+static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
+				     size_t count, loff_t *pos)
 {
+	struct task_struct *tsk;
+	struct mm_struct *mm;
+	char *page;
+	unsigned long arg_start, arg_end, env_start, env_end;
+	unsigned long len1, len2, len;
+	unsigned long p;
+	char c;
+	ssize_t rv;
+
+	BUG_ON(*pos < 0);
+
+	tsk = get_proc_task(file_inode(file));
+	if (!tsk)
+		return -ESRCH;
+	mm = get_task_mm(tsk);
+	put_task_struct(tsk);
+	if (!mm)
+		return 0;
+	if (!mm->arg_end) {
+		rv = 0;
+		goto out_mmput;
+	}
+
+	page = (char *)__get_free_page(GFP_TEMPORARY);
+	if (!page) {
+		rv = -ENOMEM;
+		goto out_mmput;
+	}
+
+	down_read(&mm->mmap_sem);
+	arg_start = mm->arg_start;
+	arg_end = mm->arg_end;
+	env_start = mm->env_start;
+	env_end = mm->env_end;
+	up_read(&mm->mmap_sem);
+
+	BUG_ON(arg_start > arg_end);
+	BUG_ON(env_start > env_end);
+
+	len1 = arg_end - arg_start;
+	len2 = env_end - env_start;
+
 	/*
-	 * Rely on struct seq_operations::show() being called once
-	 * per internal buffer allocation. See single_open(), traverse().
+	 * Inherently racy -- command line shares address space
+	 * with code and data.
 	 */
-	BUG_ON(m->size < PAGE_SIZE);
-	m->count += get_cmdline(task, m->buf, PAGE_SIZE);
-	return 0;
+	rv = access_remote_vm(mm, arg_end - 1, &c, 1, 0);
+	if (rv <= 0)
+		goto out_free_page;
+
+	rv = 0;
+
+	if (c == '\0') {
+		/* Command line (set of strings) occupies whole ARGV. */
+		if (len1 <= *pos)
+			goto out_free_page;
+
+		p = arg_start + *pos;
+		len = len1 - *pos;
+		while (count > 0 && len > 0) {
+			unsigned int _count;
+			int nr_read;
+
+			_count = min3(count, len, PAGE_SIZE);
+			nr_read = access_remote_vm(mm, p, page, _count, 0);
+			if (nr_read < 0)
+				rv = nr_read;
+			if (nr_read <= 0)
+				goto out_free_page;
+
+			if (copy_to_user(buf, page, nr_read)) {
+				rv = -EFAULT;
+				goto out_free_page;
+			}
+
+			p	+= nr_read;
+			len	-= nr_read;
+			buf	+= nr_read;
+			count	-= nr_read;
+			rv	+= nr_read;
+		}
+	} else {
+		/*
+		 * Command line (1 string) occupies ARGV and maybe
+		 * extends into ENVP.
+		 */
+		if (len1 + len2 <= *pos)
+			goto skip_argv_envp;
+		if (len1 <= *pos)
+			goto skip_argv;
+
+		p = arg_start + *pos;
+		len = len1 - *pos;
+		while (count > 0 && len > 0) {
+			unsigned int _count, l;
+			int nr_read;
+			bool final;
+
+			_count = min3(count, len, PAGE_SIZE);
+			nr_read = access_remote_vm(mm, p, page, _count, 0);
+			if (nr_read < 0)
+				rv = nr_read;
+			if (nr_read <= 0)
+				goto out_free_page;
+
+			/*
+			 * Command line can be shorter than whole ARGV
+			 * even if last "marker" byte says it is not.
+			 */
+			final = false;
+			l = strnlen(page, nr_read);
+			if (l < nr_read) {
+				nr_read = l;
+				final = true;
+			}
+
+			if (copy_to_user(buf, page, nr_read)) {
+				rv = -EFAULT;
+				goto out_free_page;
+			}
+
+			p	+= nr_read;
+			len	-= nr_read;
+			buf	+= nr_read;
+			count	-= nr_read;
+			rv	+= nr_read;
+
+			if (final)
+				goto out_free_page;
+		}
+skip_argv:
+		/*
+		 * Command line (1 string) occupies ARGV and
+		 * extends into ENVP.
+		 */
+		if (len1 <= *pos) {
+			p = env_start + *pos - len1;
+			len = len1 + len2 - *pos;
+		} else {
+			p = env_start;
+			len = len2;
+		}
+		while (count > 0 && len > 0) {
+			unsigned int _count, l;
+			int nr_read;
+			bool final;
+
+			_count = min3(count, len, PAGE_SIZE);
+			nr_read = access_remote_vm(mm, p, page, _count, 0);
+			if (nr_read < 0)
+				rv = nr_read;
+			if (nr_read <= 0)
+				goto out_free_page;
+
+			/* Find EOS. */
+			final = false;
+			l = strnlen(page, nr_read);
+			if (l < nr_read) {
+				nr_read = l;
+				final = true;
+			}
+
+			if (copy_to_user(buf, page, nr_read)) {
+				rv = -EFAULT;
+				goto out_free_page;
+			}
+
+			p	+= nr_read;
+			len	-= nr_read;
+			buf	+= nr_read;
+			count	-= nr_read;
+			rv	+= nr_read;
+
+			if (final)
+				goto out_free_page;
+		}
+skip_argv_envp:
+		;
+	}
+
+out_free_page:
+	free_page((unsigned long)page);
+out_mmput:
+	mmput(mm);
+	if (rv > 0)
+		*pos += rv;
+	return rv;
 }
 
+static const struct file_operations proc_pid_cmdline_ops = {
+	.read	= proc_pid_cmdline_read,
+	.llseek	= generic_file_llseek,
+};
+
 static int proc_pid_auxv(struct seq_file *m, struct pid_namespace *ns,
 			 struct pid *pid, struct task_struct *task)
 {
@@ -2560,7 +2745,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	ONE("syscall",    S_IRUSR, proc_pid_syscall),
 #endif
-	ONE("cmdline",    S_IRUGO, proc_pid_cmdline),
+	REG("cmdline",    S_IRUGO, proc_pid_cmdline_ops),
 	ONE("stat",       S_IRUGO, proc_tgid_stat),
 	ONE("statm",      S_IRUGO, proc_pid_statm),
 	REG("maps",       S_IRUGO, proc_pid_maps_operations),
@@ -2906,7 +3091,7 @@ static const struct pid_entry tid_base_stuff[] = {
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	ONE("syscall",   S_IRUSR, proc_pid_syscall),
 #endif
-	ONE("cmdline",   S_IRUGO, proc_pid_cmdline),
+	REG("cmdline",   S_IRUGO, proc_pid_cmdline_ops),
 	ONE("stat",      S_IRUGO, proc_tid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_tid_maps_operations),

Re: [PATCH try #3] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline

[Jarod Wilson]

On Fri, Apr 10, 2015 at 05:13:29PM +0300, Alexey Dobriyan wrote:
> /proc/$PID/cmdline truncates output at PAGE_SIZE. It is easy to see with
> 
> 	$ cat /proc/self/cmdline $(seq 1037) 2>/dev/null
> 
> However, command line size was never limited to PAGE_SIZE but to 128 KB and
> relatively recently limitation was removed altogether.
> 
> People noticed and are asking questions:
> http://stackoverflow.com/questions/199130/how-do-i-increase-the-proc-pid-cmdline-4096-byte-limit
> 
> seq file interface is not OK, because it kmalloc's for whole output and
> open + read(, 1) + sleep will pin arbitrary amounts of kernel memory.
> To not do that, limit must be imposed which is incompatible with
> arbitrary sized command lines.
> 
> I apologize for hairy code, but this it direct consequence of command line
> layout in memory and hacks to support things like "init [3]".
> 
> The loops are "unrolled" otherwise it is either macros which hide
> control flow or functions with 7-8 arguments with equal line count.

That definitely qualifies as hairy. How big of a problem is it really in
practice if we continued using seq_file though? This only happens when
someone actually accesses /proc/$PID/cmdline, no? And if they're doing
that, they probably want that info, so is it so terrible if memory is held
on to for a bit? We're only talking about a few kB. That said, properly
walking the entire cmdline without having to specify an arbitrary limit
ahead of time does sound slightly more end-user-friendly. I'll give this
patch a spin here.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] fs/proc: allow larger /proc/<pid>/cmdline output

[Andrew Morton]

On Fri, 10 Apr 2015 17:11:40 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> > Just trying to be conservative and keep people from doing anything too
> > insane, but I didn't really have any particularly good reason beyond that
> > for capping it. I'll remove the upper bound next iteration.
> 
> There is no need for next iteration. Andrew has been ignoring real fix for more
> than a month now!

I haven't been ignoring it.  I'm desperately hoping that the patch fairy
will come up with something which is less large and complex :(

Re: [PATCH try #3] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline

[Alexey Dobriyan]

On Fri, Apr 10, 2015 at 02:01:32PM -0400, Jarod Wilson wrote:
> On Fri, Apr 10, 2015 at 05:13:29PM +0300, Alexey Dobriyan wrote:
> > /proc/$PID/cmdline truncates output at PAGE_SIZE. It is easy to see with
> > 
> > 	$ cat /proc/self/cmdline $(seq 1037) 2>/dev/null
> > 
> > However, command line size was never limited to PAGE_SIZE but to 128 KB and
> > relatively recently limitation was removed altogether.
> > 
> > People noticed and are asking questions:
> > http://stackoverflow.com/questions/199130/how-do-i-increase-the-proc-pid-cmdline-4096-byte-limit
> > 
> > seq file interface is not OK, because it kmalloc's for whole output and
> > open + read(, 1) + sleep will pin arbitrary amounts of kernel memory.
> > To not do that, limit must be imposed which is incompatible with
> > arbitrary sized command lines.
> > 
> > I apologize for hairy code, but this it direct consequence of command line
> > layout in memory and hacks to support things like "init [3]".
> > 
> > The loops are "unrolled" otherwise it is either macros which hide
> > control flow or functions with 7-8 arguments with equal line count.
> 
> That definitely qualifies as hairy. How big of a problem is it really in
> practice if we continued using seq_file though? This only happens when
> someone actually accesses /proc/$PID/cmdline, no? And if they're doing
> that, they probably want that info, so is it so terrible if memory is held
> on to for a bit? We're only talking about a few kB. That said, properly
> walking the entire cmdline without having to specify an arbitrary limit
> ahead of time does sound slightly more end-user-friendly. I'll give this
> patch a spin here.

Well, it's 8 MB at least because of kmalloc and more when it starts
to vmalloc, so either you increase but keep the limit, or allow
to pin semi-arbitrary amount of kernel memory IF you want to stay
with seqfile. My patch requires just 1 page plus whatever g_u_p
requires.

Re: [PATCH] fs/proc: allow larger /proc/<pid>/cmdline output

[Jarod Wilson]

On Fri, Apr 10, 2015 at 01:45:20PM -0700, Andrew Morton wrote:
> On Fri, 10 Apr 2015 17:11:40 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > > Just trying to be conservative and keep people from doing anything too
> > > insane, but I didn't really have any particularly good reason beyond that
> > > for capping it. I'll remove the upper bound next iteration.
> > 
> > There is no need for next iteration. Andrew has been ignoring real fix for more
> > than a month now!
> 
> I haven't been ignoring it.  I'm desperately hoping that the patch fairy
> will come up with something which is less large and complex :(

Shudder-inducing though it may be, I do have to say I'm quite happy with
the resulting behavior with Alexey's patch. It's both more
memory-efficient and more user-friendly, despite itself.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH try #3] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline

[Jarod Wilson]

On Sat, Apr 11, 2015 at 01:09:06AM +0300, Alexey Dobriyan wrote:
> On Fri, Apr 10, 2015 at 02:01:32PM -0400, Jarod Wilson wrote:
> > On Fri, Apr 10, 2015 at 05:13:29PM +0300, Alexey Dobriyan wrote:
> > > /proc/$PID/cmdline truncates output at PAGE_SIZE. It is easy to see with
> > > 
> > > 	$ cat /proc/self/cmdline $(seq 1037) 2>/dev/null
> > > 
> > > However, command line size was never limited to PAGE_SIZE but to 128 KB and
> > > relatively recently limitation was removed altogether.
> > > 
> > > People noticed and are asking questions:
> > > http://stackoverflow.com/questions/199130/how-do-i-increase-the-proc-pid-cmdline-4096-byte-limit
> > > 
> > > seq file interface is not OK, because it kmalloc's for whole output and
> > > open + read(, 1) + sleep will pin arbitrary amounts of kernel memory.
> > > To not do that, limit must be imposed which is incompatible with
> > > arbitrary sized command lines.
> > > 
> > > I apologize for hairy code, but this it direct consequence of command line
> > > layout in memory and hacks to support things like "init [3]".
> > > 
> > > The loops are "unrolled" otherwise it is either macros which hide
> > > control flow or functions with 7-8 arguments with equal line count.
> > 
> > That definitely qualifies as hairy. How big of a problem is it really in
> > practice if we continued using seq_file though? This only happens when
> > someone actually accesses /proc/$PID/cmdline, no? And if they're doing
> > that, they probably want that info, so is it so terrible if memory is held
> > on to for a bit? We're only talking about a few kB. That said, properly
> > walking the entire cmdline without having to specify an arbitrary limit
> > ahead of time does sound slightly more end-user-friendly. I'll give this
> > patch a spin here.
> 
> Well, it's 8 MB at least because of kmalloc and more when it starts
> to vmalloc, so either you increase but keep the limit, or allow
> to pin semi-arbitrary amount of kernel memory IF you want to stay
> with seqfile. My patch requires just 1 page plus whatever g_u_p
> requires.

Okay, I've tested this out some. Its definitely more user-friendly than
having to require a boot param, and as a bonus, its even more
memory-efficient. Yes, its a bit fugly, but such is life sometimes...

Though I do wonder if this should perhaps be a helper in mm/util.c like
get_cmdline, maybe replacing get_cmdline or adding an alternative that
gives you everything, rather than an arbitrarily limited length. I only
see one other place actually using get_cmdline so far.

Tested-by: Jarod Wilson <jarod@redhat.com>
Acked-by: Jarod Wilson <jarod@redhat.com>

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH try #3] proc: fix PAGE_SIZE limit of /proc/$PID/cmdline

[Andrew Morton]

On Mon, 13 Apr 2015 14:28:49 -0400 Jarod Wilson <jarod@redhat.com> wrote:

> > Well, it's 8 MB at least because of kmalloc and more when it starts
> > to vmalloc, so either you increase but keep the limit, or allow
> > to pin semi-arbitrary amount of kernel memory IF you want to stay
> > with seqfile. My patch requires just 1 page plus whatever g_u_p
> > requires.
> 
> Okay, I've tested this out some. Its definitely more user-friendly than
> having to require a boot param, and as a bonus, its even more
> memory-efficient. Yes, its a bit fugly, but such is life sometimes...
> 
> Though I do wonder if this should perhaps be a helper in mm/util.c like
> get_cmdline, maybe replacing get_cmdline or adding an alternative that
> gives you everything, rather than an arbitrarily limited length. I only
> see one other place actually using get_cmdline so far.
> 
> Tested-by: Jarod Wilson <jarod@redhat.com>
> Acked-by: Jarod Wilson <jarod@redhat.com>

Thanks, I'll take another look after the 4.0 merge flurry.