linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] Overlayfs fileattr related fixes
@ 2021-06-17 15:22 Amir Goldstein
  2021-06-17 15:22 ` [PATCH v2 1/3] ovl: pass ovl_fs to ovl_check_setxattr() Amir Goldstein
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Amir Goldstein @ 2021-06-17 15:22 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, linux-unionfs

Miklos,

With these patches, copy up preserves the vfs aware subset of fileattr
flags, so xfstest overlay/075 passes.

I've tested also some generic chattr xfstests and written a new test
to verify the logic of overlay.xflags xattr [2].

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://github.com/amir73il/xfstests/commits/ovl-xflags

Amir Goldstein (3):
  ovl: pass ovl_fs to ovl_check_setxattr()
  ovl: copy up sync/noatime fileattr flags
  ovl: consistent behavior for immutable/append-only inodes

 fs/overlayfs/copy_up.c   |  72 +++++++++++++---
 fs/overlayfs/dir.c       |   6 +-
 fs/overlayfs/inode.c     |  58 ++++++++++---
 fs/overlayfs/namei.c     |   2 +-
 fs/overlayfs/overlayfs.h |  44 +++++++++-
 fs/overlayfs/util.c      | 178 ++++++++++++++++++++++++++++++++++++++-
 6 files changed, 323 insertions(+), 37 deletions(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/3] ovl: pass ovl_fs to ovl_check_setxattr()
  2021-06-17 15:22 [PATCH v2 0/3] Overlayfs fileattr related fixes Amir Goldstein
@ 2021-06-17 15:22 ` Amir Goldstein
  2021-06-17 15:22 ` [PATCH v2 2/3] ovl: copy up sync/noatime fileattr flags Amir Goldstein
  2021-06-17 15:22 ` [PATCH v2 3/3] ovl: consistent behavior for immutable/append-only inodes Amir Goldstein
  2 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2021-06-17 15:22 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, linux-unionfs

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] ovl: copy up sync/noatime fileattr flags
  2021-06-17 15:22 [PATCH v2 0/3] Overlayfs fileattr related fixes Amir Goldstein
  2021-06-17 15:22 ` [PATCH v2 1/3] ovl: pass ovl_fs to ovl_check_setxattr() Amir Goldstein
@ 2021-06-17 15:22 ` Amir Goldstein
  2021-06-17 15:22 ` [PATCH v2 3/3] ovl: consistent behavior for immutable/append-only inodes Amir Goldstein
  2 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2021-06-17 15:22 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, linux-unionfs

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..e1d91cdf4c3f 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_xflags(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_xflags(&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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] ovl: consistent behavior for immutable/append-only inodes
  2021-06-17 15:22 [PATCH v2 0/3] Overlayfs fileattr related fixes Amir Goldstein
  2021-06-17 15:22 ` [PATCH v2 1/3] ovl: pass ovl_fs to ovl_check_setxattr() Amir Goldstein
  2021-06-17 15:22 ` [PATCH v2 2/3] ovl: copy up sync/noatime fileattr flags Amir Goldstein
@ 2021-06-17 15:22 ` Amir Goldstein
  2021-06-18 13:57   ` Miklos Szeredi
  2 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2021-06-17 15:22 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, linux-unionfs

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.xflags 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.xflags
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     |  26 +++++-
 fs/overlayfs/overlayfs.h |  28 ++++++-
 fs/overlayfs/util.c      | 171 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 235 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index e1d91cdf4c3f..6cc8b5de4121 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_xflags(struct path *old, struct path *new)
+static int ovl_copy_xflags(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_xflags(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 xflags xattr instead.
+	 */
+	if (oldfa.flags & OVL_XFLAGS_FS_FLAGS_MASK) {
+		err = ovl_set_xflags(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_xflags(&c->lowerpath, &upperpath);
+		ovl_copy_xflags(inode, &c->lowerpath, &upperpath);
 	}
 
 	/*
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index aec353a2dc80..d66e51b9c347 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 immutable/append-only STATX flags */
+	if (ovl_test_flag(OVL_XFLAGS, inode))
+		ovl_fill_xflags(inode, stat, NULL);
+
 	/*
 	 * 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,9 +561,18 @@ 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 xflags xattr when flags are cleared.
+		 */
+		err = ovl_set_xflags(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_xflags(ovl_inode_real(inode), inode);
 	}
 	ovl_drop_write(dentry);
 out:
@@ -576,6 +590,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_XFLAGS, inode))
+		ovl_fill_xflags(inode, NULL, fa);
 	revert_creds(old_cred);
 
 	return err;
@@ -1128,6 +1144,10 @@ struct inode *ovl_get_inode(struct super_block *sb,
 		}
 	}
 
+	/* Check if need to merge inode flags from xattr */
+	if (upperdentry)
+		ovl_check_xflags(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..ba3cb1bed3b8 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_XFLAGS,
 };
 
 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 xflags xattr */
+	OVL_XFLAGS,
 };
 
 enum ovl_entry_flag {
@@ -532,14 +535,24 @@ 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 xflags xattr to ovl inode */
+#define OVL_XFLAGS_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_XFLAGS_FS_FLAGS_MASK  (FS_APPEND_FL | FS_IMMUTABLE_FL)
+#define OVL_XFLAGS_FSX_FLAGS_MASK (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE)
+
+bool ovl_check_xflags(struct inode *inode, struct dentry *upper);
+int ovl_set_xflags(struct inode *inode, struct dentry *upper,
+		   struct fileattr *fa);
+void ovl_fill_xflags(struct inode *inode, struct kstat *stat,
+		     struct fileattr *fa);
 
 static inline void ovl_copyflags(struct inode *from, struct inode *to)
 {
@@ -548,6 +561,17 @@ 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 xflags xattr */
+static inline void ovl_merge_xflags(struct inode *real, struct inode *inode)
+{
+	unsigned int flags = real->i_flags & OVL_COPY_I_FLAGS_MASK;
+
+	if (ovl_test_flag(OVL_XFLAGS, inode))
+		flags |= inode->i_flags & OVL_XFLAGS_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..34c1d08f00a0 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_XFLAGS_POSTFIX	"xflags"
 
 #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_XFLAGS),
 };
 
 int ovl_check_setxattr(struct ovl_fs *ofs, struct dentry *upperdentry,
@@ -639,6 +642,174 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
 	return err;
 }
 
+
+/*
+ * Overlayfs stores immutable/append-only attributes in overlay.xflags xattr.
+ * If upper inode does have those fileattr flags set (i.e. from old kernel),
+ * overlayfs does not clear them on fileattr_get(), but it will clear them on
+ * fileattr_set().
+ */
+#define OVL_XFLAG(c, x) \
+	{ c, S_ ## x, FS_ ## x ## _FL, FS_XFLAG_ ## x, STATX_ATTR_ ## x }
+
+struct ovl_xflag {
+	char code;
+	u32 i_flag;
+	u32 fs_flag;
+	u32 fsx_flag;
+	u64 statx_attr;
+} const ovl_xflags[] = {
+	OVL_XFLAG('a', APPEND),
+	OVL_XFLAG('i', IMMUTABLE),
+};
+
+#define OVL_XFLAGS_NUM ARRAY_SIZE(ovl_xflags)
+#define OVL_XFLAGS_MAX 32 /* Reserved for future xflags */
+
+static const struct ovl_xflag *ovl_xflag(char code)
+{
+	const struct ovl_xflag *xflag = ovl_xflags;
+	int i;
+
+	for (i = 0; i < OVL_XFLAGS_NUM; i++, xflag++) {
+		if (xflag->code == code)
+			return xflag;
+	}
+
+	return NULL;
+}
+
+static int ovl_xflags_to_buf(struct inode *inode, char *buf, int len,
+			     const struct fileattr *fa)
+{
+	const struct ovl_xflag *xflag = ovl_xflags;
+	u32 iflags = 0;
+	int i, n = 0;
+
+	for (i = 0; i < OVL_XFLAGS_NUM; i++, xflag++) {
+		if (fa->flags & xflag->fs_flag) {
+			buf[n++] = xflag->code;
+			iflags |= xflag->i_flag;
+		}
+	}
+
+	inode_set_flags(inode, iflags, OVL_XFLAGS_I_FLAGS_MASK);
+
+	return n;
+}
+
+static bool ovl_xflags_from_buf(struct inode *inode, const char *buf, int len)
+{
+	const struct ovl_xflag *xflag = ovl_xflags;
+	u32 iflags = inode->i_flags & OVL_XFLAGS_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++) {
+		xflag = ovl_xflag(buf[n]);
+		if (!xflag)
+			break;
+
+		iflags |= xflag->i_flag;
+	}
+
+	inode_set_flags(inode, iflags, OVL_XFLAGS_I_FLAGS_MASK);
+
+	return buf[n] == 0;
+}
+
+/* Initialize inode flags from xflags xattr and upper inode flags */
+bool ovl_check_xflags(struct inode *inode, struct dentry *upper)
+{
+	struct ovl_fs *ofs = OVL_FS(inode->i_sb);
+	char buf[OVL_XFLAGS_MAX+1];
+	int res;
+
+	res = ovl_do_getxattr(ofs, upper, OVL_XATTR_XFLAGS, buf,
+			      OVL_XFLAGS_MAX);
+	if (res < 0)
+		return false;
+
+	buf[res] = 0;
+	if (res == 0 || !ovl_xflags_from_buf(inode, buf, res)) {
+		pr_warn_ratelimited("incompatible overlay.xflags format (%pd2, len=%d)\n",
+				    upper, res);
+	}
+
+	ovl_set_flag(OVL_XFLAGS, inode);
+	ovl_merge_xflags(d_inode(upper), inode);
+
+	return true;
+}
+
+/* Set inode flags and xflags xattr from fileattr */
+int ovl_set_xflags(struct inode *inode, struct dentry *upper,
+		   struct fileattr *fa)
+{
+	struct ovl_fs *ofs = OVL_FS(inode->i_sb);
+	char buf[OVL_XFLAGS_NUM];
+	int len, err = 0;
+
+	BUILD_BUG_ON(OVL_XFLAGS_NUM >= OVL_XFLAGS_MAX);
+	len = ovl_xflags_to_buf(inode, buf, OVL_XFLAGS_NUM, fa);
+
+	/*
+	 * Do not fail when upper doesn't support xattrs, but also do not
+	 * mask out the xattr xflags from real fileattr to continue
+	 * supporting fileattr_set() on fs without xattr support.
+	 * Remove xattr if it exist and all flags are cleared.
+	 */
+	if (len) {
+		err = ovl_check_setxattr(ofs, upper, OVL_XATTR_XFLAGS,
+					 buf, len, 0);
+	} else if (ovl_test_flag(OVL_XFLAGS, inode)) {
+		err = ovl_do_removexattr(ofs, upper, OVL_XATTR_XFLAGS);
+	}
+	if (err || ofs->noxattr)
+		return err;
+
+	/* Mask out the fileattr flags that should not be set in upper inode */
+	fa->flags &= ~OVL_XFLAGS_FS_FLAGS_MASK;
+	fa->fsx_xflags &= ~OVL_XFLAGS_FSX_FLAGS_MASK;
+
+	if (len)
+		ovl_set_flag(OVL_XFLAGS, inode);
+	else
+		ovl_clear_flag(OVL_XFLAGS, inode);
+
+	return 0;
+}
+
+/* Convert inode flags to visible fileattr/statx flags */
+void ovl_fill_xflags(struct inode *inode, struct kstat *stat,
+		     struct fileattr *fa)
+{
+	const struct ovl_xflag *xflag = ovl_xflags;
+	int i;
+
+	BUILD_BUG_ON(OVL_XFLAGS_FS_FLAGS_MASK & ~FS_COMMON_FL);
+	BUILD_BUG_ON(OVL_XFLAGS_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
+
+	for (i = 0; i < OVL_XFLAGS_NUM; i++, xflag++) {
+		if (stat)
+			stat->attributes_mask |= xflag->statx_attr;
+
+		if (!(inode->i_flags & xflag->i_flag))
+			continue;
+
+		if (stat)
+			stat->attributes |= xflag->statx_attr;
+
+		if (fa) {
+			fa->flags |= xflag->fs_flag;
+			fa->fsx_xflags |= xflag->fsx_flag;
+		}
+	}
+}
+
 /**
  * Caller must hold a reference to inode to prevent it from being freed while
  * it is marked inuse.
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 3/3] ovl: consistent behavior for immutable/append-only inodes
  2021-06-17 15:22 ` [PATCH v2 3/3] ovl: consistent behavior for immutable/append-only inodes Amir Goldstein
@ 2021-06-18 13:57   ` Miklos Szeredi
  2021-06-18 15:54     ` Amir Goldstein
  0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2021-06-18 13:57 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Chengguang Xu, overlayfs

On Thu, 17 Jun 2021 at 17:22, Amir Goldstein <amir73il@gmail.com> wrote:

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

Calling this xflags, especially near fileattr code, makes it easy to
confuse with fsx_xflags.   Can we find a more distinctive name?

> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index aec353a2dc80..d66e51b9c347 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 immutable/append-only STATX flags */
> +       if (ovl_test_flag(OVL_XFLAGS, inode))
> +               ovl_fill_xflags(inode, stat, NULL);
> +

Filesystems are doing these transformations: (already down one from
before fileattr)

internal flags -> statx->attributes
internal flags -> inode->i_flags
internal flags <-> fa->flags or fa->fsx_xflags

To further improve this situation the statx filling could be moved to
generic code based on i_flags.  I'm not asking you to convert all
filesystems (though that would be nice), but adding the helpers and
using them here would be a good first step.

> @@ -639,6 +642,174 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
>         return err;
>  }
>
> +
> +/*
> + * Overlayfs stores immutable/append-only attributes in overlay.xflags xattr.
> + * If upper inode does have those fileattr flags set (i.e. from old kernel),
> + * overlayfs does not clear them on fileattr_get(), but it will clear them on
> + * fileattr_set().
> + */
> +#define OVL_XFLAG(c, x) \
> +       { c, S_ ## x, FS_ ## x ## _FL, FS_XFLAG_ ## x, STATX_ATTR_ ## x }
> +
> +struct ovl_xflag {
> +       char code;
> +       u32 i_flag;
> +       u32 fs_flag;
> +       u32 fsx_flag;
> +       u64 statx_attr;
> +} const ovl_xflags[] = {
> +       OVL_XFLAG('a', APPEND),
> +       OVL_XFLAG('i', IMMUTABLE),
> +};

This would be really nice for a dozen flags, but for two...

My guess is that many lines of code could be saved by un-generalizing this.

> +/* Set inode flags and xflags xattr from fileattr */
> +int ovl_set_xflags(struct inode *inode, struct dentry *upper,
> +                  struct fileattr *fa)
> +{
> +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> +       char buf[OVL_XFLAGS_NUM];
> +       int len, err = 0;
> +
> +       BUILD_BUG_ON(OVL_XFLAGS_NUM >= OVL_XFLAGS_MAX);
> +       len = ovl_xflags_to_buf(inode, buf, OVL_XFLAGS_NUM, fa);
> +
> +       /*
> +        * Do not fail when upper doesn't support xattrs, but also do not
> +        * mask out the xattr xflags from real fileattr to continue
> +        * supporting fileattr_set() on fs without xattr support.
> +        * Remove xattr if it exist and all flags are cleared.
> +        */

Does this matter in practice?   I.e. is there any filesystem with
immutable/append attribute but not xattr that could be an upper layer?

If yes, then this could end up as a copy-up regression (failure to
copy up files having immutable/append).

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 3/3] ovl: consistent behavior for immutable/append-only inodes
  2021-06-18 13:57   ` Miklos Szeredi
@ 2021-06-18 15:54     ` Amir Goldstein
  0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2021-06-18 15:54 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, overlayfs

On Fri, Jun 18, 2021 at 4:57 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 17 Jun 2021 at 17:22, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > Instead, if any of the fileattr flags of interest exist on the lower
> > inode, we store them in overlay.xflags xattr on the upper inode and we
> > we read the flags from xattr on lookup and on fileattr_get().
>
> Calling this xflags, especially near fileattr code, makes it easy to
> confuse with fsx_xflags.   Can we find a more distinctive name?
>

Indeed. I'll change to overlay.protected as suggested in the v1
patch discussion.

> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index aec353a2dc80..d66e51b9c347 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 immutable/append-only STATX flags */
> > +       if (ovl_test_flag(OVL_XFLAGS, inode))
> > +               ovl_fill_xflags(inode, stat, NULL);
> > +
>
> Filesystems are doing these transformations: (already down one from
> before fileattr)
>
> internal flags -> statx->attributes
> internal flags -> inode->i_flags
> internal flags <-> fa->flags or fa->fsx_xflags
>
> To further improve this situation the statx filling could be moved to
> generic code based on i_flags.  I'm not asking you to convert all
> filesystems (though that would be nice), but adding the helpers and
> using them here would be a good first step.
>

I am afraid that the only flags this would be relevant to are (a) and (i),
so not sure it is worth the generic helper, but I will look into it.

> > @@ -639,6 +642,174 @@ int ovl_set_impure(struct dentry *dentry, struct dentry *upperdentry)
> >         return err;
> >  }
> >
> > +
> > +/*
> > + * Overlayfs stores immutable/append-only attributes in overlay.xflags xattr.
> > + * If upper inode does have those fileattr flags set (i.e. from old kernel),
> > + * overlayfs does not clear them on fileattr_get(), but it will clear them on
> > + * fileattr_set().
> > + */
> > +#define OVL_XFLAG(c, x) \
> > +       { c, S_ ## x, FS_ ## x ## _FL, FS_XFLAG_ ## x, STATX_ATTR_ ## x }
> > +
> > +struct ovl_xflag {
> > +       char code;
> > +       u32 i_flag;
> > +       u32 fs_flag;
> > +       u32 fsx_flag;
> > +       u64 statx_attr;
> > +} const ovl_xflags[] = {
> > +       OVL_XFLAG('a', APPEND),
> > +       OVL_XFLAG('i', IMMUTABLE),
> > +};
>
> This would be really nice for a dozen flags, but for two...
>
> My guess is that many lines of code could be saved by un-generalizing this.
>

Perhaps. I'll try.

> > +/* Set inode flags and xflags xattr from fileattr */
> > +int ovl_set_xflags(struct inode *inode, struct dentry *upper,
> > +                  struct fileattr *fa)
> > +{
> > +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> > +       char buf[OVL_XFLAGS_NUM];
> > +       int len, err = 0;
> > +
> > +       BUILD_BUG_ON(OVL_XFLAGS_NUM >= OVL_XFLAGS_MAX);
> > +       len = ovl_xflags_to_buf(inode, buf, OVL_XFLAGS_NUM, fa);
> > +
> > +       /*
> > +        * Do not fail when upper doesn't support xattrs, but also do not
> > +        * mask out the xattr xflags from real fileattr to continue
> > +        * supporting fileattr_set() on fs without xattr support.
> > +        * Remove xattr if it exist and all flags are cleared.
> > +        */
>
> Does this matter in practice?   I.e. is there any filesystem with
> immutable/append attribute but not xattr that could be an upper layer?

I did not find any, but did not want to take the risk, because maybe
there is a fs that does not support trusted.xattr but does support fileattr -
I did not check that. And the fallback seemed pretty safe to me.
I am also fine with failing fileattr_set() in that case.

>
> If yes, then this could end up as a copy-up regression (failure to
> copy up files having immutable/append).

No, because ovl_copy_xflags() always masks those flags on copy up:

        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);

The comment above about not clearing the immutable flag is referring
specifically to fileattr_set().

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-06-18 15:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 15:22 [PATCH v2 0/3] Overlayfs fileattr related fixes Amir Goldstein
2021-06-17 15:22 ` [PATCH v2 1/3] ovl: pass ovl_fs to ovl_check_setxattr() Amir Goldstein
2021-06-17 15:22 ` [PATCH v2 2/3] ovl: copy up sync/noatime fileattr flags Amir Goldstein
2021-06-17 15:22 ` [PATCH v2 3/3] ovl: consistent behavior for immutable/append-only inodes Amir Goldstein
2021-06-18 13:57   ` Miklos Szeredi
2021-06-18 15:54     ` Amir Goldstein

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).