[PATCH] nfs4: Fix NULL dereference at d_alloc_and_lookup()

[PATCH] nfs4: Fix NULL dereference at d_alloc_and_lookup()

[Vitaliy Gusev]

From: Gusev Vitaliy <gusev.vitaliy-x4E8uuA0+mFBDgjK7y7TUQ@public.gmane.org>

d_alloc_and_lookup() calls i_op->lookup method due to
rootfh changes his fsid.

During mount i_op of NFS root inode is set to
nfs_mountpoint_inode_operations, if rpc_ops->getroot()
and rpc_ops->getattr() return different fsid.

After that  nfs_follow_remote_path() raised oops:

   BUG: unable to handle kernel NULL pointer dereference at (null)
   IP: [<          (null)>]           (null)

stack trace:

     d_alloc_and_lookup+0x4c/0x74
     do_lookup+0x1e3/0x280
     link_path_walk+0x12e/0xab0
     nfs4_remote_get_sb+0x56/0x2c0 [nfs]
     path_walk+0x67/0xe0
     vfs_path_lookup+0x8e/0x100
     nfs_follow_remote_path+0x16f/0x3e0 [nfs]
     nfs4_try_mount+0x6f/0xd0 [nfs]
     nfs_get_sb+0x269/0x400 [nfs]
     vfs_kern_mount+0x8a/0x1f0
     do_kern_mount+0x52/0x130
     do_mount+0x20a/0x260
     sys_mount+0x90/0xe0
     system_call_fastpath+0x16/0x1b

So just refresh fsid, as RFC3530 doesn't specify behavior
in case of rootfh changes fsid.

Signed-off-by: Vitaliy Gusev <gusev.vitaliy-x4E8uuA0+mFBDgjK7y7TUQ@public.gmane.org>
---
 fs/nfs/getroot.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
index 3031ef7..914eb78 100644
--- a/fs/nfs/getroot.c
+++ b/fs/nfs/getroot.c
@@ -192,6 +192,10 @@ struct dentry *nfs4_get_root(struct super_block *sb, struct nfs_fh *mntfh)
 		goto out;
 	}
 
+	if (fattr->valid & NFS_ATTR_FATTR_FSID &&
+	    !nfs_fsid_equal(&server->fsid, &fattr->fsid))
+		memcpy(&server->fsid, &fattr->fsid, sizeof(server->fsid));
+
 	inode = nfs_fhget(sb, mntfh, fattr);
 	if (IS_ERR(inode)) {
 		dprintk("nfs_get_root: get root inode failed\n");
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfs4: Fix NULL dereference at d_alloc_and_lookup()

[Trond Myklebust]

On Wed, 2011-03-23 at 00:40 +0300, Vitaliy Gusev wrote:
> From: Gusev Vitaliy <gusev.vitaliy-x4E8uuA0+mFBDgjK7y7TUQ@public.gmane.org>
> 
> d_alloc_and_lookup() calls i_op->lookup method due to
> rootfh changes his fsid.
>
> During mount i_op of NFS root inode is set to
> nfs_mountpoint_inode_operations, if rpc_ops->getroot()
> and rpc_ops->getattr() return different fsid.

That is a server bug! Why are you trying to "fix" that on the client
instead of telling the user that their server deserves to be burned
behind the shed?

>  
> +	if (fattr->valid & NFS_ATTR_FATTR_FSID &&
> +	    !nfs_fsid_equal(&server->fsid, &fattr->fsid))
> +		memcpy(&server->fsid, &fattr->fsid, sizeof(server->fsid));

No. add a printk() to the effect that the server is insane, return -EIO
and we're done...

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfs4: Fix NULL dereference at d_alloc_and_lookup()

[Vitaliy Gusev]

On Tue, 2011-03-22 at 17:52 -0400, Trond Myklebust wrote:
> On Wed, 2011-03-23 at 00:40 +0300, Vitaliy Gusev wrote:
> > From: Gusev Vitaliy <gusev.vitaliy-x4E8uuA0+mFBDgjK7y7TUQ@public.gmane.org>
> > 
> > d_alloc_and_lookup() calls i_op->lookup method due to
> > rootfh changes his fsid.
> >
> > During mount i_op of NFS root inode is set to
> > nfs_mountpoint_inode_operations, if rpc_ops->getroot()
> > and rpc_ops->getattr() return different fsid.
> 
> That is a server bug! Why are you trying to "fix" that on the client
> instead of telling the user that their server deserves to be burned
> behind the shed?
> 

Because nfs_update_inode() does it with success and pleasure:

	if (S_ISDIR(inode->i_mode) && (fattr->valid & NFS_ATTR_FATTR_FSID) &&
			!nfs_fsid_equal(&server->fsid, &fattr->fsid) &&
			!IS_AUTOMOUNT(inode))
		server->fsid = fattr->fsid;

And what are the reasons to tell to user about broken servers during
mount, but do not tell about it after mount ?


> >  
> > +	if (fattr->valid & NFS_ATTR_FATTR_FSID &&
> > +	    !nfs_fsid_equal(&server->fsid, &fattr->fsid))
> > +		memcpy(&server->fsid, &fattr->fsid, sizeof(server->fsid));
> 
> No. add a printk() to the effect that the server is insane, return -EIO
> and we're done...

Ok. Next checks mntroot->d_inode->i_op != dir_inode_ops) from super.c
does what you pointed. But it is only for cross mounts and referrals.


	mntroot = nfs4_get_root(s, mntfh);
	if (IS_ERR(mntroot)) {
		error = PTR_ERR(mntroot);
		goto error_splat_super;
	}

	if (mntroot->d_inode->i_op !=
NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops) {
		dput(mntroot);
		error = -ESTALE;
		goto error_splat_super;
	}




-- 
Thanks,
Vitaliy Gusev

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfs4: Fix NULL dereference at d_alloc_and_lookup()

[Trond Myklebust]

On Wed, 2011-03-23 at 01:58 +0300, Vitaliy Gusev wrote:
> On Tue, 2011-03-22 at 17:52 -0400, Trond Myklebust wrote:
> > On Wed, 2011-03-23 at 00:40 +0300, Vitaliy Gusev wrote:
> > > From: Gusev Vitaliy <gusev.vitaliy-x4E8uuA0+mFBDgjK7y7TUQ@public.gmane.org>
> > > 
> > > d_alloc_and_lookup() calls i_op->lookup method due to
> > > rootfh changes his fsid.
> > >
> > > During mount i_op of NFS root inode is set to
> > > nfs_mountpoint_inode_operations, if rpc_ops->getroot()
> > > and rpc_ops->getattr() return different fsid.
> > 
> > That is a server bug! Why are you trying to "fix" that on the client
> > instead of telling the user that their server deserves to be burned
> > behind the shed?
> > 
> 
> Because nfs_update_inode() does it with success and pleasure:
> 
> 	if (S_ISDIR(inode->i_mode) && (fattr->valid & NFS_ATTR_FATTR_FSID) &&
> 			!nfs_fsid_equal(&server->fsid, &fattr->fsid) &&
> 			!IS_AUTOMOUNT(inode))
> 		server->fsid = fattr->fsid;
> 
> And what are the reasons to tell to user about broken servers during
> mount, but do not tell about it after mount ?

Sigh... That is a valid point.

Looking at the Linux NFS server, I see that it has 3 different ways to
derive a fsid: only two are guaranteed to survive a reboot. I believe
that is probably why we added the above code to nfs_update_inode().

Looking now at the uses of nfs4_get_root(), it looks as if all the users
have compared the original fsid to the super block fsid prior to getting
here, and so we know (hope) that the directory that was obtained with
this filehandle did at one point match. If that is always the case, then
we should be safe w.r.t. mistaking this for a server-side mountpoint.
Since a referral is supposed to give us an error when we try to get the
directory attributes, then we know this isn't pointing to a referral.

OK, I'll take this patch...
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfs4: Fix NULL dereference at d_alloc_and_lookup()

[Vitaliy Gusev]

On 03/23/2011 02:59 AM, Trond Myklebust wrote:
> On Wed, 2011-03-23 at 01:58 +0300, Vitaliy Gusev wrote:
>> On Tue, 2011-03-22 at 17:52 -0400, Trond Myklebust wrote:
>>> On Wed, 2011-03-23 at 00:40 +0300, Vitaliy Gusev wrote:
>>>> From: Gusev Vitaliy<gusev.vitaliy-x4E8uuA0+mFBDgjK7y7TUQ@public.gmane.org>
>>>>
>>>> d_alloc_and_lookup() calls i_op->lookup method due to
>>>> rootfh changes his fsid.
>>>>
>>>> During mount i_op of NFS root inode is set to
>>>> nfs_mountpoint_inode_operations, if rpc_ops->getroot()
>>>> and rpc_ops->getattr() return different fsid.
>>>
>>> That is a server bug! Why are you trying to "fix" that on the client
>>> instead of telling the user that their server deserves to be burned
>>> behind the shed?
>>>
>>
>> Because nfs_update_inode() does it with success and pleasure:
>>
>> 	if (S_ISDIR(inode->i_mode)&&  (fattr->valid&  NFS_ATTR_FATTR_FSID)&&
>> 			!nfs_fsid_equal(&server->fsid,&fattr->fsid)&&
>> 			!IS_AUTOMOUNT(inode))
>> 		server->fsid = fattr->fsid;
>>
>> And what are the reasons to tell to user about broken servers during
>> mount, but do not tell about it after mount ?
>
> Sigh... That is a valid point.
>
> Looking at the Linux NFS server, I see that it has 3 different ways to
> derive a fsid: only two are guaranteed to survive a reboot. I believe
> that is probably why we added the above code to nfs_update_inode().
>
> Looking now at the uses of nfs4_get_root(), it looks as if all the users
> have compared the original fsid to the super block fsid prior to getting
> here, and so we know (hope) that the directory that was obtained with
> this filehandle did at one point match. If that is always the case, then
> we should be safe w.r.t. mistaking this for a server-side mountpoint.
> Since a referral is supposed to give us an error when we try to get the
> directory attributes,

Check on referral at nfs4_get_rootfh():

	if (fsinfo.fattr->valid & NFS_ATTR_FATTR_V4_REFERRAL) {
		printk(KERN_ERR "nfs4_get_rootfh:"
		       " getroot obtained referral\n");
		ret = -EREMOTE;
		goto out;
	}

is vain and can be removed. That path never sets NFS_ATTR_FATTR_V4_REFERRAL.

then we know this isn't pointing to a referral.


Check

(A)
	if (mntroot->d_inode->i_op != 
NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops) {
		dput(mntroot);
		error = -ESTALE;
		goto error_splat_super;
	}

was initially added by commit 4c1fe2f7 and now is not required because:

   1. This patch fixes of changing i_op at nfs_fhget(). So 
nfs4_get_root() always will return i_op == dir_inode_ops.

   2. commit 4c1fe2f7 fixed BUG_ON at nfs_follow_mountpoint(). Now there 
is no BUG_ON. It was replaced with ESTALE by commit 44d5759d

So only one thing that does check (A) -  it fixes changing fsid 
behaviour for NFSv2,NFSv3 protocol - nfs_xdev_mount(). In other places 
it can be removed:

*** fs/nfs/super.c:
nfs_xdev_mount[2483]           if (mntroot->d_inode->i_op != 
NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops) {

nfs4_xdev_mount[3066]          if (mntroot->d_inode->i_op != 
NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops) {

nfs4_remote_referral_mount[3153] if (mntroot->d_inode->i_op != 
NFS_SB(s)->nfs_client->rpc_ops->dir_inode_ops) {



>
> OK, I'll take this patch...

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html