mconsole_proc rewrite (and crash fix).

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

@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Alle 22:40, lunedì 19 luglio 2004, Gerd Knorr ha scritto:

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.

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.

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

On Wed, Aug 11, 2004 at 07:57:40PM +0200, BlaisorBlade wrote:

        ^^^^^^

Did the mail really travel that long, or is your clock broken?

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

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?

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
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel