Is a NULL check missing in nfs_lookup?

Is a NULL check missing in nfs_lookup?

[Chaitanya Patti]

Hello everyone,

In the function nfs_lookup in nfs/dir.c , the following line (line # 926):

error = nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);

uses `nd' without having checked if it is NULL.

Is this correct?

This is in kernel version 2.6.19.1

Can someone check this ?

Thanks,

Chaitanya.

Re: Is a NULL check missing in nfs_lookup?

[Shaya Potter]

yes, you're writing a stackable file system (the cs.sunysb gives that
away) and have run a lookup_one_len() on a nfs mounted file system and
that means nd is null.

Erez's group is trying to fix that situation so the intents can be
passed correctly.

On Thu, 2007-01-04 at 19:00 -0500, Chaitanya Patti wrote:
> 
> Hello everyone,
> 
> In the function nfs_lookup in nfs/dir.c , the following line (line # 926):
> 
> error = nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
> 
> uses `nd' without having checked if it is NULL.
> 
> Is this correct?
> 
> This is in kernel version 2.6.19.1
> 
> Can someone check this ?
> 
> Thanks,
> 
> Chaitanya.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Is a NULL check missing in nfs_lookup?

[Josef Sipek]

On Thu, Jan 04, 2007 at 07:47:34PM -0500, Shaya Potter wrote:
> yes, you're writing a stackable file system (the cs.sunysb gives that
> away) and have run a lookup_one_len() on a nfs mounted file system and
> that means nd is null.
> 
> Erez's group is trying to fix that situation so the intents can be
> passed correctly.

If you look for the last Unionfs submission to lkml/fsdevel, one of the
patches creates lookup_one_len_nd which allows you to pass a nameidata
structure down to the lower filesystem.

Beware, NFSv4 is cranky when it comes to lookup intents. In Unionfs, we have
some additional lookup_one_len's in a number of places that have no
nameidata that we can work off of, and that's causing quite a number of
problems as NFSv4 gets confused when it doesn't see an intent.

Josef "Jeff" Sipek.

-- 
You measure democracy by the freedom it gives its dissidents, not the
freedom it gives its assimilated conformists.
		- Abbie Hoffman

Re: Is a NULL check missing in nfs_lookup?

[Ian Kent]

On Thu, 4 Jan 2007, Shaya Potter wrote:

> yes, you're writing a stackable file system (the cs.sunysb gives that
> away) and have run a lookup_one_len() on a nfs mounted file system and
> that means nd is null.

I thought that calling lookup_one_len on a filesystem outside of your own 
was not correct usage of lookup_one_len.

> 
> Erez's group is trying to fix that situation so the intents can be
> passed correctly.
> 
> On Thu, 2007-01-04 at 19:00 -0500, Chaitanya Patti wrote:
> > 
> > Hello everyone,
> > 
> > In the function nfs_lookup in nfs/dir.c , the following line (line # 926):
> > 
> > error = nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
> > 
> > uses `nd' without having checked if it is NULL.
> > 
> > Is this correct?
> > 
> > This is in kernel version 2.6.19.1
> > 
> > Can someone check this ?
> > 
> > Thanks,
> > 
> > Chaitanya.
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Is a NULL check missing in nfs_lookup?

[Trond Myklebust]

On Thu, 2007-01-04 at 19:00 -0500, Chaitanya Patti wrote:
> 
> Hello everyone,
> 
> In the function nfs_lookup in nfs/dir.c , the following line (line # 926):
> 
> error = nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
> 
> uses `nd' without having checked if it is NULL.
> 
> Is this correct?

It is quite intentional and correct. Calling ->lookup() without correct
intent information is a bug.

Trond

Re: Is a NULL check missing in nfs_lookup?

[Shaya Potter]

On Fri, 2007-01-05 at 13:22 +0100, Trond Myklebust wrote:
> On Thu, 2007-01-04 at 19:00 -0500, Chaitanya Patti wrote:
> > 
> > Hello everyone,
> > 
> > In the function nfs_lookup in nfs/dir.c , the following line (line # 926):
> > 
> > error = nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
> > 
> > uses `nd' without having checked if it is NULL.
> > 
> > Is this correct?
> 
> It is quite intentional and correct. Calling ->lookup() without correct
> intent information is a bug.

I'd agree with you (And even told the person the problem up front)
except it's not oopsing on a lack of intent information, it's oopsing
because nd is null and therefore can not access nd->mnt.

i.e. Let say I couldn't reconstruct nd perfectly (due to not knowing
vfsmnt information), I could possible construct a fake nd with the
proper intent information (i.e. very likely no intent information to be
passed) and it would still oops.

So my question,

is changing nfs_reval_fsid() from

static inline int nfs_reval_fsid(struct vfsmount *mnt...)
	that calls __nfs_revalidate_inode(...., mnt->mnt_root->d_inode);

and is called as error = nfs_reval_fsid(nd->mnt...) by nfs_lookup()

to 

static inline int nfs_reval_fsid(struct dentry * dentry...)
	that calls __nfs_revalidate_inode(server, dentry->d_inode);

and is called as error = nfs_reval_fsid(dentry->d_sb->s_root...) by
nfs_lookup() 

incorrect?

now, it could be me missing the boat here, I wouldn't be surprised.

thanks.

Re: Is a NULL check missing in nfs_lookup?

[Trond Myklebust]

On Fri, 2007-01-05 at 10:00 -0500, Shaya Potter wrote:
> I'd agree with you (And even told the person the problem up front)
> except it's not oopsing on a lack of intent information, it's oopsing
> because nd is null and therefore can not access nd->mnt.



> i.e. Let say I couldn't reconstruct nd perfectly (due to not knowing
> vfsmnt information), I could possible construct a fake nd with the
> proper intent information (i.e. very likely no intent information to be
> passed) and it would still oops.

No. The nameidata structure is _mandatory_, as is the intent info. In
order to ensure POSIX correctness, the NFS client sometimes needs to
create submounts (when crossing a mountpoint on the server). If you
don't provide a proper nameidata structure, then that is impossible.

Trond

Re: Is a NULL check missing in nfs_lookup?

[Shaya Potter]

On Fri, 2007-01-05 at 17:01 +0100, Trond Myklebust wrote:
> On Fri, 2007-01-05 at 10:00 -0500, Shaya Potter wrote:
> > I'd agree with you (And even told the person the problem up front)
> > except it's not oopsing on a lack of intent information, it's oopsing
> > because nd is null and therefore can not access nd->mnt.
> 
> 
> 
> > i.e. Let say I couldn't reconstruct nd perfectly (due to not knowing
> > vfsmnt information), I could possible construct a fake nd with the
> > proper intent information (i.e. very likely no intent information to be
> > passed) and it would still oops.
> 
> No. The nameidata structure is _mandatory_, as is the intent info. In
> order to ensure POSIX correctness, the NFS client sometimes needs to
> create submounts (when crossing a mountpoint on the server). If you
> don't provide a proper nameidata structure, then that is impossible.

ok, I guess what I don't understand is when are

dentry->d_sb->s_root and nd->mnt->mnt_root not equivalent.

I guess it would be "when crossing a mountpoint on the server", so I
look at nfs_follow_mountpoint() and basically see that you create a new
mountpoint and a new nd for that.  And since superblock objects are only
per "real" file system not per mount point, they will be different.

I guess the question is, why shouldn't a dentry object know what
vfsmount it belongs to is?  Can it belong to multiple vfsmount objects?
(Again, probably showing my igornance here).

Basically, the question I'm trying to figure out, are

a) why do intents and nd have to be so tightly wound together?
b) why can't a dentry object know about vfsmount?

Re: Is a NULL check missing in nfs_lookup?

[Al Viro]

On Fri, Jan 05, 2007 at 12:11:17PM -0500, Shaya Potter wrote:
> ok, I guess what I don't understand is when are
> 
> dentry->d_sb->s_root and nd->mnt->mnt_root not equivalent.
> 
> I guess it would be "when crossing a mountpoint on the server", so I
> look at nfs_follow_mountpoint() and basically see that you create a new
> mountpoint and a new nd for that.  And since superblock objects are only
> per "real" file system not per mount point, they will be different.
> 
> I guess the question is, why shouldn't a dentry object know what
> vfsmount it belongs to is?  Can it belong to multiple vfsmount objects?

Yes.  dentry belongs to superblock.  vfsmount refers to a subtree of some
superblock's dentry tree.  There can be any number of vfsmounts refering
to subtrees of the same fs.  Some might refer to the entire tree, some -
to its arbitrary subtrees (possibly as small as a single file).  There
can be any number of vfsmounts refering to any given subtree.

Think what happens when you create a binding.  Or when you clone an entire
namespace.  (Pieces of) the same filesystem might be mounted in a lot
of places.  vfsmount describes a part of unified tree delimited by
mountpoints; if the same fs (or its parts) are seen under several
mountpoints, you get vfsmounts that refer to the same fs instance, i.e.
the same superblock and dentry tree.

Re: Is a NULL check missing in nfs_lookup?

[Matthew Wilcox]

On Fri, Jan 05, 2007 at 12:11:17PM -0500, Shaya Potter wrote:
> I guess the question is, why shouldn't a dentry object know what
> vfsmount it belongs to is?  Can it belong to multiple vfsmount objects?
> (Again, probably showing my igornance here).

If you have the same tree mounted in two places, they'll have
different vfsmounts.

Why not pass "vfsmount" to VFS functions?

[Tetsuo Handa]

Hello.

> > I guess the question is, why shouldn't a dentry object know what
> > vfsmount it belongs to is?  Can it belong to multiple vfsmount objects?
> > (Again, probably showing my igornance here).
> 
> If you have the same tree mounted in two places, they'll have
> different vfsmounts.

That makes auditing difficult.
Recently, the importance of saving operation log is increasing,
and it is convenient to log it using d_path().
But due to "bind mount" functionality, it is impossible to
uniquely determine which pathname was requested.
It would be nice if VFS functions receive not only "dentry"
but also "vfsmount" parameter so that auditing functions can
easily calculate the requested pathname.

Re: Is a NULL check missing in nfs_lookup?

[Erez Zadok]

In message <1167999770.6050.39.camel@lade.trondhjem.org>, Trond Myklebust writes:
> On Thu, 2007-01-04 at 19:00 -0500, Chaitanya Patti wrote:
> > 
> > Hello everyone,
> > 
> > In the function nfs_lookup in nfs/dir.c , the following line (line # 926):
> > 
> > error = nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
> > 
> > uses `nd' without having checked if it is NULL.
> > 
> > Is this correct?
> 
> It is quite intentional and correct. Calling ->lookup() without correct
> intent information is a bug.

Ah, ok.  So why not put an ASSERT in there, or at least a comment, to make
the code clearer.  As it stands, anyone looking at the code in the future
can easily rediscover this "bug" that dereferences a null ptr.

Thanks,
Erez.

Re: Is a NULL check missing in nfs_lookup?

[Matthew Wilcox]

On Fri, Jan 05, 2007 at 02:10:06PM -0500, Erez Zadok wrote:
> Ah, ok.  So why not put an ASSERT in there, or at least a comment, to make
> the code clearer.  As it stands, anyone looking at the code in the future
> can easily rediscover this "bug" that dereferences a null ptr.

Because anyone poking in the VFS should take the time to understand
how it works?  Adding crap like BUG_ON(!nd) is pointless -- you don't
get a clearer backtrace from that than you do from a null pointer.

Re: Is a NULL check missing in nfs_lookup?

[Muli Ben-Yehuda]

On Fri, Jan 05, 2007 at 12:23:04PM -0700, Matthew Wilcox wrote:
> On Fri, Jan 05, 2007 at 02:10:06PM -0500, Erez Zadok wrote:
> > Ah, ok.  So why not put an ASSERT in there, or at least a comment, to make
> > the code clearer.  As it stands, anyone looking at the code in the future
> > can easily rediscover this "bug" that dereferences a null ptr.
> 
> Because anyone poking in the VFS should take the time to understand
> how it works?  Adding crap like BUG_ON(!nd) is pointless -- you don't
> get a clearer backtrace from that than you do from a null pointer.

Except that a BUG_ON makes the author's intent clear and a NULL
pointer deref doesn't. Sure, anyone poking around in the VFS should
understand how it works. That doesn't mean whoever poked there before
can't leave trail marks.

Cheers,
Muli

Re: Is a NULL check missing in nfs_lookup?

[Erez Zadok]

In message <20070105192304.GH24620@parisc-linux.org>, Matthew Wilcox writes:
> On Fri, Jan 05, 2007 at 02:10:06PM -0500, Erez Zadok wrote:
> > Ah, ok.  So why not put an ASSERT in there, or at least a comment, to make
> > the code clearer.  As it stands, anyone looking at the code in the future
> > can easily rediscover this "bug" that dereferences a null ptr.
> 
> Because anyone poking in the VFS should take the time to understand
> how it works?  Adding crap like BUG_ON(!nd) is pointless -- you don't
> get a clearer backtrace from that than you do from a null pointer.

Then stick a /* comment */ telling anyone who looks at the code what was the
author's intentions, esp. for such an obvious null/uninitialized pointer
deref.

Source code should be as clear as possible, regardless of the expertise of
its author or any reader.  If you keep this code as is, the bug will get
re-discovered and re-reported time and again, wasting everyone's time.

Erez.