Unionfs: bugfix when mounting readonly exported NFS volumes (was: nfsro)

When stacking on top of readonly exported NFS volumes, which are mounted
locally read-write, attempting to create a new file yields the proper EROFS.
But attempting to modify an existing file returns an EACCES, which
interferes with unionfs's copyup policy: only EROFS triggers a copyup,
whereas EACCES does not (and shouldn't -- that'd be a security hole).  The
old unionfs 1.x attempted to workaround this EACCES condition by supporting
a special unionfs mount option called 'nfsro'; support for this option was
left in the latest unionfs 2.0, but the mount option was not made available
until we could properly investigate this issue with the latest NFS code.

This patch removes all remnants of this 'nfsro' support.  It is no longer
needed.  Instead, users can use the existing per-branch 'ro' unionfs mount
option, which would properly return the appropriate status conditions back
from unionfs_permission.  These return conditions result in a copyup if and
only if needed, even for readonly exported NFS volumes.  In effect, unionfs
per-branch 'ro' option now simulates a true readonly localhost mount.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index 401c62ba7a5e7709751aaf3a581de59a6cf14f24..53d373a13d854ec9129aeb33b27f8125f8a93cfe 100644 (file)
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -967,26 +967,32 @@ static void unionfs_put_link(struct dentry *dentry, struct nameidata *nd,
  * Basically copied from the kernel vfs permission(), but we've changed
  * the following:
  *   (1) the IS_RDONLY check is skipped, and
- *   (2) if you set the mount option `mode=nfsro', we assume that -EACCES
- *   means that the export is read-only and we should check standard Unix
- *   permissions.  This means that NFS ACL checks (or other advanced
- *   permission features) are bypassed. Note however, that we do call
- *   security_inode_permission, and therefore security inside SELinux, etc.
- *   are performed.
+ *   (2) We return 0 (success) if the non-leftmost branch is mounted
+ *       readonly, to allow copyup to work.
+ *   (3) we do call security_inode_permission, and therefore security inside
+ *       SELinux, etc. are performed.
  */
-static int inode_permission(struct inode *inode, int mask,
+static int inode_permission(struct super_block *sb, struct inode *inode, int mask,
                            struct nameidata *nd, int bindex)
 {
        int retval, submask;
 
        if (mask & MAY_WRITE) {
+               umode_t mode = inode->i_mode;
                /* The first branch is allowed to be really readonly. */
-               if (bindex == 0) {
-                       umode_t mode = inode->i_mode;
-                       if (IS_RDONLY(inode) &&
-                           (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
-                               return -EROFS;
-               }
+               if (bindex == 0 &&
+                   IS_RDONLY(inode) &&
+                   (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
+                       return -EROFS;
+               /*
+                * For all other branches than the first one, we ignore
+                * EROFS or if the branch is mounted as readonly, to let
+                * copyup take place.
+                */
+               if (bindex > 0 &&
+                   is_robranch_super(sb, bindex) &&
+                   (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)))
+                       return 0;
                /*
                 * Nobody gets write access to an immutable file.
                 */
@@ -996,18 +1002,9 @@ static int inode_permission(struct inode *inode, int mask,
 
        /* Ordinary permission routines do not understand MAY_APPEND. */
        submask = mask & ~MAY_APPEND;
-       if (inode->i_op && inode->i_op->permission) {
+       if (inode->i_op && inode->i_op->permission)
                retval = inode->i_op->permission(inode, submask, nd);
-               if ((retval == -EACCES) && (submask & MAY_WRITE) &&
-                   (!strcmp("nfs", (inode)->i_sb->s_type->name)) &&
-                   (nd) && (nd->mnt) && (nd->mnt->mnt_sb)) {
-                       int perms;
-                       perms = branchperms(nd->mnt->mnt_sb, bindex);
-                       if (perms & MAY_NFSRO)
-                               retval = generic_permission(inode, submask,
-                                                           NULL);
-               }
-       } else
+       else
                retval = generic_permission(inode, submask, NULL);
 
        if (retval && retval != -EROFS) /* ignore EROFS */
@@ -1065,7 +1062,7 @@ static int unionfs_permission(struct inode *inode, int mask,
                 * We use our own special version of permission, such that
                 * only the first branch returns -EROFS.
                 */
-               err = inode_permission(lower_inode, mask, nd, bindex);
+               err = inode_permission(inode->i_sb, lower_inode, mask, nd, bindex);
 
                /*
                 * The permissions are an intersection of the overall directory

diff --git a/include/linux/union_fs.h b/include/linux/union_fs.h
index 223ccabf43ae710b0b3a54ff9bb3bb90ca75d9ca..9bc4e3b644f01e96834a11ae49331c51942ef025 100644 (file)
--- a/include/linux/union_fs.h
+++ b/include/linux/union_fs.h
@@ -22,8 +22,5 @@
 /* We don't support normal remount, but unionctl uses it. */
 # define UNIONFS_REMOUNT_MAGIC         0x4a5a4380
 
-/* should be at least LAST_USED_UNIONFS_PERMISSION<<1 */
-#define MAY_NFSRO                      16
-
 #endif /* _LINUX_UNIONFS_H */