[PATCH v3 0/4] Overlayfs fileattr related fixes

[PATCH v3 0/4] Overlayfs fileattr related fixes

[Amir Goldstein]

Miklos,

Following patch set addresses your comments to v2.
It passed all the old and new xfstests [3].

Thanks,
Amir.

Changes since v2 [2]:
- Rename overlay.xflags => overlay.protected
- Generic vfs helper for filling statx flags
- Un-generalize flag conversion helpers
- Do not be forgiving with noxattr upper fs

Changes since v1 [1]:
- Store (i),(a) flags in xattr text format
- Copy up (A),(S) flags to upper fileattr
- Fixes the problems with setting ovl dirs and hardlinks immutable

[1] https://lore.kernel.org/linux-unionfs/CAJfpeguMQca-+vTdzoDdDWNJraWyqMa3vYRFDWPMk_R6-L7Obw@mail.gmail.com/
[2] https://lore.kernel.org/linux-unionfs/CAOQ4uxgdWwrOa79BRzZ1PS6SxmLtywQCAr3+WLRZPx38aHHyQw@mail.gmail.com/
[3] https://github.com/amir73il/xfstests/commits/ovl-xflags

Amir Goldstein (4):
  fs: add generic helper for filling statx attribute flags
  ovl: pass ovl_fs to ovl_check_setxattr()
  ovl: copy up sync/noatime fileattr flags
  ovl: consistent behavior for immutable/append-only inodes

 fs/orangefs/inode.c      |   7 +--
 fs/overlayfs/copy_up.c   |  72 +++++++++++++++++++----
 fs/overlayfs/dir.c       |   6 +-
 fs/overlayfs/inode.c     |  74 +++++++++++++++++++-----
 fs/overlayfs/namei.c     |   2 +-
 fs/overlayfs/overlayfs.h |  44 ++++++++++++--
 fs/overlayfs/util.c      | 122 +++++++++++++++++++++++++++++++++++++--
 fs/stat.c                |  18 ++++++
 include/linux/fs.h       |   1 +
 include/linux/stat.h     |   4 ++
 10 files changed, 307 insertions(+), 43 deletions(-)

-- 
2.32.0

[PATCH v3 1/4] fs: add generic helper for filling statx attribute flags

[Amir Goldstein]

The immutable and append-only properties on an inode are published on
the inode's i_flags and enforced by the VFS.

Create a helper to fill the corresponding STATX_ATTR_ flags in the kstat
structure from the inode's i_flags.

Only orange was converted to use this helper.
Other filesystems could use it in the future.

Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/orangefs/inode.c  |  7 +------
 fs/stat.c            | 18 ++++++++++++++++++
 include/linux/fs.h   |  1 +
 include/linux/stat.h |  4 ++++
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index 6bf35a0d61f3..4092009716a3 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -883,12 +883,7 @@ int orangefs_getattr(struct user_namespace *mnt_userns, const struct path *path,
 		if (!(request_mask & STATX_SIZE))
 			stat->result_mask &= ~STATX_SIZE;
 
-		stat->attributes_mask = STATX_ATTR_IMMUTABLE |
-		    STATX_ATTR_APPEND;
-		if (inode->i_flags & S_IMMUTABLE)
-			stat->attributes |= STATX_ATTR_IMMUTABLE;
-		if (inode->i_flags & S_APPEND)
-			stat->attributes |= STATX_ATTR_APPEND;
+		generic_fill_statx_attr(inode, stat);
 	}
 	return ret;
 }
diff --git a/fs/stat.c b/fs/stat.c
index 1fa38bdec1a6..314269150b5b 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -59,6 +59,24 @@ void generic_fillattr(struct user_namespace *mnt_userns, struct inode *inode,
 }
 EXPORT_SYMBOL(generic_fillattr);
 
+/**
+ * generic_fill_statx_attr - Fill in the statx attributes from the inode flags
+ * @inode:	Inode to use as the source
+ * @stat:	Where to fill in the attribute flags
+ *
+ * Fill in the STATX_ATTR_ flags in the kstat structure for properties of the
+ * inode that are published on i_flags and enforced by the VFS.
+ */
+void generic_fill_statx_attr(struct inode *inode, struct kstat *stat)
+{
+	if (inode->i_flags & S_IMMUTABLE)
+		stat->attributes |= STATX_ATTR_IMMUTABLE;
+	if (inode->i_flags & S_APPEND)
+		stat->attributes |= STATX_ATTR_APPEND;
+	stat->attributes_mask |= KSTAT_ATTR_VFS_FLAGS;
+}
+EXPORT_SYMBOL(generic_fill_statx_attr);
+
 /**
  * vfs_getattr_nosec - getattr without security checks
  * @path: file to get attributes from
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c3c88fdb9b2a..647664316013 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3353,6 +3353,7 @@ extern int page_symlink(struct inode *inode, const char *symname, int len);
 extern const struct inode_operations page_symlink_inode_operations;
 extern void kfree_link(void *);
 void generic_fillattr(struct user_namespace *, struct inode *, struct kstat *);
+void generic_fill_statx_attr(struct inode *inode, struct kstat *stat);
 extern int vfs_getattr_nosec(const struct path *, struct kstat *, u32, unsigned int);
 extern int vfs_getattr(const struct path *, struct kstat *, u32, unsigned int);
 void __inode_add_bytes(struct inode *inode, loff_t bytes);
diff --git a/include/linux/stat.h b/include/linux/stat.h
index fff27e603814..7df06931f25d 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -34,6 +34,10 @@ struct kstat {
 	 STATX_ATTR_ENCRYPTED |				\
 	 STATX_ATTR_VERITY				\
 	 )/* Attrs corresponding to FS_*_FL flags */
+#define KSTAT_ATTR_VFS_FLAGS				\
+	(STATX_ATTR_IMMUTABLE |				\
+	 STATX_ATTR_APPEND				\
+	 ) /* Attrs corresponding to S_* flags that are enforced by the VFS */
 	u64		ino;
 	dev_t		dev;
 	dev_t		rdev;
-- 
2.32.0

[PATCH v3 2/4] ovl: pass ovl_fs to ovl_check_setxattr()

[Amir Goldstein]

Instead of passing the overlay dentry.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 10 +++++-----
 fs/overlayfs/dir.c       |  6 ++++--
 fs/overlayfs/namei.c     |  2 +-
 fs/overlayfs/overlayfs.h |  6 +++---
 fs/overlayfs/util.c      |  7 +++----
 5 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 2846b943e80c..3fa68a5cc16e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -331,8 +331,8 @@ struct ovl_fh *ovl_encode_real_fh(struct ovl_fs *ofs, struct dentry *real,
 	return ERR_PTR(err);
 }
 
-int ovl_set_origin(struct ovl_fs *ofs, struct dentry *dentry,
-		   struct dentry *lower, struct dentry *upper)
+int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
+		   struct dentry *upper)
 {
 	const struct ovl_fh *fh = NULL;
 	int err;
@@ -351,7 +351,7 @@ int ovl_set_origin(struct ovl_fs *ofs, struct dentry *dentry,
 	/*
 	 * Do not fail when upper doesn't support xattrs.
 	 */
-	err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh->buf,
+	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
 				 fh ? fh->fb.len : 0, 0);
 	kfree(fh);
 
@@ -526,13 +526,13 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	 * hard link.
 	 */
 	if (c->origin) {
-		err = ovl_set_origin(ofs, c->dentry, c->lowerpath.dentry, temp);
+		err = ovl_set_origin(ofs, c->lowerpath.dentry, temp);
 		if (err)
 			return err;
 	}
 
 	if (c->metacopy) {
-		err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,
+		err = ovl_check_setxattr(ofs, temp, OVL_XATTR_METACOPY,
 					 NULL, 0, -EOPNOTSUPP);
 		if (err)
 			return err;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 03a22954fe61..9154222883e6 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -233,9 +233,10 @@ struct dentry *ovl_create_temp(struct dentry *workdir, struct ovl_cattr *attr)
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
 			       int xerr)
 {
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	int err;
 
-	err = ovl_check_setxattr(dentry, upper, OVL_XATTR_OPAQUE, "y", 1, xerr);
+	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_OPAQUE, "y", 1, xerr);
 	if (!err)
 		ovl_dentry_set_opaque(dentry);
 
@@ -1045,6 +1046,7 @@ static bool ovl_need_absolute_redirect(struct dentry *dentry, bool samedir)
 static int ovl_set_redirect(struct dentry *dentry, bool samedir)
 {
 	int err;
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	const char *redirect = ovl_dentry_get_redirect(dentry);
 	bool absolute_redirect = ovl_need_absolute_redirect(dentry, samedir);
 
@@ -1055,7 +1057,7 @@ static int ovl_set_redirect(struct dentry *dentry, bool samedir)
 	if (IS_ERR(redirect))
 		return PTR_ERR(redirect);
 
-	err = ovl_check_setxattr(dentry, ovl_dentry_upper(dentry),
+	err = ovl_check_setxattr(ofs, ovl_dentry_upper(dentry),
 				 OVL_XATTR_REDIRECT,
 				 redirect, strlen(redirect), -EXDEV);
 	if (!err) {
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 210cd6f66e28..da063b18b419 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -811,7 +811,7 @@ static int ovl_fix_origin(struct ovl_fs *ofs, struct dentry *dentry,
 	if (err)
 		return err;
 
-	err = ovl_set_origin(ofs, dentry, lower, upper);
+	err = ovl_set_origin(ofs, lower, upper);
 	if (!err)
 		err = ovl_set_impure(dentry->d_parent, upper->d_parent);
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 29d71f253db4..6e3a1bea1c9a 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -332,7 +332,7 @@ bool ovl_already_copied_up(struct dentry *dentry, int flags);
 bool ovl_check_origin_xattr(struct ovl_fs *ofs, struct dentry *dentry);
 bool ovl_check_dir_xattr(struct super_block *sb, struct dentry *dentry,
 			 enum ovl_xattr ox);
-int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
+int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
 		       enum ovl_xattr ox, const void *value, size_t size,
 		       int xerr);
 int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry);
@@ -573,8 +573,8 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
 int ovl_set_attr(struct dentry *upper, struct kstat *stat);
 struct ovl_fh *ovl_encode_real_fh(struct ovl_fs *ofs, struct dentry *real,
 				  bool is_upper);
-int ovl_set_origin(struct ovl_fs *ofs, struct dentry *dentry,
-		   struct dentry *lower, struct dentry *upper);
+int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
+		   struct dentry *upper);
 
 /* export.c */
 extern const struct export_operations ovl_export_operations;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index b9d03627f364..81b8f135445a 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -600,12 +600,11 @@ const char *const ovl_xattr_table[][2] = {
 	OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY),
 };
 
-int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
+int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
 		       enum ovl_xattr ox, const void *value, size_t size,
 		       int xerr)
 {
 	int err;
-	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 
 	if (ofs->noxattr)
 		return xerr;
@@ -623,6 +622,7 @@ int ovl_check_setxattr(struct dentry *dentry, struct dentry *upperdentry,
 
 int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
 {
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	int err;
 
 	if (ovl_test_flag(OVL_IMPURE, d_inode(dentry)))
@@ -632,8 +632,7 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
 	 * Do not fail when upper doesn't support xattrs.
 	 * Upper inodes won't have origin nor redirect xattr anyway.
 	 */
-	err = ovl_check_setxattr(dentry, upperdentry, OVL_XATTR_IMPURE,
-				 "y", 1, 0);
+	err = ovl_check_setxattr(ofs, upperdentry, OVL_XATTR_IMPURE, "y", 1, 0);
 	if (!err)
 		ovl_set_flag(OVL_IMPURE, d_inode(dentry));
 
-- 
2.32.0

[PATCH v3 3/4] ovl: copy up sync/noatime fileattr flags

[Amir Goldstein]

When a lower file has sync/noatime fileattr flags, the behavior of
overlayfs post copy up is inconsistent.

Immediattely after copy up, ovl inode still has the S_SYNC/S_NOATIME
inode flags copied from lower inode, so vfs code still treats the ovl
inode as sync/noatime.  After ovl inode evict or mount cycle,
the ovl inode does not have these inode flags anymore.

To fix this inconsitency, try to copy the fileattr flags on copy up
if the upper fs supports the fileattr_set() method.

This gives consistent behavior post copy up regardless of inode eviction
from cache.

We cannot copy up the immutable/append-only inode flags in a similar
manner, because immutable/append-only inodes cannot be linked and because
overlayfs will not be able to set overlay.* xattr on the upper inodes.

Those flags will be addressed by a followup patch.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++++++++++++++------
 fs/overlayfs/inode.c     | 36 ++++++++++++++++++-----------
 fs/overlayfs/overlayfs.h | 14 +++++++++++-
 3 files changed, 78 insertions(+), 21 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 3fa68a5cc16e..a06b423ca5d1 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -8,6 +8,7 @@
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/file.h>
+#include <linux/fileattr.h>
 #include <linux/splice.h>
 #include <linux/xattr.h>
 #include <linux/security.h>
@@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
 	return error;
 }
 
+static int ovl_copy_fileattr(struct path *old, struct path *new)
+{
+	struct fileattr oldfa = { .flags_valid = true };
+	struct fileattr newfa = { .flags_valid = true };
+	int err;
+
+	err = ovl_real_fileattr(old, &oldfa, false);
+	if (err)
+		return err;
+
+	err = ovl_real_fileattr(new, &newfa, false);
+	if (err)
+		return err;
+
+	BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
+	newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
+	newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
+
+	BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
+	newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK;
+	newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK);
+
+	return ovl_real_fileattr(new, &newfa, true);
+}
+
 static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
 			    struct path *new, loff_t len)
 {
@@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
+	struct inode *inode = d_inode(c->dentry);
+	struct path upperpath, datapath;
 	int err;
 
+	ovl_path_upper(c->dentry, &upperpath);
+	if (WARN_ON(upperpath.dentry != NULL))
+		return -EIO;
+
+	upperpath.dentry = temp;
+
 	/*
 	 * Copy up data first and then xattrs. Writing data after
 	 * xattrs will remove security.capability xattr automatically.
 	 */
 	if (S_ISREG(c->stat.mode) && !c->metacopy) {
-		struct path upperpath, datapath;
-
-		ovl_path_upper(c->dentry, &upperpath);
-		if (WARN_ON(upperpath.dentry != NULL))
-			return -EIO;
-		upperpath.dentry = temp;
-
 		ovl_path_lowerdata(c->dentry, &datapath);
 		err = ovl_copy_up_data(ofs, &datapath, &upperpath,
 				       c->stat.size);
@@ -518,6 +545,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	if (err)
 		return err;
 
+	if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
+		/*
+		 * Copy the fileattr inode flags that are the source of already
+		 * copied i_flags (best effort).
+		 */
+		ovl_copy_fileattr(&c->lowerpath, &upperpath);
+	}
+
 	/*
 	 * Store identifier of lower inode in upper inode xattr to
 	 * allow lookup of the copy up origin inode.
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 5e828a1c98a8..aec353a2dc80 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -503,16 +503,14 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
  * Introducing security_inode_fileattr_get/set() hooks would solve this issue
  * properly.
  */
-static int ovl_security_fileattr(struct dentry *dentry, struct fileattr *fa,
+static int ovl_security_fileattr(struct path *realpath, struct fileattr *fa,
 				 bool set)
 {
-	struct path realpath;
 	struct file *file;
 	unsigned int cmd;
 	int err;
 
-	ovl_path_real(dentry, &realpath);
-	file = dentry_open(&realpath, O_RDONLY, current_cred());
+	file = dentry_open(realpath, O_RDONLY, current_cred());
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
@@ -527,11 +525,25 @@ static int ovl_security_fileattr(struct dentry *dentry, struct fileattr *fa,
 	return err;
 }
 
+int ovl_real_fileattr(struct path *realpath, struct fileattr *fa, bool set)
+{
+	int err;
+
+	err = ovl_security_fileattr(realpath, fa, set);
+	if (err)
+		return err;
+
+	if (set)
+		return vfs_fileattr_set(&init_user_ns, realpath->dentry, fa);
+	else
+		return vfs_fileattr_get(realpath->dentry, fa);
+}
+
 int ovl_fileattr_set(struct user_namespace *mnt_userns,
 		     struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
-	struct dentry *upperdentry;
+	struct path upperpath;
 	const struct cred *old_cred;
 	int err;
 
@@ -541,12 +553,10 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
 
 	err = ovl_copy_up(dentry);
 	if (!err) {
-		upperdentry = ovl_dentry_upper(dentry);
+		ovl_path_real(dentry, &upperpath);
 
 		old_cred = ovl_override_creds(inode->i_sb);
-		err = ovl_security_fileattr(dentry, fa, true);
-		if (!err)
-			err = vfs_fileattr_set(&init_user_ns, upperdentry, fa);
+		err = ovl_real_fileattr(&upperpath, fa, true);
 		revert_creds(old_cred);
 		ovl_copyflags(ovl_inode_real(inode), inode);
 	}
@@ -558,14 +568,14 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
 int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
-	struct dentry *realdentry = ovl_dentry_real(dentry);
+	struct path realpath;
 	const struct cred *old_cred;
 	int err;
 
+	ovl_path_real(dentry, &realpath);
+
 	old_cred = ovl_override_creds(inode->i_sb);
-	err = ovl_security_fileattr(dentry, fa, false);
-	if (!err)
-		err = vfs_fileattr_get(realdentry, fa);
+	err = ovl_real_fileattr(&realpath, fa, false);
 	revert_creds(old_cred);
 
 	return err;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6e3a1bea1c9a..1e964e4e45d4 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -530,9 +530,20 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 	i_size_write(to, i_size_read(from));
 }
 
+/* vfs inode flags copied from real to ovl inode */
+#define OVL_COPY_I_FLAGS_MASK	(S_SYNC | S_NOATIME | S_APPEND | S_IMMUTABLE)
+
+/*
+ * fileattr flags copied from lower to upper inode on copy up.
+ * We cannot copy immutable/append-only flags, because that would prevevnt
+ * linking temp inode to upper dir.
+ */
+#define OVL_COPY_FS_FLAGS_MASK	(FS_SYNC_FL | FS_NOATIME_FL)
+#define OVL_COPY_FSX_FLAGS_MASK	(FS_XFLAG_SYNC | FS_XFLAG_NOATIME)
+
 static inline void ovl_copyflags(struct inode *from, struct inode *to)
 {
-	unsigned int mask = S_SYNC | S_IMMUTABLE | S_APPEND | S_NOATIME;
+	unsigned int mask = OVL_COPY_I_FLAGS_MASK;
 
 	inode_set_flags(to, from->i_flags & mask, mask);
 }
@@ -560,6 +571,7 @@ struct dentry *ovl_create_temp(struct dentry *workdir, struct ovl_cattr *attr);
 extern const struct file_operations ovl_file_operations;
 int __init ovl_aio_request_cache_init(void);
 void ovl_aio_request_cache_destroy(void);
+int ovl_real_fileattr(struct path *realpath, struct fileattr *fa, bool set);
 int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa);
 int ovl_fileattr_set(struct user_namespace *mnt_userns,
 		     struct dentry *dentry, struct fileattr *fa);
-- 
2.32.0

[PATCH v3 4/4] ovl: consistent behavior for immutable/append-only inodes

[Amir Goldstein]

When a lower file has immutable/append-only fileattr flags, the behavior
of overlayfs post copy up is inconsistent.

Immediattely after copy up, ovl inode still has the S_IMMUTABLE/S_APPEND
inode flags copied from lower inode, so vfs code still treats the ovl
inode as immutable/append-only.  After ovl inode evict or mount cycle,
the ovl inode does not have these inode flags anymore.

We cannot copy up the immutable and append-only fileattr flags, because
immutable/append-only inodes cannot be linked and because overlayfs will
not be able to set overlay.* xattr on the upper inodes.

Instead, if any of the fileattr flags of interest exist on the lower
inode, we store them in overlay.protected xattr on the upper inode and we
we read the flags from xattr on lookup and on fileattr_get().

This gives consistent behavior post copy up regardless of inode eviction
from cache.

When user sets new fileattr flags, we update or remove the
overlay.protected xattr.

Storing immutable/append-only fileattr flags in an xattr instead of upper
fileattr also solves other non-standard behavior issues - overlayfs can
now copy up children of "ovl-immutable" directories and lower aliases of
"ovl-immutable" hardlinks.

Reported-by: Chengguang Xu <cgxu519@mykernel.net>
Link: https://lore.kernel.org/linux-unionfs/20201226104618.239739-1-cgxu519@mykernel.net/
Link: https://lore.kernel.org/linux-unionfs/20210210190334.1212210-5-amir73il@gmail.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  17 +++++-
 fs/overlayfs/inode.c     |  42 +++++++++++++-
 fs/overlayfs/overlayfs.h |  28 +++++++++-
 fs/overlayfs/util.c      | 115 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 195 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index a06b423ca5d1..fc9ffcf32d0c 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -131,7 +131,8 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
 	return error;
 }
 
-static int ovl_copy_fileattr(struct path *old, struct path *new)
+static int ovl_copy_fileattr(struct inode *inode, struct path *old,
+			     struct path *new)
 {
 	struct fileattr oldfa = { .flags_valid = true };
 	struct fileattr newfa = { .flags_valid = true };
@@ -145,6 +146,18 @@ static int ovl_copy_fileattr(struct path *old, struct path *new)
 	if (err)
 		return err;
 
+	/*
+	 * We cannot set immutable and append-only flags on upper inode,
+	 * because we would not be able to link upper inode to upper dir
+	 * not set overlay private xattr on upper inode.
+	 * Store these flags in overlay.protected xattr instead.
+	 */
+	if (oldfa.flags & OVL_PROT_FS_FLAGS_MASK) {
+		err = ovl_set_protected(inode, new->dentry, &oldfa);
+		if (err)
+			return err;
+	}
+
 	BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
 	newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
 	newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
@@ -550,7 +563,7 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 		 * Copy the fileattr inode flags that are the source of already
 		 * copied i_flags (best effort).
 		 */
-		ovl_copy_fileattr(&c->lowerpath, &upperpath);
+		ovl_copy_fileattr(inode, &c->lowerpath, &upperpath);
 	}
 
 	/*
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index aec353a2dc80..9c621ed17a61 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -162,7 +162,8 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
 	enum ovl_path_type type;
 	struct path realpath;
 	const struct cred *old_cred;
-	bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
+	struct inode *inode = d_inode(dentry);
+	bool is_dir = S_ISDIR(inode->i_mode);
 	int fsid = 0;
 	int err;
 	bool metacopy_blocks = false;
@@ -175,6 +176,10 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
 	if (err)
 		goto out;
 
+	/* Report the effective immutable/append-only STATX flags */
+	if (ovl_test_flag(OVL_PROTECTED, inode))
+		generic_fill_statx_attr(inode, stat);
+
 	/*
 	 * For non-dir or same fs, we use st_ino of the copy up origin.
 	 * This guaranties constant st_dev/st_ino across copy up.
@@ -556,15 +561,40 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
 		ovl_path_real(dentry, &upperpath);
 
 		old_cred = ovl_override_creds(inode->i_sb);
-		err = ovl_real_fileattr(&upperpath, fa, true);
+		/*
+		 * Store immutable/append-only flags in xattr and clear them
+		 * in upper fileattr (in case they were set by older kernel)
+		 * so children of "ovl-immutable" directories lower aliases of
+		 * "ovl-immutable" hardlinks could be copied up.
+		 * Clear xattr when flags are cleared.
+		 */
+		err = ovl_set_protected(inode, upperpath.dentry, fa);
+		if (!err)
+			err = ovl_real_fileattr(&upperpath, fa, true);
 		revert_creds(old_cred);
-		ovl_copyflags(ovl_inode_real(inode), inode);
+		ovl_merge_prot_flags(ovl_inode_real(inode), inode);
 	}
 	ovl_drop_write(dentry);
 out:
 	return err;
 }
 
+/* Convert inode protection flags to fileattr flags */
+static void ovl_fileattr_prot_flags(struct inode *inode, struct fileattr *fa)
+{
+	BUILD_BUG_ON(OVL_PROT_FS_FLAGS_MASK & ~FS_COMMON_FL);
+	BUILD_BUG_ON(OVL_PROT_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
+
+	if (inode->i_flags & S_APPEND) {
+		fa->flags |= FS_APPEND_FL;
+		fa->fsx_xflags |= FS_XFLAG_APPEND;
+	}
+	if (inode->i_flags & S_IMMUTABLE) {
+		fa->flags |= FS_IMMUTABLE_FL;
+		fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
+	}
+}
+
 int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
@@ -576,6 +606,8 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	err = ovl_real_fileattr(&realpath, fa, false);
+	if (!err && ovl_test_flag(OVL_PROTECTED, inode))
+		ovl_fileattr_prot_flags(inode, fa);
 	revert_creds(old_cred);
 
 	return err;
@@ -1128,6 +1160,10 @@ struct inode *ovl_get_inode(struct super_block *sb,
 		}
 	}
 
+	/* Check for immutable/append-only inode flags in xattr */
+	if (upperdentry)
+		ovl_check_protected(inode, upperdentry);
+
 	if (inode->i_state & I_NEW)
 		unlock_new_inode(inode);
 out:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 1e964e4e45d4..840b6e1a71ea 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -34,6 +34,7 @@ enum ovl_xattr {
 	OVL_XATTR_NLINK,
 	OVL_XATTR_UPPER,
 	OVL_XATTR_METACOPY,
+	OVL_XATTR_PROTECTED,
 };
 
 enum ovl_inode_flag {
@@ -45,6 +46,8 @@ enum ovl_inode_flag {
 	OVL_UPPERDATA,
 	/* Inode number will remain constant over copy up. */
 	OVL_CONST_INO,
+	/* Has overlay.protected xattr */
+	OVL_PROTECTED,
 };
 
 enum ovl_entry_flag {
@@ -532,14 +535,22 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 
 /* vfs inode flags copied from real to ovl inode */
 #define OVL_COPY_I_FLAGS_MASK	(S_SYNC | S_NOATIME | S_APPEND | S_IMMUTABLE)
+/* vfs inode flags read from overlay.protected xattr to ovl inode */
+#define OVL_PROT_I_FLAGS_MASK	(S_APPEND | S_IMMUTABLE)
 
 /*
  * fileattr flags copied from lower to upper inode on copy up.
- * We cannot copy immutable/append-only flags, because that would prevevnt
- * linking temp inode to upper dir.
+ * We cannot copy up immutable/append-only flags, because that would prevevnt
+ * linking temp inode to upper dir, so we store them in xattr instead.
  */
 #define OVL_COPY_FS_FLAGS_MASK	(FS_SYNC_FL | FS_NOATIME_FL)
 #define OVL_COPY_FSX_FLAGS_MASK	(FS_XFLAG_SYNC | FS_XFLAG_NOATIME)
+#define OVL_PROT_FS_FLAGS_MASK  (FS_APPEND_FL | FS_IMMUTABLE_FL)
+#define OVL_PROT_FSX_FLAGS_MASK (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE)
+
+bool ovl_check_protected(struct inode *inode, struct dentry *upper);
+int ovl_set_protected(struct inode *inode, struct dentry *upper,
+		      struct fileattr *fa);
 
 static inline void ovl_copyflags(struct inode *from, struct inode *to)
 {
@@ -548,6 +559,19 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
 	inode_set_flags(to, from->i_flags & mask, mask);
 }
 
+/* Merge real inode flags with inode flags read from overlay.protected xattr */
+static inline void ovl_merge_prot_flags(struct inode *real, struct inode *inode)
+{
+	unsigned int flags = real->i_flags & OVL_COPY_I_FLAGS_MASK;
+
+	BUILD_BUG_ON(OVL_PROT_I_FLAGS_MASK & ~OVL_COPY_I_FLAGS_MASK);
+
+	if (ovl_test_flag(OVL_PROTECTED, inode))
+		flags |= inode->i_flags & OVL_PROT_I_FLAGS_MASK;
+
+	inode_set_flags(inode, flags, OVL_COPY_I_FLAGS_MASK);
+}
+
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
 int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 81b8f135445a..202377ca2ee9 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -10,6 +10,7 @@
 #include <linux/cred.h>
 #include <linux/xattr.h>
 #include <linux/exportfs.h>
+#include <linux/fileattr.h>
 #include <linux/uuid.h>
 #include <linux/namei.h>
 #include <linux/ratelimit.h>
@@ -585,6 +586,7 @@ bool ovl_check_dir_xattr(struct super_block *sb, struct dentry *dentry,
 #define OVL_XATTR_NLINK_POSTFIX		"nlink"
 #define OVL_XATTR_UPPER_POSTFIX		"upper"
 #define OVL_XATTR_METACOPY_POSTFIX	"metacopy"
+#define OVL_XATTR_PROTECTED_POSTFIX	"protected"
 
 #define OVL_XATTR_TAB_ENTRY(x) \
 	[x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \
@@ -598,6 +600,7 @@ const char *const ovl_xattr_table[][2] = {
 	OVL_XATTR_TAB_ENTRY(OVL_XATTR_NLINK),
 	OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER),
 	OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY),
+	OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTECTED),
 };
 
 int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
@@ -639,6 +642,118 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
 	return err;
 }
 
+
+/*
+ * Initialize inode flags from overlay.protected xattr and upper inode flags.
+ * If upper inode has those fileattr flags set (i.e. from old kernel), we do not
+ * clear them on ovl_get_inode(), but we will clear them on next fileattr_set().
+ */
+static bool ovl_prot_flags_from_buf(struct inode *inode, const char *buf,
+				    int len)
+{
+	u32 iflags = inode->i_flags & OVL_PROT_I_FLAGS_MASK;
+	int n;
+
+	/*
+	 * We cannot clear flags that are set on real inode.
+	 * We can only set flags that are not set in inode.
+	 */
+	for (n = 0; n < len && buf[n]; n++) {
+		if (buf[n] == 'a')
+			iflags |= S_APPEND;
+		else if (buf[n] == 'i')
+			iflags |= S_IMMUTABLE;
+		else
+			break;
+	}
+
+	inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);
+
+	return buf[n] == 0;
+}
+
+#define OVL_PROTECTED_MAX 32 /* Reserved for future flags */
+
+bool ovl_check_protected(struct inode *inode, struct dentry *upper)
+{
+	struct ovl_fs *ofs = OVL_FS(inode->i_sb);
+	char buf[OVL_PROTECTED_MAX+1];
+	int res;
+
+	res = ovl_do_getxattr(ofs, upper, OVL_XATTR_PROTECTED, buf,
+			      OVL_PROTECTED_MAX);
+	if (res < 0)
+		return false;
+
+	buf[res] = 0;
+	if (res == 0 || !ovl_prot_flags_from_buf(inode, buf, res)) {
+		pr_warn_ratelimited("incompatible overlay.protected format (%pd2, len=%d)\n",
+				    upper, res);
+	}
+
+	ovl_set_flag(OVL_PROTECTED, inode);
+	ovl_merge_prot_flags(d_inode(upper), inode);
+
+	return true;
+}
+
+/* Set inode flags and overlay.protected xattr from fileattr */
+static int ovl_prot_flags_to_buf(struct inode *inode, char *buf,
+				 const struct fileattr *fa)
+{
+	u32 iflags = 0;
+	int n = 0;
+
+	if (fa->flags & FS_APPEND_FL) {
+		buf[n++] = 'a';
+		iflags |= S_APPEND;
+	}
+	if (fa->flags & FS_IMMUTABLE_FL) {
+		buf[n++] = 'i';
+		iflags |= S_IMMUTABLE;
+	}
+
+	inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);
+
+	return n;
+}
+
+int ovl_set_protected(struct inode *inode, struct dentry *upper,
+		      struct fileattr *fa)
+{
+	struct ovl_fs *ofs = OVL_FS(inode->i_sb);
+	char buf[OVL_PROTECTED_MAX];
+	int len, err = 0;
+
+	BUILD_BUG_ON(HWEIGHT32(OVL_PROT_FS_FLAGS_MASK) > OVL_PROTECTED_MAX);
+	len = ovl_prot_flags_to_buf(inode, buf, fa);
+
+	/*
+	 * Do not allow to set protection flags when upper doesn't support
+	 * xattrs, because we do not set those fileattr flags on upper inode.
+	 * Remove xattr if it exist and all protection flags are cleared.
+	 */
+	if (len) {
+		err = ovl_check_setxattr(ofs, upper, OVL_XATTR_PROTECTED,
+					 buf, len, -EPERM);
+	} else if (ovl_test_flag(OVL_PROTECTED, inode)) {
+		err = ovl_do_removexattr(ofs, upper, OVL_XATTR_PROTECTED);
+	}
+	if (err)
+		return err;
+
+	/* Mask out the fileattr flags that should not be set in upper inode */
+	fa->flags &= ~OVL_PROT_FS_FLAGS_MASK;
+	fa->fsx_xflags &= ~OVL_PROT_FSX_FLAGS_MASK;
+
+	if (len)
+		ovl_set_flag(OVL_PROTECTED, inode);
+	else
+		ovl_clear_flag(OVL_PROTECTED, inode);
+
+	return 0;
+}
+
 /**
  * Caller must hold a reference to inode to prevent it from being freed while
  * it is marked inuse.
-- 
2.32.0

Re: [PATCH v3 1/4] fs: add generic helper for filling statx attribute flags

[Amir Goldstein]

Forgot to CC linux-fsdevel and vfs/orangefs maintainers

On Sat, Jun 19, 2021 at 12:26 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> The immutable and append-only properties on an inode are published on
> the inode's i_flags and enforced by the VFS.
>
> Create a helper to fill the corresponding STATX_ATTR_ flags in the kstat
> structure from the inode's i_flags.
>
> Only orange was converted to use this helper.
> Other filesystems could use it in the future.
>
> Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/orangefs/inode.c  |  7 +------
>  fs/stat.c            | 18 ++++++++++++++++++
>  include/linux/fs.h   |  1 +
>  include/linux/stat.h |  4 ++++
>  4 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> index 6bf35a0d61f3..4092009716a3 100644
> --- a/fs/orangefs/inode.c
> +++ b/fs/orangefs/inode.c
> @@ -883,12 +883,7 @@ int orangefs_getattr(struct user_namespace *mnt_userns, const struct path *path,
>                 if (!(request_mask & STATX_SIZE))
>                         stat->result_mask &= ~STATX_SIZE;
>
> -               stat->attributes_mask = STATX_ATTR_IMMUTABLE |
> -                   STATX_ATTR_APPEND;
> -               if (inode->i_flags & S_IMMUTABLE)
> -                       stat->attributes |= STATX_ATTR_IMMUTABLE;
> -               if (inode->i_flags & S_APPEND)
> -                       stat->attributes |= STATX_ATTR_APPEND;
> +               generic_fill_statx_attr(inode, stat);
>         }
>         return ret;
>  }
> diff --git a/fs/stat.c b/fs/stat.c
> index 1fa38bdec1a6..314269150b5b 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -59,6 +59,24 @@ void generic_fillattr(struct user_namespace *mnt_userns, struct inode *inode,
>  }
>  EXPORT_SYMBOL(generic_fillattr);
>
> +/**
> + * generic_fill_statx_attr - Fill in the statx attributes from the inode flags
> + * @inode:     Inode to use as the source
> + * @stat:      Where to fill in the attribute flags
> + *
> + * Fill in the STATX_ATTR_ flags in the kstat structure for properties of the
> + * inode that are published on i_flags and enforced by the VFS.
> + */
> +void generic_fill_statx_attr(struct inode *inode, struct kstat *stat)
> +{
> +       if (inode->i_flags & S_IMMUTABLE)
> +               stat->attributes |= STATX_ATTR_IMMUTABLE;
> +       if (inode->i_flags & S_APPEND)
> +               stat->attributes |= STATX_ATTR_APPEND;
> +       stat->attributes_mask |= KSTAT_ATTR_VFS_FLAGS;
> +}
> +EXPORT_SYMBOL(generic_fill_statx_attr);
> +
>  /**
>   * vfs_getattr_nosec - getattr without security checks
>   * @path: file to get attributes from
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c3c88fdb9b2a..647664316013 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3353,6 +3353,7 @@ extern int page_symlink(struct inode *inode, const char *symname, int len);
>  extern const struct inode_operations page_symlink_inode_operations;
>  extern void kfree_link(void *);
>  void generic_fillattr(struct user_namespace *, struct inode *, struct kstat *);
> +void generic_fill_statx_attr(struct inode *inode, struct kstat *stat);
>  extern int vfs_getattr_nosec(const struct path *, struct kstat *, u32, unsigned int);
>  extern int vfs_getattr(const struct path *, struct kstat *, u32, unsigned int);
>  void __inode_add_bytes(struct inode *inode, loff_t bytes);
> diff --git a/include/linux/stat.h b/include/linux/stat.h
> index fff27e603814..7df06931f25d 100644
> --- a/include/linux/stat.h
> +++ b/include/linux/stat.h
> @@ -34,6 +34,10 @@ struct kstat {
>          STATX_ATTR_ENCRYPTED |                         \
>          STATX_ATTR_VERITY                              \
>          )/* Attrs corresponding to FS_*_FL flags */
> +#define KSTAT_ATTR_VFS_FLAGS                           \
> +       (STATX_ATTR_IMMUTABLE |                         \
> +        STATX_ATTR_APPEND                              \
> +        ) /* Attrs corresponding to S_* flags that are enforced by the VFS */
>         u64             ino;
>         dev_t           dev;
>         dev_t           rdev;
> --
> 2.32.0
>

Re: [PATCH v3 4/4] ovl: consistent behavior for immutable/append-only inodes

[Miklos Szeredi]

On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
>
> When a lower file has immutable/append-only fileattr flags, the behavior
> of overlayfs post copy up is inconsistent.
>
> Immediattely after copy up, ovl inode still has the S_IMMUTABLE/S_APPEND
> inode flags copied from lower inode, so vfs code still treats the ovl
> inode as immutable/append-only.  After ovl inode evict or mount cycle,
> the ovl inode does not have these inode flags anymore.
>
> We cannot copy up the immutable and append-only fileattr flags, because
> immutable/append-only inodes cannot be linked and because overlayfs will
> not be able to set overlay.* xattr on the upper inodes.
>
> Instead, if any of the fileattr flags of interest exist on the lower
> inode, we store them in overlay.protected xattr on the upper inode and we
> we read the flags from xattr on lookup and on fileattr_get().
>
> This gives consistent behavior post copy up regardless of inode eviction
> from cache.
>
> When user sets new fileattr flags, we update or remove the
> overlay.protected xattr.
>
> Storing immutable/append-only fileattr flags in an xattr instead of upper
> fileattr also solves other non-standard behavior issues - overlayfs can
> now copy up children of "ovl-immutable" directories and lower aliases of
> "ovl-immutable" hardlinks.
>
> Reported-by: Chengguang Xu <cgxu519@mykernel.net>
> Link: https://lore.kernel.org/linux-unionfs/20201226104618.239739-1-cgxu519@mykernel.net/
> Link: https://lore.kernel.org/linux-unionfs/20210210190334.1212210-5-amir73il@gmail.com/
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Some notes and questions below.  I can take care of fixing these up.

> ---
>  fs/overlayfs/copy_up.c   |  17 +++++-
>  fs/overlayfs/inode.c     |  42 +++++++++++++-
>  fs/overlayfs/overlayfs.h |  28 +++++++++-
>  fs/overlayfs/util.c      | 115 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 195 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index a06b423ca5d1..fc9ffcf32d0c 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -131,7 +131,8 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
>         return error;
>  }
>
> -static int ovl_copy_fileattr(struct path *old, struct path *new)
> +static int ovl_copy_fileattr(struct inode *inode, struct path *old,
> +                            struct path *new)
>  {
>         struct fileattr oldfa = { .flags_valid = true };
>         struct fileattr newfa = { .flags_valid = true };
> @@ -145,6 +146,18 @@ static int ovl_copy_fileattr(struct path *old, struct path *new)
>         if (err)
>                 return err;
>
> +       /*
> +        * We cannot set immutable and append-only flags on upper inode,
> +        * because we would not be able to link upper inode to upper dir
> +        * not set overlay private xattr on upper inode.
> +        * Store these flags in overlay.protected xattr instead.
> +        */
> +       if (oldfa.flags & OVL_PROT_FS_FLAGS_MASK) {
> +               err = ovl_set_protected(inode, new->dentry, &oldfa);
> +               if (err)
> +                       return err;
> +       }
> +
>         BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
>         newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
>         newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> @@ -550,7 +563,7 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>                  * Copy the fileattr inode flags that are the source of already
>                  * copied i_flags (best effort).
>                  */
> -               ovl_copy_fileattr(&c->lowerpath, &upperpath);
> +               ovl_copy_fileattr(inode, &c->lowerpath, &upperpath);
>         }
>
>         /*
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index aec353a2dc80..9c621ed17a61 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -162,7 +162,8 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
>         enum ovl_path_type type;
>         struct path realpath;
>         const struct cred *old_cred;
> -       bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
> +       struct inode *inode = d_inode(dentry);
> +       bool is_dir = S_ISDIR(inode->i_mode);
>         int fsid = 0;
>         int err;
>         bool metacopy_blocks = false;
> @@ -175,6 +176,10 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
>         if (err)
>                 goto out;
>
> +       /* Report the effective immutable/append-only STATX flags */
> +       if (ovl_test_flag(OVL_PROTECTED, inode))
> +               generic_fill_statx_attr(inode, stat);

Assuming i_flags is correct this doesn't need to be conditional on
OVL_PROTECTED, right?


> +
>         /*
>          * For non-dir or same fs, we use st_ino of the copy up origin.
>          * This guaranties constant st_dev/st_ino across copy up.
> @@ -556,15 +561,40 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
>                 ovl_path_real(dentry, &upperpath);
>
>                 old_cred = ovl_override_creds(inode->i_sb);
> -               err = ovl_real_fileattr(&upperpath, fa, true);
> +               /*
> +                * Store immutable/append-only flags in xattr and clear them
> +                * in upper fileattr (in case they were set by older kernel)
> +                * so children of "ovl-immutable" directories lower aliases of
> +                * "ovl-immutable" hardlinks could be copied up.
> +                * Clear xattr when flags are cleared.
> +                */
> +               err = ovl_set_protected(inode, upperpath.dentry, fa);
> +               if (!err)
> +                       err = ovl_real_fileattr(&upperpath, fa, true);
>                 revert_creds(old_cred);
> -               ovl_copyflags(ovl_inode_real(inode), inode);
> +               ovl_merge_prot_flags(ovl_inode_real(inode), inode);
>         }
>         ovl_drop_write(dentry);
>  out:
>         return err;
>  }
>
> +/* Convert inode protection flags to fileattr flags */
> +static void ovl_fileattr_prot_flags(struct inode *inode, struct fileattr *fa)
> +{
> +       BUILD_BUG_ON(OVL_PROT_FS_FLAGS_MASK & ~FS_COMMON_FL);
> +       BUILD_BUG_ON(OVL_PROT_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> +
> +       if (inode->i_flags & S_APPEND) {
> +               fa->flags |= FS_APPEND_FL;
> +               fa->fsx_xflags |= FS_XFLAG_APPEND;
> +       }
> +       if (inode->i_flags & S_IMMUTABLE) {
> +               fa->flags |= FS_IMMUTABLE_FL;
> +               fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
> +       }
> +}
> +
>  int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
>  {
>         struct inode *inode = d_inode(dentry);
> @@ -576,6 +606,8 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
>
>         old_cred = ovl_override_creds(inode->i_sb);
>         err = ovl_real_fileattr(&realpath, fa, false);
> +       if (!err && ovl_test_flag(OVL_PROTECTED, inode))
> +               ovl_fileattr_prot_flags(inode, fa);

Again, I don't see a reason making this conditional on OVL_PROTECTED.

>         revert_creds(old_cred);
>
>         return err;
> @@ -1128,6 +1160,10 @@ struct inode *ovl_get_inode(struct super_block *sb,
>                 }
>         }
>
> +       /* Check for immutable/append-only inode flags in xattr */
> +       if (upperdentry)
> +               ovl_check_protected(inode, upperdentry);
> +
>         if (inode->i_state & I_NEW)
>                 unlock_new_inode(inode);
>  out:
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 1e964e4e45d4..840b6e1a71ea 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -34,6 +34,7 @@ enum ovl_xattr {
>         OVL_XATTR_NLINK,
>         OVL_XATTR_UPPER,
>         OVL_XATTR_METACOPY,
> +       OVL_XATTR_PROTECTED,
>  };
>
>  enum ovl_inode_flag {
> @@ -45,6 +46,8 @@ enum ovl_inode_flag {
>         OVL_UPPERDATA,
>         /* Inode number will remain constant over copy up. */
>         OVL_CONST_INO,
> +       /* Has overlay.protected xattr */
> +       OVL_PROTECTED,
>  };
>
>  enum ovl_entry_flag {
> @@ -532,14 +535,22 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
>
>  /* vfs inode flags copied from real to ovl inode */
>  #define OVL_COPY_I_FLAGS_MASK  (S_SYNC | S_NOATIME | S_APPEND | S_IMMUTABLE)
> +/* vfs inode flags read from overlay.protected xattr to ovl inode */
> +#define OVL_PROT_I_FLAGS_MASK  (S_APPEND | S_IMMUTABLE)
>
>  /*
>   * fileattr flags copied from lower to upper inode on copy up.
> - * We cannot copy immutable/append-only flags, because that would prevevnt
> - * linking temp inode to upper dir.
> + * We cannot copy up immutable/append-only flags, because that would prevevnt
> + * linking temp inode to upper dir, so we store them in xattr instead.
>   */
>  #define OVL_COPY_FS_FLAGS_MASK (FS_SYNC_FL | FS_NOATIME_FL)
>  #define OVL_COPY_FSX_FLAGS_MASK        (FS_XFLAG_SYNC | FS_XFLAG_NOATIME)
> +#define OVL_PROT_FS_FLAGS_MASK  (FS_APPEND_FL | FS_IMMUTABLE_FL)
> +#define OVL_PROT_FSX_FLAGS_MASK (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE)
> +
> +bool ovl_check_protected(struct inode *inode, struct dentry *upper);
> +int ovl_set_protected(struct inode *inode, struct dentry *upper,
> +                     struct fileattr *fa);
>
>  static inline void ovl_copyflags(struct inode *from, struct inode *to)
>  {
> @@ -548,6 +559,19 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
>         inode_set_flags(to, from->i_flags & mask, mask);
>  }
>
> +/* Merge real inode flags with inode flags read from overlay.protected xattr */
> +static inline void ovl_merge_prot_flags(struct inode *real, struct inode *inode)
> +{
> +       unsigned int flags = real->i_flags & OVL_COPY_I_FLAGS_MASK;
> +
> +       BUILD_BUG_ON(OVL_PROT_I_FLAGS_MASK & ~OVL_COPY_I_FLAGS_MASK);
> +
> +       if (ovl_test_flag(OVL_PROTECTED, inode))
> +               flags |= inode->i_flags & OVL_PROT_I_FLAGS_MASK;

And here also.

> +
> +       inode_set_flags(inode, flags, OVL_COPY_I_FLAGS_MASK);
> +}
> +
>  /* dir.c */
>  extern const struct inode_operations ovl_dir_inode_operations;
>  int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 81b8f135445a..202377ca2ee9 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -10,6 +10,7 @@
>  #include <linux/cred.h>
>  #include <linux/xattr.h>
>  #include <linux/exportfs.h>
> +#include <linux/fileattr.h>
>  #include <linux/uuid.h>
>  #include <linux/namei.h>
>  #include <linux/ratelimit.h>
> @@ -585,6 +586,7 @@ bool ovl_check_dir_xattr(struct super_block *sb, struct dentry *dentry,
>  #define OVL_XATTR_NLINK_POSTFIX                "nlink"
>  #define OVL_XATTR_UPPER_POSTFIX                "upper"
>  #define OVL_XATTR_METACOPY_POSTFIX     "metacopy"
> +#define OVL_XATTR_PROTECTED_POSTFIX    "protected"
>
>  #define OVL_XATTR_TAB_ENTRY(x) \
>         [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \
> @@ -598,6 +600,7 @@ const char *const ovl_xattr_table[][2] = {
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_NLINK),
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER),
>         OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY),
> +       OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTECTED),
>  };
>
>  int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
> @@ -639,6 +642,118 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
>         return err;
>  }
>
> +
> +/*
> + * Initialize inode flags from overlay.protected xattr and upper inode flags.
> + * If upper inode has those fileattr flags set (i.e. from old kernel), we do not
> + * clear them on ovl_get_inode(), but we will clear them on next fileattr_set().
> + */
> +static bool ovl_prot_flags_from_buf(struct inode *inode, const char *buf,
> +                                   int len)
> +{
> +       u32 iflags = inode->i_flags & OVL_PROT_I_FLAGS_MASK;
> +       int n;
> +
> +       /*
> +        * We cannot clear flags that are set on real inode.
> +        * We can only set flags that are not set in inode.
> +        */
> +       for (n = 0; n < len && buf[n]; n++) {
> +               if (buf[n] == 'a')
> +                       iflags |= S_APPEND;
> +               else if (buf[n] == 'i')
> +                       iflags |= S_IMMUTABLE;
> +               else
> +                       break;
> +       }
> +
> +       inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);
> +
> +       return buf[n] == 0;
> +}
> +
> +#define OVL_PROTECTED_MAX 32 /* Reserved for future flags */
> +
> +bool ovl_check_protected(struct inode *inode, struct dentry *upper)
> +{
> +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> +       char buf[OVL_PROTECTED_MAX+1];
> +       int res;
> +
> +       res = ovl_do_getxattr(ofs, upper, OVL_XATTR_PROTECTED, buf,
> +                             OVL_PROTECTED_MAX);
> +       if (res < 0)
> +               return false;
> +
> +       buf[res] = 0;
> +       if (res == 0 || !ovl_prot_flags_from_buf(inode, buf, res)) {
> +               pr_warn_ratelimited("incompatible overlay.protected format (%pd2, len=%d)\n",
> +                                   upper, res);
> +       }
> +
> +       ovl_set_flag(OVL_PROTECTED, inode);
> +       ovl_merge_prot_flags(d_inode(upper), inode);

ovl_prot_flags_from_buf() should have updated i_flags, so this looks
like a no-op.  Or am I missing something?

> +
> +       return true;
> +}
> +
> +/* Set inode flags and overlay.protected xattr from fileattr */
> +static int ovl_prot_flags_to_buf(struct inode *inode, char *buf,
> +                                const struct fileattr *fa)
> +{
> +       u32 iflags = 0;
> +       int n = 0;
> +
> +       if (fa->flags & FS_APPEND_FL) {
> +               buf[n++] = 'a';
> +               iflags |= S_APPEND;
> +       }
> +       if (fa->flags & FS_IMMUTABLE_FL) {
> +               buf[n++] = 'i';
> +               iflags |= S_IMMUTABLE;
> +       }
> +
> +       inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);

Looks like the wrong place to update i_flags, since the setxattr may yet fail.

> +
> +       return n;
> +}
> +
> +int ovl_set_protected(struct inode *inode, struct dentry *upper,
> +                     struct fileattr *fa)
> +{
> +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> +       char buf[OVL_PROTECTED_MAX];
> +       int len, err = 0;
> +
> +       BUILD_BUG_ON(HWEIGHT32(OVL_PROT_FS_FLAGS_MASK) > OVL_PROTECTED_MAX);
> +       len = ovl_prot_flags_to_buf(inode, buf, fa);
> +
> +       /*
> +        * Do not allow to set protection flags when upper doesn't support
> +        * xattrs, because we do not set those fileattr flags on upper inode.
> +        * Remove xattr if it exist and all protection flags are cleared.
> +        */
> +       if (len) {
> +               err = ovl_check_setxattr(ofs, upper, OVL_XATTR_PROTECTED,
> +                                        buf, len, -EPERM);
> +       } else if (ovl_test_flag(OVL_PROTECTED, inode)) {

Last place OVL_PROTECTED is tested.   What about just translating
ENODATA/EOPNOTSUPP to success?

That way OVL_PROTECTED could be dropped altogether.

> +               err = ovl_do_removexattr(ofs, upper, OVL_XATTR_PROTECTED);
> +       }
> +       if (err)
> +               return err;
> +
> +       /* Mask out the fileattr flags that should not be set in upper inode */
> +       fa->flags &= ~OVL_PROT_FS_FLAGS_MASK;
> +       fa->fsx_xflags &= ~OVL_PROT_FSX_FLAGS_MASK;
> +
> +       if (len)
> +               ovl_set_flag(OVL_PROTECTED, inode);
> +       else
> +               ovl_clear_flag(OVL_PROTECTED, inode);
> +
> +       return 0;
> +}
> +
>  /**
>   * Caller must hold a reference to inode to prevent it from being freed while
>   * it is marked inuse.
> --
> 2.32.0
>

Re: [PATCH v3 4/4] ovl: consistent behavior for immutable/append-only inodes

[Amir Goldstein]

On Mon, Jul 12, 2021 at 3:21 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > When a lower file has immutable/append-only fileattr flags, the behavior
> > of overlayfs post copy up is inconsistent.
> >
> > Immediattely after copy up, ovl inode still has the S_IMMUTABLE/S_APPEND
> > inode flags copied from lower inode, so vfs code still treats the ovl
> > inode as immutable/append-only.  After ovl inode evict or mount cycle,
> > the ovl inode does not have these inode flags anymore.
> >
> > We cannot copy up the immutable and append-only fileattr flags, because
> > immutable/append-only inodes cannot be linked and because overlayfs will
> > not be able to set overlay.* xattr on the upper inodes.
> >
> > Instead, if any of the fileattr flags of interest exist on the lower
> > inode, we store them in overlay.protected xattr on the upper inode and we
> > we read the flags from xattr on lookup and on fileattr_get().
> >
> > This gives consistent behavior post copy up regardless of inode eviction
> > from cache.
> >
> > When user sets new fileattr flags, we update or remove the
> > overlay.protected xattr.
> >
> > Storing immutable/append-only fileattr flags in an xattr instead of upper
> > fileattr also solves other non-standard behavior issues - overlayfs can
> > now copy up children of "ovl-immutable" directories and lower aliases of
> > "ovl-immutable" hardlinks.
> >
> > Reported-by: Chengguang Xu <cgxu519@mykernel.net>
> > Link: https://lore.kernel.org/linux-unionfs/20201226104618.239739-1-cgxu519@mykernel.net/
> > Link: https://lore.kernel.org/linux-unionfs/20210210190334.1212210-5-amir73il@gmail.com/
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Some notes and questions below.  I can take care of fixing these up.
>
> > ---
> >  fs/overlayfs/copy_up.c   |  17 +++++-
> >  fs/overlayfs/inode.c     |  42 +++++++++++++-
> >  fs/overlayfs/overlayfs.h |  28 +++++++++-
> >  fs/overlayfs/util.c      | 115 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 195 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index a06b423ca5d1..fc9ffcf32d0c 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -131,7 +131,8 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
> >         return error;
> >  }
> >
> > -static int ovl_copy_fileattr(struct path *old, struct path *new)
> > +static int ovl_copy_fileattr(struct inode *inode, struct path *old,
> > +                            struct path *new)
> >  {
> >         struct fileattr oldfa = { .flags_valid = true };
> >         struct fileattr newfa = { .flags_valid = true };
> > @@ -145,6 +146,18 @@ static int ovl_copy_fileattr(struct path *old, struct path *new)
> >         if (err)
> >                 return err;
> >
> > +       /*
> > +        * We cannot set immutable and append-only flags on upper inode,
> > +        * because we would not be able to link upper inode to upper dir
> > +        * not set overlay private xattr on upper inode.
> > +        * Store these flags in overlay.protected xattr instead.
> > +        */
> > +       if (oldfa.flags & OVL_PROT_FS_FLAGS_MASK) {
> > +               err = ovl_set_protected(inode, new->dentry, &oldfa);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> >         BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
> >         newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
> >         newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> > @@ -550,7 +563,7 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >                  * Copy the fileattr inode flags that are the source of already
> >                  * copied i_flags (best effort).
> >                  */
> > -               ovl_copy_fileattr(&c->lowerpath, &upperpath);
> > +               ovl_copy_fileattr(inode, &c->lowerpath, &upperpath);
> >         }
> >
> >         /*
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index aec353a2dc80..9c621ed17a61 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -162,7 +162,8 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
> >         enum ovl_path_type type;
> >         struct path realpath;
> >         const struct cred *old_cred;
> > -       bool is_dir = S_ISDIR(dentry->d_inode->i_mode);
> > +       struct inode *inode = d_inode(dentry);
> > +       bool is_dir = S_ISDIR(inode->i_mode);
> >         int fsid = 0;
> >         int err;
> >         bool metacopy_blocks = false;
> > @@ -175,6 +176,10 @@ int ovl_getattr(struct user_namespace *mnt_userns, const struct path *path,
> >         if (err)
> >                 goto out;
> >
> > +       /* Report the effective immutable/append-only STATX flags */
> > +       if (ovl_test_flag(OVL_PROTECTED, inode))
> > +               generic_fill_statx_attr(inode, stat);
>
> Assuming i_flags is correct this doesn't need to be conditional on
> OVL_PROTECTED, right?
>

Right.


>
> > +
> >         /*
> >          * For non-dir or same fs, we use st_ino of the copy up origin.
> >          * This guaranties constant st_dev/st_ino across copy up.
> > @@ -556,15 +561,40 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
> >                 ovl_path_real(dentry, &upperpath);
> >
> >                 old_cred = ovl_override_creds(inode->i_sb);
> > -               err = ovl_real_fileattr(&upperpath, fa, true);
> > +               /*
> > +                * Store immutable/append-only flags in xattr and clear them
> > +                * in upper fileattr (in case they were set by older kernel)
> > +                * so children of "ovl-immutable" directories lower aliases of
> > +                * "ovl-immutable" hardlinks could be copied up.
> > +                * Clear xattr when flags are cleared.
> > +                */
> > +               err = ovl_set_protected(inode, upperpath.dentry, fa);
> > +               if (!err)
> > +                       err = ovl_real_fileattr(&upperpath, fa, true);
> >                 revert_creds(old_cred);
> > -               ovl_copyflags(ovl_inode_real(inode), inode);
> > +               ovl_merge_prot_flags(ovl_inode_real(inode), inode);
> >         }
> >         ovl_drop_write(dentry);
> >  out:
> >         return err;
> >  }
> >
> > +/* Convert inode protection flags to fileattr flags */
> > +static void ovl_fileattr_prot_flags(struct inode *inode, struct fileattr *fa)
> > +{
> > +       BUILD_BUG_ON(OVL_PROT_FS_FLAGS_MASK & ~FS_COMMON_FL);
> > +       BUILD_BUG_ON(OVL_PROT_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> > +
> > +       if (inode->i_flags & S_APPEND) {
> > +               fa->flags |= FS_APPEND_FL;
> > +               fa->fsx_xflags |= FS_XFLAG_APPEND;
> > +       }
> > +       if (inode->i_flags & S_IMMUTABLE) {
> > +               fa->flags |= FS_IMMUTABLE_FL;
> > +               fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
> > +       }
> > +}
> > +
> >  int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> >  {
> >         struct inode *inode = d_inode(dentry);
> > @@ -576,6 +606,8 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> >
> >         old_cred = ovl_override_creds(inode->i_sb);
> >         err = ovl_real_fileattr(&realpath, fa, false);
> > +       if (!err && ovl_test_flag(OVL_PROTECTED, inode))
> > +               ovl_fileattr_prot_flags(inode, fa);
>
> Again, I don't see a reason making this conditional on OVL_PROTECTED.

Right, I guess I was being over defensive about possible regressions
or it's a leftover from the more generic OVL_XFLAGS version.

>
> >         revert_creds(old_cred);
> >
> >         return err;
> > @@ -1128,6 +1160,10 @@ struct inode *ovl_get_inode(struct super_block *sb,
> >                 }
> >         }
> >
> > +       /* Check for immutable/append-only inode flags in xattr */
> > +       if (upperdentry)
> > +               ovl_check_protected(inode, upperdentry);
> > +
> >         if (inode->i_state & I_NEW)
> >                 unlock_new_inode(inode);
> >  out:
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 1e964e4e45d4..840b6e1a71ea 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -34,6 +34,7 @@ enum ovl_xattr {
> >         OVL_XATTR_NLINK,
> >         OVL_XATTR_UPPER,
> >         OVL_XATTR_METACOPY,
> > +       OVL_XATTR_PROTECTED,
> >  };
> >
> >  enum ovl_inode_flag {
> > @@ -45,6 +46,8 @@ enum ovl_inode_flag {
> >         OVL_UPPERDATA,
> >         /* Inode number will remain constant over copy up. */
> >         OVL_CONST_INO,
> > +       /* Has overlay.protected xattr */
> > +       OVL_PROTECTED,
> >  };
> >
> >  enum ovl_entry_flag {
> > @@ -532,14 +535,22 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
> >
> >  /* vfs inode flags copied from real to ovl inode */
> >  #define OVL_COPY_I_FLAGS_MASK  (S_SYNC | S_NOATIME | S_APPEND | S_IMMUTABLE)
> > +/* vfs inode flags read from overlay.protected xattr to ovl inode */
> > +#define OVL_PROT_I_FLAGS_MASK  (S_APPEND | S_IMMUTABLE)
> >
> >  /*
> >   * fileattr flags copied from lower to upper inode on copy up.
> > - * We cannot copy immutable/append-only flags, because that would prevevnt
> > - * linking temp inode to upper dir.
> > + * We cannot copy up immutable/append-only flags, because that would prevevnt
> > + * linking temp inode to upper dir, so we store them in xattr instead.
> >   */
> >  #define OVL_COPY_FS_FLAGS_MASK (FS_SYNC_FL | FS_NOATIME_FL)
> >  #define OVL_COPY_FSX_FLAGS_MASK        (FS_XFLAG_SYNC | FS_XFLAG_NOATIME)
> > +#define OVL_PROT_FS_FLAGS_MASK  (FS_APPEND_FL | FS_IMMUTABLE_FL)
> > +#define OVL_PROT_FSX_FLAGS_MASK (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE)
> > +
> > +bool ovl_check_protected(struct inode *inode, struct dentry *upper);
> > +int ovl_set_protected(struct inode *inode, struct dentry *upper,
> > +                     struct fileattr *fa);
> >
> >  static inline void ovl_copyflags(struct inode *from, struct inode *to)
> >  {
> > @@ -548,6 +559,19 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
> >         inode_set_flags(to, from->i_flags & mask, mask);
> >  }
> >
> > +/* Merge real inode flags with inode flags read from overlay.protected xattr */
> > +static inline void ovl_merge_prot_flags(struct inode *real, struct inode *inode)
> > +{
> > +       unsigned int flags = real->i_flags & OVL_COPY_I_FLAGS_MASK;
> > +
> > +       BUILD_BUG_ON(OVL_PROT_I_FLAGS_MASK & ~OVL_COPY_I_FLAGS_MASK);
> > +
> > +       if (ovl_test_flag(OVL_PROTECTED, inode))
> > +               flags |= inode->i_flags & OVL_PROT_I_FLAGS_MASK;
>
> And here also.

True.

>
> > +
> > +       inode_set_flags(inode, flags, OVL_COPY_I_FLAGS_MASK);
> > +}
> > +
> >  /* dir.c */
> >  extern const struct inode_operations ovl_dir_inode_operations;
> >  int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 81b8f135445a..202377ca2ee9 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/cred.h>
> >  #include <linux/xattr.h>
> >  #include <linux/exportfs.h>
> > +#include <linux/fileattr.h>
> >  #include <linux/uuid.h>
> >  #include <linux/namei.h>
> >  #include <linux/ratelimit.h>
> > @@ -585,6 +586,7 @@ bool ovl_check_dir_xattr(struct super_block *sb, struct dentry *dentry,
> >  #define OVL_XATTR_NLINK_POSTFIX                "nlink"
> >  #define OVL_XATTR_UPPER_POSTFIX                "upper"
> >  #define OVL_XATTR_METACOPY_POSTFIX     "metacopy"
> > +#define OVL_XATTR_PROTECTED_POSTFIX    "protected"
> >
> >  #define OVL_XATTR_TAB_ENTRY(x) \
> >         [x] = { [false] = OVL_XATTR_TRUSTED_PREFIX x ## _POSTFIX, \
> > @@ -598,6 +600,7 @@ const char *const ovl_xattr_table[][2] = {
> >         OVL_XATTR_TAB_ENTRY(OVL_XATTR_NLINK),
> >         OVL_XATTR_TAB_ENTRY(OVL_XATTR_UPPER),
> >         OVL_XATTR_TAB_ENTRY(OVL_XATTR_METACOPY),
> > +       OVL_XATTR_TAB_ENTRY(OVL_XATTR_PROTECTED),
> >  };
> >
> >  int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
> > @@ -639,6 +642,118 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
> >         return err;
> >  }
> >
> > +
> > +/*
> > + * Initialize inode flags from overlay.protected xattr and upper inode flags.
> > + * If upper inode has those fileattr flags set (i.e. from old kernel), we do not
> > + * clear them on ovl_get_inode(), but we will clear them on next fileattr_set().
> > + */
> > +static bool ovl_prot_flags_from_buf(struct inode *inode, const char *buf,
> > +                                   int len)
> > +{
> > +       u32 iflags = inode->i_flags & OVL_PROT_I_FLAGS_MASK;
> > +       int n;
> > +
> > +       /*
> > +        * We cannot clear flags that are set on real inode.
> > +        * We can only set flags that are not set in inode.
> > +        */
> > +       for (n = 0; n < len && buf[n]; n++) {
> > +               if (buf[n] == 'a')
> > +                       iflags |= S_APPEND;
> > +               else if (buf[n] == 'i')
> > +                       iflags |= S_IMMUTABLE;
> > +               else
> > +                       break;
> > +       }
> > +
> > +       inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);
> > +
> > +       return buf[n] == 0;
> > +}
> > +
> > +#define OVL_PROTECTED_MAX 32 /* Reserved for future flags */
> > +
> > +bool ovl_check_protected(struct inode *inode, struct dentry *upper)
> > +{
> > +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> > +       char buf[OVL_PROTECTED_MAX+1];
> > +       int res;
> > +
> > +       res = ovl_do_getxattr(ofs, upper, OVL_XATTR_PROTECTED, buf,
> > +                             OVL_PROTECTED_MAX);
> > +       if (res < 0)
> > +               return false;
> > +
> > +       buf[res] = 0;
> > +       if (res == 0 || !ovl_prot_flags_from_buf(inode, buf, res)) {
> > +               pr_warn_ratelimited("incompatible overlay.protected format (%pd2, len=%d)\n",
> > +                                   upper, res);
> > +       }
> > +
> > +       ovl_set_flag(OVL_PROTECTED, inode);
> > +       ovl_merge_prot_flags(d_inode(upper), inode);
>
> ovl_prot_flags_from_buf() should have updated i_flags, so this looks
> like a no-op.  Or am I missing something?

I suppose you are right.
I guess I implemented the flag merge logic in ovl_prot_flags_from_buf()
later and forgot to remove this.

>
> > +
> > +       return true;
> > +}
> > +
> > +/* Set inode flags and overlay.protected xattr from fileattr */
> > +static int ovl_prot_flags_to_buf(struct inode *inode, char *buf,
> > +                                const struct fileattr *fa)
> > +{
> > +       u32 iflags = 0;
> > +       int n = 0;
> > +
> > +       if (fa->flags & FS_APPEND_FL) {
> > +               buf[n++] = 'a';
> > +               iflags |= S_APPEND;
> > +       }
> > +       if (fa->flags & FS_IMMUTABLE_FL) {
> > +               buf[n++] = 'i';
> > +               iflags |= S_IMMUTABLE;
> > +       }
> > +
> > +       inode_set_flags(inode, iflags, OVL_PROT_I_FLAGS_MASK);
>
> Looks like the wrong place to update i_flags, since the setxattr may yet fail.
>

Right.

> > +
> > +       return n;
> > +}
> > +
> > +int ovl_set_protected(struct inode *inode, struct dentry *upper,
> > +                     struct fileattr *fa)
> > +{
> > +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> > +       char buf[OVL_PROTECTED_MAX];
> > +       int len, err = 0;
> > +
> > +       BUILD_BUG_ON(HWEIGHT32(OVL_PROT_FS_FLAGS_MASK) > OVL_PROTECTED_MAX);
> > +       len = ovl_prot_flags_to_buf(inode, buf, fa);
> > +
> > +       /*
> > +        * Do not allow to set protection flags when upper doesn't support
> > +        * xattrs, because we do not set those fileattr flags on upper inode.
> > +        * Remove xattr if it exist and all protection flags are cleared.
> > +        */
> > +       if (len) {
> > +               err = ovl_check_setxattr(ofs, upper, OVL_XATTR_PROTECTED,
> > +                                        buf, len, -EPERM);

> > +       } else if (ovl_test_flag(OVL_PROTECTED, inode)) {
>
> Last place OVL_PROTECTED is tested.   What about just translating
> ENODATA/EOPNOTSUPP to success?
>
> That way OVL_PROTECTED could be dropped altogether.
>

OK.

Please test with https://github.com/amir73il/xfstests/commits/ovl-xflags
tests overlay/075 and overlay/078.
I will post overlay/078 after the patches are in overlayfs-next.

Thanks,
Amir.

Re: [PATCH v3 3/4] ovl: copy up sync/noatime fileattr flags

[Miklos Szeredi]

On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
>
> When a lower file has sync/noatime fileattr flags, the behavior of
> overlayfs post copy up is inconsistent.
>
> Immediattely after copy up, ovl inode still has the S_SYNC/S_NOATIME
> inode flags copied from lower inode, so vfs code still treats the ovl
> inode as sync/noatime.  After ovl inode evict or mount cycle,
> the ovl inode does not have these inode flags anymore.
>
> To fix this inconsitency, try to copy the fileattr flags on copy up
> if the upper fs supports the fileattr_set() method.
>
> This gives consistent behavior post copy up regardless of inode eviction
> from cache.
>
> We cannot copy up the immutable/append-only inode flags in a similar
> manner, because immutable/append-only inodes cannot be linked and because
> overlayfs will not be able to set overlay.* xattr on the upper inodes.
>
> Those flags will be addressed by a followup patch.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++++++++++++++------
>  fs/overlayfs/inode.c     | 36 ++++++++++++++++++-----------
>  fs/overlayfs/overlayfs.h | 14 +++++++++++-
>  3 files changed, 78 insertions(+), 21 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 3fa68a5cc16e..a06b423ca5d1 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -8,6 +8,7 @@
>  #include <linux/fs.h>
>  #include <linux/slab.h>
>  #include <linux/file.h>
> +#include <linux/fileattr.h>
>  #include <linux/splice.h>
>  #include <linux/xattr.h>
>  #include <linux/security.h>
> @@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
>         return error;
>  }
>
> +static int ovl_copy_fileattr(struct path *old, struct path *new)
> +{
> +       struct fileattr oldfa = { .flags_valid = true };
> +       struct fileattr newfa = { .flags_valid = true };
> +       int err;
> +
> +       err = ovl_real_fileattr(old, &oldfa, false);
> +       if (err)
> +               return err;
> +
> +       err = ovl_real_fileattr(new, &newfa, false);
> +       if (err)
> +               return err;
> +
> +       BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
> +       newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
> +       newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> +
> +       BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> +       newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK;
> +       newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK);
> +
> +       return ovl_real_fileattr(new, &newfa, true);
> +}
> +
>  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
>                             struct path *new, loff_t len)
>  {
> @@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
>  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>  {
>         struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> +       struct inode *inode = d_inode(c->dentry);
> +       struct path upperpath, datapath;
>         int err;
>
> +       ovl_path_upper(c->dentry, &upperpath);
> +       if (WARN_ON(upperpath.dentry != NULL))
> +               return -EIO;
> +
> +       upperpath.dentry = temp;
> +
>         /*
>          * Copy up data first and then xattrs. Writing data after
>          * xattrs will remove security.capability xattr automatically.
>          */
>         if (S_ISREG(c->stat.mode) && !c->metacopy) {
> -               struct path upperpath, datapath;
> -
> -               ovl_path_upper(c->dentry, &upperpath);
> -               if (WARN_ON(upperpath.dentry != NULL))
> -                       return -EIO;
> -               upperpath.dentry = temp;
> -
>                 ovl_path_lowerdata(c->dentry, &datapath);
>                 err = ovl_copy_up_data(ofs, &datapath, &upperpath,
>                                        c->stat.size);
> @@ -518,6 +545,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>         if (err)
>                 return err;
>
> +       if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
> +               /*
> +                * Copy the fileattr inode flags that are the source of already
> +                * copied i_flags (best effort).
> +                */
> +               ovl_copy_fileattr(&c->lowerpath, &upperpath);

I'm not sure this should be ignoring errors.  Was this done to prevent
regressing cases where the upper fs cannot store the flags?  Do you
have a concrete example?

Thanks,
Miklos

Re: [PATCH v3 3/4] ovl: copy up sync/noatime fileattr flags

[Amir Goldstein]

On Mon, Jul 12, 2021 at 5:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > When a lower file has sync/noatime fileattr flags, the behavior of
> > overlayfs post copy up is inconsistent.
> >
> > Immediattely after copy up, ovl inode still has the S_SYNC/S_NOATIME
> > inode flags copied from lower inode, so vfs code still treats the ovl
> > inode as sync/noatime.  After ovl inode evict or mount cycle,
> > the ovl inode does not have these inode flags anymore.
> >
> > To fix this inconsitency, try to copy the fileattr flags on copy up
> > if the upper fs supports the fileattr_set() method.
> >
> > This gives consistent behavior post copy up regardless of inode eviction
> > from cache.
> >
> > We cannot copy up the immutable/append-only inode flags in a similar
> > manner, because immutable/append-only inodes cannot be linked and because
> > overlayfs will not be able to set overlay.* xattr on the upper inodes.
> >
> > Those flags will be addressed by a followup patch.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++++++++++++++------
> >  fs/overlayfs/inode.c     | 36 ++++++++++++++++++-----------
> >  fs/overlayfs/overlayfs.h | 14 +++++++++++-
> >  3 files changed, 78 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 3fa68a5cc16e..a06b423ca5d1 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/fs.h>
> >  #include <linux/slab.h>
> >  #include <linux/file.h>
> > +#include <linux/fileattr.h>
> >  #include <linux/splice.h>
> >  #include <linux/xattr.h>
> >  #include <linux/security.h>
> > @@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
> >         return error;
> >  }
> >
> > +static int ovl_copy_fileattr(struct path *old, struct path *new)
> > +{
> > +       struct fileattr oldfa = { .flags_valid = true };
> > +       struct fileattr newfa = { .flags_valid = true };
> > +       int err;
> > +
> > +       err = ovl_real_fileattr(old, &oldfa, false);
> > +       if (err)
> > +               return err;
> > +
> > +       err = ovl_real_fileattr(new, &newfa, false);
> > +       if (err)
> > +               return err;
> > +
> > +       BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
> > +       newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
> > +       newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> > +
> > +       BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> > +       newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK;
> > +       newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK);
> > +
> > +       return ovl_real_fileattr(new, &newfa, true);
> > +}
> > +
> >  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> >                             struct path *new, loff_t len)
> >  {
> > @@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
> >  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >  {
> >         struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> > +       struct inode *inode = d_inode(c->dentry);
> > +       struct path upperpath, datapath;
> >         int err;
> >
> > +       ovl_path_upper(c->dentry, &upperpath);
> > +       if (WARN_ON(upperpath.dentry != NULL))
> > +               return -EIO;
> > +
> > +       upperpath.dentry = temp;
> > +
> >         /*
> >          * Copy up data first and then xattrs. Writing data after
> >          * xattrs will remove security.capability xattr automatically.
> >          */
> >         if (S_ISREG(c->stat.mode) && !c->metacopy) {
> > -               struct path upperpath, datapath;
> > -
> > -               ovl_path_upper(c->dentry, &upperpath);
> > -               if (WARN_ON(upperpath.dentry != NULL))
> > -                       return -EIO;
> > -               upperpath.dentry = temp;
> > -
> >                 ovl_path_lowerdata(c->dentry, &datapath);
> >                 err = ovl_copy_up_data(ofs, &datapath, &upperpath,
> >                                        c->stat.size);
> > @@ -518,6 +545,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> >         if (err)
> >                 return err;
> >
> > +       if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
> > +               /*
> > +                * Copy the fileattr inode flags that are the source of already
> > +                * copied i_flags (best effort).
> > +                */
> > +               ovl_copy_fileattr(&c->lowerpath, &upperpath);
>
> I'm not sure this should be ignoring errors.  Was this done to prevent
> regressing cases where the upper fs cannot store the flags?

Yes.

> Do you have a concrete example?

Unpriv userns mount??

Thanks,
Amir.

Re: [PATCH v3 3/4] ovl: copy up sync/noatime fileattr flags

[Amir Goldstein]

On Mon, Jul 12, 2021 at 6:51 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 5:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > When a lower file has sync/noatime fileattr flags, the behavior of
> > > overlayfs post copy up is inconsistent.
> > >
> > > Immediattely after copy up, ovl inode still has the S_SYNC/S_NOATIME
> > > inode flags copied from lower inode, so vfs code still treats the ovl
> > > inode as sync/noatime.  After ovl inode evict or mount cycle,
> > > the ovl inode does not have these inode flags anymore.
> > >
> > > To fix this inconsitency, try to copy the fileattr flags on copy up
> > > if the upper fs supports the fileattr_set() method.
> > >
> > > This gives consistent behavior post copy up regardless of inode eviction
> > > from cache.
> > >
> > > We cannot copy up the immutable/append-only inode flags in a similar
> > > manner, because immutable/append-only inodes cannot be linked and because
> > > overlayfs will not be able to set overlay.* xattr on the upper inodes.
> > >
> > > Those flags will be addressed by a followup patch.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++++++++++++++------
> > >  fs/overlayfs/inode.c     | 36 ++++++++++++++++++-----------
> > >  fs/overlayfs/overlayfs.h | 14 +++++++++++-
> > >  3 files changed, 78 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > index 3fa68a5cc16e..a06b423ca5d1 100644
> > > --- a/fs/overlayfs/copy_up.c
> > > +++ b/fs/overlayfs/copy_up.c
> > > @@ -8,6 +8,7 @@
> > >  #include <linux/fs.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/file.h>
> > > +#include <linux/fileattr.h>
> > >  #include <linux/splice.h>
> > >  #include <linux/xattr.h>
> > >  #include <linux/security.h>
> > > @@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
> > >         return error;
> > >  }
> > >
> > > +static int ovl_copy_fileattr(struct path *old, struct path *new)
> > > +{
> > > +       struct fileattr oldfa = { .flags_valid = true };
> > > +       struct fileattr newfa = { .flags_valid = true };
> > > +       int err;
> > > +
> > > +       err = ovl_real_fileattr(old, &oldfa, false);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       err = ovl_real_fileattr(new, &newfa, false);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
> > > +       newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
> > > +       newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> > > +
> > > +       BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> > > +       newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK;
> > > +       newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK);
> > > +
> > > +       return ovl_real_fileattr(new, &newfa, true);
> > > +}
> > > +
> > >  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> > >                             struct path *new, loff_t len)
> > >  {
> > > @@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
> > >  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > >  {
> > >         struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> > > +       struct inode *inode = d_inode(c->dentry);
> > > +       struct path upperpath, datapath;
> > >         int err;
> > >
> > > +       ovl_path_upper(c->dentry, &upperpath);
> > > +       if (WARN_ON(upperpath.dentry != NULL))
> > > +               return -EIO;
> > > +
> > > +       upperpath.dentry = temp;
> > > +
> > >         /*
> > >          * Copy up data first and then xattrs. Writing data after
> > >          * xattrs will remove security.capability xattr automatically.
> > >          */
> > >         if (S_ISREG(c->stat.mode) && !c->metacopy) {
> > > -               struct path upperpath, datapath;
> > > -
> > > -               ovl_path_upper(c->dentry, &upperpath);
> > > -               if (WARN_ON(upperpath.dentry != NULL))
> > > -                       return -EIO;
> > > -               upperpath.dentry = temp;
> > > -
> > >                 ovl_path_lowerdata(c->dentry, &datapath);
> > >                 err = ovl_copy_up_data(ofs, &datapath, &upperpath,
> > >                                        c->stat.size);
> > > @@ -518,6 +545,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > >         if (err)
> > >                 return err;
> > >
> > > +       if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
> > > +               /*
> > > +                * Copy the fileattr inode flags that are the source of already
> > > +                * copied i_flags (best effort).
> > > +                */
> > > +               ovl_copy_fileattr(&c->lowerpath, &upperpath);
> >
> > I'm not sure this should be ignoring errors.  Was this done to prevent
> > regressing cases where the upper fs cannot store the flags?
>
> Yes.
>
> > Do you have a concrete example?
>
> Unpriv userns mount??
>

Upperfs that does not support fileattr (FUSE?)

Thanks,
Amir.

Re: [PATCH v3 3/4] ovl: copy up sync/noatime fileattr flags

[Miklos Szeredi]

On Mon, 12 Jul 2021 at 17:52, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 6:51 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 5:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > When a lower file has sync/noatime fileattr flags, the behavior of
> > > > overlayfs post copy up is inconsistent.
> > > >
> > > > Immediattely after copy up, ovl inode still has the S_SYNC/S_NOATIME
> > > > inode flags copied from lower inode, so vfs code still treats the ovl
> > > > inode as sync/noatime.  After ovl inode evict or mount cycle,
> > > > the ovl inode does not have these inode flags anymore.
> > > >
> > > > To fix this inconsitency, try to copy the fileattr flags on copy up
> > > > if the upper fs supports the fileattr_set() method.
> > > >
> > > > This gives consistent behavior post copy up regardless of inode eviction
> > > > from cache.
> > > >
> > > > We cannot copy up the immutable/append-only inode flags in a similar
> > > > manner, because immutable/append-only inodes cannot be linked and because
> > > > overlayfs will not be able to set overlay.* xattr on the upper inodes.
> > > >
> > > > Those flags will be addressed by a followup patch.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++++++++++++++------
> > > >  fs/overlayfs/inode.c     | 36 ++++++++++++++++++-----------
> > > >  fs/overlayfs/overlayfs.h | 14 +++++++++++-
> > > >  3 files changed, 78 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > index 3fa68a5cc16e..a06b423ca5d1 100644
> > > > --- a/fs/overlayfs/copy_up.c
> > > > +++ b/fs/overlayfs/copy_up.c
> > > > @@ -8,6 +8,7 @@
> > > >  #include <linux/fs.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/file.h>
> > > > +#include <linux/fileattr.h>
> > > >  #include <linux/splice.h>
> > > >  #include <linux/xattr.h>
> > > >  #include <linux/security.h>
> > > > @@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
> > > >         return error;
> > > >  }
> > > >
> > > > +static int ovl_copy_fileattr(struct path *old, struct path *new)
> > > > +{
> > > > +       struct fileattr oldfa = { .flags_valid = true };
> > > > +       struct fileattr newfa = { .flags_valid = true };
> > > > +       int err;
> > > > +
> > > > +       err = ovl_real_fileattr(old, &oldfa, false);
> > > > +       if (err)
> > > > +               return err;
> > > > +
> > > > +       err = ovl_real_fileattr(new, &newfa, false);
> > > > +       if (err)
> > > > +               return err;
> > > > +
> > > > +       BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
> > > > +       newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
> > > > +       newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> > > > +
> > > > +       BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> > > > +       newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK;
> > > > +       newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK);
> > > > +
> > > > +       return ovl_real_fileattr(new, &newfa, true);
> > > > +}
> > > > +
> > > >  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> > > >                             struct path *new, loff_t len)
> > > >  {
> > > > @@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
> > > >  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > > >  {
> > > >         struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> > > > +       struct inode *inode = d_inode(c->dentry);
> > > > +       struct path upperpath, datapath;
> > > >         int err;
> > > >
> > > > +       ovl_path_upper(c->dentry, &upperpath);
> > > > +       if (WARN_ON(upperpath.dentry != NULL))
> > > > +               return -EIO;
> > > > +
> > > > +       upperpath.dentry = temp;
> > > > +
> > > >         /*
> > > >          * Copy up data first and then xattrs. Writing data after
> > > >          * xattrs will remove security.capability xattr automatically.
> > > >          */
> > > >         if (S_ISREG(c->stat.mode) && !c->metacopy) {
> > > > -               struct path upperpath, datapath;
> > > > -
> > > > -               ovl_path_upper(c->dentry, &upperpath);
> > > > -               if (WARN_ON(upperpath.dentry != NULL))
> > > > -                       return -EIO;
> > > > -               upperpath.dentry = temp;
> > > > -
> > > >                 ovl_path_lowerdata(c->dentry, &datapath);
> > > >                 err = ovl_copy_up_data(ofs, &datapath, &upperpath,
> > > >                                        c->stat.size);
> > > > @@ -518,6 +545,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > > >         if (err)
> > > >                 return err;
> > > >
> > > > +       if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
> > > > +               /*
> > > > +                * Copy the fileattr inode flags that are the source of already
> > > > +                * copied i_flags (best effort).
> > > > +                */
> > > > +               ovl_copy_fileattr(&c->lowerpath, &upperpath);
> > >
> > > I'm not sure this should be ignoring errors.  Was this done to prevent
> > > regressing cases where the upper fs cannot store the flags?
> >
> > Yes.
> >
> > > Do you have a concrete example?
> >
> > Unpriv userns mount??
> >
>
> Upperfs that does not support fileattr (FUSE?)

Okay, so two subcases:

  - no xattr on upper
  - no fileattr on upper

FUSE is considered "remote" and overlayfs enforces xattr support, but
not fileattr support.  So yeah, it seems theoretically these are
possible.

But I think it might be the best to risk it and return the error,
hoping that this is such a rare corner case that there's no existing
use.  If a regression is reported, than we need to go for a more
complex solution where this case is detected on startup and handled
accordingly.

Thanks,
Miklos

Re: [PATCH v3 3/4] ovl: copy up sync/noatime fileattr flags

[Amir Goldstein]

On Wed, Jul 14, 2021 at 11:47 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 12 Jul 2021 at 17:52, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 6:51 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 5:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > When a lower file has sync/noatime fileattr flags, the behavior of
> > > > > overlayfs post copy up is inconsistent.
> > > > >
> > > > > Immediattely after copy up, ovl inode still has the S_SYNC/S_NOATIME
> > > > > inode flags copied from lower inode, so vfs code still treats the ovl
> > > > > inode as sync/noatime.  After ovl inode evict or mount cycle,
> > > > > the ovl inode does not have these inode flags anymore.
> > > > >
> > > > > To fix this inconsitency, try to copy the fileattr flags on copy up
> > > > > if the upper fs supports the fileattr_set() method.
> > > > >
> > > > > This gives consistent behavior post copy up regardless of inode eviction
> > > > > from cache.
> > > > >
> > > > > We cannot copy up the immutable/append-only inode flags in a similar
> > > > > manner, because immutable/append-only inodes cannot be linked and because
> > > > > overlayfs will not be able to set overlay.* xattr on the upper inodes.
> > > > >
> > > > > Those flags will be addressed by a followup patch.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > >  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++++++++++++++------
> > > > >  fs/overlayfs/inode.c     | 36 ++++++++++++++++++-----------
> > > > >  fs/overlayfs/overlayfs.h | 14 +++++++++++-
> > > > >  3 files changed, 78 insertions(+), 21 deletions(-)
> > > > >
> > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > > index 3fa68a5cc16e..a06b423ca5d1 100644
> > > > > --- a/fs/overlayfs/copy_up.c
> > > > > +++ b/fs/overlayfs/copy_up.c
> > > > > @@ -8,6 +8,7 @@
> > > > >  #include <linux/fs.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/file.h>
> > > > > +#include <linux/fileattr.h>
> > > > >  #include <linux/splice.h>
> > > > >  #include <linux/xattr.h>
> > > > >  #include <linux/security.h>
> > > > > @@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
> > > > >         return error;
> > > > >  }
> > > > >
> > > > > +static int ovl_copy_fileattr(struct path *old, struct path *new)
> > > > > +{
> > > > > +       struct fileattr oldfa = { .flags_valid = true };
> > > > > +       struct fileattr newfa = { .flags_valid = true };
> > > > > +       int err;
> > > > > +
> > > > > +       err = ovl_real_fileattr(old, &oldfa, false);
> > > > > +       if (err)
> > > > > +               return err;
> > > > > +
> > > > > +       err = ovl_real_fileattr(new, &newfa, false);
> > > > > +       if (err)
> > > > > +               return err;
> > > > > +
> > > > > +       BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
> > > > > +       newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
> > > > > +       newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
> > > > > +
> > > > > +       BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
> > > > > +       newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK;
> > > > > +       newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK);
> > > > > +
> > > > > +       return ovl_real_fileattr(new, &newfa, true);
> > > > > +}
> > > > > +
> > > > >  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> > > > >                             struct path *new, loff_t len)
> > > > >  {
> > > > > @@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
> > > > >  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > > > >  {
> > > > >         struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> > > > > +       struct inode *inode = d_inode(c->dentry);
> > > > > +       struct path upperpath, datapath;
> > > > >         int err;
> > > > >
> > > > > +       ovl_path_upper(c->dentry, &upperpath);
> > > > > +       if (WARN_ON(upperpath.dentry != NULL))
> > > > > +               return -EIO;
> > > > > +
> > > > > +       upperpath.dentry = temp;
> > > > > +
> > > > >         /*
> > > > >          * Copy up data first and then xattrs. Writing data after
> > > > >          * xattrs will remove security.capability xattr automatically.
> > > > >          */
> > > > >         if (S_ISREG(c->stat.mode) && !c->metacopy) {
> > > > > -               struct path upperpath, datapath;
> > > > > -
> > > > > -               ovl_path_upper(c->dentry, &upperpath);
> > > > > -               if (WARN_ON(upperpath.dentry != NULL))
> > > > > -                       return -EIO;
> > > > > -               upperpath.dentry = temp;
> > > > > -
> > > > >                 ovl_path_lowerdata(c->dentry, &datapath);
> > > > >                 err = ovl_copy_up_data(ofs, &datapath, &upperpath,
> > > > >                                        c->stat.size);
> > > > > @@ -518,6 +545,14 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
> > > > >         if (err)
> > > > >                 return err;
> > > > >
> > > > > +       if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
> > > > > +               /*
> > > > > +                * Copy the fileattr inode flags that are the source of already
> > > > > +                * copied i_flags (best effort).
> > > > > +                */
> > > > > +               ovl_copy_fileattr(&c->lowerpath, &upperpath);
> > > >
> > > > I'm not sure this should be ignoring errors.  Was this done to prevent
> > > > regressing cases where the upper fs cannot store the flags?
> > >
> > > Yes.
> > >
> > > > Do you have a concrete example?
> > >
> > > Unpriv userns mount??
> > >
> >
> > Upperfs that does not support fileattr (FUSE?)
>
> Okay, so two subcases:
>
>   - no xattr on upper
>   - no fileattr on upper
>
> FUSE is considered "remote" and overlayfs enforces xattr support, but
> not fileattr support.  So yeah, it seems theoretically these are
> possible.
>
> But I think it might be the best to risk it and return the error,
> hoping that this is such a rare corner case that there's no existing
> use.  If a regression is reported, than we need to go for a more
> complex solution where this case is detected on startup and handled
> accordingly.
>

Ok.

Thanks,
Amir.

Re: [PATCH v3 0/4] Overlayfs fileattr related fixes

[Miklos Szeredi]

On Sat, 19 Jun 2021 at 11:26, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Miklos,
>
> Following patch set addresses your comments to v2.
> It passed all the old and new xfstests [3].

Applied with modifications and pushed to vfs.git#overlayfs-next.

Thanks,
Miklos