From: Gusev Vitaliy <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> >
> > 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
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<[email protected]>
>>>>
>>>> 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...
On Wed, 2011-03-23 at 00:40 +0300, Vitaliy Gusev wrote:
> From: Gusev Vitaliy <[email protected]>
>
> 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
[email protected]
http://www.netapp.com
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 <[email protected]>
> > >
> > > 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
[email protected]
http://www.netapp.com