From: <bla...@ya...> - 2005-08-26 15:04:40
|
From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> Update hppfs for the symlink functions prototype change. Should be trivial, but please verify it's correct. Yes, I know the code I leave there is still _bogus_, see next patch for this. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- linux-2.6.git-paolo/fs/hppfs/hppfs_kern.c | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff -puN fs/hppfs/hppfs_kern.c~hppfs-fix-symlink fs/hppfs/hppfs_kern.c --- linux-2.6.git/fs/hppfs/hppfs_kern.c~hppfs-fix-symlink 2005-08-24 17:43:56.000000000 +0200 +++ linux-2.6.git-paolo/fs/hppfs/hppfs_kern.c 2005-08-24 18:17:46.000000000 +0200 @@ -679,25 +679,25 @@ static int hppfs_readlink(struct dentry return(n); } -static int hppfs_follow_link(struct dentry *dentry, struct nameidata *nd) +static void* hppfs_follow_link(struct dentry *dentry, struct nameidata *nd) { struct file *proc_file; struct dentry *proc_dentry; - int (*follow_link)(struct dentry *, struct nameidata *); - int err, n; + void * (*follow_link)(struct dentry *, struct nameidata *); + void *ret; proc_dentry = HPPFS_I(dentry->d_inode)->proc_dentry; proc_file = dentry_open(dget(proc_dentry), NULL, O_RDONLY); - err = PTR_ERR(proc_dentry); - if(IS_ERR(proc_dentry)) - return(err); + + if (IS_ERR(proc_dentry)) + return proc_dentry; follow_link = proc_dentry->d_inode->i_op->follow_link; - n = (*follow_link)(proc_dentry, nd); + ret = (*follow_link)(proc_dentry, nd); fput(proc_file); - return(n); + return ret; } static struct inode_operations hppfs_dir_iops = { _ |
From: Al V. <vi...@pa...> - 2005-08-26 19:01:04
|
On Fri, Aug 26, 2005 at 04:57:44PM +0200, bla...@ya... wrote: > > From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> > > Update hppfs for the symlink functions prototype change. > Should be trivial, but please verify it's correct. > > Yes, I know the code I leave there is still _bogus_, see next patch for this. Assuming that the next patch was "hppfs: fix symlink error path", you've still left BS in there - > proc_file = dentry_open(dget(proc_dentry), NULL, O_RDONLY); is obviously wrong; at the very least you need vfsmount in there. |
From: Blaisorblade <bla...@ya...> - 2005-08-26 20:08:58
|
On Friday 26 August 2005 21:03, Al Viro wrote: > On Fri, Aug 26, 2005 at 04:57:44PM +0200, bla...@ya... wrote: > > From: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> > > Update hppfs for the symlink functions prototype change. > > Should be trivial, but please verify it's correct. > > Yes, I know the code I leave there is still _bogus_, see next patch for > > this. About what it's doing, hppfs (HoneyPot Proc FS) is a wrapper for procfs, which must be able to hide part of the content for avoid an hacker inside UML realize he's hacking a virtual machine, and it's normally mounted on /proc, if used. > Assuming that the next patch was "hppfs: fix symlink error path", Yes. > you've still left BS in there - BullShit? Thanks for improving my acronym dictionary! > > proc_file = dentry_open(dget(proc_dentry), NULL, O_RDONLY); > is obviously wrong; at the very least you need vfsmount in there. And beyond that what? I cannot even think what's the rest *. And "obvious" doesn't hold with me. I'm _not_ a VFS hacker, I don't go beyond Documentation/filesystems/vfs.txt, so I'd better leave fixing that to you. At least what you don't mention. I'll fix vfsmount in these days (if you want to do it yourself, I've put together needed info below). I had to check dentry_open prototype to realize you're referring to the NULL there and not to dget. And the dentries you see are all descendants of the root one, taken in hppfs_fill_super() from err = init_inode(root_inode, proc_sb->s_root); I guess the current hack could be replaced with reading fs/proc/inode.c:proc_mnt... I wouldn't pass proc_mnt directly because we don't know we took _that_ mount inside hppfs_fill_super(), but I like replacing list_entry(get_fs_type("proc")->fs_supers.next,...) with proc_mnt->mnt_sb (assuming it's always filled in - IIRC I already checked the initialization order). * I've verified that there's no missing dput() in failure case as that's handled by dentry_open(). -- Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!". Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894) http://www.user-mode-linux.org/~blaisorblade ___________________________________ Yahoo! Messenger: chiamate gratuite in tutto il mondo http://it.beta.messenger.yahoo.com |
From: Al V. <vi...@pa...> - 2005-08-26 21:45:43
|
On Fri, Aug 26, 2005 at 10:04:43PM +0200, Blaisorblade wrote: > And beyond that what? I cannot even think what's the rest *. And "obvious" > doesn't hold with me. vfsmount *mnt = do_kern_mount("proc", 0, "proc", NULL); done at init time, mntput(mnt); at exit and mntget(mnt) instead of your NULL in dentry_open(). Do not mess with get_fs_type() anywhere - the above will give you access to procfs superblock just fine. The real issue is what you are doing with procfs dentries there. You do *not* call ->d_revalidate(). And you do not evict these suckers when procfs dentry goes away. E.g. when process dies... What the hell is going on with iget() calls, BTW? Especially since all of them get the same inumber... Looks completely broken. Why does is_pid() bother with checks for fs dentry belongs to? copy_from_user() return value needs to be checked. Use of file->f_pos is blatantly racy; don't do that. ->permission() is missing on hppfs; since procfs is not using generic one, we have a problem. read_proc() is a guaranteed fsckup if hppfs_open() is called with KERNEL_DS. That's from the quick look through the current code... |
From: Blaisorblade <bla...@ya...> - 2005-09-18 14:21:47
|
On Friday 26 August 2005 23:48, Al Viro wrote: > On Fri, Aug 26, 2005 at 10:04:43PM +0200, Blaisorblade wrote: > > And beyond that what? I cannot even think what's the rest *. And > > "obvious" doesn't hold with me. Al, while at it, can I get a bit of help from you? We have a commented out version of arch/um/drivers/mconsole_kern.c:mconsole_proc(), which is supposed to read the contents of procfs from the internal kernel mount, rather than /proc (to avoid being faked out by hppfs). As remarked in comments, that code is broken (run on the host uml_mconsole <umid> proc <filename>, which will call that code, for 4-5 times gives you an oops inside UML). Can you help with that? > vfsmount *mnt = do_kern_mount("proc", 0, "proc", NULL); > done at init time, > mntput(mnt); > at exit > and mntget(mnt) instead of your NULL in dentry_open(). Done. Awk, mntget. dentry_open can call mntput so we *need* to do a mntget() at each call; for the rest, we'll have dropped all hppfs dentries on unmount, before unloading the module and calling mntput(mnt), so we're doing excess atomic ops - isn't this an issue normally? > Do not mess with get_fs_type() anywhere - the above will give you access > to procfs superblock just fine. Done. Ok, that's perfectly fine. I've thought about using proc_mnt one, which is already instantiated, but I later saw it wasn't exported. > The real issue is what you are doing with procfs dentries there. You do > *not* call ->d_revalidate(). And you do not evict these suckers when > procfs dentry goes away. E.g. when process dies... Well, on this point I guess I'll need more help. I've tried to add a forward-only d_revalidate (i.e. calling the underlying one). And after looking at fs/namei.c, I realized that I must call it in hppfs_lookup, when a dentry is found in the dcache for the underlying thing. However, VFS, i.e. fs/namei.c, is inconsistent about that. Sometimes (do_lookup:need_revalidate branch) we retry lookup, sometimes (real_lookup) we fail. Based on the difference (in the first case we don't hold dir->i_sem, in the second we do), since we always take the semaphore for the wrapped procfs directory (no fast-path), I've copied real_lookup() code. It should fix the "evict when needed" thing, right? For what I see, it's d_revalidate() to accomplish this. Otherwise, I've no idea. I have also another problem. I.e., the nameidata to pass. In current code, we either use NULL or the same one we got. But from a quick look, it is apparent we should at least replace ->dentry and ->mnt with the underlying dentry and mnt, and (probably) after increasing their reference count. Actually the dget() and mntget() from __link_path_walk are really hidden, but they seem to exist, implicitly - and the *put() are called. > What the hell is going on with iget() calls, BTW? > Especially since all > of them get the same inumber... Looks completely broken. Why especially? You mean that ->lookup is not supposed to iget()? ext2 does it, both for lookup and for fill_super. For the point of the same inumber...Argh... never realized how broken this could be - until now. We're always reusing the *same* inode! No idea, didn't write the code... On using 0, in practice hostfs has been working almost perfectly (but I'd underline *almost*) in the same way... I think it should be fixed but I don't know how (we have an *intrusive* fix for hostfs). However, since we often (not always) have the underlying procfs entry, maybe we could reuse those inode numbers. When I checked, anyway, having the same inumber only turned the inode hash table into an inode list. Yes, host apps won't be happy, but for now I've no idea about how to fix it. > Why does is_pid() bother with checks for fs dentry belongs to? You mean (sb->s_op != &hppfs_sbops), right? Don't know, maybe intended to catch things mounted under /proc (but hppfs wouldn't know about them, right?), to avoid walking up beyond /proc (which is wrong, because that dentry has dentry->d_parent == dentry) or maybe to match the setting in hppfs_lookup(). I'm deleting it. > copy_from_user() return value needs to be checked. Done. > Use of file->f_pos is blatantly racy; don't do that. Done (I think). The sys_read's pattern (file_pos_{read,write}) is ok here too, right? I must normally change ppos, not file->f_pos. It depends - read_proc() uses a struct file * from dentry_open, which is private to us, but ok, that's still racy. I'm going to use a private local var like vfs_read. *ppos would do just fine, right? On this subject, is the following racy, since the write is not atomic? Multiple lseek's giving one of the offsets is fully ok, but a corrupted offset is not. drivers/char/mem.c:memory_lseek() file->f_pos += offset; > ->permission() is missing on hppfs; since procfs is not using generic one, > we have a problem. Ok, I'm calling the underlying ->permission if set. > read_proc() is a guaranteed fsckup if hppfs_open() is called with > KERNEL_DS. Ok, seen, trivial. Done. > That's from the quick look through the current code... -- Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!". Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894) http://www.user-mode-linux.org/~blaisorblade ___________________________________ Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB http://mail.yahoo.it |
From: Al V. <vi...@ft...> - 2005-09-21 03:23:59
|
On Sun, Sep 18, 2005 at 02:00:35PM +0200, Blaisorblade wrote: > On Friday 26 August 2005 23:48, Al Viro wrote: > > On Fri, Aug 26, 2005 at 10:04:43PM +0200, Blaisorblade wrote: > > > And beyond that what? I cannot even think what's the rest *. And > > > "obvious" doesn't hold with me. > Al, while at it, can I get a bit of help from you? > > We have a commented out version of > arch/um/drivers/mconsole_kern.c:mconsole_proc(), which is supposed to read > the contents of procfs from the internal kernel mount, rather than /proc (to > avoid being faked out by hppfs). > > As remarked in comments, that code is broken (run on the host uml_mconsole > <umid> proc <filename>, which will call that code, for 4-5 times gives you an > oops inside UML). Can you help with that? nd.mnt is what? NULL? There's your oops. Grab fs/nfsctl.c::do_open() and s/nfsd/proc/g in there. FWIW, it's probably better off in libfs.c with fs type as argument... For now, just copy it - it's trivial and small enough; when I move the sucker to libfs, we'll switch to that. And lose that deactivate_super(), obviously - you won't be touching superblock anymore. And for pity sake, ... goto out; ... out: ; } is spelled ... return; ... |
From: Al V. <vi...@ft...> - 2005-09-21 03:41:00
|
On Sun, Sep 18, 2005 at 02:00:35PM +0200, Blaisorblade wrote: > Well, on this point I guess I'll need more help. [snip] Ugh. What you need to do is * lock underlying directory (procfs one) * call lookup_one_len() * unlock and be done with that. And yes, ->d_revalidate() for your dentries should call the underlying one *if* dentry is positive. For negative ones you'll obviously have to repeat lookup, so just return 0. > > What the hell is going on with iget() calls, BTW? > > > Especially since all > > of them get the same inumber... Looks completely broken. > Why especially? You mean that ->lookup is not supposed to iget()? ext2 does > it, both for lookup and for fill_super. Lookup is supposed to iget when there are useful inode numbers and a chance to find something in icache. You have neither... > For the point of the same inumber...Argh... never realized how broken this > could be - until now. We're always reusing the *same* inode! > > No idea, didn't write the code... > > On using 0, in practice hostfs has been working almost perfectly (but > I'd underline *almost*) in the same way... I think it should be fixed but I > don't know how (we have an *intrusive* fix for hostfs). Why bother them, anyway? Just allocate a new inode upon ->lookup() and have ->drop_inode = generic_delete_inode(). > However, since we often (not always) have the underlying procfs entry, maybe > we could reuse those inode numbers. You want ->getattr() anyway, just call the underlying one or generic_fillattr() if there's none (both for underlying inode). That'll give you inumbers from procfs among other things... > Multiple lseek's giving one of the offsets is fully ok, but a corrupted offset > is not. > > drivers/char/mem.c:memory_lseek() > file->f_pos += offset; ... has down(&file->f_dentry->d_inode->i_sem); in the very beginning. |