2008-04-11 18:10:13

by Jeff Layton

[permalink] [raw]
Subject: [PATCH] NFS: implement option checking when remounting NFS filesystems

When remounting an NFS or NFS4 filesystem, the new NFS options are not
respected, yet the remount will still return success. This patch adds
a remount_fs sb op for NFS that checks any new nfs mount options against
the existing ones and fails the mount if any have changed.

This is only implemented for string-based mount options since doing
this with binary options isn't really feasible.

Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/super.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index f4ee9d9..b49f90f 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -207,6 +207,7 @@ static int nfs_xdev_get_sb(struct file_system_type *fs_type,
int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
static void nfs_kill_super(struct super_block *);
static void nfs_put_super(struct super_block *);
+static int nfs_remount(struct super_block *sb, int *flags, char *raw_data);

static struct file_system_type nfs_fs_type = {
.owner = THIS_MODULE,
@@ -234,6 +235,7 @@ static const struct super_operations nfs_sops = {
.umount_begin = nfs_umount_begin,
.show_options = nfs_show_options,
.show_stats = nfs_show_stats,
+ .remount_fs = nfs_remount,
};

#ifdef CONFIG_NFS_V4
@@ -278,6 +280,7 @@ static const struct super_operations nfs4_sops = {
.umount_begin = nfs_umount_begin,
.show_options = nfs_show_options,
.show_stats = nfs_show_stats,
+ .remount_fs = nfs_remount,
};
#endif

@@ -1327,6 +1330,73 @@ out_invalid_fh:
return -EINVAL;
}

+static int
+nfs_compare_remount_data(struct nfs_server *nfss,
+ struct nfs_parsed_mount_data *data)
+{
+ if (data->flags != nfss->flags ||
+ data->rsize != nfss->rsize ||
+ data->wsize != nfss->wsize ||
+ data->retrans != nfss->client->cl_timeout->to_retries ||
+ data->auth_flavors[0] != nfss->client->cl_auth->au_flavor ||
+ data->acregmin != nfss->acregmin / HZ ||
+ data->acregmax != nfss->acregmax / HZ ||
+ data->acdirmin != nfss->acdirmin / HZ ||
+ data->acdirmax != nfss->acdirmax / HZ ||
+ data->timeo != (10U * nfss->client->cl_timeout->to_initval / HZ))
+ return -EINVAL;
+ else
+ return 0;
+}
+
+static int
+nfs_remount(struct super_block *sb, int *flags, char *raw_data)
+{
+ int error;
+ struct nfs_server *nfss = sb->s_fs_info;
+ struct nfs_parsed_mount_data *data;
+ struct nfs_mount_data *options = (struct nfs_mount_data *)raw_data;
+ struct nfs4_mount_data *options4 = (struct nfs4_mount_data *)raw_data;
+
+ /*
+ * Userspace mount programs that send binary options generally send
+ * them populated with default values. We have no way to know which
+ * ones were explicitly specified. Fall back to legacy behavior and
+ * just return success.
+ */
+ if ((sb->s_type == &nfs4_fs_type && options4->version == 1) ||
+ (sb->s_type == &nfs_fs_type && options->version >= 1 &&
+ options->version <= 6))
+ return 0;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (data == NULL)
+ return -ENOMEM;
+
+ /* fill out struct with values from existing mount */
+ data->flags = nfss->flags;
+ data->rsize = nfss->rsize;
+ data->wsize = nfss->wsize;
+ data->retrans = nfss->client->cl_timeout->to_retries;
+ data->auth_flavors[0] = nfss->client->cl_auth->au_flavor;
+ data->acregmin = nfss->acregmin / HZ;
+ data->acregmax = nfss->acregmax / HZ;
+ data->acdirmin = nfss->acdirmin / HZ;
+ data->acdirmax = nfss->acdirmax / HZ;
+ data->timeo = 10U * nfss->client->cl_timeout->to_initval / HZ;
+
+ /* overwrite those values with any that were specified */
+ error = nfs_parse_mount_options((char *)options, data);
+ if (error < 0)
+ goto out;
+
+ /* compare new mount options with old ones */
+ error = nfs_compare_remount_data(nfss, data);
+out:
+ kfree(data);
+ return error;
+}
+
/*
* Initialise the common bits of the superblock
*/
--
1.5.3.6


2008-04-11 19:05:06

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

On Fri, 11 Apr 2008 15:01:33 -0400
Chuck Lever <[email protected]> wrote:

> Jeff Layton wrote:
> > When remounting an NFS or NFS4 filesystem, the new NFS options are not
> > respected, yet the remount will still return success. This patch adds
> > a remount_fs sb op for NFS that checks any new nfs mount options against
> > the existing ones and fails the mount if any have changed.
> >
> > This is only implemented for string-based mount options since doing
> > this with binary options isn't really feasible.
>
> It might be feasible if mount.nfs were smart enough to read /etc/mtab
> and merge the old mount options with the remount options.

Yeah, but in that case you'd need a new kernel + a new nfs-utils. If
you're going to do that then you might as well just use the (far
superior) string based options anyway.

IMO, it's not worth the effort to implement this for binary options...

--
Jeff Layton <[email protected]>

2008-04-11 19:19:15

by Peter Staubach

[permalink] [raw]
Subject: Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

Jeff Layton wrote:
> When remounting an NFS or NFS4 filesystem, the new NFS options are not
> respected, yet the remount will still return success. This patch adds
> a remount_fs sb op for NFS that checks any new nfs mount options against
> the existing ones and fails the mount if any have changed.
>
> This is only implemented for string-based mount options since doing
> this with binary options isn't really feasible.

What about respecting the new options as makes sense and rejecting
those which absolutely can't be changed dynamically?

ps

2008-04-11 19:32:27

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] NFS: implement option checking when remounting NFS filesystems


On Fri, 2008-04-11 at 15:19 -0400, Peter Staubach wrote:
> Jeff Layton wrote:
> > When remounting an NFS or NFS4 filesystem, the new NFS options are not
> > respected, yet the remount will still return success. This patch adds
> > a remount_fs sb op for NFS that checks any new nfs mount options against
> > the existing ones and fails the mount if any have changed.
> >
> > This is only implemented for string-based mount options since doing
> > this with binary options isn't really feasible.
>
> What about respecting the new options as makes sense and rejecting
> those which absolutely can't be changed dynamically?

If we were to do this, then how should superblocks that are shared
between multiple mountpoints behave?

Cheers
Trond

2008-04-11 19:47:21

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH] NFS: implement option checking when remounting NFS filesystems

On Fri, 11 Apr 2008 15:32:27 -0400
Trond Myklebust <[email protected]> wrote:

>
> On Fri, 2008-04-11 at 15:19 -0400, Peter Staubach wrote:
> > Jeff Layton wrote:
> > > When remounting an NFS or NFS4 filesystem, the new NFS options are not
> > > respected, yet the remount will still return success. This patch adds
> > > a remount_fs sb op for NFS that checks any new nfs mount options against
> > > the existing ones and fails the mount if any have changed.
> > >
> > > This is only implemented for string-based mount options since doing
> > > this with binary options isn't really feasible.
> >
> > What about respecting the new options as makes sense and rejecting
> > those which absolutely can't be changed dynamically?
>
> If we were to do this, then how should superblocks that are shared
> between multiple mountpoints behave?
>

I was thinking the same thing and it occurs to me that that's an
existing bug that's not addressed by this patch. Right now, if you have
2 mounts that share a superblock, you can remount one with 'ro' and
*both* will end up being 'ro'.

I think I need to add a check for the sb reference count and return
-EBUSY or something if the sb is shared.

But, back to Peter's question...

In the case of a non-shared superblock, there aren't many options that
I can see that we can just apply. Some of the flags might be doable,
along with some of the ac* settings.

Changing the rsize/wsize means doing a fsinfo call, I think. The timeo
and retries are also tricky. If you have 2 separate sb's sharing a
nfs_client, then you have to be careful.

If we want to do that, then we certainly can consider it in the future,
this patch (with a check for shared sb's) should give us a starting
point if we want to do that.

--
Jeff Layton <[email protected]>