Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1573733 > unrolled thread

Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-04 20:20 +0100
    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Amir Goldstein <amir73il@gmail.com> - 2017-02-05 09:00 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-06 02:20 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Amir Goldstein <amir73il@gmail.com> - 2017-02-06 08:00 +0100
          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-06 15:50 +0100
    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount "J. R. Okajima" <hooanon05g@gmail.com> - 2017-02-06 04:30 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Amir Goldstein <amir73il@gmail.com> - 2017-02-06 07:40 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-06 17:40 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-06 07:50 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Theodore Ts'o <tytso@mit.edu> - 2017-02-06 16:00 +0100
          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-06 16:20 +0100
            Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount lkml@pengaru.com - 2017-02-06 16:40 +0100
              Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-06 18:40 +0100
            Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount bfields@fieldses.org (J. Bruce Fields) - 2017-02-06 23:00 +0100
              Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-07 01:20 +0100
                Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount "J. Bruce Fields" <bfields@fieldses.org> - 2017-02-07 02:40 +0100
                  Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-07 20:10 +0100
                    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Christoph Hellwig <hch@infradead.org> - 2017-02-07 20:50 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount "J. R. Okajima" <hooanon05g@gmail.com> - 2017-02-06 17:30 +0100
    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Christoph Hellwig <hch@infradead.org> - 2017-02-07 10:20 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Djalal Harouni <tixxdz@gmail.com> - 2017-02-07 10:40 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Christoph Hellwig <hch@infradead.org> - 2017-02-07 11:00 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-07 17:40 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Amir Goldstein <amir73il@gmail.com> - 2017-02-07 19:00 +0100
          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Christoph Hellwig <hch@infradead.org> - 2017-02-07 19:20 +0100
            Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-07 20:10 +0100
              Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Christoph Hellwig <hch@infradead.org> - 2017-02-07 21:00 +0100
                Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-07 21:10 +0100
                  Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Amir Goldstein <amir73il@gmail.com> - 2017-02-07 22:10 +0100
                    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Christoph Hellwig <hch@infradead.org> - 2017-02-07 23:50 +0100
                      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-08 00:50 +0100
                        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Amir Goldstein <amir73il@gmail.com> - 2017-02-08 08:00 +0100
                          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Konstantin Khlebnikov <khlebnikov@yandex-team.ru> - 2017-02-08 13:10 +0100
                          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-08 16:00 +0100
                          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-08 16:30 +0100
                Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Josh Triplett <josh@joshtriplett.org> - 2017-02-08 03:00 +0100
                  Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-08 16:30 +0100
                    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Josh Triplett <josh@joshtriplett.org> - 2017-02-09 11:40 +0100
                      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-09 17:40 +0100
                        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount ebiederm@xmission.com (Eric W. Biederman) - 2017-02-13 11:30 +0100
          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-07 19:30 +0100
            Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Djalal Harouni <tixxdz@gmail.com> - 2017-02-07 20:50 +0100

Page 1 of 3  [1] 2 3  Next page →

#1573733 — Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount

This allows any subtree to be uid/gid shifted and bound elsewhere.  It
does this by operating simlarly to overlayfs.  Its primary use is for
shifting the underlying uids of filesystems used to support
unpriviliged (uid shifted) containers.  The usual use case here is
that the container is operating with an uid shifted unprivileged root
but sometimes needs to make use of or work with a filesystem image
that has root at real uid 0.

The mechanism is to allow any subordinate mount namespace to mount a
shiftfs filesystem (by marking it FS_USERNS_MOUNT) but only allowing
it to mount marked subtrees (using the -o mark option as root).  Once
mounted, the subtree is mapped via the super block user namespace so
that the interior ids of the mounting user namespace are the ids
written to the filesystem.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

v1: - based on original shiftfs with uid mappings now done via s_user_ns
---
 fs/Kconfig                 |   8 +
 fs/Makefile                |   1 +
 fs/shiftfs.c               | 728 +++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/magic.h |   2 +
 4 files changed, 739 insertions(+)
 create mode 100644 fs/shiftfs.c

diff --git a/fs/Kconfig b/fs/Kconfig
index c2a377c..b6adac0 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -104,6 +104,14 @@ source "fs/autofs4/Kconfig"
 source "fs/fuse/Kconfig"
 source "fs/overlayfs/Kconfig"
 
+config SHIFT_FS
+	tristate "UID/GID shifting overlay filesystem for containers"
+	help
+	  This filesystem can overlay any mounted filesystem and shift
+	  the uid/gid the files appear at.  The idea is that
+	  unprivileged containers can use this to mount root volumes
+	  using this technique.
+
 menu "Caches"
 
 source "fs/fscache/Kconfig"
diff --git a/fs/Makefile b/fs/Makefile
index 7bbaca9..2aa3ad4 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -128,3 +128,4 @@ obj-y				+= exofs/ # Multiple modules
 obj-$(CONFIG_CEPH_FS)		+= ceph/
 obj-$(CONFIG_PSTORE)		+= pstore/
 obj-$(CONFIG_EFIVAR_FS)		+= efivarfs/
+obj-$(CONFIG_SHIFT_FS)		+= shiftfs.o
diff --git a/fs/shiftfs.c b/fs/shiftfs.c
new file mode 100644
index 0000000..a4a1f98
--- /dev/null
+++ b/fs/shiftfs.c
@@ -0,0 +1,728 @@
+#include <linux/cred.h>
+#include <linux/mount.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/namei.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/magic.h>
+#include <linux/parser.h>
+#include <linux/seq_file.h>
+#include <linux/statfs.h>
+#include <linux/slab.h>
+#include <linux/user_namespace.h>
+#include <linux/uidgid.h>
+#include <linux/xattr.h>
+
+struct shiftfs_super_info {
+	struct vfsmount *mnt;
+	struct user_namespace *userns;
+	bool mark;
+};
+
+static struct inode *shiftfs_new_inode(struct super_block *sb, umode_t mode,
+				       struct dentry *dentry);
+
+enum {
+	OPT_MARK,
+	OPT_LAST,
+};
+
+/* global filesystem options */
+static const match_table_t tokens = {
+	{ OPT_MARK, "mark" },
+	{ OPT_LAST, NULL }
+};
+
+static const struct cred *shiftfs_get_up_creds(struct super_block *sb)
+{
+	struct shiftfs_super_info *ssi = sb->s_fs_info;
+	struct cred *cred = prepare_creds();
+
+	if (!cred)
+		return NULL;
+
+	cred->fsuid = KUIDT_INIT(from_kuid(sb->s_user_ns, cred->fsuid));
+	cred->fsgid = KGIDT_INIT(from_kgid(sb->s_user_ns, cred->fsgid));
+	cred->user_ns = ssi->userns;
+
+	return cred;
+}
+
+static const struct cred *shiftfs_new_creds(const struct cred **newcred,
+					    struct super_block *sb)
+{
+	const struct cred *cred = shiftfs_get_up_creds(sb);
+
+	*newcred = cred;
+
+	if (cred)
+		cred = override_creds(cred);
+	else
+		printk(KERN_ERR "shiftfs: Credential override failed: no memory\n");
+
+	return cred;
+}
+
+static void shiftfs_old_creds(const struct cred *oldcred,
+			      const struct cred **newcred)
+{
+	if (!*newcred)
+		return;
+
+	revert_creds(oldcred);
+	put_cred(*newcred);
+}
+
+static int shiftfs_parse_options(struct shiftfs_super_info *ssi, char *options)
+{
+	char *p;
+	substring_t args[MAX_OPT_ARGS];
+
+	ssi->mark = false;
+
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
+
+		if (!*p)
+			continue;
+
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case OPT_MARK:
+			ssi->mark = true;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
+static void shiftfs_d_release(struct dentry *dentry)
+{
+	struct dentry *real = dentry->d_fsdata;
+
+	dput(real);
+}
+
+static struct dentry *shiftfs_d_real(struct dentry *dentry,
+				     const struct inode *inode,
+				     unsigned int flags)
+{
+	struct dentry *real = dentry->d_fsdata;
+
+	if (unlikely(real->d_flags & DCACHE_OP_REAL))
+		return real->d_op->d_real(real, real->d_inode, flags);
+
+	return real;
+}
+
+static const struct dentry_operations shiftfs_dentry_ops = {
+	.d_release	= shiftfs_d_release,
+	.d_real		= shiftfs_d_real,
+};
+
+static int shiftfs_readlink(struct dentry *dentry, char __user *data,
+			    int flags)
+{
+	struct dentry *real = dentry->d_fsdata;
+	const struct inode_operations *iop = real->d_inode->i_op;
+
+	if (iop->readlink)
+		return iop->readlink(real, data, flags);
+
+	return -EINVAL;
+}
+
+static const char *shiftfs_get_link(struct dentry *dentry, struct inode *inode,
+				    struct delayed_call *done)
+{
+	if (dentry) {
+		struct dentry *real = dentry->d_fsdata;
+		struct inode *reali = real->d_inode;
+		const struct inode_operations *iop = reali->i_op;
+		const char *res = ERR_PTR(-EPERM);
+
+		if (iop->get_link)
+			res = iop->get_link(real, reali, done);
+
+		return res;
+	} else {
+		/* RCU lookup not supported */
+		return ERR_PTR(-ECHILD);
+	}
+}
+
+static int shiftfs_setxattr(struct dentry *dentry, struct inode *inode,
+			    const char *name, const void *value,
+			    size_t size, int flags)
+{
+	struct dentry *real = dentry->d_fsdata;
+	int err = -EOPNOTSUPP;
+	const struct cred *oldcred, *newcred;
+
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+	err = vfs_setxattr(real, name, value, size, flags);
+	shiftfs_old_creds(oldcred, &newcred);
+
+	return err;
+}
+
+static int shiftfs_xattr_get(const struct xattr_handler *handler,
+			     struct dentry *dentry, struct inode *inode,
+			     const char *name, void *value, size_t size)
+{
+	struct dentry *real = dentry->d_fsdata;
+	int err;
+	const struct cred *oldcred, *newcred;
+
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+	err = vfs_getxattr(real, name, value, size);
+	shiftfs_old_creds(oldcred, &newcred);
+
+	return err;
+}
+
+static ssize_t shiftfs_listxattr(struct dentry *dentry, char *list,
+				 size_t size)
+{
+	struct dentry *real = dentry->d_fsdata;
+	int err;
+	const struct cred *oldcred, *newcred;
+
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+	err = vfs_listxattr(real, list, size);
+	shiftfs_old_creds(oldcred, &newcred);
+
+	return err;
+}
+
+static int shiftfs_removexattr(struct dentry *dentry, const char *name)
+{
+	struct dentry *real = dentry->d_fsdata;
+	int err;
+	const struct cred *oldcred, *newcred;
+
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+	err = vfs_removexattr(real, name);
+	shiftfs_old_creds(oldcred, &newcred);
+
+	return err;
+}
+
+static int shiftfs_xattr_set(const struct xattr_handler *handler,
+			     struct dentry *dentry, struct inode *inode,
+			     const char *name, const void *value, size_t size,
+			     int flags)
+{
+	if (!value)
+		return shiftfs_removexattr(dentry, name);
+	return shiftfs_setxattr(dentry, inode, name, value, size, flags);
+}
+
+static void shiftfs_fill_inode(struct inode *inode, struct dentry *dentry)
+{
+	struct inode *reali;
+
+	if (!dentry)
+		return;
+
+	reali = dentry->d_inode;
+
+	if (!reali->i_op->get_link)
+		inode->i_opflags |= IOP_NOFOLLOW;
+
+	inode->i_mapping = reali->i_mapping;
+	inode->i_private = dentry;
+
+	i_uid_write(inode, __kuid_val(reali->i_uid));
+	i_gid_write(inode, __kgid_val(reali->i_gid));
+}
+
+static int shiftfs_make_object(struct inode *dir, struct dentry *dentry,
+			       umode_t mode, const char *symlink,
+			       struct dentry *hardlink, bool excl)
+{
+	struct dentry *real = dir->i_private, *new = dentry->d_fsdata;
+	struct inode *reali = real->d_inode, *newi;
+	const struct inode_operations *iop = reali->i_op;
+	int err;
+	const struct cred *oldcred, *newcred;
+	bool op_ok = false;
+
+	if (hardlink) {
+		op_ok = iop->link;
+	} else {
+		switch (mode & S_IFMT) {
+		case S_IFDIR:
+			op_ok = iop->mkdir;
+			break;
+		case S_IFREG:
+			op_ok = iop->create;
+			break;
+		case S_IFLNK:
+			op_ok = iop->symlink;
+		}
+	}
+	if (!op_ok)
+		return -EINVAL;
+
+
+	newi = shiftfs_new_inode(dentry->d_sb, mode, NULL);
+	if (!newi)
+		return -ENOMEM;
+
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+
+	inode_lock_nested(reali, I_MUTEX_PARENT);
+
+	err = -EINVAL;		/* shut gcc up about uninit var */
+	if (hardlink) {
+		struct dentry *realhardlink = hardlink->d_fsdata;
+
+		err = vfs_link(realhardlink, reali, new, NULL);
+	} else {
+		switch (mode & S_IFMT) {
+		case S_IFDIR:
+			err = vfs_mkdir(reali, new, mode);
+			break;
+		case S_IFREG:
+			err = vfs_create(reali, new, mode, excl);
+			break;
+		case S_IFLNK:
+			err = vfs_symlink(reali, new, symlink);
+		}
+	}
+
+	shiftfs_old_creds(oldcred, &newcred);
+
+	if (err)
+		goto out_dput;
+
+	shiftfs_fill_inode(newi, new);
+
+	d_instantiate(dentry, newi);
+
+	new = NULL;
+	newi = NULL;
+
+ out_dput:
+	dput(new);
+	iput(newi);
+	inode_unlock(reali);
+
+	return err;
+}
+
+static int shiftfs_create(struct inode *dir, struct dentry *dentry,
+			  umode_t mode,  bool excl)
+{
+	mode |= S_IFREG;
+
+	return shiftfs_make_object(dir, dentry, mode, NULL, NULL, excl);
+}
+
+static int shiftfs_mkdir(struct inode *dir, struct dentry *dentry,
+			 umode_t mode)
+{
+	mode |= S_IFDIR;
+
+	return shiftfs_make_object(dir, dentry, mode, NULL, NULL, false);
+}
+
+static int shiftfs_link(struct dentry *hardlink, struct inode *dir,
+			struct dentry *dentry)
+{
+	return shiftfs_make_object(dir, dentry, 0, NULL, hardlink, false);
+}
+
+static int shiftfs_symlink(struct inode *dir, struct dentry *dentry,
+			   const char *symlink)
+{
+	return shiftfs_make_object(dir, dentry, S_IFLNK, symlink, NULL, false);
+}
+
+static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
+{
+	struct dentry *real = dir->i_private, *new = dentry->d_fsdata;
+	struct inode *reali = real->d_inode;
+	int err;
+	const struct cred *oldcred, *newcred;
+
+	inode_lock_nested(reali, I_MUTEX_PARENT);
+
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+
+	if (rmdir)
+		err = vfs_rmdir(reali, new);
+	else
+		err = vfs_unlink(reali, new, NULL);
+
+	shiftfs_old_creds(oldcred, &newcred);
+	inode_unlock(reali);
+
+	return err;
+}
+
+static int shiftfs_unlink(struct inode *dir, struct dentry *dentry)
+{
+	return shiftfs_rm(dir, dentry, false);
+}
+
+static int shiftfs_rmdir(struct inode *dir, struct dentry *dentry)
+{
+	return shiftfs_rm(dir, dentry, true);
+}
+
+static int shiftfs_rename(struct inode *olddir, struct dentry *old,
+			  struct inode *newdir, struct dentry *new,
+			  unsigned int flags)
+{
+	struct dentry *rodd = olddir->i_private, *rndd = newdir->i_private,
+		*realold = old->d_fsdata,
+		*realnew = new->d_fsdata, *trap;
+	struct inode *realolddir = rodd->d_inode, *realnewdir = rndd->d_inode;
+	int err = -EINVAL;
+	const struct cred *oldcred, *newcred;
+
+	trap = lock_rename(rndd, rodd);
+
+	if (trap == realold || trap == realnew)
+		goto out_unlock;
+
+	oldcred = shiftfs_new_creds(&newcred, old->d_sb);
+
+	err = vfs_rename(realolddir, realold, realnewdir,
+			 realnew, NULL, flags);
+
+	shiftfs_old_creds(oldcred, &newcred);
+
+ out_unlock:
+	unlock_rename(rndd, rodd);
+
+	return err;
+}
+
+static struct dentry *shiftfs_lookup(struct inode *dir, struct dentry *dentry,
+				     unsigned int flags)
+{
+	struct dentry *real = dir->i_private, *new;
+	struct inode *reali = real->d_inode, *newi;
+	const struct cred *oldcred, *newcred;
+
+	inode_lock(reali);
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+	new = lookup_one_len(dentry->d_name.name, real, dentry->d_name.len);
+	shiftfs_old_creds(oldcred, &newcred);
+	inode_unlock(reali);
+
+	if (IS_ERR(new))
+		return new;
+
+	dentry->d_fsdata = new;
+
+	if (!new->d_inode)
+		return NULL;
+
+	newi = shiftfs_new_inode(dentry->d_sb, new->d_inode->i_mode, new);
+	if (!newi) {
+		dput(new);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	d_splice_alias(newi, dentry);
+
+	return NULL;
+}
+
+static int shiftfs_permission(struct inode *inode, int mask)
+{
+	struct dentry *real = inode->i_private;
+	struct inode *reali = real->d_inode;
+	const struct inode_operations *iop = reali->i_op;
+	int err;
+	const struct cred *oldcred, *newcred;
+
+	if (mask & MAY_NOT_BLOCK)
+		return -ECHILD;
+
+	oldcred = shiftfs_new_creds(&newcred, inode->i_sb);
+	if (iop->permission)
+		err = iop->permission(reali, mask);
+	else
+		err = generic_permission(reali, mask);
+	shiftfs_old_creds(oldcred, &newcred);
+
+	return err;
+}
+
+static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
+{
+	struct dentry *real = dentry->d_fsdata;
+	struct inode *reali = real->d_inode;
+	const struct inode_operations *iop = reali->i_op;
+	struct iattr newattr = *attr;
+	const struct cred *oldcred, *newcred;
+	struct super_block *sb = dentry->d_sb;
+	int err;
+
+	newattr.ia_uid = KUIDT_INIT(from_kuid(sb->s_user_ns, attr->ia_uid));
+	newattr.ia_gid = KGIDT_INIT(from_kgid(sb->s_user_ns, attr->ia_gid));
+
+	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+	inode_lock(reali);
+	if (iop->setattr)
+		err = iop->setattr(real, &newattr);
+	else
+		err = simple_setattr(real, &newattr);
+	inode_unlock(reali);
+	shiftfs_old_creds(oldcred, &newcred);
+
+	if (err)
+		return err;
+
+	/* all OK, reflect the change on our inode */
+	setattr_copy(d_inode(dentry), attr);
+	return 0;
+}
+
+static int shiftfs_getattr(struct vfsmount *mnt, struct dentry *dentry,
+			   struct kstat *stat)
+{
+	struct inode *inode = dentry->d_inode;
+	struct dentry *real = inode->i_private;
+	struct inode *reali = real->d_inode;
+	const struct inode_operations *iop = reali->i_op;
+	int err = 0;
+
+	mnt = dentry->d_sb->s_fs_info;
+
+	if (iop->getattr)
+		err = iop->getattr(mnt, real, stat);
+	else
+		generic_fillattr(reali, stat);
+
+	if (err)
+		return err;
+
+	stat->uid = inode->i_uid;
+	stat->gid = inode->i_gid;
+	return 0;
+}
+
+static const struct inode_operations shiftfs_inode_ops = {
+	.lookup		= shiftfs_lookup,
+	.getattr	= shiftfs_getattr,
+	.setattr	= shiftfs_setattr,
+	.permission	= shiftfs_permission,
+	.mkdir		= shiftfs_mkdir,
+	.symlink	= shiftfs_symlink,
+	.get_link	= shiftfs_get_link,
+	.readlink	= shiftfs_readlink,
+	.unlink		= shiftfs_unlink,
+	.rmdir		= shiftfs_rmdir,
+	.rename		= shiftfs_rename,
+	.link		= shiftfs_link,
+	.create		= shiftfs_create,
+	.mknod		= NULL,	/* no special files currently */
+	.listxattr	= shiftfs_listxattr,
+};
+
+static struct inode *shiftfs_new_inode(struct super_block *sb, umode_t mode,
+				       struct dentry *dentry)
+{
+	struct inode *inode;
+
+	inode = new_inode(sb);
+	if (!inode)
+		return NULL;
+
+	mode &= S_IFMT;
+
+	inode->i_ino = get_next_ino();
+	inode->i_mode = mode;
+	inode->i_flags |= S_NOATIME | S_NOCMTIME;
+
+	inode->i_op = &shiftfs_inode_ops;
+
+	shiftfs_fill_inode(inode, dentry);
+
+	return inode;
+}
+
+static int shiftfs_show_options(struct seq_file *m, struct dentry *dentry)
+{
+	struct super_block *sb = dentry->d_sb;
+	struct shiftfs_super_info *ssi = sb->s_fs_info;
+
+	if (ssi->mark)
+		seq_show_option(m, "mark", NULL);
+
+	return 0;
+}
+
+static int shiftfs_statfs(struct dentry *dentry, struct kstatfs *buf)
+{
+	struct super_block *sb = dentry->d_sb;
+	struct shiftfs_super_info *ssi = sb->s_fs_info;
+	struct dentry *root = sb->s_root;
+	struct dentry *realroot = root->d_fsdata;
+	struct path realpath = { .mnt = ssi->mnt, .dentry = realroot };
+	int err;
+
+	err = vfs_statfs(&realpath, buf);
+	if (err)
+		return err;
+
+	buf->f_type = sb->s_magic;
+
+	return 0;
+}
+
+static void shiftfs_put_super(struct super_block *sb)
+{
+	struct shiftfs_super_info *ssi = sb->s_fs_info;
+
+	mntput(ssi->mnt);
+	put_user_ns(ssi->userns);
+	kfree(ssi);
+}
+
+static const struct xattr_handler shiftfs_xattr_handler = {
+	.prefix = "",
+	.get    = shiftfs_xattr_get,
+	.set    = shiftfs_xattr_set,
+};
+
+const struct xattr_handler *shiftfs_xattr_handlers[] = {
+	&shiftfs_xattr_handler,
+	NULL
+};
+
+static const struct super_operations shiftfs_super_ops = {
+	.put_super	= shiftfs_put_super,
+	.show_options	= shiftfs_show_options,
+	.statfs		= shiftfs_statfs,
+};
+
+struct shiftfs_data {
+	void *data;
+	const char *path;
+};
+
+static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
+			      int silent)
+{
+	struct shiftfs_data *data = raw_data;
+	char *name = kstrdup(data->path, GFP_KERNEL);
+	int err = -ENOMEM;
+	struct shiftfs_super_info *ssi = NULL;
+	struct path path;
+	struct dentry *dentry;
+
+	if (!name)
+		goto out;
+
+	ssi = kzalloc(sizeof(*ssi), GFP_KERNEL);
+	if (!ssi)
+		goto out;
+
+	err = -EPERM;
+	err = shiftfs_parse_options(ssi, data->data);
+	if (err)
+		goto out;
+
+	/* to mark a mount point, must be real root */
+	if (ssi->mark && !capable(CAP_SYS_ADMIN))
+		goto out;
+
+	/* else to mount a mark, must be userns admin */
+	if (!ssi->mark && !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
+		goto out;
+
+	err = kern_path(name, LOOKUP_FOLLOW, &path);
+	if (err)
+		goto out;
+
+	err = -EPERM;
+	if (!S_ISDIR(path.dentry->d_inode->i_mode)) {
+		err = -ENOTDIR;
+		goto out_put;
+	}
+	if (ssi->mark) {
+		/*
+		 * this part is visible unshifted, so make sure no
+		 * executables that could be used to give suid
+		 * privileges
+		 */
+		sb->s_iflags = SB_I_NOEXEC;
+		ssi->mnt = path.mnt;
+		dentry = path.dentry;
+	} else {
+		struct shiftfs_super_info *mp_ssi;
+
+		/*
+		 * this leg executes if we're admin capable in
+		 * the namespace, so be very careful
+		 */
+		if (path.dentry->d_sb->s_magic != SHIFTFS_MAGIC)
+			goto out_put;
+		mp_ssi = path.dentry->d_sb->s_fs_info;
+		if (!mp_ssi->mark)
+			goto out_put;
+		ssi->mnt = mntget(mp_ssi->mnt);
+		dentry = dget(path.dentry->d_fsdata);
+		path_put(&path);
+	}
+	ssi->userns = get_user_ns(dentry->d_sb->s_user_ns);
+	sb->s_fs_info = ssi;
+	sb->s_magic = SHIFTFS_MAGIC;
+	sb->s_op = &shiftfs_super_ops;
+	sb->s_xattr = shiftfs_xattr_handlers;
+	sb->s_d_op = &shiftfs_dentry_ops;
+	sb->s_root = d_make_root(shiftfs_new_inode(sb, S_IFDIR, dentry));
+	sb->s_root->d_fsdata = dentry;
+
+	return 0;
+
+ out_put:
+	path_put(&path);
+ out:
+	kfree(name);
+	kfree(ssi);
+	return err;
+}
+
+static struct dentry *shiftfs_mount(struct file_system_type *fs_type,
+				    int flags, const char *dev_name, void *data)
+{
+	struct shiftfs_data d = { data, dev_name };
+
+	return mount_nodev(fs_type, flags, &d, shiftfs_fill_super);
+}
+
+static struct file_system_type shiftfs_type = {
+	.owner		= THIS_MODULE,
+	.name		= "shiftfs",
+	.mount		= shiftfs_mount,
+	.kill_sb	= kill_anon_super,
+	.fs_flags	= FS_USERNS_MOUNT,
+};
+
+static int __init shiftfs_init(void)
+{
+	return register_filesystem(&shiftfs_type);
+}
+
+static void __exit shiftfs_exit(void)
+{
+	unregister_filesystem(&shiftfs_type);
+}
+
+MODULE_ALIAS_FS("shiftfs");
+MODULE_AUTHOR("James Bottomley");
+MODULE_DESCRIPTION("uid/gid shifting bind filesystem");
+MODULE_LICENSE("GPL v2");
+module_init(shiftfs_init)
+module_exit(shiftfs_exit)
diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
index e230af2..a2fdb01 100644
--- a/include/uapi/linux/magic.h
+++ b/include/uapi/linux/magic.h
@@ -85,4 +85,6 @@
 #define BALLOON_KVM_MAGIC	0x13661366
 #define ZSMALLOC_MAGIC		0x58295829
 
+#define SHIFTFS_MAGIC		0x6a656a62
+
 #endif /* __LINUX_MAGIC_H__ */
-- 
2.6.6

[toc] | [next] | [standalone]

#1573838

On Sat, Feb 4, 2017 at 9:19 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> This allows any subtree to be uid/gid shifted and bound elsewhere.  It
> does this by operating simlarly to overlayfs.  Its primary use is for
> shifting the underlying uids of filesystems used to support
> unpriviliged (uid shifted) containers.  The usual use case here is
> that the container is operating with an uid shifted unprivileged root
> but sometimes needs to make use of or work with a filesystem image
> that has root at real uid 0.
>
> The mechanism is to allow any subordinate mount namespace to mount a
> shiftfs filesystem (by marking it FS_USERNS_MOUNT) but only allowing
> it to mount marked subtrees (using the -o mark option as root).  Once
> mounted, the subtree is mapped via the super block user namespace so
> that the interior ids of the mounting user namespace are the ids
> written to the filesystem.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>

James,

Allow me to point out some problems in this patch and offer a slightly different
approach.

First of all, the subject says "uid/gid shifting bind mount", but it's not
really a bind mount. What it is is a stackable mount and 2 levels of
stack no less.
So one thing that is missing is increasing of sb->s_stack_depth and that
also means that shiftfs cannot be used to recursively shift uids in child userns
if that was ever the intention.

The other problem is that by forking overlayfs functionality, shiftfs is going
to miss out on overlayfs bug fixes related to user credentials differ
from mounter
credentials, like fd3220d ("ovl: update S_ISGID when setting posix ACLs").
I am not sure that this specific case is relevant to shiftfs, but
there could be other.

So how about, instead of forking a new containers specialized stackable fs,
that the needed functionality be merged into overlayfs code?
I think overlayfs container users may also benefit from shiftfs
functionality, no?
In any case, overlayfs has considerable millage used as fs for containers,
so many issues related to running with different userns may have already been
addressed.

Overlayfs already stores the mounter's credentials and uses them to perform
most of the operations on upper.

I know it wasn't the original purpose of overlayfs to run as a single layer, but
there is nothing really preventing from doing that. In fact, I am
doing just that
with my snapshot mount patches, see:
https://github.com/amir73il/linux/commit/acc6c25eab03c176c9ef736544fab3fba663765d#diff-2b85a3c5bea4263d08a2bdff639192c3
I registered a new fs type ("snapshot"), which reuses most of the existing
overlayfs operations.
With this patch it is possible to mount an overlay with only upper layer, so all
the operations are pass through except for the credentials, e.g.:

mount -t snapshot -o upper=<origin> shiftfs_test <mark location>

If you think this concept is workable, then the functionality of
mounting overlayfs
with only upper should be integrated into plain overlayfs and shiftfs could be
a very thin variant of overlayfs mount using shitfs_fs_type, just for the sake
of having FS_USERNS_MOUNT, e.g:

+ /*
+  * XXX: reusing ovl_mount()/ovl_fill_super(), but could also just reuse
+  * ovl_dentry_operations/ovl_super_operations/ovl_xattr_handlers/ovl_new_inode()
+  */
+static struct file_system_type shiftfs_type = {
+       .owner          = THIS_MODULE,
+       .name           = "shiftfs",
+       .mount          = ovl_mount,
+       .kill_sb        = kill_anon_super,
+       .fs_flags       = FS_USERNS_MOUNT,
+};
+MODULE_ALIAS_FS("shiftfs");
+MODULE_ALIAS("shiftfs");
+#define IS_SHIFTFS_SB(sb) ((sb)->s_type == &shiftfs_type)

And instead of verifying that shiftfs is mounted inside container over shiftfs,
verify that it is mounted over an overlayfs noexec mount e.g.:

+       if (IS_SHIFTFS_SB(sb)) {
+               /*
+                * this leg executes if we're admin capable in
+                * the namespace, so be very careful
+                */
+               if (path.dentry->d_sb->s_magic != OVERLAYFS_MAGIC ||
!(path.dentry->d_sb->s_iflags & SB_I_NOEXEC))
+                       goto out_put;

From users manual POV:

in host:
mount -t overlay -o noexec,upper=<origin> container_visible <mark location>

in container:
mount -t shiftfs -o upper=<mark location> container_writable
<somewhere in my local mount ns>

Thought?

[toc] | [prev] | [next] | [standalone]

#1574315

On Sun, 2017-02-05 at 09:51 +0200, Amir Goldstein wrote:
> On Sat, Feb 4, 2017 at 9:19 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > This allows any subtree to be uid/gid shifted and bound elsewhere. 
> >  It does this by operating simlarly to overlayfs.  Its primary use 
> > is for shifting the underlying uids of filesystems used to support
> > unpriviliged (uid shifted) containers.  The usual use case here is
> > that the container is operating with an uid shifted unprivileged 
> > root but sometimes needs to make use of or work with a filesystem 
> > image that has root at real uid 0.
> > 
> > The mechanism is to allow any subordinate mount namespace to mount 
> > a shiftfs filesystem (by marking it FS_USERNS_MOUNT) but only
> > allowing it to mount marked subtrees (using the -o mark option as 
> > root).   Once mounted, the subtree is mapped via the super block 
> > user namespace so that the interior ids of the mounting user 
> > namespace are the ids written to the filesystem.
> > 
> > Signed-off-by: James Bottomley <
> > James.Bottomley@HansenPartnership.com>
> > 
> 
> James,
> 
> Allow me to point out some problems in this patch and offer a 
> slightly different approach.
> 
> First of all, the subject says "uid/gid shifting bind mount", but 
> it's not really a bind mount. What it is is a stackable mount and 2 
> levels of stack no less.

The reason for the description is to have it behave exactly like a bind
mount.  You can assert that a bind mount is, in fact, a stacked mount,
but we don't currently.  I'm also not sure where you get your 2 levels
from?

>  So one thing that is missing is increasing of sb->s_stack_depth and
> that also means that shiftfs cannot be used to recursively shift uids
> in child userns if that was ever the intention.

I can't think of a use case that would ever need that, but perhaps
other container people can.

> The other problem is that by forking overlayfs functionality,

So this wouldn't really be the right way to look at it: shiftfs shares
no code with overlayfs at all, so is definitely not a fork.  The only
piece of functionality it has which is similar to overlayfs is the way
it does lookups via a new dentry cache.  However, that functionality is
not unique to overlayfs and if you look, you'll see that
shiftfs_lookup() actually has far more in common with
ecryptfs_lookup().

>  shiftfs is going to miss out on overlayfs bug fixes related to user 
> credentials differ from mounter credentials, like fd3220d ("ovl: 
> update S_ISGID when setting posix ACLs"). I am not sure that this 
> specific case is relevant to shiftfs, but there could be other.

OK, so shiftfs doesn't have this bug and the reason why is
illustrative: basically shiftfs does three things

   1. lookups via a uid/gid shifted dentry cache
   2. shifted credential inode operations permission checks on the
      underlying filesystem
   3. location marking for unprivileged mount

I think we've already seen that 1. isn't from overlayfs but the
functionality could be added to overlayfs, I suppose.  The big problem
is 2.  The overlayfs code emulates the permission checks, which makes
it rather complex (this is where you get your bugs like the above
from).  I did actually look at adding 2. to overlayfs on the theory
that a single layer overlay might be closest to what this is, but
eventually concluded I'd have to take the special cases and add a whole
lot more to them ... it really would increase the maintenance burden
substantially and make the code an unreadable rats nest.

When you think about it this way, it becomes obvious that the clean
separation is if shiftfs functionality is layered on top of overlayfs
and when you do that, doing it as its own filesystem is more logical.

> So how about, instead of forking a new containers specialized 
> stackable fs, that the needed functionality be merged into overlayfs 
> code? I think overlayfs container users may also benefit from shiftfs
> functionality, no?

I think I covered the why not merge the code above.  As to the
functionality, since Docker already has a graph driver, the graph
driver can do the shifting on top of the overlays.

>  In any case, overlayfs has considerable millage used as fs for
> containers, so many issues related to running with different userns
> may have already been addressed.

Overlayfs is s_user_ns blind so it's highly unlikely to have seen any
issues with the user namespaces, let alone addressed them.  This will
also be compounded by the fact that its primary user: docker, has
rather a weak use of the user namespace currently.

The other thing is the use case: Most immutable infrastructure
container systems create the overlays in the host and then bind them
into the container.  This binding is an additional mount operation. 
 Now the could mount from an overlay as an overlay but it's adding
complexity because the container itself cannot control the overlay
(it's a host provided thing) so it is definitely cleaner to make the
second mount a different filesystem (i.e. shiftfs) where the nature of
the overlay is hidden from the container.

> Overlayfs already stores the mounter's credentials and uses them to 
> perform most of the operations on upper.

OK, that's case 2. again.  So I think you may be labouring under the
misapprehension that shiftfs and overlayfs do the same thing with
override credentials?  They don't: overlayfs emulates the permission
lookups and then overrides based on *historical* admin credentials to
force what it's already decided on the underlying fielsystems.  Shiftfs
overrides the *current* credentials with a uid/gid and namespace shift
and then runs the permission checks.  Thus if I wanted to add what
shiftfs does to overlayfs, I'd have to add another load of overriding
based on current credentials in the currently unoverriden emulated
permission checks.  I think you can see that simply running the real
permission checks on the underlying filesystem with overridden
credentials is much simpler.

> I know it wasn't the original purpose of overlayfs to run as a single 
> layer, but there is nothing really preventing from doing that. In 
> fact, I am doing just that with my snapshot mount patches, see:
> https://github.com/amir73il/linux/commit/acc6c25eab03c176c9ef736544fa
> b3fba663765d#diff-2b85a3c5bea4263d08a2bdff639192c3
> I registered a new fs type ("snapshot"), which reuses most of the 
> existing overlayfs operations. With this patch it is possible to 
> mount an overlay with only upper layer, so all the operations are
> pass through except for the credentials, e.g.:
> 
> mount -t snapshot -o upper=<origin> shiftfs_test <mark location>

OK, so since you don't need to special case the permission checks, I
can see why this might work for you because you don't need to modify
overlayfs to do this.  Since I can't consume the overlay code as is, it
doesn't work for me because I'd have to add lots of special case code
to it.

James

> If you think this concept is workable, then the functionality of
> mounting overlayfs with only upper should be integrated into plain 
> overlayfs and shiftfs could be a very thin variant of overlayfs mount 
> using shitfs_fs_type, just for the sake of having FS_USERNS_MOUNT,
> e.g:
> 
> + /*
> +  * XXX: reusing ovl_mount()/ovl_fill_super(), but could also just
> reuse
> +  *
> ovl_dentry_operations/ovl_super_operations/ovl_xattr_handlers/ovl_new
> _inode()
> +  */
> +static struct file_system_type shiftfs_type = {
> +       .owner          = THIS_MODULE,
> +       .name           = "shiftfs",
> +       .mount          = ovl_mount,
> +       .kill_sb        = kill_anon_super,
> +       .fs_flags       = FS_USERNS_MOUNT,
> +};
> +MODULE_ALIAS_FS("shiftfs");
> +MODULE_ALIAS("shiftfs");
> +#define IS_SHIFTFS_SB(sb) ((sb)->s_type == &shiftfs_type)
> 
> And instead of verifying that shiftfs is mounted inside container
> over shiftfs,
> verify that it is mounted over an overlayfs noexec mount e.g.:
> 
> +       if (IS_SHIFTFS_SB(sb)) {
> +               /*
> +                * this leg executes if we're admin capable in
> +                * the namespace, so be very careful
> +                */
> +               if (path.dentry->d_sb->s_magic != OVERLAYFS_MAGIC ||
> !(path.dentry->d_sb->s_iflags & SB_I_NOEXEC))
> +                       goto out_put;
> 
> From users manual POV:
> 
> in host:
> mount -t overlay -o noexec,upper=<origin> container_visible <mark
> location>
> 
> in container:
> mount -t shiftfs -o upper=<mark location> container_writable
> <somewhere in my local mount ns>
> 
> Thought?
> 

[toc] | [prev] | [next] | [standalone]

#1574388

On Mon, Feb 6, 2017 at 3:18 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Sun, 2017-02-05 at 09:51 +0200, Amir Goldstein wrote:
>> On Sat, Feb 4, 2017 at 9:19 PM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>> > This allows any subtree to be uid/gid shifted and bound elsewhere.
>> >  It does this by operating simlarly to overlayfs.  Its primary use
>> > is for shifting the underlying uids of filesystems used to support
>> > unpriviliged (uid shifted) containers.  The usual use case here is
>> > that the container is operating with an uid shifted unprivileged
>> > root but sometimes needs to make use of or work with a filesystem
>> > image that has root at real uid 0.
>> >
>> > The mechanism is to allow any subordinate mount namespace to mount
>> > a shiftfs filesystem (by marking it FS_USERNS_MOUNT) but only
>> > allowing it to mount marked subtrees (using the -o mark option as
>> > root).   Once mounted, the subtree is mapped via the super block
>> > user namespace so that the interior ids of the mounting user
>> > namespace are the ids written to the filesystem.
>> >
>> > Signed-off-by: James Bottomley <
>> > James.Bottomley@HansenPartnership.com>
>> >
>>
>> James,
>>
>> Allow me to point out some problems in this patch and offer a
>> slightly different approach.
>>
>> First of all, the subject says "uid/gid shifting bind mount", but
>> it's not really a bind mount. What it is is a stackable mount and 2
>> levels of stack no less.
>
> The reason for the description is to have it behave exactly like a bind
> mount.  You can assert that a bind mount is, in fact, a stacked mount,
> but we don't currently.  I'm also not sure where you get your 2 levels
> from?
>

A bind mount does not incur recursion into VFS code, a stacked fs does.
And there is a programmable limit of stack depth of 2, which stacked
fs need to comply with.
Your proposed setup has 2 stacked fs, the mark shitfs by admin
and the uid shitfs by container user. Or maybe I misunderstood.


>>  So one thing that is missing is increasing of sb->s_stack_depth and
>> that also means that shiftfs cannot be used to recursively shift uids
>> in child userns if that was ever the intention.
>
> I can't think of a use case that would ever need that, but perhaps
> other container people can.
>
>> The other problem is that by forking overlayfs functionality,
>
> So this wouldn't really be the right way to look at it: shiftfs shares
> no code with overlayfs at all, so is definitely not a fork.  The only
> piece of functionality it has which is similar to overlayfs is the way
> it does lookups via a new dentry cache.  However, that functionality is
> not unique to overlayfs and if you look, you'll see that
> shiftfs_lookup() actually has far more in common with
> ecryptfs_lookup().

That's a good point. All stackable file systems may share similar problems
and solutions (e.g. consistent st_ino/st_dev). Perhaps it calls for shared
library code or more generic VFS code.
At the moment ecryptfs is not seeing much development, so everything
happens in overlayfs. If there is going to be more than 1 actively developed
stackable fs, we need to see about that.

>
>>  shiftfs is going to miss out on overlayfs bug fixes related to user
>> credentials differ from mounter credentials, like fd3220d ("ovl:
>> update S_ISGID when setting posix ACLs"). I am not sure that this
>> specific case is relevant to shiftfs, but there could be other.
>
> OK, so shiftfs doesn't have this bug and the reason why is
> illustrative: basically shiftfs does three things
>
>    1. lookups via a uid/gid shifted dentry cache
>    2. shifted credential inode operations permission checks on the
>       underlying filesystem
>    3. location marking for unprivileged mount
>
> I think we've already seen that 1. isn't from overlayfs but the
> functionality could be added to overlayfs, I suppose.  The big problem
> is 2.  The overlayfs code emulates the permission checks, which makes
> it rather complex (this is where you get your bugs like the above
> from).  I did actually look at adding 2. to overlayfs on the theory
> that a single layer overlay might be closest to what this is, but
> eventually concluded I'd have to take the special cases and add a whole
> lot more to them ... it really would increase the maintenance burden
> substantially and make the code an unreadable rats nest.
>

The use cases for uid shifting are still overwelming for me.
I take your word for it that its going to be a maintanace burdon
to add this functionality to overlayfs.

> When you think about it this way, it becomes obvious that the clean
> separation is if shiftfs functionality is layered on top of overlayfs
> and when you do that, doing it as its own filesystem is more logical.
>

Yes, I agree with that statement. This is inline with the solution I outlined
at the end of my previous email, where single layer overlayfs is used
for the host "mark" mount, although I wonder if the same cannot be
achieved with a bind mount?

in host:
mount -t overlay -o noexec,upper=<origin> container_visible <mark location>

in container:
mount -t shiftfs -o <mark location> <somewhere in my local mount ns>

[toc] | [prev] | [next] | [standalone]

#1574854

On Mon, 2017-02-06 at 08:59 +0200, Amir Goldstein wrote:
> On Mon, Feb 6, 2017 at 3:18 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Sun, 2017-02-05 at 09:51 +0200, Amir Goldstein wrote:
> > > On Sat, Feb 4, 2017 at 9:19 PM, James Bottomley
> > > <James.Bottomley@hansenpartnership.com> wrote:
> > > > This allows any subtree to be uid/gid shifted and bound 
> > > > elsewhere.  It does this by operating simlarly to overlayfs. 
> > > >  Its primary use is for shifting the underlying uids of 
> > > > filesystems used to support unpriviliged (uid shifted) 
> > > > containers.  The usual use case here is that the container is 
> > > > operating with an uid shifted unprivileged root but sometimes 
> > > > needs to make use of or work with a filesystem image that has
> > > > root at real uid 0.
> > > > 
> > > > The mechanism is to allow any subordinate mount namespace to 
> > > > mount a shiftfs filesystem (by marking it FS_USERNS_MOUNT) but 
> > > > only allowing it to mount marked subtrees (using the -o mark 
> > > > option as root).   Once mounted, the subtree is mapped via the 
> > > > super block user namespace so that the interior ids of the 
> > > > mounting user namespace are the ids written to the filesystem.
> > > > 
> > > > Signed-off-by: James Bottomley <
> > > > James.Bottomley@HansenPartnership.com>
> > > > 
> > > 
> > > James,
> > > 
> > > Allow me to point out some problems in this patch and offer a
> > > slightly different approach.
> > > 
> > > First of all, the subject says "uid/gid shifting bind mount", but
> > > it's not really a bind mount. What it is is a stackable mount and 
> > > 2 levels of stack no less.
> > 
> > The reason for the description is to have it behave exactly like a 
> > bind mount.  You can assert that a bind mount is, in fact, a 
> > stacked mount, but we don't currently.  I'm also not sure where you 
> > get your 2 levels from?
> > 
> 
> A bind mount does not incur recursion into VFS code, a stacked fs 
> does. And there is a programmable limit of stack depth of 2, which 
> stacked fs need to comply with. Your proposed setup has 2 stacked fs, 
> the mark shitfs by admin and the uid shitfs by container user. Or
> maybe I misunderstood.

Oh, right, actually, it wouldn't be 2 because once the unprivileged
mount uses the marked filesystem, what it uses is the mnt and dentry
from the underlying filesystem (what you would have got from a path
lookup on it).

That said, it does perform recursive calls to the underlying filesystem
unlike a true bind mount, so I can add the depth easily enough.

> > >  So one thing that is missing is increasing of sb->s_stack_depth 
> > > and that also means that shiftfs cannot be used to recursively 
> > > shift uids in child userns if that was ever the intention.
> > 
> > I can't think of a use case that would ever need that, but perhaps
> > other container people can.
> > 
> > > The other problem is that by forking overlayfs functionality,
> > 
> > So this wouldn't really be the right way to look at it: shiftfs 
> > shares no code with overlayfs at all, so is definitely not a fork. 
> >  The only piece of functionality it has which is similar to 
> > overlayfs is the way it does lookups via a new dentry cache. 
> >  However, that functionality is not unique to overlayfs and if you 
> > look, you'll see that shiftfs_lookup() actually has far more in 
> > common with ecryptfs_lookup().
> 
> That's a good point. All stackable file systems may share similar 
> problems and solutions (e.g. consistent st_ino/st_dev). Perhaps it 
> calls for shared library code or more generic VFS code. At the moment 
> ecryptfs is not seeing much development, so everything happens in 
> overlayfs. If there is going to be more than 1 actively developed
> stackable fs, we need to see about that.

I believe we already do ... if you look at the lookup functions of each
of them, you see the only common thing is encapsulated in a variant of
the lookup_one_len() functions.  After that, even simple things like
our negative dentry handling differs.

> > >  shiftfs is going to miss out on overlayfs bug fixes related to 
> > > user credentials differ from mounter credentials, like fd3220d 
> > > ("ovl: update S_ISGID when setting posix ACLs"). I am not sure 
> > > that this specific case is relevant to shiftfs, but there could
> > > be other.
> > 
> > OK, so shiftfs doesn't have this bug and the reason why is
> > illustrative: basically shiftfs does three things
> > 
> >    1. lookups via a uid/gid shifted dentry cache
> >    2. shifted credential inode operations permission checks on the
> >       underlying filesystem
> >    3. location marking for unprivileged mount
> > 
> > I think we've already seen that 1. isn't from overlayfs but the
> > functionality could be added to overlayfs, I suppose.  The big 
> > problem is 2.  The overlayfs code emulates the permission checks, 
> > which makes it rather complex (this is where you get your bugs like 
> > the above from).  I did actually look at adding 2. to overlayfs on 
> > the theory that a single layer overlay might be closest to what 
> > this is, but eventually concluded I'd have to take the special 
> > cases and add a whole lot more to them ... it really would increase 
> > the maintenance burden substantially and make the code an
> > unreadable rats nest.
> > 
> 
> The use cases for uid shifting are still overwelming for me.
> I take your word for it that its going to be a maintanace burdon
> to add this functionality to overlayfs.
> 
> > When you think about it this way, it becomes obvious that the clean
> > separation is if shiftfs functionality is layered on top of 
> > overlayfs and when you do that, doing it as its own filesystem is 
> > more logical.
> > 
> 
> Yes, I agree with that statement. This is inline with the solution I 
> outlined at the end of my previous email, where single layer 
> overlayfs is used for the host "mark" mount, although I wonder if the 
> same cannot be achieved with a bind mount?

I understand, but once I can't consume overlayfs to construct it, the
idea of trying to use it becomes a negative not a positive.

We could achieve the same thing using bind mounts, if the vfsmount
structure carried a private field, but it doesn't.  I think given the
prevalence of this structure throughout the mount tree, that's a
deliberate decision to keep it thin.

> in host:
> mount -t overlay -o noexec,upper=<origin> container_visible <mark
> location>
> 
> in container:
> mount -t shiftfs -o <mark location> <somewhere in my local mount ns>

So I'm not sure it's a more widespread problem: mount --bind is usable
inside an unprivileged container, which means you can bridge filesystem
subtrees even only being local container admin.  The problem is
mounting other filesystems types.  Marking a type safe for mounting is
done by the FS_USERNS_MOUNT flag but it means for things like shiftfs
that you do have to restrict the source location, but for most
filesystem types, that source will be a device, so they will need other
checking than a mount mark.

James

[toc] | [prev] | [next] | [standalone]

#1574342

James Bottomley:
> This allows any subtree to be uid/gid shifted and bound elsewhere.  It
	:::

Interesting.
But I am afraid that the inconsistency problem of the inode numbers will
happen.

shiftfs_new_inode() uses get_next_ino() which means
- 1st time: inodeA is created and cached, inumA is assigned
- after using inodeA, it will be discarded from the cache
- 2nd time: inodeA is looked-up again, and another inode number (inumB)
  is assgined.

This inconsistency will not be a problem for the "pure virtual" fs such
as procfs and sysfs. But your shiftfs is not pure as them. Shiftfs will
be used as a wrapper (or "binder" which means bind-mount) of an orginary
filesystem.
The symptom of this problem from users perspective will be
- find -inum doesn't work
- git-status doesn't work, which keeps st_dev and st_ino and compares
  the current files.
Of course they will be limited to when the target dir is huge and/or
system memory is low. As long as the inode cache is large enough to hold
all necessary inodes, the problem won't happen.

If shiftfs will supports exporting via NFS in the future, the
consistency of inum will be important too.


J. R. Okajima

[toc] | [prev] | [next] | [standalone]

#1574378

On Mon, Feb 6, 2017 at 5:25 AM, J. R. Okajima <hooanon05g@gmail.com> wrote:
> James Bottomley:
>> This allows any subtree to be uid/gid shifted and bound elsewhere.  It
>         :::
>
> Interesting.
> But I am afraid that the inconsistency problem of the inode numbers will
> happen.
>

Yet another example that overlayfs already is in the process of solving
(it is fixed for stat of merged directory inode).
In fact, fir the case of single layer overlay (as well as shiftfs) the
solution is trivial -
preserve underlying inode st_ino/d_ino and use the overlayed fs st_dev.

> shiftfs_new_inode() uses get_next_ino() which means
> - 1st time: inodeA is created and cached, inumA is assigned
> - after using inodeA, it will be discarded from the cache
> - 2nd time: inodeA is looked-up again, and another inode number (inumB)
>   is assgined.
>
> This inconsistency will not be a problem for the "pure virtual" fs such
> as procfs and sysfs. But your shiftfs is not pure as them. Shiftfs will
> be used as a wrapper (or "binder" which means bind-mount) of an orginary
> filesystem.
> The symptom of this problem from users perspective will be
> - find -inum doesn't work
> - git-status doesn't work, which keeps st_dev and st_ino and compares
>   the current files.
> Of course they will be limited to when the target dir is huge and/or
> system memory is low. As long as the inode cache is large enough to hold
> all necessary inodes, the problem won't happen.
>
> If shiftfs will supports exporting via NFS in the future, the
> consistency of inum will be important too.
>
>
> J. R. Okajima

[toc] | [prev] | [next] | [standalone]

#1574973

On Mon, 2017-02-06 at 08:38 +0200, Amir Goldstein wrote:
> On Mon, Feb 6, 2017 at 5:25 AM, J. R. Okajima <hooanon05g@gmail.com>
> wrote:
> > James Bottomley:
> > > This allows any subtree to be uid/gid shifted and bound
> > > elsewhere.  It
> >         :::
> > 
> > Interesting.
> > But I am afraid that the inconsistency problem of the inode numbers 
> > will happen.
> > 
> 
> Yet another example that overlayfs already is in the process of 
> solving (it is fixed for stat of merged directory inode).
> In fact, fir the case of single layer overlay (as well as shiftfs) 
> the solution is trivial - preserve underlying inode st_ino/d_ino and 
> use the overlayed fs st_dev.

not sure I follow what st_ino is, do you mean  s_root->d_inode->i_ino?
or did you mean s_dev (which is more traditional)?

The problem with this is there's no way to ensure global uniqueness in
a mapping that goes (ino, ino) -> (ino) (or (s_dev, ino) -> (ino)) and
I believe global uniqueness is more important because the i_ino is used
in the hashed lookups.  Secondly you're not guaranteed that s_root
->d_inode->i_ino is unique ... historically a lot of filesystems use a
well known inode number as the root, that's why filehandles
traditionally used something representing the device and the inode
number (we also have s_dev uniqueness problems for tmpfs which is used
in some overlays).

We can certainly construct a filehandle using an export operations
override that is unique and can be used to lookup the underlying object
(based on the underlying device and inode).

James

[toc] | [prev] | [next] | [standalone]

#1574385

On Mon, 2017-02-06 at 12:25 +0900, J. R. Okajima wrote:
> James Bottomley:
> > This allows any subtree to be uid/gid shifted and bound elsewhere. 
> >  It
> 	:::
> 
> Interesting.
> But I am afraid that the inconsistency problem of the inode numbers 
> will happen.
> 
> shiftfs_new_inode() uses get_next_ino() which means
> - 1st time: inodeA is created and cached, inumA is assigned
> - after using inodeA, it will be discarded from the cache
> - 2nd time: inodeA is looked-up again, and another inode number 
> (inumB)   is assgined.

Yes, I know the problem.  However, I believe most current linux
filesystems no longer guarantee stable, for the lifetime of the file,
inode numbers.  The usual docker container root is overlayfs, which,
similarly doesn't support stable inode numbers.  I see the odd
complaint about docker with overlayfs having unstable inode numbers,
but none seems to have any serious repercussions.

[...]
> If shiftfs will supports exporting via NFS in the future, the
> consistency of inum will be important too.

If it's a problem, then it's fixable with s_export_op, but I was mostly
thinking that because it's not a problem for overlayfs based
containers, it wouldn't be one for shiftfs based ones, which is why I
didn't implement it.

James

[toc] | [prev] | [next] | [standalone]

#1574872

On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley wrote:
> Yes, I know the problem.  However, I believe most current linux
> filesystems no longer guarantee stable, for the lifetime of the file,
> inode numbers.  The usual docker container root is overlayfs, which,
> similarly doesn't support stable inode numbers.  I see the odd
> complaint about docker with overlayfs having unstable inode numbers,
> but none seems to have any serious repercussions.

Um, no.  Most current linux file systems *do* guarantee stable inode
numbers.  For one thing, NFS would break horribly if you didn't have
stable inode numbers.  Never mind applications which depend on POSIX
semantics.  And you wouldn't be able to save games in rogue or
nethack, either.  :-)

Overlayfs may not, currently, but it's considered a bug.

	      	   	      	       - Ted

[toc] | [prev] | [next] | [standalone]

#1574885

On Mon, 2017-02-06 at 09:50 -0500, Theodore Ts'o wrote:
> On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley wrote:
> > Yes, I know the problem.  However, I believe most current linux
> > filesystems no longer guarantee stable, for the lifetime of the 
> > file, inode numbers.  The usual docker container root is overlayfs,
> > which, similarly doesn't support stable inode numbers.  I see the 
> > odd complaint about docker with overlayfs having unstable inode
> > numbers, but none seems to have any serious repercussions.
> 
> Um, no.  Most current linux file systems *do* guarantee stable inode
> numbers.  For one thing, NFS would break horribly if you didn't have
> stable inode numbers.  Never mind applications which depend on POSIX
> semantics.  And you wouldn't be able to save games in rogue or
> nethack, either.  :-)

I believe that's why we have the superblock export operations to
manufacture unique filehandles in the absence of inode number
stability.  The generic one uses inode numbers, but it doesn't have to.
 I thought reiserfs (if we can go back that far) was the first
generally used filesystem that didn't guarantee stable inode numbers,
so we have a lot of historical precedence.

Thanks to reiserfs, I thought we also iterated to weak stability
guarantees for inode numbers which mean no inconsistencies in
applications that use inode numbers for caching?  It's still not POSIX,
but I thought it was good enough for most use cases.

> Overlayfs may not, currently, but it's considered a bug.

James

[toc] | [prev] | [next] | [standalone]

#1574910

On Mon, Feb 06, 2017 at 07:18:16AM -0800, James Bottomley wrote:
> On Mon, 2017-02-06 at 09:50 -0500, Theodore Ts'o wrote:
> > On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley wrote:
> > > Yes, I know the problem.  However, I believe most current linux
> > > filesystems no longer guarantee stable, for the lifetime of the 
> > > file, inode numbers.  The usual docker container root is overlayfs,
> > > which, similarly doesn't support stable inode numbers.  I see the 
> > > odd complaint about docker with overlayfs having unstable inode
> > > numbers, but none seems to have any serious repercussions.
> > 
> > Um, no.  Most current linux file systems *do* guarantee stable inode
> > numbers.  For one thing, NFS would break horribly if you didn't have
> > stable inode numbers.  Never mind applications which depend on POSIX
> > semantics.  And you wouldn't be able to save games in rogue or
> > nethack, either.  :-)
> 
> I believe that's why we have the superblock export operations to
> manufacture unique filehandles in the absence of inode number
> stability.  The generic one uses inode numbers, but it doesn't have to.
>  I thought reiserfs (if we can go back that far) was the first
> generally used filesystem that didn't guarantee stable inode numbers,
> so we have a lot of historical precedence.
> 
> Thanks to reiserfs, I thought we also iterated to weak stability
> guarantees for inode numbers which mean no inconsistencies in
> applications that use inode numbers for caching?  It's still not POSIX,
> but I thought it was good enough for most use cases.
> 

Even plain tar extraction is sensitive to directory inode stability:
http://git.savannah.gnu.org/cgit/tar.git/tree/src/extract.c?h=release_1_29#n867

This caused errors on overlayfs if the extraction churned through enough
of the dentry cache to evict the relevant directory (can be forced to
reproduce reliably via drop_caches).

Regards,
Vito Caputo

[toc] | [prev] | [next] | [standalone]

#1575021

On Mon, 2017-02-06 at 09:38 -0600, lkml@pengaru.com wrote:
> On Mon, Feb 06, 2017 at 07:18:16AM -0800, James Bottomley wrote:
> > On Mon, 2017-02-06 at 09:50 -0500, Theodore Ts'o wrote:
> > > On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley wrote:
> > > > Yes, I know the problem.  However, I believe most current linux
> > > > filesystems no longer guarantee stable, for the lifetime of the
> > > > file, inode numbers.  The usual docker container root is
> > > > overlayfs,
> > > > which, similarly doesn't support stable inode numbers.  I see
> > > > the 
> > > > odd complaint about docker with overlayfs having unstable inode
> > > > numbers, but none seems to have any serious repercussions.
> > > 
> > > Um, no.  Most current linux file systems *do* guarantee stable
> > > inode
> > > numbers.  For one thing, NFS would break horribly if you didn't
> > > have
> > > stable inode numbers.  Never mind applications which depend on
> > > POSIX
> > > semantics.  And you wouldn't be able to save games in rogue or
> > > nethack, either.  :-)
> > 
> > I believe that's why we have the superblock export operations to
> > manufacture unique filehandles in the absence of inode number
> > stability.  The generic one uses inode numbers, but it doesn't have
> > to.
> >  I thought reiserfs (if we can go back that far) was the first
> > generally used filesystem that didn't guarantee stable inode
> > numbers,
> > so we have a lot of historical precedence.
> > 
> > Thanks to reiserfs, I thought we also iterated to weak stability
> > guarantees for inode numbers which mean no inconsistencies in
> > applications that use inode numbers for caching?  It's still not
> > POSIX,
> > but I thought it was good enough for most use cases.
> > 
> 
> Even plain tar extraction is sensitive to directory inode stability:
> http://git.savannah.gnu.org/cgit/tar.git/tree/src/extract.c?h=release
> _1_29#n867
> 
> This caused errors on overlayfs if the extraction churned through 
> enough of the dentry cache to evict the relevant directory (can be 
> forced to reproduce reliably via drop_caches).

Yes, I know the bug.  I think it's up to tar maintainers, but if they
want to support weakly posix filesystems, they should really be using
the filehandle for this check, not device and inode number.

That said, I believe reiserfs was our only other filesystem with weak
inode number stability guarantees and that's hardly in common use
today, so if we can find a solution that gives strong stability
guarantees for out current problem filesystems, there's no reason not
to use it generally.

James

[toc] | [prev] | [next] | [standalone]

#1575181

On Mon, Feb 06, 2017 at 07:18:16AM -0800, James Bottomley wrote:
> On Mon, 2017-02-06 at 09:50 -0500, Theodore Ts'o wrote:
> > On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley wrote:
> > > Yes, I know the problem.  However, I believe most current linux
> > > filesystems no longer guarantee stable, for the lifetime of the 
> > > file, inode numbers.  The usual docker container root is overlayfs,
> > > which, similarly doesn't support stable inode numbers.  I see the 
> > > odd complaint about docker with overlayfs having unstable inode
> > > numbers, but none seems to have any serious repercussions.
> > 
> > Um, no.  Most current linux file systems *do* guarantee stable inode
> > numbers.  For one thing, NFS would break horribly if you didn't have
> > stable inode numbers.  Never mind applications which depend on POSIX
> > semantics.  And you wouldn't be able to save games in rogue or
> > nethack, either.  :-)
> 
> I believe that's why we have the superblock export operations to
> manufacture unique filehandles in the absence of inode number
> stability.

Where did you hear that?

I'd expect an NFS client to handle non-unique filehandles
better than non-unique inode numbers.  I believe our client will -EIO on
encountering an inode number change (see nfs_check_inode_attributes().)

See also https://tools.ietf.org/html/rfc5661#section-10.3.4.

--b.

> The generic one uses inode numbers, but it doesn't have to.
>  I thought reiserfs (if we can go back that far) was the first
> generally used filesystem that didn't guarantee stable inode numbers,
> so we have a lot of historical precedence.
> 
> Thanks to reiserfs, I thought we also iterated to weak stability
> guarantees for inode numbers which mean no inconsistencies in
> applications that use inode numbers for caching?  It's still not POSIX,
> but I thought it was good enough for most use cases.
> 
> > Overlayfs may not, currently, but it's considered a bug.
> 
> James
> 

[toc] | [prev] | [next] | [standalone]

#1575275

On Mon, 2017-02-06 at 16:52 -0500, J. Bruce Fields wrote:
> On Mon, Feb 06, 2017 at 07:18:16AM -0800, James Bottomley wrote:
> > On Mon, 2017-02-06 at 09:50 -0500, Theodore Ts'o wrote:
> > > On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley wrote:
> > > > Yes, I know the problem.  However, I believe most current linux
> > > > filesystems no longer guarantee stable, for the lifetime of the
> > > > file, inode numbers.  The usual docker container root is 
> > > > overlayfs, which, similarly doesn't support stable inode 
> > > > numbers.  I see the odd complaint about docker with overlayfs 
> > > > having unstable inode numbers, but none seems to have any
> > > > serious repercussions.
> > > 
> > > Um, no.  Most current linux file systems *do* guarantee stable 
> > > inode numbers.  For one thing, NFS would break horribly if you 
> > > didn't have stable inode numbers.  Never mind applications which 
> > > depend on POSIX semantics.  And you wouldn't be able to save 
> > > games in rogue or nethack, either.  :-)
> > 
> > I believe that's why we have the superblock export operations to
> > manufacture unique filehandles in the absence of inode number
> > stability.
> 
> Where did you hear that?
> 
> I'd expect an NFS client to handle non-unique filehandles
> better than non-unique inode numbers.  I believe our client will -EIO 
> on encountering an inode number change (see
> nfs_check_inode_attributes().)
> 
> See also https://tools.ietf.org/html/rfc5661#section-10.3.4.

Could you clarify your point a bit further, please?  Both the
check_inode_attributes() code and section 10.3.4 are talking about
fileids, which are the things that are constructed in the export_ops
... admittedly a lot of fileid_types are based on inode numbers, but
several aren't.  For those that aren't, I believe NFS doesn't care
about the underlying inode number of the exported file.

James

[toc] | [prev] | [next] | [standalone]

#1575316

On Mon, Feb 06, 2017 at 04:10:11PM -0800, James Bottomley wrote:
> On Mon, 2017-02-06 at 16:52 -0500, J. Bruce Fields wrote:
> > On Mon, Feb 06, 2017 at 07:18:16AM -0800, James Bottomley wrote:
> > > On Mon, 2017-02-06 at 09:50 -0500, Theodore Ts'o wrote:
> > > > On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley wrote:
> > > > > Yes, I know the problem.  However, I believe most current linux
> > > > > filesystems no longer guarantee stable, for the lifetime of the
> > > > > file, inode numbers.  The usual docker container root is 
> > > > > overlayfs, which, similarly doesn't support stable inode 
> > > > > numbers.  I see the odd complaint about docker with overlayfs 
> > > > > having unstable inode numbers, but none seems to have any
> > > > > serious repercussions.
> > > > 
> > > > Um, no.  Most current linux file systems *do* guarantee stable 
> > > > inode numbers.  For one thing, NFS would break horribly if you 
> > > > didn't have stable inode numbers.  Never mind applications which 
> > > > depend on POSIX semantics.  And you wouldn't be able to save 
> > > > games in rogue or nethack, either.  :-)
> > > 
> > > I believe that's why we have the superblock export operations to
> > > manufacture unique filehandles in the absence of inode number
> > > stability.
> > 
> > Where did you hear that?
> > 
> > I'd expect an NFS client to handle non-unique filehandles
> > better than non-unique inode numbers.  I believe our client will -EIO 
> > on encountering an inode number change (see
> > nfs_check_inode_attributes().)
> > 
> > See also https://tools.ietf.org/html/rfc5661#section-10.3.4.
> 
> Could you clarify your point a bit further, please?  Both the
> check_inode_attributes() code and section 10.3.4 are talking about
> fileids, which are the things that are constructed in the export_ops

No, the filehandle structure isn't discussed in the rfc at all, that's
opaque to clients, and the "fileid" you see in the export code isn't
what's discussed here.

The "fileid" here is an NFS attribute, really just the NFS protocol's
name for the inode number.  The server code that returns fileid's:

	if (bmval0 & FATTR4_WORD0_FILEID) {
		p = xdr_reserve_space(xdr, 8);
		if (!p)
			goto out_resource;
		p = xdr_encode_hyper(p, stat.ino);
	}

The client getattr code:

	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));

--b.

> ... admittedly a lot of fileid_types are based on inode numbers, but
> several aren't.  For those that aren't, I believe NFS doesn't care
> about the underlying inode number of the exported file.

[toc] | [prev] | [next] | [standalone]

#1575964

On Mon, 2017-02-06 at 20:35 -0500, J. Bruce Fields wrote:
> On Mon, Feb 06, 2017 at 04:10:11PM -0800, James Bottomley wrote:
> > On Mon, 2017-02-06 at 16:52 -0500, J. Bruce Fields wrote:
> > > On Mon, Feb 06, 2017 at 07:18:16AM -0800, James Bottomley wrote:
> > > > On Mon, 2017-02-06 at 09:50 -0500, Theodore Ts'o wrote:
> > > > > On Sun, Feb 05, 2017 at 10:46:23PM -0800, James Bottomley
> > > > > wrote:
> > > > > > Yes, I know the problem.  However, I believe most current
> > > > > > linux
> > > > > > filesystems no longer guarantee stable, for the lifetime of
> > > > > > the
> > > > > > file, inode numbers.  The usual docker container root is 
> > > > > > overlayfs, which, similarly doesn't support stable inode 
> > > > > > numbers.  I see the odd complaint about docker with
> > > > > > overlayfs 
> > > > > > having unstable inode numbers, but none seems to have any
> > > > > > serious repercussions.
> > > > > 
> > > > > Um, no.  Most current linux file systems *do* guarantee
> > > > > stable 
> > > > > inode numbers.  For one thing, NFS would break horribly if
> > > > > you 
> > > > > didn't have stable inode numbers.  Never mind applications
> > > > > which 
> > > > > depend on POSIX semantics.  And you wouldn't be able to save 
> > > > > games in rogue or nethack, either.  :-)
> > > > 
> > > > I believe that's why we have the superblock export operations
> > > > to
> > > > manufacture unique filehandles in the absence of inode number
> > > > stability.
> > > 
> > > Where did you hear that?
> > > 
> > > I'd expect an NFS client to handle non-unique filehandles
> > > better than non-unique inode numbers.  I believe our client will 
> > > -EIO 
> > > on encountering an inode number change (see
> > > nfs_check_inode_attributes().)
> > > 
> > > See also https://tools.ietf.org/html/rfc5661#section-10.3.4.
> > 
> > Could you clarify your point a bit further, please?  Both the
> > check_inode_attributes() code and section 10.3.4 are talking about
> > fileids, which are the things that are constructed in the
> > export_ops
> 
> No, the filehandle structure isn't discussed in the rfc at all,
> that's
> opaque to clients, and the "fileid" you see in the export code isn't
> what's discussed here.
> 
> The "fileid" here is an NFS attribute, really just the NFS protocol's
> name for the inode number.  The server code that returns fileid's:
> 
> 	if (bmval0 & FATTR4_WORD0_FILEID) {
> 		p = xdr_reserve_space(xdr, 8);
> 		if (!p)
> 			goto out_resource;
> 		p = xdr_encode_hyper(p, stat.ino);
> 	}
> 
> The client getattr code:
> 
> 	stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));

OK, I now believe we may be talking about different things.  When I
said

> I believe that's why we have the superblock export operations to
> manufacture unique filehandles in the absence of inode number
> stability. 

I was talking about inode stability in the filesystem underlying the
export.  I believe you're talking about inode number stability
guarantees of the nfs client code itself, which are unrelated to the
inode number guarantees of the exported filesystem?

James

[toc] | [prev] | [next] | [standalone]

#1575993

On Tue, Feb 07, 2017 at 11:01:08AM -0800, James Bottomley wrote:
> I was talking about inode stability in the filesystem underlying the
> export.  I believe you're talking about inode number stability
> guarantees of the nfs client code itself, which are unrelated to the
> inode number guarantees of the exported filesystem?

They are 1:1 correlated for a Linux server at least.

[toc] | [prev] | [next] | [standalone]

#1574960

James Bottomley:
> Yes, I know the problem.  However, I believe most current linux
> filesystems no longer guarantee stable, for the lifetime of the file,
> inode numbers.  The usual docker container root is overlayfs, which,
> similarly doesn't support stable inode numbers.  I see the odd
> complaint about docker with overlayfs having unstable inode numbers,
> but none seems to have any serious repercussions.

I think it serious.
Reusing the backend fs' inum is a good approach which Amir wrote.
Based on this, I'd suggest you to support the hardlinks.

bakend_dentry = lookup_one_len()
if (d_inode->i_nlink != 1)
	shiftfs_inode = ilookup();
if (!shiftfs_inode) {
	shiftfs_inode = new_inode();
	shiftfs_inode->i_ino = bakend_dentry->d_inode->i_ino;
}


J. R. Okajima

[toc] | [prev] | [next] | [standalone]

#1575497

On Sat, Feb 04, 2017 at 11:19:32AM -0800, James Bottomley wrote:
> This allows any subtree to be uid/gid shifted and bound elsewhere.  It
> does this by operating simlarly to overlayfs.  Its primary use is for
> shifting the underlying uids of filesystems used to support
> unpriviliged (uid shifted) containers.  The usual use case here is
> that the container is operating with an uid shifted unprivileged root
> but sometimes needs to make use of or work with a filesystem image
> that has root at real uid 0.
> 
> The mechanism is to allow any subordinate mount namespace to mount a
> shiftfs filesystem (by marking it FS_USERNS_MOUNT) but only allowing
> it to mount marked subtrees (using the -o mark option as root).  Once
> mounted, the subtree is mapped via the super block user namespace so
> that the interior ids of the mounting user namespace are the ids
> written to the filesystem.

Please move this into VFS instead of a stackable fs.  We might need
addtional parameters to getattr/setattr to specify the ID translation,
but that's why better than a horrible hack like this.

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web