* /proc/ioports overrun patch @ 2003-08-29 13:30 Marcelo Tosatti 2003-08-29 18:59 ` john stultz 2003-09-24 19:42 ` [PATCH] [2.4] " olof 0 siblings, 2 replies; 8+ messages in thread From: Marcelo Tosatti @ 2003-08-29 13:30 UTC (permalink / raw) To: olof; +Cc: lkml 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: /proc/ioports overrun patch 2003-08-29 13:30 /proc/ioports overrun patch Marcelo Tosatti @ 2003-08-29 18:59 ` john stultz 2003-09-24 19:42 ` [PATCH] [2.4] " olof 1 sibling, 0 replies; 8+ messages in thread From: john stultz @ 2003-08-29 18:59 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: olof, lkml, Randy.Dunlap 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] [2.4] Re: /proc/ioports overrun patch 2003-08-29 13:30 /proc/ioports overrun patch Marcelo Tosatti 2003-08-29 18:59 ` john stultz @ 2003-09-24 19:42 ` olof 2003-09-24 19:51 ` viro 1 sibling, 1 reply; 8+ messages in thread From: olof @ 2003-09-24 19:42 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: lkml 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [2.4] Re: /proc/ioports overrun patch 2003-09-24 19:42 ` [PATCH] [2.4] " olof @ 2003-09-24 19:51 ` viro 2003-09-24 19:59 ` viro 0 siblings, 1 reply; 8+ messages in thread From: viro @ 2003-09-24 19:51 UTC (permalink / raw) To: olof; +Cc: Marcelo Tosatti, lkml 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... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [2.4] Re: /proc/ioports overrun patch 2003-09-24 19:51 ` viro @ 2003-09-24 19:59 ` viro 2003-09-24 21:47 ` viro 0 siblings, 1 reply; 8+ messages in thread From: viro @ 2003-09-24 19:59 UTC (permalink / raw) To: olof; +Cc: Marcelo Tosatti, lkml 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... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [2.4] Re: /proc/ioports overrun patch 2003-09-24 19:59 ` viro @ 2003-09-24 21:47 ` viro 2003-09-24 22:36 ` viro 0 siblings, 1 reply; 8+ messages in thread From: viro @ 2003-09-24 21:47 UTC (permalink / raw) To: Linus Torvalds; +Cc: Marcelo Tosatti, lkml, olof > 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [2.4] Re: /proc/ioports overrun patch 2003-09-24 21:47 ` viro @ 2003-09-24 22:36 ` viro 2003-09-24 22:54 ` Linus Torvalds 0 siblings, 1 reply; 8+ messages in thread From: viro @ 2003-09-24 22:36 UTC (permalink / raw) To: Linus Torvalds; +Cc: Marcelo Tosatti, lkml, olof 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... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [2.4] Re: /proc/ioports overrun patch 2003-09-24 22:36 ` viro @ 2003-09-24 22:54 ` Linus Torvalds 0 siblings, 0 replies; 8+ messages in thread From: Linus Torvalds @ 2003-09-24 22:54 UTC (permalink / raw) To: viro; +Cc: Marcelo Tosatti, lkml, olof 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2003-09-24 22:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-08-29 13:30 /proc/ioports overrun patch Marcelo Tosatti 2003-08-29 18:59 ` john stultz 2003-09-24 19:42 ` [PATCH] [2.4] " olof 2003-09-24 19:51 ` viro 2003-09-24 19:59 ` viro 2003-09-24 21:47 ` viro 2003-09-24 22:36 ` viro 2003-09-24 22:54 ` Linus Torvalds
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).