* [uml-devel] [patch] mconsole_proc rewrite (and crash fix).
@ 2004-07-19 20:40 Gerd Knorr
2004-08-11 17:57 ` BlaisorBlade
0 siblings, 1 reply; 3+ messages in thread
From: Gerd Knorr @ 2004-07-19 20:40 UTC (permalink / raw)
To: Jeff Dike, uml devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [uml-devel] [patch] mconsole_proc rewrite (and crash fix).
2004-07-19 20:40 [uml-devel] [patch] mconsole_proc rewrite (and crash fix) Gerd Knorr
@ 2004-08-11 17:57 ` BlaisorBlade
2004-08-20 11:08 ` Gerd Knorr
0 siblings, 1 reply; 3+ messages in thread
From: BlaisorBlade @ 2004-08-11 17:57 UTC (permalink / raw)
To: user-mode-linux-devel; +Cc: Gerd Knorr, Jeff Dike
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [uml-devel] [patch] mconsole_proc rewrite (and crash fix).
2004-08-11 17:57 ` BlaisorBlade
@ 2004-08-20 11:08 ` Gerd Knorr
0 siblings, 0 replies; 3+ messages in thread
From: Gerd Knorr @ 2004-08-20 11:08 UTC (permalink / raw)
To: BlaisorBlade; +Cc: user-mode-linux-devel, Jeff Dike
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2004-08-20 11:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-07-19 20:40 [uml-devel] [patch] mconsole_proc rewrite (and crash fix) Gerd Knorr
2004-08-11 17:57 ` BlaisorBlade
2004-08-20 11:08 ` Gerd Knorr
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.