[uml-devel] [patch] mconsole_proc rewrite (and crash fix).

[uml-devel] [patch] mconsole_proc rewrite (and crash fix).

[Gerd Knorr]

  Hi,

Below is a rewrite of the mconsole_proc function.

The old code had the problem that the kernel crashed after calling
"uml_mconsole proc <somefile>" a few times.  I havn't tracked what
exactly causes the problem, I guess trying to access the procfs without
actually mounting it somewhere causes some corruption of kernel data
structures.

The new code simply openes /proc/<file> via sys_open().  That simplifies
the function alot.  It also doesn't crash any more ;)
Drawback is that it only works when procfs is actually mounted below
/proc.  One suggestion I've received to fix the later issue was to mount
the procfs within a kernel thread with a private namespace, but I havn't
tried that so far.

  Gerd

--- a/arch/um/drivers/mconsole_kern.c.proc	2004-07-14 10:54:03.089015009 +0200
+++ b/arch/um/drivers/mconsole_kern.c	2004-07-14 20:03:52.812664794 +0200
@@ -19,6 +19,7 @@
 #include "linux/fs.h"
 #include "linux/namei.h"
 #include "linux/proc_fs.h"
+#include "linux/syscalls.h"
 #include "asm/irq.h"
 #include "asm/uaccess.h"
 #include "user_util.h"
@@ -120,77 +121,50 @@
 
 void mconsole_proc(struct mc_request *req)
 {
-	struct nameidata nd;
-	struct file_system_type *proc;
-	struct super_block *super;
-	struct file *file;
-	int n, err;
-	char *ptr = req->request.data, *buf;
-	
+	char path[64];
+	char *buf;
+	int len;
+	int fd;
+	char *ptr = req->request.data;
+
 	ptr += strlen("proc");
 	while(isspace(*ptr)) ptr++;
+	snprintf(path, sizeof(path), "/proc/%s", ptr);
 
-	proc = get_fs_type("proc");
-	if(proc == NULL){
-		mconsole_reply(req, "procfs not registered", 1, 0);
-		goto out;
-	}
-
-	super = (*proc->get_sb)(proc, 0, NULL, NULL);
-	put_filesystem(proc);
-	if(super == NULL){
-		mconsole_reply(req, "Failed to get procfs superblock", 1, 0);
-		goto out;
-	}
-	up_write(&super->s_umount);
-
-	nd.dentry = super->s_root;
-	nd.mnt = NULL;
-	nd.flags = O_RDONLY + 1;
-	nd.last_type = LAST_ROOT;
-
-	err = link_path_walk(ptr, &nd);
-	if(err){
-		mconsole_reply(req, "Failed to look up file", 1, 0);
-		goto out_kill;
-	}
-
-	file = dentry_open(nd.dentry, nd.mnt, O_RDONLY);
-	if(IS_ERR(file)){
+	fd = sys_open(path, 0, 0);
+	if (fd < 0) {
 		mconsole_reply(req, "Failed to open file", 1, 0);
-		goto out_kill;
+		printk("open %s: %d\n",path,fd);
+		goto out;
 	}
 
 	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if(buf == NULL){
 		mconsole_reply(req, "Failed to allocate buffer", 1, 0);
-		goto out_fput;
+		goto out_close;
 	}
 
-	if((file->f_op != NULL) && (file->f_op->read != NULL)){
-		do {
-			n = (*file->f_op->read)(file, buf, PAGE_SIZE - 1, 
-						&file->f_pos);
-			if(n >= 0){
-				buf[n] = '\0';
-				mconsole_reply(req, buf, 0, (n > 0));
-			}
-			else {
-				mconsole_reply(req, "Read of file failed", 
-					       1, 0);
-				goto out_free;
-			}
-		} while(n > 0);
+	for (;;) {
+		len = sys_read(fd, buf, PAGE_SIZE-1);
+		if (len < 0) {
+			mconsole_reply(req, "Read of file failed", 1, 0);
+			goto out_free;
+		} else if (len == PAGE_SIZE-1) {
+			buf[len] = '\0';
+			mconsole_reply(req, buf, 0, 1);
+		} else {
+			buf[len] = '\0';
+			mconsole_reply(req, buf, 0, 0);
+			break;
+		}
 	}
-	else mconsole_reply(req, "", 0, 0);
 
  out_free:
 	kfree(buf);
- out_fput:
-	fput(file);
- out_kill:
-	deactivate_super(super);
- out: ;
+ out_close:
+	sys_close(fd);
+ out:
+	/* nothing */;
 }
 
 #define UML_MCONSOLE_HELPTEXT \


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=4721&alloc_id=10040&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [patch] mconsole_proc rewrite (and crash fix).

[BlaisorBlade]

Alle 22:40, lunedì 19 luglio 2004, Gerd Knorr ha scritto:
>   Hi,
>
> Below is a rewrite of the mconsole_proc function.

First: I'm collecting it inside my 2.6-bb patchset (which is then what I send 
for -mm inclusion).

Second: since there is in that tree the mconsole exec support (which allows 
executing any command including cat /proc/file > /dev/ttyN and cat /sys/*), 
which is as reliable as this version of mconsole_proc() but by far more 
general, I'm not sure if we should keep the proc command.

Actually the exec command cannot send anything to the mconsole, but since it 
invokes sh -c it allows for redirections to new consoles.

> The old code had the problem that the kernel crashed after calling
> "uml_mconsole proc <somefile>" a few times.  I havn't tracked what
> exactly causes the problem, I guess trying to access the procfs without
> actually mounting it somewhere causes some corruption of kernel data
> structures.

Who said that he didn't have mounted /proc inside the guest? I'm more 
convinced that while replicating the whole sys_open code Jeff didn't get 
something right. And it would have been worse if he ever managed to do it: it 
would have broken at the first little VFS change :-(. But it's only IMHO; on 
the other side, creating a kernel thread with his own namespace is safer but 
not cleaner than implementing sys_open ourselves.

However, I see basically two bugs:
1) why put_filesystem is not called at the end?
2) why don't we call dget() onto sb->s_root? I have some doubts about it, 
anyway.

> The new code simply openes /proc/<file> via sys_open().  That simplifies
> the function alot.  It also doesn't crash any more ;)
> Drawback is that it only works when procfs is actually mounted below
> /proc.  One suggestion I've received to fix the later issue was to mount
> the procfs within a kernel thread with a private namespace, but I havn't
> tried that so far.
I checked if kern_mount() would have worked but it's not straightforward, 
since it does not add the mounted fs anywhere.

However, sys_mount() and do_mount(), apart from argument checking, do a 
path_lookup(), to get a struct nameidata representing the mount point, and 
pass it to do_add_mount(). And only path_lookup() checks the current->fs 
datas, i.e. the root (not the ->namespace). Maybe that could help us, but I'm 
not sure...
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729



-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [patch] mconsole_proc rewrite (and crash fix).

[Gerd Knorr]

On Wed, Aug 11, 2004 at 07:57:40PM +0200, BlaisorBlade wrote:
        ^^^^^^
Did the mail really travel that long, or is your clock broken?

> Actually the exec command cannot send anything to the mconsole, but since it 
> invokes sh -c it allows for redirections to new consoles.

Having the output go directly to mconsole is much easier to handle
through.

> > The old code had the problem that the kernel crashed after calling
> > "uml_mconsole proc <somefile>" a few times.  I havn't tracked what
> > exactly causes the problem, I guess trying to access the procfs without
> > actually mounting it somewhere causes some corruption of kernel data
> > structures.
> 
> Who said that he didn't have mounted /proc inside the guest?

Code looks like that, as it uses get_filesystem and all that instead of
doing a simple /proc/... path lookup.  What is the point of doing it
that way, other than make it work even if procfs is not mounted below
/proc?

> However, I see basically two bugs:
> 1) why put_filesystem is not called at the end?

It is called earlier, right after ->get_sb() call.  Which looks sane to
me, as a successfull get_sb() should increase the refcount and thus
releasing the filesystems afterwards should be safe.  I'm no VFS expert
through.

  Gerd

-- 
return -ENOSIG;


-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel