[patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[blaisorblade]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

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 <blaisorblade@yahoo.it>
---

 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 = {
_

Re: [patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[Al Viro]

On Fri, Aug 26, 2005 at 04:57:44PM +0200, blaisorblade@yahoo.it wrote:
> 
> From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> 
> 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.

Re: [patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[Blaisorblade]

On Friday 26 August 2005 21:03, Al Viro wrote:
> On Fri, Aug 26, 2005 at 04:57:44PM +0200, blaisorblade@yahoo.it wrote:
> > From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

> > 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

Re: [patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[Al Viro]

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...

Re: [uml-devel] Re: [patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[Blaisorblade]

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

Re: [uml-devel] Re: [patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[Al Viro]

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;
	...

Re: [uml-devel] Re: [patch 1/2] Fixup symlink function pointers for hppfs [for 2.6.13]

[Al Viro]

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.