| Message ID | 1312197013.3800.1.camel@sprabhu.fab.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 01, 2011 at 12:10:12PM +0100, Sachin Prabhu wrote: > Do not allow multiple mounts on same mountpoint when using -o noac The patch content really doesn't seem to match the subject line and most of the content. > However when using the option 'noac', the user is able to mount the same > share on the same mountpoint multiple times. This happens because a > share mounted with the noac option is automatically assigned the 'sync' > flag MS_SYNCHRONOUS in nfs_initialise_sb(). This flag is set after the > check for already existing superblocks is done in sget(). The check for > the mount flags in nfs_compare_mount_options() does not take into > account the 'sync' flag applied later on in the code path. This means > that when using 'noac', a new superblock structure is assigned for every > new mount of the same share and multiple shares on the same mountpoint > are allowed. > > ie. > # mount -onoac localhost:/nfsv3 /mnt > can be run multiple times. > The patch checks for noac and assigns the sync flag before sget() is > called to obtain an already existing superblock structure. That's a fine fix, but the the whole patch subject and description focusses on a side effect rather than the underlying real problem. The underlying issue is that youwant to share superblocks when having multiple mounts with -o noac, which requires assigning the sync flag ealier. The check in do_add_mount simply prevents mounts with the same superblock and root dentry at the same mountpoint, so it will fail for any two nfs mounts that might be from the same server but do not share the superblock due to different options. I'm actually not sure we want to prevent that in general, and I can't see how it could be done easily. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2011-08-01 at 07:19 -0400, Christoph Hellwig wrote: > On Mon, Aug 01, 2011 at 12:10:12PM +0100, Sachin Prabhu wrote: > > Do not allow multiple mounts on same mountpoint when using -o noac > > The patch content really doesn't seem to match the subject line and > most of the content. > > > However when using the option 'noac', the user is able to mount the same > > share on the same mountpoint multiple times. This happens because a > > share mounted with the noac option is automatically assigned the 'sync' > > flag MS_SYNCHRONOUS in nfs_initialise_sb(). This flag is set after the > > check for already existing superblocks is done in sget(). The check for > > the mount flags in nfs_compare_mount_options() does not take into > > account the 'sync' flag applied later on in the code path. This means > > that when using 'noac', a new superblock structure is assigned for every > > new mount of the same share and multiple shares on the same mountpoint > > are allowed. > > > > ie. > > # mount -onoac localhost:/nfsv3 /mnt > > can be run multiple times. > > > The patch checks for noac and assigns the sync flag before sget() is > > called to obtain an already existing superblock structure. > > That's a fine fix, but the the whole patch subject and description > focusses on a side effect rather than the underlying real problem. > > The underlying issue is that youwant to share superblocks when > having multiple mounts with -o noac, which requires assigning the > sync flag ealier. > > The check in do_add_mount simply prevents mounts with the same > superblock and root dentry at the same mountpoint, so it will fail for > any two nfs mounts that might be from the same server but do not share > the superblock due to different options. I'm actually not sure we want > to prevent that in general, and I can't see how it could be done easily. > Yes. That is essentially what the problem is. The problem was reported by a user who noticed that passing the 'noac' mount option allowed them to repeatedly mount the same share on the same mount point. I just used the same perspective when reporting the issue upstream. Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/super.c b/fs/nfs/super.c index b961cea..9b7dd70 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -2035,9 +2035,6 @@ static inline void nfs_initialise_sb(struct super_block *sb) sb->s_blocksize = nfs_block_bits(server->wsize, &sb->s_blocksize_bits); - if (server->flags & NFS_MOUNT_NOAC) - sb->s_flags |= MS_SYNCHRONOUS; - sb->s_bdi = &server->backing_dev_info; nfs_super_set_maxbytes(sb, server->maxfilesize); @@ -2249,6 +2246,10 @@ static struct dentry *nfs_fs_mount(struct file_system_type *fs_type, if (server->flags & NFS_MOUNT_UNSHARED) compare_super = NULL; + /* -o noac implies -o sync */ + if (server->flags & NFS_MOUNT_NOAC) + sb_mntdata.mntflags |= MS_SYNCHRONOUS; + /* Get a superblock - note that we may end up sharing one that already exists */ s = sget(fs_type, compare_super, nfs_set_super, &sb_mntdata); if (IS_ERR(s)) { @@ -2361,6 +2362,10 @@ nfs_xdev_mount(struct file_system_type *fs_type, int flags, if (server->flags & NFS_MOUNT_UNSHARED) compare_super = NULL; + /* -o noac implies -o sync */ + if (server->flags & NFS_MOUNT_NOAC) + sb_mntdata.mntflags |= MS_SYNCHRONOUS; + /* Get a superblock - note that we may end up sharing one that already exists */ s = sget(&nfs_fs_type, compare_super, nfs_set_super, &sb_mntdata); if (IS_ERR(s)) { @@ -2628,6 +2633,10 @@ nfs4_remote_mount(struct file_system_type *fs_type, int flags, if (server->flags & NFS4_MOUNT_UNSHARED) compare_super = NULL; + /* -o noac implies -o sync */ + if (server->flags & NFS_MOUNT_NOAC) + sb_mntdata.mntflags |= MS_SYNCHRONOUS; + /* Get a superblock - note that we may end up sharing one that already exists */ s = sget(&nfs4_fs_type, compare_super, nfs_set_super, &sb_mntdata); if (IS_ERR(s)) { @@ -2916,6 +2925,10 @@ nfs4_xdev_mount(struct file_system_type *fs_type, int flags, if (server->flags & NFS4_MOUNT_UNSHARED) compare_super = NULL; + /* -o noac implies -o sync */ + if (server->flags & NFS_MOUNT_NOAC) + sb_mntdata.mntflags |= MS_SYNCHRONOUS; + /* Get a superblock - note that we may end up sharing one that already exists */ s = sget(&nfs4_fs_type, compare_super, nfs_set_super, &sb_mntdata); if (IS_ERR(s)) { @@ -3003,6 +3016,10 @@ nfs4_remote_referral_mount(struct file_system_type *fs_type, int flags, if (server->flags & NFS4_MOUNT_UNSHARED) compare_super = NULL; + /* -o noac implies -o sync */ + if (server->flags & NFS_MOUNT_NOAC) + sb_mntdata.mntflags |= MS_SYNCHRONOUS; + /* Get a superblock - note that we may end up sharing one that already exists */ s = sget(&nfs4_fs_type, compare_super, nfs_set_super, &sb_mntdata); if (IS_ERR(s)) {