/proc/ioports overrun patch

/proc/ioports overrun patch

[Marcelo Tosatti]

Hello Olof,

Let me make sure I understood the patch right:

Your change to do_resource_list() will avoid copying out of bound by
truncating the resource output. Which means users might get truncated
information (only information that fits in the buffer) and not the full
information.

Is that correct?

If so, I would prefer to have a fix which outputs the full resource
information. For that we would need seq_file().

Please correct me if my reading of the code is wrong.

Thanks

Re: /proc/ioports overrun patch

[john stultz]

On Fri, 2003-08-29 at 06:30, Marcelo Tosatti wrote:

> If so, I would prefer to have a fix which outputs the full resource
> information. For that we would need seq_file().

I have a patch that converts /proc/interrupts to use seq_file as well,
however it would be much cleaner if Randy's backport of the "single"
interface went in first  

http://www.ussg.iu.edu/hypermail/linux/kernel/0308.3/0296.html

Are you planning on taking that patch? Or should I just resend my patch
w/o it?

thanks
-john

[PATCH] [2.4] Re: /proc/ioports overrun patch

[olof]

Marcelo,

On Fri, 29 Aug 2003, Marcelo Tosatti wrote:

> Your change to do_resource_list() will avoid copying out of bound by
> truncating the resource output. Which means users might get truncated
> information (only information that fits in the buffer) and not the full
> information.
>
> Is that correct?
>
> If so, I would prefer to have a fix which outputs the full resource
> information. For that we would need seq_file().

I finally got some time to revisit this and convert /proc/ioports and
/proc/iomem to seq_file. See below patch -- it's backed against the
current BK tree.


Thanks,

Olof


Olof Johansson                                        Office: 4E002/905
pSeries Linux Development                             IBM Systems Group
Email: olof@austin.ibm.com                          Phone: 512-838-9858
All opinions are my own and not those of IBM



===== fs/proc/proc_misc.c 1.23 vs edited =====
--- 1.23/fs/proc/proc_misc.c	Fri Sep 12 08:44:05 2003
+++ edited/fs/proc/proc_misc.c	Wed Sep 24 12:35:00 2003
@@ -430,6 +430,53 @@
 };
 #endif /* !CONFIG_X86 */

+extern int show_ioports_resources(struct seq_file *p, void *v);
+extern int show_iomem_resources(struct seq_file *p, void *v);
+
+static int io_open(struct inode *inode, struct file *file,
+                   int (*f)(struct seq_file *, void*))
+{
+	unsigned size = PAGE_SIZE * (1 + smp_num_cpus / 8);
+	char *buf = kmalloc(size, GFP_KERNEL);
+	struct seq_file *m;
+	int res;
+
+	if (!buf)
+		return -ENOMEM;
+	res = single_open(file, f, NULL);
+	if (!res) {
+		m = file->private_data;
+		m->buf = buf;
+		m->size = size;
+	} else
+		kfree(buf);
+	return res;
+}
+
+static int ioports_open(struct inode *inode, struct file *file)
+{
+	return io_open(inode, file, show_ioports_resources);
+}
+
+static int iomem_open(struct inode *inode, struct file *file)
+{
+	return io_open(inode, file, show_iomem_resources);
+}
+
+static struct file_operations proc_ioports_operations = {
+	.open		= ioports_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static struct file_operations proc_iomem_operations = {
+	.open		= iomem_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static int filesystems_read_proc(char *page, char **start, off_t off,
 				 int count, int *eof, void *data)
 {
@@ -444,13 +491,6 @@
 	return proc_calc_metrics(page, start, off, count, eof, len);
 }

-static int ioports_read_proc(char *page, char **start, off_t off,
-				 int count, int *eof, void *data)
-{
-	int len = get_ioport_list(page);
-	return proc_calc_metrics(page, start, off, count, eof, len);
-}
-
 static int cmdline_read_proc(char *page, char **start, off_t off,
 				 int count, int *eof, void *data)
 {
@@ -495,13 +535,6 @@
 	return proc_calc_metrics(page, start, off, count, eof, len);
 }

-static int memory_read_proc(char *page, char **start, off_t off,
-				 int count, int *eof, void *data)
-{
-	int len = get_mem_list(page);
-	return proc_calc_metrics(page, start, off, count, eof, len);
-}
-
 /*
  * This function accesses profiling information. The returned data is
  * binary: the sampling step and the actual contents of the profile
@@ -625,14 +658,12 @@
 #endif
 		{"filesystems",	filesystems_read_proc},
 		{"dma",		dma_read_proc},
-		{"ioports",	ioports_read_proc},
 		{"cmdline",	cmdline_read_proc},
 #ifdef CONFIG_SGI_DS1286
 		{"rtc",		ds1286_read_proc},
 #endif
 		{"locks",	locks_read_proc},
 		{"swaps",	swaps_read_proc},
-		{"iomem",	memory_read_proc},
 		{"execdomains",	execdomains_read_proc},
 		{NULL,}
 	};
@@ -649,6 +680,8 @@
 #if defined(CONFIG_X86)
 	create_seq_entry("interrupts", 0, &proc_interrupts_operations);
 #endif
+	create_seq_entry("ioports", 0, &proc_ioports_operations);
+	create_seq_entry("iomem", 0, &proc_iomem_operations);
 	create_seq_entry("partitions", 0, &proc_partitions_operations);
 	create_seq_entry("slabinfo",S_IWUSR|S_IRUGO,&proc_slabinfo_operations);
 #ifdef CONFIG_MODULES
===== include/linux/ioport.h 1.2 vs edited =====
--- 1.2/include/linux/ioport.h	Tue Sep 17 09:08:45 2002
+++ edited/include/linux/ioport.h	Wed Sep 24 12:38:30 2003
@@ -79,11 +79,16 @@
 #define IORESOURCE_MEM_SHADOWABLE	(1<<5)	/* dup: IORESOURCE_SHADOWABLE */
 #define IORESOURCE_MEM_EXPANSIONROM	(1<<6)

+/* Just define it here instead of adding an include of seq_file. */
+
+struct seq_file;
+
 /* PC/ISA/whatever - the normal PC address spaces: IO and memory */
 extern struct resource ioport_resource;
 extern struct resource iomem_resource;

-extern int get_resource_list(struct resource *, char *buf, int size);
+extern int show_ioports_resources(struct seq_file *p, void *data);
+extern int show_iomem_resources(struct seq_file *p, void *data);

 extern int check_resource(struct resource *root, unsigned long, unsigned long);
 extern int request_resource(struct resource *root, struct resource *new);
@@ -110,9 +115,6 @@

 extern int __check_region(struct resource *, unsigned long, unsigned long);
 extern void __release_region(struct resource *, unsigned long, unsigned long);
-
-#define get_ioport_list(buf)	get_resource_list(&ioport_resource, buf, PAGE_SIZE)
-#define get_mem_list(buf)	get_resource_list(&iomem_resource, buf, PAGE_SIZE)

 #define HAVE_AUTOIRQ
 extern void autoirq_setup(int waittime);
===== kernel/resource.c 1.4 vs edited =====
--- 1.4/kernel/resource.c	Tue Sep 17 09:08:45 2002
+++ edited/kernel/resource.c	Wed Sep 24 12:41:32 2003
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/seq_file.h>
 #include <asm/io.h>

 struct resource ioport_resource = { "PCI IO", 0x0000, IO_SPACE_LIMIT, IORESOURCE_IO };
@@ -23,7 +24,7 @@
 /*
  * This generates reports for /proc/ioports and /proc/iomem
  */
-static char * do_resource_list(struct resource *entry, const char *fmt, int offset, char *buf, char *end)
+static int do_resource_list(struct resource *entry, const char *fmt, int offset, struct seq_file *p)
 {
 	if (offset < 0)
 		offset = 0;
@@ -32,24 +33,21 @@
 		const char *name = entry->name;
 		unsigned long from, to;

-		if ((int) (end-buf) < 80)
-			return buf;
-
 		from = entry->start;
 		to = entry->end;
 		if (!name)
 			name = "<BAD>";

-		buf += sprintf(buf, fmt + offset, from, to, name);
+		seq_printf(p, fmt + offset, from, to, name);
 		if (entry->child)
-			buf = do_resource_list(entry->child, fmt, offset-2, buf, end);
+			do_resource_list(entry->child, fmt, offset-2, p);
 		entry = entry->sibling;
 	}

-	return buf;
+	return 0;
 }

-int get_resource_list(struct resource *root, char *buf, int size)
+static int show_io_resources(struct seq_file *p, struct resource *root)
 {
 	char *fmt;
 	int retval;
@@ -58,10 +56,20 @@
 	if (root->end < 0x10000)
 		fmt = "        %04lx-%04lx : %s\n";
 	read_lock(&resource_lock);
-	retval = do_resource_list(root->child, fmt, 8, buf, buf + size) - buf;
+	retval = do_resource_list(root->child, fmt, 8, p);
 	read_unlock(&resource_lock);
 	return retval;
 }
+
+int show_ioports_resources(struct seq_file *p, void *v)
+{
+	return show_io_resources(p, &ioport_resource);
+}
+
+int show_iomem_resources(struct seq_file *p, void *v)
+{
+	return show_io_resources(p, &iomem_resource);
+}

 /* Return the conflict entry if you can't request it */
 static struct resource * __request_resource(struct resource *root, struct resource *new)

Re: [PATCH] [2.4] Re: /proc/ioports overrun patch

[viro]

On Wed, Sep 24, 2003 at 02:42:44PM -0500, olof@austin.ibm.com wrote:
> Marcelo,
> 
> On Fri, 29 Aug 2003, Marcelo Tosatti wrote:
> 
> > Your change to do_resource_list() will avoid copying out of bound by
> > truncating the resource output. Which means users might get truncated
> > information (only information that fits in the buffer) and not the full
> > information.
> >
> > Is that correct?
> >
> > If so, I would prefer to have a fix which outputs the full resource
> > information. For that we would need seq_file().
> 
> I finally got some time to revisit this and convert /proc/ioports and
> /proc/iomem to seq_file. See below patch -- it's backed against the
> current BK tree.

Hmm...  Why not make the iterator traverse the resource tree instead?
After all, all it takes is addition of pointer to parent resource into
struct resource.  Goes for both 2.4 and 2.6...

Re: [PATCH] [2.4] Re: /proc/ioports overrun patch

[viro]

On Wed, Sep 24, 2003 at 08:51:33PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
 
> Hmm...  Why not make the iterator traverse the resource tree instead?
> After all, all it takes is addition of pointer to parent resource into
> struct resource.  Goes for both 2.4 and 2.6...

Hey - it's already there.  That makes life very easy - ->next() should
do the following:
	if (resource->child)
		return resource->child;
	while (!resource->sibling) {
		resource = resource->parent;
		if (!resource)
			return NULL;
	}
	return resource->sibling;

AFAICS that should be it - walks the tree in right order.  Depth can be
trivially found by ->show(), so there's no problems either...

Re: [PATCH] [2.4] Re: /proc/ioports overrun patch

[viro]

> Hey - it's already there.  That makes life very easy - ->next() should
> do the following:
> 	if (resource->child)
> 		return resource->child;
> 	while (!resource->sibling) {
> 		resource = resource->parent;
> 		if (!resource)
> 			return NULL;
> 	}
> 	return resource->sibling;
> 
> AFAICS that should be it - walks the tree in right order.  Depth can be
> trivially found by ->show(), so there's no problems either...

OK, here's the 2.6 variant; it should also apply on top of 2.4 backport,
AFAICS.  Works here...

It replaces iterator of /proc/io{ports,mem} with normal tree traversal and
cleans the thing up a bit.

diff -urN B5-09241809/kernel/resource.c B5-current/kernel/resource.c
--- B5-09241809/kernel/resource.c	Sat Aug  9 02:21:03 2003
+++ B5-current/kernel/resource.c	Wed Sep 24 17:28:22 2003
@@ -38,75 +38,91 @@
 
 #ifdef CONFIG_PROC_FS
 
-#define MAX_IORES_LEVEL		5
+enum { MAX_IORES_LEVEL = 5 };
 
-/*
- * do_resource_list():
- * for reports of /proc/ioports and /proc/iomem;
- * do current entry, then children, then siblings;
- */
-static int do_resource_list(struct seq_file *m, struct resource *res, const char *fmt, int level)
-{
-	while (res) {
-		const char *name;
-
-		name = res->name ? res->name : "<BAD>";
-		if (level > MAX_IORES_LEVEL)
-			level = MAX_IORES_LEVEL;
-		seq_printf (m, fmt + 2 * MAX_IORES_LEVEL - 2 * level,
-				res->start, res->end, name);
-
-		if (res->child)
-			do_resource_list(m, res->child, fmt, level + 1);
-
-		res = res->sibling;
-	}
-
-	return 0;
+static void *r_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct resource *p = v;
+	(*pos)++;
+	if (p->child)
+		return p->child;
+	while (!p->sibling && p->parent)
+		p = p->parent;
+	return p->sibling;
 }
 
-static int ioresources_show(struct seq_file *m, void *v)
+static void *r_start(struct seq_file *m, loff_t *pos)
 {
-	struct resource *root = m->private;
-	char *fmt;
-	int retval;
-
-	fmt = root->end < 0x10000
-		? "          %04lx-%04lx : %s\n"
-		: "          %08lx-%08lx : %s\n";
+	struct resource *p = m->private;
+	loff_t l = 0;
 	read_lock(&resource_lock);
-	retval = do_resource_list(m, root->child, fmt, 0);
+	for (p = p->child; p && l < *pos; p = r_next(m, p, &l))
+		;
+	return p;
+}
+
+static void r_stop(struct seq_file *m, void *v)
+{
 	read_unlock(&resource_lock);
-	return retval;
 }
 
-static int ioresources_open(struct file *file, struct resource *root)
+static int r_show(struct seq_file *m, void *v)
 {
-	return single_open(file, ioresources_show, root);
+	struct resource *root = m->private;
+	struct resource *r = v, *p;
+	int width = root->end < 0x10000 ? 4 : 8;
+	int depth;
+
+	for (depth = 0, p = r; depth < MAX_IORES_LEVEL; depth++, p = p->parent)
+		if (p->parent == root)
+			break;
+	seq_printf(m, "%*s%0*lx-%0*lx : %s\n",
+			depth * 2, "",
+			width, r->start,
+			width, r->end,
+			r->name ? r->name : "<BAD>");
+	return 0;
 }
 
+struct seq_operations resource_op = {
+	.start	= r_start,
+	.next	= r_next,
+	.stop	= r_stop,
+	.show	= r_show,
+};
+
 static int ioports_open(struct inode *inode, struct file *file)
 {
-	return ioresources_open(file, &ioport_resource);
+	int res = seq_open(file, &resource_op);
+	if (!res) {
+		struct seq_file *m = file->private_data;
+		m->private = &ioport_resource;
+	}
+	return res;
+}
+
+static int iomem_open(struct inode *inode, struct file *file)
+{
+	int res = seq_open(file, &resource_op);
+	if (!res) {
+		struct seq_file *m = file->private_data;
+		m->private = &iomem_resource;
+	}
+	return res;
 }
 
 static struct file_operations proc_ioports_operations = {
 	.open		= ioports_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= single_release,
+	.release	= seq_release,
 };
 
-static int iomem_open(struct inode *inode, struct file *file)
-{
-	return ioresources_open(file, &iomem_resource);
-}
-
 static struct file_operations proc_iomem_operations = {
 	.open		= iomem_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= single_release,
+	.release	= seq_release,
 };
 
 static int __init ioresources_init(void)

Re: [PATCH] [2.4] Re: /proc/ioports overrun patch

[viro]

	Umm...  Linus, output truncation is 2.4 problem; any seq_file-based
variant (including one already in 2.6 and being backported to 2.4) solves
that.  The thing being, variant we had in 2.6 was ugly - it had walk through
the tree shoved into ->show() instead of having the iterator do that for
us.  And that's what this patch fixed - it's not that old 2.6 variant would
break, it's that it was done in wrong way.  IOW, changeset comment is
misleading...

Re: [PATCH] [2.4] Re: /proc/ioports overrun patch

[Linus Torvalds]

On Wed, 24 Sep 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:
>
> 	Umm...  Linus, output truncation is 2.4 problem; any seq_file-based
> variant (including one already in 2.6 and being backported to 2.4) solves
> that.  The thing being, variant we had in 2.6 was ugly - it had walk through
> the tree shoved into ->show() instead of having the iterator do that for
> us.

Yeah, my bad for trying to clean up comments. I just looked at the
seq_printf() usage without error checking, without thinking about the loop 
and checking in the outer layer (ie the seq_read() loop).

Me bad.

You can't undo something in BK (once it is out), but feel free to send me
a patch that adds the appropriate comments, and calls me a pinhead for the
changelog ;)

		Linus

Re: /proc/ioports overrun patch

[Marcelo Tosatti]

> If so, I would prefer to have a fix which outputs the full resource
> information. For that we would need seq_file().

> I have a patch that converts /proc/interrupts to use seq_file as well,
> however it would be much cleaner if Randy's backport of the "single"
> interface went in first

>http://www.ussg.iu.edu/hypermail/linux/kernel/0308.3/0296.html

>Are you planning on taking that patch? Or should I just resend my patch
> w/o it?

I applied the seq file single interfaces now.