[PATCH 0/2] ovl: constant d_ino across copy up

[PATCH 0/2] ovl: constant d_ino across copy up

[Amir Goldstein]

Miklos,

Following patches fix constant d_ino.
I've tested them with the improved xfstest overlay/017.

Fixing constat d_ino for this merge cycle is not urgent,
although you may consider the patch simple enough to be
a candidate for rc2.

However, I do recommend that you consider the "impure"
dir patch for this merge cycle. If you include it, it will
guaranty the on-disk rule:
A) An upper directory that contains entries with valid
   "origin" is either "merge" or marked "impure"

If you don't include it in the same merge window as constant
st_ino, then we will need to deal with violations of this rule.
OTOH, we already do need to deal with the situation of:
B) Non-merge upper directory that contains dir entries with
   "redirect" that is not marked "impure"

Assuming that we will not want to give up on the "impure"
optimization for ovl_iterate(), then dealing with B) is quite
simple - let old redirected dirs have d_ino != st_ino.
'find -ino' never looks at d_ino of subdirs and I don't know
which programs do care about it. Also, d_ino != st_ino for
subdirs also in past kernels, so it won't be a regression.

Dealing with A) (if "impure" doesn't go into 4.12) will be
more challenging, because it will allow the following combinations:
- non constant st_ino, which is consistent with d_ino (<= v4.11)
- constant st_ino, which is consistent with d_ino (>= v4.13)
- constant st_ino, which is sometimes consistent with d_ino
  (depending on whether the file was moved with kernel v4.12 or v4.13)

Thoughts?

Amir Goldstein (2):
  ovl: mark upper dir with type origin entries "impure"
  ovl: constant d_ino across copy up

 fs/overlayfs/dir.c       |  41 ++++++++++++++++-
 fs/overlayfs/namei.c     |  25 +++++++++--
 fs/overlayfs/overlayfs.h |   5 ++-
 fs/overlayfs/ovl_entry.h |   1 +
 fs/overlayfs/readdir.c   | 115 ++++++++++++++++++++++++++++++++++++++++++++---
 fs/overlayfs/util.c      |  15 +++++++
 6 files changed, 189 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH 1/2] ovl: mark upper dir with type origin entries "impure"

[Amir Goldstein]

When moving a merge dir or non-dir with copy up origin into
a non-merge upper dir (a.k.a pure upper dir), we are marking
the target parent dir "impure". ovl_iterate() iterates pure
upper dirs directly, because there is no need to filter out
whiteouts and merge dir content with lower dir. But for the
case of an "impure" upper dir, ovl_iterate() will not be able
to iterate the real upper dir directly, because it will need
to lookup the origin inode and use it to fill d_ino.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 41 ++++++++++++++++++++++++++++++++++++++++-
 fs/overlayfs/namei.c     | 18 ++++++++++++++++--
 fs/overlayfs/overlayfs.h |  3 +++
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/util.c      | 15 +++++++++++++++
 5 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 723b98b..205e491 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -138,6 +138,17 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
 	return err;
 }
 
+static int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
+{
+	int err;
+
+	err = ovl_do_setxattr(upperdentry, OVL_XATTR_IMPURE, "y", 1, 0);
+	if (!err)
+		ovl_dentry_set_impure(dentry);
+
+	return err;
+}
+
 /* Common operations required to be done after creation of file on upper */
 static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
 			    struct dentry *newdentry, bool hardlink)
@@ -162,6 +173,11 @@ static bool ovl_type_merge(struct dentry *dentry)
 	return OVL_TYPE_MERGE(ovl_path_type(dentry));
 }
 
+static bool ovl_type_origin(struct dentry *dentry)
+{
+	return OVL_TYPE_ORIGIN(ovl_path_type(dentry));
+}
+
 static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 			    struct cattr *attr, struct dentry *hardlink)
 {
@@ -943,6 +959,30 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 	old_upperdir = ovl_dentry_upper(old->d_parent);
 	new_upperdir = ovl_dentry_upper(new->d_parent);
 
+	if (ovl_same_sb(old->d_sb) && !samedir) {
+		/*
+		 * When moving a merge dir or non-dir with copy up origin into
+		 * a non-merge upper dir (a.k.a pure upper dir), we are making
+		 * the target parent dir "impure". ovl_iterate() iterates pure
+		 * upper dirs directly, because there is no need to filter out
+		 * whiteouts and merge dir content with lower dir. But for the
+		 * case of an "impure" upper dir, ovl_iterate() cannot iterate
+		 * the real directory directly, because it looks for the inode
+		 * numbers to fill d_ino in the entries origin inode.
+		 */
+		if (ovl_type_origin(old) && !ovl_type_merge(new->d_parent)) {
+			err = ovl_set_impure(new->d_parent, new_upperdir);
+			if (err)
+				goto out_dput;
+		}
+		if (!overwrite && ovl_type_origin(new) &&
+		    !ovl_type_merge(old->d_parent)) {
+			err = ovl_set_impure(old->d_parent, old_upperdir);
+			if (err)
+				goto out_dput;
+		}
+	}
+
 	trap = lock_rename(new_upperdir, old_upperdir);
 
 	olddentry = lookup_one_len(old->d_name.name, old_upperdir,
@@ -1004,7 +1044,6 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
 		if (err)
 			goto out_dput;
 	}
-
 	err = ovl_do_rename(old_upperdir->d_inode, olddentry,
 			    new_upperdir->d_inode, newdentry, flags);
 	if (err)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index bad0f66..0c72a59 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -167,7 +167,7 @@ static struct dentry *ovl_get_origin(struct dentry *dentry,
 	goto out;
 }
 
-static bool ovl_is_opaquedir(struct dentry *dentry)
+static bool ovl_check_dir_xattr(struct dentry *dentry, const char *name)
 {
 	int res;
 	char val;
@@ -175,13 +175,23 @@ static bool ovl_is_opaquedir(struct dentry *dentry)
 	if (!d_is_dir(dentry))
 		return false;
 
-	res = vfs_getxattr(dentry, OVL_XATTR_OPAQUE, &val, 1);
+	res = vfs_getxattr(dentry, name, &val, 1);
 	if (res == 1 && val == 'y')
 		return true;
 
 	return false;
 }
 
+static bool ovl_is_opaquedir(struct dentry *dentry)
+{
+	return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
+}
+
+static bool ovl_is_impuredir(struct dentry *dentry)
+{
+	return ovl_check_dir_xattr(dentry, OVL_XATTR_IMPURE);
+}
+
 static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 			     const char *name, unsigned int namelen,
 			     size_t prelen, const char *post,
@@ -351,6 +361,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	unsigned int ctr = 0;
 	struct inode *inode = NULL;
 	bool upperopaque = false;
+	bool upperimpure = false;
 	char *upperredirect = NULL;
 	struct dentry *this;
 	unsigned int i;
@@ -395,6 +406,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 				poe = roe;
 		}
 		upperopaque = d.opaque;
+		if (upperdentry && d.is_dir)
+			upperimpure = ovl_is_impuredir(upperdentry);
 	}
 
 	if (!d.stop && poe->numlower) {
@@ -463,6 +476,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 	revert_creds(old_cred);
 	oe->opaque = upperopaque;
+	oe->impure = upperimpure;
 	oe->redirect = upperredirect;
 	oe->__upperdentry = upperdentry;
 	memcpy(oe->lowerstack, stack, sizeof(struct path) * ctr);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index caa36cb..fae52f5 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -24,6 +24,7 @@ enum ovl_path_type {
 #define OVL_XATTR_OPAQUE OVL_XATTR_PREFIX "opaque"
 #define OVL_XATTR_REDIRECT OVL_XATTR_PREFIX "redirect"
 #define OVL_XATTR_ORIGIN OVL_XATTR_PREFIX "origin"
+#define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
 
 /*
  * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
@@ -203,8 +204,10 @@ struct dentry *ovl_dentry_real(struct dentry *dentry);
 struct ovl_dir_cache *ovl_dir_cache(struct dentry *dentry);
 void ovl_set_dir_cache(struct dentry *dentry, struct ovl_dir_cache *cache);
 bool ovl_dentry_is_opaque(struct dentry *dentry);
+bool ovl_dentry_is_impure(struct dentry *dentry);
 bool ovl_dentry_is_whiteout(struct dentry *dentry);
 void ovl_dentry_set_opaque(struct dentry *dentry);
+void ovl_dentry_set_impure(struct dentry *dentry);
 bool ovl_redirect_dir(struct super_block *sb);
 void ovl_clear_redirect_dir(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index b2023ddb..9b9361f 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -42,6 +42,7 @@ struct ovl_entry {
 			u64 version;
 			const char *redirect;
 			bool opaque;
+			bool impure;
 			bool copying;
 		};
 		struct rcu_head rcu;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index cfdea47..b17a936 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -172,9 +172,17 @@ void ovl_set_dir_cache(struct dentry *dentry, struct ovl_dir_cache *cache)
 bool ovl_dentry_is_opaque(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
+
 	return oe->opaque;
 }
 
+bool ovl_dentry_is_impure(struct dentry *dentry)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+
+	return oe->impure;
+}
+
 bool ovl_dentry_is_whiteout(struct dentry *dentry)
 {
 	return !dentry->d_inode && ovl_dentry_is_opaque(dentry);
@@ -187,6 +195,13 @@ void ovl_dentry_set_opaque(struct dentry *dentry)
 	oe->opaque = true;
 }
 
+void ovl_dentry_set_impure(struct dentry *dentry)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+
+	oe->impure = true;
+}
+
 bool ovl_redirect_dir(struct super_block *sb)
 {
 	struct ovl_fs *ofs = sb->s_fs_info;
-- 
2.7.4

[PATCH 2/2] ovl: constant d_ino across copy up

[Amir Goldstein]

This patch is based on an earlier POC by Miklos Szeredi.

When all layers are on the same fs, and iterating a directory which
may contain copy up entries, call vfs_getattr() on the overlay entries
to make sure that d_ino will be consistent with st_ino from stat(2).

There is an overhead of lookup per upper entry in readdir.
That overhead is a waste for a merge dir with only pure upper
entries (i.e. no copy ups), but that can be optimized later.

The overhead is minimal if the iterated entries are already in dcache.
It is also quite useful for the common case of 'ls -l' that readdir()
pre populates the dcache with the listed entries, making the following
stat() calls faster.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/namei.c     |   7 ++-
 fs/overlayfs/overlayfs.h |   2 +-
 fs/overlayfs/readdir.c   | 115 ++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 0c72a59..d8daecf 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -331,18 +331,21 @@ static int ovl_check_origin(struct dentry *dentry, struct dentry *upperdentry,
  * Returns next layer in stack starting from top.
  * Returns -1 if this is the last layer.
  */
-int ovl_path_next(int idx, struct dentry *dentry, struct path *path)
+int ovl_path_next(int idx, struct dentry *dentry, struct path *path, int *idxp)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
 
 	BUG_ON(idx < 0);
 	if (idx == 0) {
 		ovl_path_upper(dentry, path);
-		if (path->dentry)
+		if (path->dentry) {
+			*idxp = 0;
 			return oe->numlower ? 1 : -1;
+		}
 		idx++;
 	}
 	BUG_ON(idx > oe->numlower);
+	*idxp = idx;
 	*path = oe->lowerstack[idx - 1];
 
 	return (idx < oe->numlower) ? idx + 1 : -1;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index fae52f5..2a44b05 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -224,7 +224,7 @@ int ovl_copy_up_start(struct dentry *dentry);
 void ovl_copy_up_end(struct dentry *dentry);
 
 /* namei.c */
-int ovl_path_next(int idx, struct dentry *dentry, struct path *path);
+int ovl_path_next(int idx, struct dentry *dentry, struct path *path, int *idxp);
 struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags);
 bool ovl_lower_positive(struct dentry *dentry);
 
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index f241b4e..d999899 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -15,15 +15,18 @@
 #include <linux/rbtree.h>
 #include <linux/security.h>
 #include <linux/cred.h>
+#include <linux/ratelimit.h>
 #include "overlayfs.h"
 
 struct ovl_cache_entry {
 	unsigned int len;
 	unsigned int type;
+	u64 real_ino;
 	u64 ino;
 	struct list_head l_node;
 	struct rb_node node;
 	struct ovl_cache_entry *next_maybe_whiteout;
+	int idx;
 	bool is_whiteout;
 	char name[];
 };
@@ -43,6 +46,7 @@ struct ovl_readdir_data {
 	struct list_head middle;
 	struct ovl_cache_entry *first_maybe_whiteout;
 	int count;
+	int idx;
 	int err;
 	bool d_type_supported;
 };
@@ -97,8 +101,11 @@ static struct ovl_cache_entry *ovl_cache_entry_new(struct ovl_readdir_data *rdd,
 	p->name[len] = '\0';
 	p->len = len;
 	p->type = d_type;
-	p->ino = ino;
+	p->real_ino = ino;
+	/* Defer setting d_ino for upper entry to ovl_iterate() */
+	p->ino = rdd->idx ? ino : 0;
 	p->is_whiteout = false;
+	p->idx = rdd->idx;
 
 	if (d_type == DT_CHR) {
 		p->next_maybe_whiteout = rdd->first_maybe_whiteout;
@@ -225,6 +232,7 @@ static int ovl_check_whiteouts(struct dentry *dir, struct ovl_readdir_data *rdd)
 	}
 	revert_creds(old_cred);
 
+
 	return err;
 }
 
@@ -256,21 +264,38 @@ static inline int ovl_dir_read(struct path *realpath,
 	return err;
 }
 
+/* Can we iterate real dir directly? */
+static bool ovl_dir_is_real(struct dentry *dir)
+{
+	enum ovl_path_type type = ovl_path_type(dir);
+
+	if (OVL_TYPE_MERGE(type))
+		return false;
+	/* Upper dir may contain copied up entries that were moved into it */
+	if (ovl_same_sb(dir->d_sb))
+		return !OVL_TYPE_UPPER(type) || !ovl_dentry_is_impure(dir);
+	return true;
+}
+
 static void ovl_dir_reset(struct file *file)
 {
 	struct ovl_dir_file *od = file->private_data;
 	struct ovl_dir_cache *cache = od->cache;
 	struct dentry *dentry = file->f_path.dentry;
-	enum ovl_path_type type = ovl_path_type(dentry);
+	bool is_real;
 
 	if (cache && ovl_dentry_version_get(dentry) != cache->version) {
 		ovl_cache_put(od, dentry);
 		od->cache = NULL;
 		od->cursor = NULL;
 	}
-	WARN_ON(!od->is_real && !OVL_TYPE_MERGE(type));
-	if (od->is_real && OVL_TYPE_MERGE(type))
+	is_real = ovl_dir_is_real(dentry);
+	if (od->is_real != is_real) {
+		/* is_real can only become false (after dir copy up) */
+		if (WARN_ON(is_real))
+			return;
 		od->is_real = false;
+	}
 }
 
 static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list)
@@ -287,7 +312,7 @@ static int ovl_dir_read_merged(struct dentry *dentry, struct list_head *list)
 	int idx, next;
 
 	for (idx = 0; idx != -1; idx = next) {
-		next = ovl_path_next(idx, dentry, &realpath);
+		next = ovl_path_next(idx, dentry, &realpath, &rdd.idx);
 
 		if (next != -1) {
 			err = ovl_dir_read(&realpath, &rdd);
@@ -353,11 +378,81 @@ static struct ovl_dir_cache *ovl_cache_get(struct dentry *dentry)
 	return cache;
 }
 
+/*
+ * Set d_ino for upper entries. Non-upper entries should always report
+ * the uppermost real inode ino and should not call this function.
+ *
+ * When not all layer are on same fs, report real ino also for upper.
+ *
+ * When all layers are on the same fs, and upper has a reference to
+ * copy up origin, call vfs_getattr() on the overlay entry to make
+ * sure that d_ino will be consistent with st_ino from stat(2).
+ */
+static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
+
+{
+	struct dentry *dir = path->dentry;
+	struct dentry *this = NULL;
+	enum ovl_path_type type;
+	u64 ino = p->real_ino;
+	int err = 0;
+
+	if (!ovl_same_sb(dir->d_sb) || WARN_ON(p->idx))
+		goto out;
+
+	if (p->name[0] == '.') {
+		if (p->len == 1) {
+			this = dget(dir);
+			goto get;
+		}
+		if (p->len == 2 && p->name[1] == '.') {
+			/* we shall not be moved */
+			this = dget(dir->d_parent);
+			goto get;
+		}
+	}
+	this = lookup_one_len(p->name, dir, p->len);
+	if (IS_ERR_OR_NULL(this) || !this->d_inode) {
+		if (IS_ERR(this)) {
+			err = PTR_ERR(this);
+			this = NULL;
+			goto fail;
+		}
+		goto out;
+	}
+
+get:
+	type = ovl_path_type(this);
+	if (OVL_TYPE_ORIGIN(type)) {
+		struct kstat stat;
+		struct path statpath = *path;
+
+		statpath.dentry = this;
+		err = vfs_getattr(&statpath, &stat, STATX_INO, 0);
+		if (err)
+			goto fail;
+
+		WARN_ON_ONCE(dir->d_sb->s_dev != stat.dev);
+		ino = stat.ino;
+	}
+
+out:
+	p->ino = ino;
+	dput(this);
+	return err;
+
+fail:
+	pr_warn_ratelimited("overlay: failed to look up (%s) for ino (%i)\n",
+			    p->name, err);
+	goto out;
+}
+
 static int ovl_iterate(struct file *file, struct dir_context *ctx)
 {
 	struct ovl_dir_file *od = file->private_data;
 	struct dentry *dentry = file->f_path.dentry;
 	struct ovl_cache_entry *p;
+	int err;
 
 	if (!ctx->pos)
 		ovl_dir_reset(file);
@@ -378,9 +473,15 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 
 	while (od->cursor != &od->cache->entries) {
 		p = list_entry(od->cursor, struct ovl_cache_entry, l_node);
-		if (!p->is_whiteout)
+		if (!p->is_whiteout) {
+			if (!p->ino) {
+				err = ovl_cache_update_ino(&file->f_path, p);
+				if (err)
+					return err;
+			}
 			if (!dir_emit(ctx, p->name, p->len, p->ino, p->type))
 				break;
+		}
 		od->cursor = p->l_node.next;
 		ctx->pos++;
 	}
@@ -502,7 +603,7 @@ static int ovl_dir_open(struct inode *inode, struct file *file)
 		return PTR_ERR(realfile);
 	}
 	od->realfile = realfile;
-	od->is_real = !OVL_TYPE_MERGE(type);
+	od->is_real = ovl_dir_is_real(file->f_path.dentry);
 	od->is_upper = OVL_TYPE_UPPER(type);
 	file->private_data = od;
 
-- 
2.7.4

Re: [PATCH 2/2] ovl: constant d_ino across copy up

[Amir Goldstein]

On Thu, May 11, 2017 at 4:42 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> This patch is based on an earlier POC by Miklos Szeredi.
>
> When all layers are on the same fs, and iterating a directory which
> may contain copy up entries, call vfs_getattr() on the overlay entries
> to make sure that d_ino will be consistent with st_ino from stat(2).
>
[...]
>
> +/*
> + * Set d_ino for upper entries. Non-upper entries should always report
> + * the uppermost real inode ino and should not call this function.
> + *
> + * When not all layer are on same fs, report real ino also for upper.
> + *
> + * When all layers are on the same fs, and upper has a reference to
> + * copy up origin, call vfs_getattr() on the overlay entry to make
> + * sure that d_ino will be consistent with st_ino from stat(2).
> + */
> +static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
> +
> +{
> +       struct dentry *dir = path->dentry;
> +       struct dentry *this = NULL;
> +       enum ovl_path_type type;
> +       u64 ino = p->real_ino;
> +       int err = 0;
> +
> +       if (!ovl_same_sb(dir->d_sb) || WARN_ON(p->idx))
> +               goto out;
> +
> +       if (p->name[0] == '.') {
> +               if (p->len == 1) {
> +                       this = dget(dir);
> +                       goto get;
> +               }
> +               if (p->len == 2 && p->name[1] == '.') {
> +                       /* we shall not be moved */
> +                       this = dget(dir->d_parent);
> +                       goto get;
> +               }
> +       }
> +       this = lookup_one_len(p->name, dir, p->len);

Mmm... that reeks of layer violation, but I can't find anything obvious
that could go wrong from calling lookup on overlay dir here, is there?

It's just so convenient to use overlay lookup here, get the overlay
entry from cache and stat it, so I can't think of a better alternative.

BTW, couldn't find anything obviously wrong with stat'ing overlay
dir and dir->d_parent here either, is there?

> +get:
> +       type = ovl_path_type(this);
> +       if (OVL_TYPE_ORIGIN(type)) {
> +               struct kstat stat;
> +               struct path statpath = *path;
> +
> +               statpath.dentry = this;
> +               err = vfs_getattr(&statpath, &stat, STATX_INO, 0);
> +               if (err)
> +                       goto fail;
> +
> +               WARN_ON_ONCE(dir->d_sb->s_dev != stat.dev);
> +               ino = stat.ino;
> +       }
> +

Re: [PATCH 1/2] ovl: mark upper dir with type origin entries "impure"

[Miklos Szeredi]

On Thu, May 11, 2017 at 3:42 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> When moving a merge dir or non-dir with copy up origin into
> a non-merge upper dir (a.k.a pure upper dir), we are marking
> the target parent dir "impure". ovl_iterate() iterates pure
> upper dirs directly, because there is no need to filter out
> whiteouts and merge dir content with lower dir. But for the
> case of an "impure" upper dir, ovl_iterate() will not be able
> to iterate the real upper dir directly, because it will need
> to lookup the origin inode and use it to fill d_ino.

Applied, thanks.

Miklos

>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/dir.c       | 41 ++++++++++++++++++++++++++++++++++++++++-
>  fs/overlayfs/namei.c     | 18 ++++++++++++++++--
>  fs/overlayfs/overlayfs.h |  3 +++
>  fs/overlayfs/ovl_entry.h |  1 +
>  fs/overlayfs/util.c      | 15 +++++++++++++++
>  5 files changed, 75 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 723b98b..205e491 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -138,6 +138,17 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
>         return err;
>  }
>
> +static int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
> +{
> +       int err;
> +
> +       err = ovl_do_setxattr(upperdentry, OVL_XATTR_IMPURE, "y", 1, 0);
> +       if (!err)
> +               ovl_dentry_set_impure(dentry);
> +
> +       return err;
> +}
> +
>  /* Common operations required to be done after creation of file on upper */
>  static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
>                             struct dentry *newdentry, bool hardlink)
> @@ -162,6 +173,11 @@ static bool ovl_type_merge(struct dentry *dentry)
>         return OVL_TYPE_MERGE(ovl_path_type(dentry));
>  }
>
> +static bool ovl_type_origin(struct dentry *dentry)
> +{
> +       return OVL_TYPE_ORIGIN(ovl_path_type(dentry));
> +}
> +
>  static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>                             struct cattr *attr, struct dentry *hardlink)
>  {
> @@ -943,6 +959,30 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>         old_upperdir = ovl_dentry_upper(old->d_parent);
>         new_upperdir = ovl_dentry_upper(new->d_parent);
>
> +       if (ovl_same_sb(old->d_sb) && !samedir) {
> +               /*
> +                * When moving a merge dir or non-dir with copy up origin into
> +                * a non-merge upper dir (a.k.a pure upper dir), we are making
> +                * the target parent dir "impure". ovl_iterate() iterates pure
> +                * upper dirs directly, because there is no need to filter out
> +                * whiteouts and merge dir content with lower dir. But for the
> +                * case of an "impure" upper dir, ovl_iterate() cannot iterate
> +                * the real directory directly, because it looks for the inode
> +                * numbers to fill d_ino in the entries origin inode.
> +                */
> +               if (ovl_type_origin(old) && !ovl_type_merge(new->d_parent)) {
> +                       err = ovl_set_impure(new->d_parent, new_upperdir);
> +                       if (err)
> +                               goto out_dput;
> +               }
> +               if (!overwrite && ovl_type_origin(new) &&
> +                   !ovl_type_merge(old->d_parent)) {
> +                       err = ovl_set_impure(old->d_parent, old_upperdir);
> +                       if (err)
> +                               goto out_dput;
> +               }
> +       }
> +
>         trap = lock_rename(new_upperdir, old_upperdir);
>
>         olddentry = lookup_one_len(old->d_name.name, old_upperdir,
> @@ -1004,7 +1044,6 @@ static int ovl_rename(struct inode *olddir, struct dentry *old,
>                 if (err)
>                         goto out_dput;
>         }
> -
>         err = ovl_do_rename(old_upperdir->d_inode, olddentry,
>                             new_upperdir->d_inode, newdentry, flags);
>         if (err)
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index bad0f66..0c72a59 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -167,7 +167,7 @@ static struct dentry *ovl_get_origin(struct dentry *dentry,
>         goto out;
>  }
>
> -static bool ovl_is_opaquedir(struct dentry *dentry)
> +static bool ovl_check_dir_xattr(struct dentry *dentry, const char *name)
>  {
>         int res;
>         char val;
> @@ -175,13 +175,23 @@ static bool ovl_is_opaquedir(struct dentry *dentry)
>         if (!d_is_dir(dentry))
>                 return false;
>
> -       res = vfs_getxattr(dentry, OVL_XATTR_OPAQUE, &val, 1);
> +       res = vfs_getxattr(dentry, name, &val, 1);
>         if (res == 1 && val == 'y')
>                 return true;
>
>         return false;
>  }
>
> +static bool ovl_is_opaquedir(struct dentry *dentry)
> +{
> +       return ovl_check_dir_xattr(dentry, OVL_XATTR_OPAQUE);
> +}
> +
> +static bool ovl_is_impuredir(struct dentry *dentry)
> +{
> +       return ovl_check_dir_xattr(dentry, OVL_XATTR_IMPURE);
> +}
> +
>  static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
>                              const char *name, unsigned int namelen,
>                              size_t prelen, const char *post,
> @@ -351,6 +361,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         unsigned int ctr = 0;
>         struct inode *inode = NULL;
>         bool upperopaque = false;
> +       bool upperimpure = false;
>         char *upperredirect = NULL;
>         struct dentry *this;
>         unsigned int i;
> @@ -395,6 +406,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                                 poe = roe;
>                 }
>                 upperopaque = d.opaque;
> +               if (upperdentry && d.is_dir)
> +                       upperimpure = ovl_is_impuredir(upperdentry);
>         }
>
>         if (!d.stop && poe->numlower) {
> @@ -463,6 +476,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>
>         revert_creds(old_cred);
>         oe->opaque = upperopaque;
> +       oe->impure = upperimpure;
>         oe->redirect = upperredirect;
>         oe->__upperdentry = upperdentry;
>         memcpy(oe->lowerstack, stack, sizeof(struct path) * ctr);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index caa36cb..fae52f5 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -24,6 +24,7 @@ enum ovl_path_type {
>  #define OVL_XATTR_OPAQUE OVL_XATTR_PREFIX "opaque"
>  #define OVL_XATTR_REDIRECT OVL_XATTR_PREFIX "redirect"
>  #define OVL_XATTR_ORIGIN OVL_XATTR_PREFIX "origin"
> +#define OVL_XATTR_IMPURE OVL_XATTR_PREFIX "impure"
>
>  /*
>   * The tuple (fh,uuid) is a universal unique identifier for a copy up origin,
> @@ -203,8 +204,10 @@ struct dentry *ovl_dentry_real(struct dentry *dentry);
>  struct ovl_dir_cache *ovl_dir_cache(struct dentry *dentry);
>  void ovl_set_dir_cache(struct dentry *dentry, struct ovl_dir_cache *cache);
>  bool ovl_dentry_is_opaque(struct dentry *dentry);
> +bool ovl_dentry_is_impure(struct dentry *dentry);
>  bool ovl_dentry_is_whiteout(struct dentry *dentry);
>  void ovl_dentry_set_opaque(struct dentry *dentry);
> +void ovl_dentry_set_impure(struct dentry *dentry);
>  bool ovl_redirect_dir(struct super_block *sb);
>  void ovl_clear_redirect_dir(struct super_block *sb);
>  const char *ovl_dentry_get_redirect(struct dentry *dentry);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index b2023ddb..9b9361f 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -42,6 +42,7 @@ struct ovl_entry {
>                         u64 version;
>                         const char *redirect;
>                         bool opaque;
> +                       bool impure;
>                         bool copying;
>                 };
>                 struct rcu_head rcu;
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index cfdea47..b17a936 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -172,9 +172,17 @@ void ovl_set_dir_cache(struct dentry *dentry, struct ovl_dir_cache *cache)
>  bool ovl_dentry_is_opaque(struct dentry *dentry)
>  {
>         struct ovl_entry *oe = dentry->d_fsdata;
> +
>         return oe->opaque;
>  }
>
> +bool ovl_dentry_is_impure(struct dentry *dentry)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +
> +       return oe->impure;
> +}
> +
>  bool ovl_dentry_is_whiteout(struct dentry *dentry)
>  {
>         return !dentry->d_inode && ovl_dentry_is_opaque(dentry);
> @@ -187,6 +195,13 @@ void ovl_dentry_set_opaque(struct dentry *dentry)
>         oe->opaque = true;
>  }
>
> +void ovl_dentry_set_impure(struct dentry *dentry)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +
> +       oe->impure = true;
> +}
> +
>  bool ovl_redirect_dir(struct super_block *sb)
>  {
>         struct ovl_fs *ofs = sb->s_fs_info;
> --
> 2.7.4
>

Re: [PATCH 2/2] ovl: constant d_ino across copy up

[Miklos Szeredi]

On Sat, May 13, 2017 at 4:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, May 11, 2017 at 4:42 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> This patch is based on an earlier POC by Miklos Szeredi.
>>
>> When all layers are on the same fs, and iterating a directory which
>> may contain copy up entries, call vfs_getattr() on the overlay entries
>> to make sure that d_ino will be consistent with st_ino from stat(2).
>>
> [...]
>>
>> +/*
>> + * Set d_ino for upper entries. Non-upper entries should always report
>> + * the uppermost real inode ino and should not call this function.
>> + *
>> + * When not all layer are on same fs, report real ino also for upper.
>> + *
>> + * When all layers are on the same fs, and upper has a reference to
>> + * copy up origin, call vfs_getattr() on the overlay entry to make
>> + * sure that d_ino will be consistent with st_ino from stat(2).
>> + */
>> +static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
>> +
>> +{
>> +       struct dentry *dir = path->dentry;
>> +       struct dentry *this = NULL;
>> +       enum ovl_path_type type;
>> +       u64 ino = p->real_ino;
>> +       int err = 0;
>> +
>> +       if (!ovl_same_sb(dir->d_sb) || WARN_ON(p->idx))
>> +               goto out;
>> +
>> +       if (p->name[0] == '.') {
>> +               if (p->len == 1) {
>> +                       this = dget(dir);
>> +                       goto get;
>> +               }
>> +               if (p->len == 2 && p->name[1] == '.') {
>> +                       /* we shall not be moved */
>> +                       this = dget(dir->d_parent);
>> +                       goto get;
>> +               }
>> +       }
>> +       this = lookup_one_len(p->name, dir, p->len);
>
> Mmm... that reeks of layer violation, but I can't find anything obvious
> that could go wrong from calling lookup on overlay dir here, is there?
>
> It's just so convenient to use overlay lookup here, get the overlay
> entry from cache and stat it, so I can't think of a better alternative.
>
> BTW, couldn't find anything obviously wrong with stat'ing overlay
> dir and dir->d_parent here either, is there?

One issue is permission checking: lookup needs MAY_EXEC while readdir
needs MAY_READ.  So we can't just call lookup_one_len(), but will have
to do the thing by hand.  Also no need to construct the overlay
dentry, just pre-loading the underlying dentries will have almost all
the pros (and cons).

Thanks,
Miklos

Re: [PATCH 2/2] ovl: constant d_ino across copy up

[Amir Goldstein]

On Fri, May 19, 2017 at 12:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Sat, May 13, 2017 at 4:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Thu, May 11, 2017 at 4:42 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> This patch is based on an earlier POC by Miklos Szeredi.
>>>
>>> When all layers are on the same fs, and iterating a directory which
>>> may contain copy up entries, call vfs_getattr() on the overlay entries
>>> to make sure that d_ino will be consistent with st_ino from stat(2).
>>>
>> [...]
>>>
>>> +/*
>>> + * Set d_ino for upper entries. Non-upper entries should always report
>>> + * the uppermost real inode ino and should not call this function.
>>> + *
>>> + * When not all layer are on same fs, report real ino also for upper.
>>> + *
>>> + * When all layers are on the same fs, and upper has a reference to
>>> + * copy up origin, call vfs_getattr() on the overlay entry to make
>>> + * sure that d_ino will be consistent with st_ino from stat(2).
>>> + */
>>> +static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
>>> +
>>> +{
>>> +       struct dentry *dir = path->dentry;
>>> +       struct dentry *this = NULL;
>>> +       enum ovl_path_type type;
>>> +       u64 ino = p->real_ino;
>>> +       int err = 0;
>>> +
>>> +       if (!ovl_same_sb(dir->d_sb) || WARN_ON(p->idx))
>>> +               goto out;
>>> +
>>> +       if (p->name[0] == '.') {
>>> +               if (p->len == 1) {
>>> +                       this = dget(dir);
>>> +                       goto get;
>>> +               }
>>> +               if (p->len == 2 && p->name[1] == '.') {
>>> +                       /* we shall not be moved */
>>> +                       this = dget(dir->d_parent);
>>> +                       goto get;
>>> +               }
>>> +       }
>>> +       this = lookup_one_len(p->name, dir, p->len);
>>
>> Mmm... that reeks of layer violation, but I can't find anything obvious
>> that could go wrong from calling lookup on overlay dir here, is there?
>>
>> It's just so convenient to use overlay lookup here, get the overlay
>> entry from cache and stat it, so I can't think of a better alternative.
>>
>> BTW, couldn't find anything obviously wrong with stat'ing overlay
>> dir and dir->d_parent here either, is there?
>
> One issue is permission checking: lookup needs MAY_EXEC while readdir
> needs MAY_READ.  So we can't just call lookup_one_len(), but will have
> to do the thing by hand.  Also no need to construct the overlay
> dentry, just pre-loading the underlying dentries will have almost all
> the pros (and cons).
>

OK. Let me see if I get this.
We will need to do by hand:

upperdentry = lookup_one_len(p->name, upperdir, p->len);
ovl_check_origin(dir, upperdentry, &originpath);

And then call some diminished variant of ovl_getattr() to
stat them both and figure out the official ino.

Not the end of the world when lower and upper dentries are
cached as you noted.

BUT, for lookup_one_len(upperdir) we anyway need to
ovl_override_creds().

Doesn't that pull the rug from under your MAY_EXEC argument?
May as well do ovl_override_creds(); lookup_one_len(dir)
and be done with it.

Unless there are other reasons why this is wrong?

Thanks,
Amir.

Re: [PATCH 2/2] ovl: constant d_ino across copy up

[Miklos Szeredi]

On Fri, May 19, 2017 at 1:34 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, May 19, 2017 at 12:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Sat, May 13, 2017 at 4:25 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Thu, May 11, 2017 at 4:42 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> This patch is based on an earlier POC by Miklos Szeredi.
>>>>
>>>> When all layers are on the same fs, and iterating a directory which
>>>> may contain copy up entries, call vfs_getattr() on the overlay entries
>>>> to make sure that d_ino will be consistent with st_ino from stat(2).
>>>>
>>> [...]
>>>>
>>>> +/*
>>>> + * Set d_ino for upper entries. Non-upper entries should always report
>>>> + * the uppermost real inode ino and should not call this function.
>>>> + *
>>>> + * When not all layer are on same fs, report real ino also for upper.
>>>> + *
>>>> + * When all layers are on the same fs, and upper has a reference to
>>>> + * copy up origin, call vfs_getattr() on the overlay entry to make
>>>> + * sure that d_ino will be consistent with st_ino from stat(2).
>>>> + */
>>>> +static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
>>>> +
>>>> +{
>>>> +       struct dentry *dir = path->dentry;
>>>> +       struct dentry *this = NULL;
>>>> +       enum ovl_path_type type;
>>>> +       u64 ino = p->real_ino;
>>>> +       int err = 0;
>>>> +
>>>> +       if (!ovl_same_sb(dir->d_sb) || WARN_ON(p->idx))
>>>> +               goto out;
>>>> +
>>>> +       if (p->name[0] == '.') {
>>>> +               if (p->len == 1) {
>>>> +                       this = dget(dir);
>>>> +                       goto get;
>>>> +               }
>>>> +               if (p->len == 2 && p->name[1] == '.') {
>>>> +                       /* we shall not be moved */
>>>> +                       this = dget(dir->d_parent);
>>>> +                       goto get;
>>>> +               }
>>>> +       }
>>>> +       this = lookup_one_len(p->name, dir, p->len);
>>>
>>> Mmm... that reeks of layer violation, but I can't find anything obvious
>>> that could go wrong from calling lookup on overlay dir here, is there?
>>>
>>> It's just so convenient to use overlay lookup here, get the overlay
>>> entry from cache and stat it, so I can't think of a better alternative.
>>>
>>> BTW, couldn't find anything obviously wrong with stat'ing overlay
>>> dir and dir->d_parent here either, is there?
>>
>> One issue is permission checking: lookup needs MAY_EXEC while readdir
>> needs MAY_READ.  So we can't just call lookup_one_len(), but will have
>> to do the thing by hand.  Also no need to construct the overlay
>> dentry, just pre-loading the underlying dentries will have almost all
>> the pros (and cons).
>>
>
> OK. Let me see if I get this.
> We will need to do by hand:
>
> upperdentry = lookup_one_len(p->name, upperdir, p->len);
> ovl_check_origin(dir, upperdentry, &originpath);
>
> And then call some diminished variant of ovl_getattr() to
> stat them both and figure out the official ino.
>
> Not the end of the world when lower and upper dentries are
> cached as you noted.
>
> BUT, for lookup_one_len(upperdir) we anyway need to
> ovl_override_creds().
>
> Doesn't that pull the rug from under your MAY_EXEC argument?
> May as well do ovl_override_creds(); lookup_one_len(dir)
> and be done with it.
>
> Unless there are other reasons why this is wrong?

We are supposed use mounter's creds for the underlying fs and task's
creds for the overlay.  The permission we get with mounter's creds is
not necessarily a superset of the permissions with the task's creds.

Vivek, please correct me if I'm wrong.

Thanks,
Miklos

Re: [PATCH 2/2] ovl: constant d_ino across copy up

[Amir Goldstein]

On Fri, May 19, 2017 at 3:22 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, May 19, 2017 at 1:34 PM, Amir Goldstein <amir73il@gmail.com> wrote:

>>>> BTW, couldn't find anything obviously wrong with stat'ing overlay
>>>> dir and dir->d_parent here either, is there?
>>>
>>> One issue is permission checking: lookup needs MAY_EXEC while readdir
>>> needs MAY_READ.  So we can't just call lookup_one_len(), but will have
>>> to do the thing by hand.  Also no need to construct the overlay
>>> dentry, just pre-loading the underlying dentries will have almost all
>>> the pros (and cons).
>>>
>>
>> OK. Let me see if I get this.
>> We will need to do by hand:
>>
>> upperdentry = lookup_one_len(p->name, upperdir, p->len);
>> ovl_check_origin(dir, upperdentry, &originpath);
>>
>> And then call some diminished variant of ovl_getattr() to
>> stat them both and figure out the official ino.
>>
>> Not the end of the world when lower and upper dentries are
>> cached as you noted.
>>
>> BUT, for lookup_one_len(upperdir) we anyway need to
>> ovl_override_creds().
>>
>> Doesn't that pull the rug from under your MAY_EXEC argument?
>> May as well do ovl_override_creds(); lookup_one_len(dir)
>> and be done with it.
>>
>> Unless there are other reasons why this is wrong?
>
> We are supposed use mounter's creds for the underlying fs and task's
> creds for the overlay.  The permission we get with mounter's creds is
> not necessarily a superset of the permissions with the task's creds.
>

Right, so perhaps mounter's creds isn't the right choice.
What we need is to install new credentials with elevated permissions
for lookup. Can we do that?

But looking at this from another perspective, if user can readdir,
but cannot lookup inside it, then user cannot stat the files listed by
readdir and therefore user cannot observe the st_ino/d_ino
inconsistency due to lookup failure. If we say that we don't really
care about this corner case, then we silently ignore -EACCESS
and report the upper d_ino.

But I am mostly debating this for the sake of debating. If you prefer
to do this "by hand" because it's the "right thing to do", that seems
like a good enough reason to me. As I wrote, this reeks of a layer
violation that may come back to bite us in the future,  even if right now,
we cannot figure out why.

Amir.