linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ovl: consistent behavior for immutable/append-only inodes
@ 2021-06-06 14:46 Amir Goldstein
  2021-06-08 13:52 ` Miklos Szeredi
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2021-06-06 14:46 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, linux-unionfs

When a lower file has immutable/append-only attributes, 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 set an xattr overlay.xflags on the upper inode as an indication
to merge the origin inode fileattr flags on lookup.

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

When user sets new fileattr flags, we break the connection with the
origin fileattr by removing the overlay.xflags xattr.

Note that having the S_IMMUTABLE/S_APPEND on the ovl inode does not
provide the same level of protection as setting those flags on the real
upper inode, because some filesystem check those flags internally in
addition or instead of the vfs checks (e.g. btrfs_may_delete()), but
that is the way it has always been for overlayfs.

As can be seen in the comment above ovl_check_origin_xflags(), the
"xflags merge" feature is designed to solve other non-standard behavior
issues related to immutable directories and hardlinks in the future, but
this commit does not bother to fix those cases because those are corner
cases that are probably not so important to fix.

A word about the design decision to merge the origin and upper xflags -
Because we do not copy up fileattr and because fileattr_set breaks the
link to origin xflags, the only cases where origin and upper inodes both
have xflags is if upper inode was modified not via overlayfs or if the
system crashed during ovl_fileattr_set() before removing the
overlay.xflags xattr.  In both cases, modifiying the upper inode is not
going to be permitted, so it is better to reflect this in the overlay
inode flags.

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

Miklos,

This is the minial and most sane patch I could come up with to fix
xfstest overlay/075 failure.

Note that there is currently a test bug in overlay/075 in upstream
xfstests. I have already posted a fix for the test bug [1].
I also have an improvement to the test to cover the case of breaking
connection to origin xflags on fileattr_set. I will post it later.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/20210605094326.352107-1-amir73il@gmail.com/

 fs/overlayfs/copy_up.c   | 20 ++++++++-
 fs/overlayfs/inode.c     | 91 +++++++++++++++++++++++++++++++---------
 fs/overlayfs/overlayfs.h | 20 ++++++++-
 fs/overlayfs/util.c      | 39 +++++++++++++++++
 4 files changed, 148 insertions(+), 22 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 2846b943e80c..6115773ad56a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -526,9 +526,27 @@ 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);
+		struct dentry *origin = c->lowerpath.dentry;
+
+		err = ovl_set_origin(ofs, c->dentry, origin, temp);
 		if (err)
 			return err;
+		/*
+		 * We cannot copy up immutable and append-only flags, because
+		 * immutable/append-only inodes cannot be linked and cannot set
+		 * xattr on those inodes.  After copy up, ovl inode still has
+		 * those inode flags copied from lower inode, so vfs code still
+		 * treats the ovl inode as immutable/append-only.
+		 * Set this xattr as indication to copy the origin inode flags
+		 * on lookup in order to be consistent with post copy up state.
+		 */
+		if (origin->d_inode->i_flags & OVL_I_FLAGS_MASK) {
+			err = ovl_check_setxattr(c->dentry, temp,
+						 OVL_XATTR_XFLAGS,
+						 NULL, 0, 0);
+			if (err)
+				return err;
+		}
 	}
 
 	if (c->metacopy) {
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 5e828a1c98a8..f97cad498faa 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,
-				 bool set)
+static int ovl_security_fileattr(const 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,26 @@ static int ovl_security_fileattr(struct dentry *dentry, struct fileattr *fa,
 	return err;
 }
 
+static int ovl_real_fileattr(const 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 +554,13 @@ 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);
+		if (!err) {
+			/* No merge of origin fileattr flags from now on */
+			ovl_remove_origin_xflags(dentry);
+		}
 		revert_creds(old_cred);
 		ovl_copyflags(ovl_inode_real(inode), inode);
 	}
@@ -555,17 +569,50 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
 	return err;
 }
 
+int ovl_merge_origin_xflags(struct dentry *dentry, struct fileattr *fa)
+{
+	struct fileattr realfa = {
+		.flags_valid = fa->flags_valid,
+		.fsx_valid = fa->fsx_valid,
+	};
+	struct path realpath;
+	u32 flags;
+	int err;
+
+	ovl_path_lower(dentry, &realpath);
+	err = ovl_real_fileattr(&realpath, &realfa, false);
+	if (err)
+		return err;
+
+	if (!realfa.flags && !realfa.fsx_xflags) {
+		/* No use to merge zero origin flags */
+		ovl_remove_origin_xflags(dentry);
+		return 0;
+	}
+
+	flags = (fa->flags | realfa.flags) & OVL_FS_FLAGS_MASK;
+	set_mask_bits(&fa->flags, OVL_FS_FLAGS_MASK, flags);
+	flags = (fa->fsx_xflags | realfa.fsx_xflags) & OVL_FS_XFLAGS_MASK;
+	set_mask_bits(&fa->fsx_xflags, OVL_FS_XFLAGS_MASK, flags);
+
+	return 0;
+}
+
 int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct inode *inode = d_inode(dentry);
-	struct dentry *realdentry = ovl_dentry_real(dentry);
+	struct path realpath;
+	enum ovl_path_type type = ovl_path_real(dentry, &realpath);
 	const struct cred *old_cred;
 	int err;
 
 	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);
+	if (!err && OVL_TYPE_ORIGIN(type) &&
+	    ovl_check_origin_xflags(ofs, realpath.dentry)) {
+		err = ovl_merge_origin_xflags(dentry, fa);
+	}
 	revert_creds(old_cred);
 
 	return err;
@@ -1037,9 +1084,11 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	struct ovl_fs *ofs = OVL_FS(sb);
 	struct dentry *upperdentry = oip->upperdentry;
 	struct ovl_path *lowerpath = oip->lowerpath;
-	struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
 	struct inode *inode;
 	struct dentry *lowerdentry = lowerpath ? lowerpath->dentry : NULL;
+	struct inode *lowerinode = lowerdentry ? d_inode(lowerdentry) : NULL;
+	struct inode *realinode = upperdentry ? d_inode(upperdentry) :
+						lowerinode;
 	bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry,
 					oip->index);
 	int fsid = bylower ? lowerpath->layer->fsid : 0;
@@ -1047,9 +1096,6 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	unsigned long ino = 0;
 	int err = oip->newinode ? -EEXIST : -ENOMEM;
 
-	if (!realinode)
-		realinode = d_inode(lowerdentry);
-
 	/*
 	 * Copy up origin (lower) may exist for non-indexed upper, but we must
 	 * not use lower as hash key if this is a broken hardlink.
@@ -1118,6 +1164,13 @@ struct inode *ovl_get_inode(struct super_block *sb,
 		}
 	}
 
+	/* Check if need to merge fileattr flags from origin */
+	if (upperdentry && bylower &&
+	    (lowerinode->i_flags & OVL_I_FLAGS_MASK) &&
+	    ovl_check_origin_xflags(ofs, upperdentry)) {
+		ovl_mergeflags(lowerinode, inode);
+	}
+
 	if (inode->i_state & I_NEW)
 		unlock_new_inode(inode);
 out:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 29d71f253db4..aef70bb7a47b 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 {
@@ -330,6 +331,8 @@ int ovl_copy_up_start(struct dentry *dentry, int flags);
 void ovl_copy_up_end(struct dentry *dentry);
 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_origin_xflags(struct ovl_fs *ofs, struct dentry *upperdentry);
+void ovl_remove_origin_xflags(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,
@@ -530,11 +533,24 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 	i_size_write(to, i_size_read(from));
 }
 
+#define OVL_I_FLAGS_MASK (S_SYNC | S_IMMUTABLE | S_APPEND | S_NOATIME)
+#define OVL_FS_FLAGS_MASK (FS_SYNC_FL | FS_IMMUTABLE_FL | \
+			   FS_APPEND_FL | FS_NOATIME_FL)
+#define OVL_FS_XFLAGS_MASK (FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | \
+			    FS_XFLAG_APPEND | 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 flags = from->i_flags & OVL_I_FLAGS_MASK;
+
+	inode_set_flags(to, flags, OVL_I_FLAGS_MASK);
+}
+
+static inline void ovl_mergeflags(struct inode *from, struct inode *to)
+{
+	unsigned int flags = (from->i_flags | to->i_flags) & OVL_I_FLAGS_MASK;
 
-	inode_set_flags(to, from->i_flags & mask, mask);
+	inode_set_flags(to, flags, OVL_I_FLAGS_MASK);
 }
 
 /* dir.c */
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index b9d03627f364..5c08383d9f9f 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -562,6 +562,43 @@ bool ovl_check_origin_xattr(struct ovl_fs *ofs, struct dentry *dentry)
 	return false;
 }
 
+/*
+ * By design, overlay inode fileattr can be in one of three states depending
+ * on the value of xattr overlay.xflags on upper inode:
+ *
+ * 1) No xattr (or no upper) - ovl fileattr are copied from real inode
+ * 2) Empty xattr - ovl fileattr are a merge of fileattr of upper inode and
+ *    some fileattr flags from lower inode
+ * 3) Non-empty xattr - ovl fileattr are a merge of real inode fileattr and
+ *    some fileattr flags read from xattr
+ *
+ * Option 3 is meant to allow setting immutable flag on overlay merged
+ * directories without preventing copy up of children and to allow setting
+ * append-only flag on overlay copied up hardlinks without preventing copy up
+ * of their lower aliases.
+ *
+ * Option 3 is not implemented at this time.
+ */
+bool ovl_check_origin_xflags(struct ovl_fs *ofs, struct dentry *upperdentry)
+{
+	int res;
+
+	res = ovl_do_getxattr(ofs, upperdentry, OVL_XATTR_XFLAGS, NULL, 0);
+	if (res > 0) {
+		pr_warn_ratelimited("incompatible overlay.xflags format (%pd2, len=%d) ignored\n",
+				    upperdentry, res);
+	}
+
+	return res == 0;
+}
+
+void ovl_remove_origin_xflags(struct dentry *dentry)
+{
+	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
+
+	ovl_do_removexattr(ofs, ovl_dentry_upper(dentry), OVL_XATTR_XFLAGS);
+}
+
 bool ovl_check_dir_xattr(struct super_block *sb, struct dentry *dentry,
 			 enum ovl_xattr ox)
 {
@@ -585,6 +622,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 +636,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 dentry *dentry, struct dentry *upperdentry,
-- 
2.31.1


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

* Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes
  2021-06-06 14:46 [PATCH] ovl: consistent behavior for immutable/append-only inodes Amir Goldstein
@ 2021-06-08 13:52 ` Miklos Szeredi
  2021-06-08 14:37   ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Miklos Szeredi @ 2021-06-08 13:52 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Chengguang Xu, overlayfs

On Sun, 6 Jun 2021 at 16:46, Amir Goldstein <amir73il@gmail.com> wrote:
>
> When a lower file has immutable/append-only attributes, 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.

Ugh.

> Instead, if any of the fileattr flags of interest exist on the lower
> inode, we set an xattr overlay.xflags on the upper inode as an indication
> to merge the origin inode fileattr flags on lookup.
>
> This gives consistent behavior post copy up regardless of inode eviction
> from cache.
>
> When user sets new fileattr flags, we break the connection with the
> origin fileattr by removing the overlay.xflags xattr.
>
> Note that having the S_IMMUTABLE/S_APPEND on the ovl inode does not
> provide the same level of protection as setting those flags on the real
> upper inode, because some filesystem check those flags internally in
> addition or instead of the vfs checks (e.g. btrfs_may_delete()), but
> that is the way it has always been for overlayfs.

That's fine, underlying filesystem is just a backing store.

> As can be seen in the comment above ovl_check_origin_xflags(), the
> "xflags merge" feature is designed to solve other non-standard behavior
> issues related to immutable directories and hardlinks in the future, but
> this commit does not bother to fix those cases because those are corner
> cases that are probably not so important to fix.
>
> A word about the design decision to merge the origin and upper xflags -
> Because we do not copy up fileattr and because fileattr_set breaks the
> link to origin xflags, the only cases where origin and upper inodes both
> have xflags is if upper inode was modified not via overlayfs or if the
> system crashed during ovl_fileattr_set() before removing the
> overlay.xflags xattr.  In both cases, modifiying the upper inode is not
> going to be permitted, so it is better to reflect this in the overlay
> inode flags.

So why not implement the non-merge (#3) behavior unconditionally?
That would solve all issues related to fileattr, right?

Thanks,
Miklos

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

* Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes
  2021-06-08 13:52 ` Miklos Szeredi
@ 2021-06-08 14:37   ` Amir Goldstein
  2021-06-08 14:49     ` Miklos Szeredi
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2021-06-08 14:37 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, overlayfs

On Tue, Jun 8, 2021 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, 6 Jun 2021 at 16:46, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > When a lower file has immutable/append-only attributes, 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.
>
> Ugh.
>
> > Instead, if any of the fileattr flags of interest exist on the lower
> > inode, we set an xattr overlay.xflags on the upper inode as an indication
> > to merge the origin inode fileattr flags on lookup.
> >
> > This gives consistent behavior post copy up regardless of inode eviction
> > from cache.
> >
> > When user sets new fileattr flags, we break the connection with the
> > origin fileattr by removing the overlay.xflags xattr.
> >
> > Note that having the S_IMMUTABLE/S_APPEND on the ovl inode does not
> > provide the same level of protection as setting those flags on the real
> > upper inode, because some filesystem check those flags internally in
> > addition or instead of the vfs checks (e.g. btrfs_may_delete()), but
> > that is the way it has always been for overlayfs.
>
> That's fine, underlying filesystem is just a backing store.
>

Immutability of underlying files was not my concern.
My concern was that vfs does not provide full protection and that some
protection is provided in fs level, because I saw IS_APPEND/IS_IMMUTABLE
sprinkled all over the place in fs (e.g. ext4_setattr()), but I guess those are
just leftovers and I was over concerned.

> > As can be seen in the comment above ovl_check_origin_xflags(), the
> > "xflags merge" feature is designed to solve other non-standard behavior
> > issues related to immutable directories and hardlinks in the future, but
> > this commit does not bother to fix those cases because those are corner
> > cases that are probably not so important to fix.
> >
> > A word about the design decision to merge the origin and upper xflags -
> > Because we do not copy up fileattr and because fileattr_set breaks the
> > link to origin xflags, the only cases where origin and upper inodes both
> > have xflags is if upper inode was modified not via overlayfs or if the
> > system crashed during ovl_fileattr_set() before removing the
> > overlay.xflags xattr.  In both cases, modifiying the upper inode is not
> > going to be permitted, so it is better to reflect this in the overlay
> > inode flags.
>
> So why not implement the non-merge (#3) behavior unconditionally?
> That would solve all issues related to fileattr, right?
>

I suppose so. Note that #3 fileattr_get is still a merge between upper fileattr
and the 4 overlay stored flags, but for inode flags it will not be a merge.

I can give this a shot.

While you are here, do you think that will be sufficient for the on-disk format
of overlay.xflags?

struct ovl_xflags {
        __le32 xflags;
        __le32 xflags_mask;
}

Thanks,
Amir.

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

* Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes
  2021-06-08 14:37   ` Amir Goldstein
@ 2021-06-08 14:49     ` Miklos Szeredi
  2021-06-08 15:33       ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Miklos Szeredi @ 2021-06-08 14:49 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Chengguang Xu, overlayfs

On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jun 8, 2021 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sun, 6 Jun 2021 at 16:46, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > When a lower file has immutable/append-only attributes, 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.
> >
> > Ugh.
> >
> > > Instead, if any of the fileattr flags of interest exist on the lower
> > > inode, we set an xattr overlay.xflags on the upper inode as an indication
> > > to merge the origin inode fileattr flags on lookup.
> > >
> > > This gives consistent behavior post copy up regardless of inode eviction
> > > from cache.
> > >
> > > When user sets new fileattr flags, we break the connection with the
> > > origin fileattr by removing the overlay.xflags xattr.
> > >
> > > Note that having the S_IMMUTABLE/S_APPEND on the ovl inode does not
> > > provide the same level of protection as setting those flags on the real
> > > upper inode, because some filesystem check those flags internally in
> > > addition or instead of the vfs checks (e.g. btrfs_may_delete()), but
> > > that is the way it has always been for overlayfs.
> >
> > That's fine, underlying filesystem is just a backing store.
> >
>
> Immutability of underlying files was not my concern.
> My concern was that vfs does not provide full protection and that some
> protection is provided in fs level, because I saw IS_APPEND/IS_IMMUTABLE
> sprinkled all over the place in fs (e.g. ext4_setattr()), but I guess those are
> just leftovers and I was over concerned.

Would be a nice cleanup to get rid of these.   It would also prove
that the vfs protection is sufficient.

>
> > > As can be seen in the comment above ovl_check_origin_xflags(), the
> > > "xflags merge" feature is designed to solve other non-standard behavior
> > > issues related to immutable directories and hardlinks in the future, but
> > > this commit does not bother to fix those cases because those are corner
> > > cases that are probably not so important to fix.
> > >
> > > A word about the design decision to merge the origin and upper xflags -
> > > Because we do not copy up fileattr and because fileattr_set breaks the
> > > link to origin xflags, the only cases where origin and upper inodes both
> > > have xflags is if upper inode was modified not via overlayfs or if the
> > > system crashed during ovl_fileattr_set() before removing the
> > > overlay.xflags xattr.  In both cases, modifiying the upper inode is not
> > > going to be permitted, so it is better to reflect this in the overlay
> > > inode flags.
> >
> > So why not implement the non-merge (#3) behavior unconditionally?
> > That would solve all issues related to fileattr, right?
> >
>
> I suppose so. Note that #3 fileattr_get is still a merge between upper fileattr
> and the 4 overlay stored flags, but for inode flags it will not be a merge.
>
> I can give this a shot.
>
> While you are here, do you think that will be sufficient for the on-disk format
> of overlay.xflags?
>
> struct ovl_xflags {
>         __le32 xflags;
>         __le32 xflags_mask;
> }

I think I'd prefer a slightly more complex, but user friendlier
"+i,-a,..." format.

Thanks,
Miklos

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

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

On Tue, Jun 8, 2021 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Jun 8, 2021 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Sun, 6 Jun 2021 at 16:46, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > When a lower file has immutable/append-only attributes, 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.
> > >
> > > Ugh.
> > >
> > > > Instead, if any of the fileattr flags of interest exist on the lower
> > > > inode, we set an xattr overlay.xflags on the upper inode as an indication
> > > > to merge the origin inode fileattr flags on lookup.
> > > >
> > > > This gives consistent behavior post copy up regardless of inode eviction
> > > > from cache.
> > > >
> > > > When user sets new fileattr flags, we break the connection with the
> > > > origin fileattr by removing the overlay.xflags xattr.
> > > >
> > > > Note that having the S_IMMUTABLE/S_APPEND on the ovl inode does not
> > > > provide the same level of protection as setting those flags on the real
> > > > upper inode, because some filesystem check those flags internally in
> > > > addition or instead of the vfs checks (e.g. btrfs_may_delete()), but
> > > > that is the way it has always been for overlayfs.
> > >
> > > That's fine, underlying filesystem is just a backing store.
> > >
> >
> > Immutability of underlying files was not my concern.
> > My concern was that vfs does not provide full protection and that some
> > protection is provided in fs level, because I saw IS_APPEND/IS_IMMUTABLE
> > sprinkled all over the place in fs (e.g. ext4_setattr()), but I guess those are
> > just leftovers and I was over concerned.
>
> Would be a nice cleanup to get rid of these.   It would also prove
> that the vfs protection is sufficient.
>
> >
> > > > As can be seen in the comment above ovl_check_origin_xflags(), the
> > > > "xflags merge" feature is designed to solve other non-standard behavior
> > > > issues related to immutable directories and hardlinks in the future, but
> > > > this commit does not bother to fix those cases because those are corner
> > > > cases that are probably not so important to fix.
> > > >
> > > > A word about the design decision to merge the origin and upper xflags -
> > > > Because we do not copy up fileattr and because fileattr_set breaks the
> > > > link to origin xflags, the only cases where origin and upper inodes both
> > > > have xflags is if upper inode was modified not via overlayfs or if the
> > > > system crashed during ovl_fileattr_set() before removing the
> > > > overlay.xflags xattr.  In both cases, modifiying the upper inode is not
> > > > going to be permitted, so it is better to reflect this in the overlay
> > > > inode flags.
> > >
> > > So why not implement the non-merge (#3) behavior unconditionally?
> > > That would solve all issues related to fileattr, right?
> > >
> >
> > I suppose so. Note that #3 fileattr_get is still a merge between upper fileattr
> > and the 4 overlay stored flags, but for inode flags it will not be a merge.
> >
> > I can give this a shot.
> >
> > While you are here, do you think that will be sufficient for the on-disk format
> > of overlay.xflags?
> >
> > struct ovl_xflags {
> >         __le32 xflags;
> >         __le32 xflags_mask;
> > }
>
> I think I'd prefer a slightly more complex, but user friendlier
> "+i,-a,..." format.
>

OK, but since this is not a merge, we'd only need:
overlay.xflags = "ia..."

Which is compatible with the format of:
chattr =<xflags> <file>

Thanks,
Amir.

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

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

On Tue, 8 Jun 2021 at 17:33, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jun 8, 2021 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Jun 8, 2021 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Sun, 6 Jun 2021 at 16:46, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > When a lower file has immutable/append-only attributes, 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.
> > > >
> > > > Ugh.
> > > >
> > > > > Instead, if any of the fileattr flags of interest exist on the lower
> > > > > inode, we set an xattr overlay.xflags on the upper inode as an indication
> > > > > to merge the origin inode fileattr flags on lookup.
> > > > >
> > > > > This gives consistent behavior post copy up regardless of inode eviction
> > > > > from cache.
> > > > >
> > > > > When user sets new fileattr flags, we break the connection with the
> > > > > origin fileattr by removing the overlay.xflags xattr.
> > > > >
> > > > > Note that having the S_IMMUTABLE/S_APPEND on the ovl inode does not
> > > > > provide the same level of protection as setting those flags on the real
> > > > > upper inode, because some filesystem check those flags internally in
> > > > > addition or instead of the vfs checks (e.g. btrfs_may_delete()), but
> > > > > that is the way it has always been for overlayfs.
> > > >
> > > > That's fine, underlying filesystem is just a backing store.
> > > >
> > >
> > > Immutability of underlying files was not my concern.
> > > My concern was that vfs does not provide full protection and that some
> > > protection is provided in fs level, because I saw IS_APPEND/IS_IMMUTABLE
> > > sprinkled all over the place in fs (e.g. ext4_setattr()), but I guess those are
> > > just leftovers and I was over concerned.
> >
> > Would be a nice cleanup to get rid of these.   It would also prove
> > that the vfs protection is sufficient.
> >
> > >
> > > > > As can be seen in the comment above ovl_check_origin_xflags(), the
> > > > > "xflags merge" feature is designed to solve other non-standard behavior
> > > > > issues related to immutable directories and hardlinks in the future, but
> > > > > this commit does not bother to fix those cases because those are corner
> > > > > cases that are probably not so important to fix.
> > > > >
> > > > > A word about the design decision to merge the origin and upper xflags -
> > > > > Because we do not copy up fileattr and because fileattr_set breaks the
> > > > > link to origin xflags, the only cases where origin and upper inodes both
> > > > > have xflags is if upper inode was modified not via overlayfs or if the
> > > > > system crashed during ovl_fileattr_set() before removing the
> > > > > overlay.xflags xattr.  In both cases, modifiying the upper inode is not
> > > > > going to be permitted, so it is better to reflect this in the overlay
> > > > > inode flags.
> > > >
> > > > So why not implement the non-merge (#3) behavior unconditionally?
> > > > That would solve all issues related to fileattr, right?
> > > >
> > >
> > > I suppose so. Note that #3 fileattr_get is still a merge between upper fileattr
> > > and the 4 overlay stored flags, but for inode flags it will not be a merge.
> > >
> > > I can give this a shot.
> > >
> > > While you are here, do you think that will be sufficient for the on-disk format
> > > of overlay.xflags?
> > >
> > > struct ovl_xflags {
> > >         __le32 xflags;
> > >         __le32 xflags_mask;
> > > }
> >
> > I think I'd prefer a slightly more complex, but user friendlier
> > "+i,-a,..." format.
> >
>
> OK, but since this is not a merge, we'd only need:
> overlay.xflags = "ia..."
>
> Which is compatible with the format of:
> chattr =<xflags> <file>

Fine.   Not sure what xflags_mask would be useful for in your proposal, though.

Thanks,
Miklos

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

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

On Tue, Jun 8, 2021 at 9:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 8 Jun 2021 at 17:33, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Jun 8, 2021 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Jun 8, 2021 at 4:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > On Sun, 6 Jun 2021 at 16:46, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > >
> > > > > > When a lower file has immutable/append-only attributes, 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.
> > > > >
> > > > > Ugh.
> > > > >
> > > > > > Instead, if any of the fileattr flags of interest exist on the lower
> > > > > > inode, we set an xattr overlay.xflags on the upper inode as an indication
> > > > > > to merge the origin inode fileattr flags on lookup.
> > > > > >
> > > > > > This gives consistent behavior post copy up regardless of inode eviction
> > > > > > from cache.
> > > > > >
> > > > > > When user sets new fileattr flags, we break the connection with the
> > > > > > origin fileattr by removing the overlay.xflags xattr.
> > > > > >
> > > > > > Note that having the S_IMMUTABLE/S_APPEND on the ovl inode does not
> > > > > > provide the same level of protection as setting those flags on the real
> > > > > > upper inode, because some filesystem check those flags internally in
> > > > > > addition or instead of the vfs checks (e.g. btrfs_may_delete()), but
> > > > > > that is the way it has always been for overlayfs.
> > > > >
> > > > > That's fine, underlying filesystem is just a backing store.
> > > > >
> > > >
> > > > Immutability of underlying files was not my concern.
> > > > My concern was that vfs does not provide full protection and that some
> > > > protection is provided in fs level, because I saw IS_APPEND/IS_IMMUTABLE
> > > > sprinkled all over the place in fs (e.g. ext4_setattr()), but I guess those are
> > > > just leftovers and I was over concerned.
> > >
> > > Would be a nice cleanup to get rid of these.   It would also prove
> > > that the vfs protection is sufficient.
> > >
> > > >
> > > > > > As can be seen in the comment above ovl_check_origin_xflags(), the
> > > > > > "xflags merge" feature is designed to solve other non-standard behavior
> > > > > > issues related to immutable directories and hardlinks in the future, but
> > > > > > this commit does not bother to fix those cases because those are corner
> > > > > > cases that are probably not so important to fix.
> > > > > >
> > > > > > A word about the design decision to merge the origin and upper xflags -
> > > > > > Because we do not copy up fileattr and because fileattr_set breaks the
> > > > > > link to origin xflags, the only cases where origin and upper inodes both
> > > > > > have xflags is if upper inode was modified not via overlayfs or if the
> > > > > > system crashed during ovl_fileattr_set() before removing the
> > > > > > overlay.xflags xattr.  In both cases, modifiying the upper inode is not
> > > > > > going to be permitted, so it is better to reflect this in the overlay
> > > > > > inode flags.
> > > > >
> > > > > So why not implement the non-merge (#3) behavior unconditionally?
> > > > > That would solve all issues related to fileattr, right?
> > > > >
> > > >
> > > > I suppose so. Note that #3 fileattr_get is still a merge between upper fileattr
> > > > and the 4 overlay stored flags, but for inode flags it will not be a merge.
> > > >
> > > > I can give this a shot.
> > > >
> > > > While you are here, do you think that will be sufficient for the on-disk format
> > > > of overlay.xflags?
> > > >
> > > > struct ovl_xflags {
> > > >         __le32 xflags;
> > > >         __le32 xflags_mask;
> > > > }
> > >
> > > I think I'd prefer a slightly more complex, but user friendlier
> > > "+i,-a,..." format.
> > >
> >
> > OK, but since this is not a merge, we'd only need:
> > overlay.xflags = "ia..."
> >
> > Which is compatible with the format of:
> > chattr =<xflags> <file>
>
> Fine.   Not sure what xflags_mask would be useful for in your proposal, though.
>

The idea was that in the context of fileattr_get(), any specific xflag
value can be one of: SET, CLEAR, REAL.

For most inodes all flags are REAL (no xflags xattr)
All flags but the 4 in OVL_FS_XFLAGS_MASK are always REAL
(i.e. taken from fileattr_get() on real inode).

If we ever decide to extend OVL_FS_XFLAGS_MASK, say to include
DIRSYNC, then an upper inode with DIRSYNC that was in state
REAL before upgrade would become CLEAR after upgrade unless
we kept the old xflags_mask in xattr.

With the string format, this is not a concern.
Therefore, I like the string format better.

Thanks,
Amir.

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

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

On Wed, 9 Jun 2021 at 08:08, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jun 8, 2021 at 9:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 8 Jun 2021 at 17:33, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Jun 8, 2021 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:

> > > > > While you are here, do you think that will be sufficient for the on-disk format
> > > > > of overlay.xflags?
> > > > >
> > > > > struct ovl_xflags {
> > > > >         __le32 xflags;
> > > > >         __le32 xflags_mask;
> > > > > }
> > > >
> > > > I think I'd prefer a slightly more complex, but user friendlier
> > > > "+i,-a,..." format.
> > > >
> > >
> > > OK, but since this is not a merge, we'd only need:
> > > overlay.xflags = "ia..."
> > >
> > > Which is compatible with the format of:
> > > chattr =<xflags> <file>
> >
> > Fine.   Not sure what xflags_mask would be useful for in your proposal, though.
> >
>
> The idea was that in the context of fileattr_get(), any specific xflag
> value can be one of: SET, CLEAR, REAL.
>
> For most inodes all flags are REAL (no xflags xattr)
> All flags but the 4 in OVL_FS_XFLAGS_MASK are always REAL
> (i.e. taken from fileattr_get() on real inode).
>
> If we ever decide to extend OVL_FS_XFLAGS_MASK, say to include
> DIRSYNC, then an upper inode with DIRSYNC that was in state
> REAL before upgrade would become CLEAR after upgrade unless
> we kept the old xflags_mask in xattr.
>
> With the string format, this is not a concern.
> Therefore, I like the string format better.

Hmm, so if the attribute letters would have fixed places in the string
and clear attributes would be represented by a space or a "-" then
that would be similarly extensible.   Just having a list of set
attribute letters would not allow having three states.

Thanks,
Miklos

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

* Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes
  2021-06-09  7:28             ` Miklos Szeredi
@ 2021-06-09  7:57               ` Amir Goldstein
  2021-06-11  7:31                 ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2021-06-09  7:57 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, overlayfs

On Wed, Jun 9, 2021 at 10:28 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 9 Jun 2021 at 08:08, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Jun 8, 2021 at 9:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, 8 Jun 2021 at 17:33, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Tue, Jun 8, 2021 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > > > > > While you are here, do you think that will be sufficient for the on-disk format
> > > > > > of overlay.xflags?
> > > > > >
> > > > > > struct ovl_xflags {
> > > > > >         __le32 xflags;
> > > > > >         __le32 xflags_mask;
> > > > > > }
> > > > >
> > > > > I think I'd prefer a slightly more complex, but user friendlier
> > > > > "+i,-a,..." format.
> > > > >
> > > >
> > > > OK, but since this is not a merge, we'd only need:
> > > > overlay.xflags = "ia..."
> > > >
> > > > Which is compatible with the format of:
> > > > chattr =<xflags> <file>
> > >
> > > Fine.   Not sure what xflags_mask would be useful for in your proposal, though.
> > >
> >
> > The idea was that in the context of fileattr_get(), any specific xflag
> > value can be one of: SET, CLEAR, REAL.
> >
> > For most inodes all flags are REAL (no xflags xattr)
> > All flags but the 4 in OVL_FS_XFLAGS_MASK are always REAL
> > (i.e. taken from fileattr_get() on real inode).
> >
> > If we ever decide to extend OVL_FS_XFLAGS_MASK, say to include
> > DIRSYNC, then an upper inode with DIRSYNC that was in state
> > REAL before upgrade would become CLEAR after upgrade unless
> > we kept the old xflags_mask in xattr.
> >
> > With the string format, this is not a concern.
> > Therefore, I like the string format better.
>
> Hmm, so if the attribute letters would have fixed places in the string
> and clear attributes would be represented by a space or a "-" then
> that would be similarly extensible.   Just having a list of set
> attribute letters would not allow having three states.
>

Right. Will do that.

Thanks,
Amir.

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

* Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes
  2021-06-09  7:57               ` Amir Goldstein
@ 2021-06-11  7:31                 ` Amir Goldstein
  2021-06-11  7:55                   ` Miklos Szeredi
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2021-06-11  7:31 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, overlayfs

On Wed, Jun 9, 2021 at 10:57 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jun 9, 2021 at 10:28 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 9 Jun 2021 at 08:08, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Jun 8, 2021 at 9:20 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, 8 Jun 2021 at 17:33, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jun 8, 2021 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > > >
> > > > > > On Tue, 8 Jun 2021 at 16:37, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > > > > > While you are here, do you think that will be sufficient for the on-disk format
> > > > > > > of overlay.xflags?
> > > > > > >
> > > > > > > struct ovl_xflags {
> > > > > > >         __le32 xflags;
> > > > > > >         __le32 xflags_mask;
> > > > > > > }
> > > > > >
> > > > > > I think I'd prefer a slightly more complex, but user friendlier
> > > > > > "+i,-a,..." format.
> > > > > >
> > > > >
> > > > > OK, but since this is not a merge, we'd only need:
> > > > > overlay.xflags = "ia..."
> > > > >
> > > > > Which is compatible with the format of:
> > > > > chattr =<xflags> <file>
> > > >
> > > > Fine.   Not sure what xflags_mask would be useful for in your proposal, though.
> > > >
> > >
> > > The idea was that in the context of fileattr_get(), any specific xflag
> > > value can be one of: SET, CLEAR, REAL.
> > >
> > > For most inodes all flags are REAL (no xflags xattr)
> > > All flags but the 4 in OVL_FS_XFLAGS_MASK are always REAL
> > > (i.e. taken from fileattr_get() on real inode).
> > >
> > > If we ever decide to extend OVL_FS_XFLAGS_MASK, say to include
> > > DIRSYNC, then an upper inode with DIRSYNC that was in state
> > > REAL before upgrade would become CLEAR after upgrade unless
> > > we kept the old xflags_mask in xattr.
> > >
> > > With the string format, this is not a concern.
> > > Therefore, I like the string format better.
> >
> > Hmm, so if the attribute letters would have fixed places in the string
> > and clear attributes would be represented by a space or a "-" then
> > that would be similarly extensible.   Just having a list of set
> > attribute letters would not allow having three states.
> >
>
> Right. Will do that.
>

Taking a step back.

The main problem this is trying to solve is losing persistent inode flags
on copy-up.

If this was just NOATIME and SYNC the solution would have been
simple - copy up the flags along with other metadata we copy up.

We wouldn't even need to limit ourselves to the 4 vfs inode flags
in ovl_copyflags(). We could add the the copied up flags more
fs specific flags that we know to be safe and rational to copy
such as NOCOW, NODUMP and DIRSYNC.

The secondary problem is that copying IMMUTABLE/APPEND
to upper inode on copy up is not an option, so the solution is to
store those properties in an xattr.

I think we should split the solution to the primary and secondary
problems and avoid an over-designed generic future extendable
xflags xattr feature.

So I am leaning towards a more focused solution for
IMMUTABLE/APPEND in the form of either two boolean
xattr overlay.{immutable,appendonly} or one single bytes
xattr overlay.protected.

Thanks,
Amir.

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

* Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes
  2021-06-11  7:31                 ` Amir Goldstein
@ 2021-06-11  7:55                   ` Miklos Szeredi
  2021-06-11  8:37                     ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Miklos Szeredi @ 2021-06-11  7:55 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Chengguang Xu, overlayfs

On Fri, 11 Jun 2021 at 09:31, Amir Goldstein <amir73il@gmail.com> wrote:

>
> Taking a step back.
>
> The main problem this is trying to solve is losing persistent inode flags
> on copy-up.
>
> If this was just NOATIME and SYNC the solution would have been
> simple - copy up the flags along with other metadata we copy up.
>
> We wouldn't even need to limit ourselves to the 4 vfs inode flags
> in ovl_copyflags(). We could add the the copied up flags more
> fs specific flags that we know to be safe and rational to copy
> such as NOCOW, NODUMP and DIRSYNC.
>
> The secondary problem is that copying IMMUTABLE/APPEND
> to upper inode on copy up is not an option, so the solution is to
> store those properties in an xattr.
>
> I think we should split the solution to the primary and secondary
> problems and avoid an over-designed generic future extendable
> xflags xattr feature.
>
> So I am leaning towards a more focused solution for
> IMMUTABLE/APPEND in the form of either two boolean
> xattr overlay.{immutable,appendonly} or one single bytes
> xattr overlay.protected.

Makes sense.

Not sure how you'd make it single byte and user friendly at the same
time. I.e. how'd you represent +ia?.   Otherwise I'm fine with either.

Thanks,
Miklos

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

* Re: [PATCH] ovl: consistent behavior for immutable/append-only inodes
  2021-06-11  7:55                   ` Miklos Szeredi
@ 2021-06-11  8:37                     ` Amir Goldstein
  2021-06-11  8:39                       ` Miklos Szeredi
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2021-06-11  8:37 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Chengguang Xu, overlayfs

On Fri, Jun 11, 2021 at 10:55 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 11 Jun 2021 at 09:31, Amir Goldstein <amir73il@gmail.com> wrote:
>
> >
> > Taking a step back.
> >
> > The main problem this is trying to solve is losing persistent inode flags
> > on copy-up.
> >
> > If this was just NOATIME and SYNC the solution would have been
> > simple - copy up the flags along with other metadata we copy up.
> >
> > We wouldn't even need to limit ourselves to the 4 vfs inode flags
> > in ovl_copyflags(). We could add the the copied up flags more
> > fs specific flags that we know to be safe and rational to copy
> > such as NOCOW, NODUMP and DIRSYNC.
> >
> > The secondary problem is that copying IMMUTABLE/APPEND
> > to upper inode on copy up is not an option, so the solution is to
> > store those properties in an xattr.
> >
> > I think we should split the solution to the primary and secondary
> > problems and avoid an over-designed generic future extendable
> > xflags xattr feature.
> >
> > So I am leaning towards a more focused solution for
> > IMMUTABLE/APPEND in the form of either two boolean
> > xattr overlay.{immutable,appendonly} or one single bytes
> > xattr overlay.protected.
>
> Makes sense.
>
> Not sure how you'd make it single byte and user friendly at the same
> time. I.e. how'd you represent +ia?.   Otherwise I'm fine with either.
>

I had not considered user friendliness.
I was thinking about the lower byte of i_flags.
I can go with text format as planned for xflags
but with no need for the fixed positions of letters.
This format will be compatible with chattr so easy
for script that "offline merge" overlay upper to lower.

Thanks,
Amir.

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

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

On Fri, 11 Jun 2021 at 10:37, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jun 11, 2021 at 10:55 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Fri, 11 Jun 2021 at 09:31, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > >
> > > Taking a step back.
> > >
> > > The main problem this is trying to solve is losing persistent inode flags
> > > on copy-up.
> > >
> > > If this was just NOATIME and SYNC the solution would have been
> > > simple - copy up the flags along with other metadata we copy up.
> > >
> > > We wouldn't even need to limit ourselves to the 4 vfs inode flags
> > > in ovl_copyflags(). We could add the the copied up flags more
> > > fs specific flags that we know to be safe and rational to copy
> > > such as NOCOW, NODUMP and DIRSYNC.
> > >
> > > The secondary problem is that copying IMMUTABLE/APPEND
> > > to upper inode on copy up is not an option, so the solution is to
> > > store those properties in an xattr.
> > >
> > > I think we should split the solution to the primary and secondary
> > > problems and avoid an over-designed generic future extendable
> > > xflags xattr feature.
> > >
> > > So I am leaning towards a more focused solution for
> > > IMMUTABLE/APPEND in the form of either two boolean
> > > xattr overlay.{immutable,appendonly} or one single bytes
> > > xattr overlay.protected.
> >
> > Makes sense.
> >
> > Not sure how you'd make it single byte and user friendly at the same
> > time. I.e. how'd you represent +ia?.   Otherwise I'm fine with either.
> >
>
> I had not considered user friendliness.
> I was thinking about the lower byte of i_flags.
> I can go with text format as planned for xflags
> but with no need for the fixed positions of letters.
> This format will be compatible with chattr so easy
> for script that "offline merge" overlay upper to lower.

Okay, let's do that, then.

Thanks,
Miklos

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

end of thread, other threads:[~2021-06-11  8:39 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-06 14:46 [PATCH] ovl: consistent behavior for immutable/append-only inodes Amir Goldstein
2021-06-08 13:52 ` Miklos Szeredi
2021-06-08 14:37   ` Amir Goldstein
2021-06-08 14:49     ` Miklos Szeredi
2021-06-08 15:33       ` Amir Goldstein
2021-06-08 18:20         ` Miklos Szeredi
2021-06-09  6:08           ` Amir Goldstein
2021-06-09  7:28             ` Miklos Szeredi
2021-06-09  7:57               ` Amir Goldstein
2021-06-11  7:31                 ` Amir Goldstein
2021-06-11  7:55                   ` Miklos Szeredi
2021-06-11  8:37                     ` Amir Goldstein
2021-06-11  8:39                       ` Miklos Szeredi

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