Skip to contentSkip to conversation

Xattr issues with overlayfs

PATCH New hujianyang 2 participants 5 messages Nov 24, 2014 – Nov 26, 2014

Subsystems (2)
VFS Overlay filesystem
lore
Share & tools
lore

hujianyang

author
#0 lore raw

Hi Miklos,

I'm learning overlayfs and finding some issues in xattr support. I don't know if they are real problems or misunderstandings. I wish you could take some time to help.

As is said in Documentation/filesystems/overlayfs.txt:

"""
A directory is made opaque by setting the xattr "trusted.overlay.opaque" to "y". Where the upper filesystem contains an opaque directory, any directory in the lower filesystem with the same name is ignored. """

I think users could set *ovl_opaque_xattr* to hide the files in lower directory in userspace like this:

	setxattr(path, "trusted.overlay.opaque", &value, 1, XATTR_CREATE);

But It seems not true because we forbid this operation in userspace:

ovl_setxattr():

        if (ovl_is_private_xattr(name))
                goto out_drop_write;

I think current xattr operations in overlayfs including setxattr, getxattr, listxattr, removexattr are used for upper/lower xattrs and overlayfs private xattrs also.

Is ovl_set/remove_opauqe() related to this?

I'd like to introduce some of my work.

1) Use macros to indicate overlayfs_private_xattr.
2) Enable overlayfs_private_xattr setting an removing in userspace.
* I'm not clear with this one

Thanks~!

Hu

author
#1 lore raw

PATCH 1/1 ovl: Use macros to present ovl_xattr

This patch adds two macros:

OVL_XATTR_PRE_NAME and OVL_XATTR_PRE_LEN

to present ovl_xattr name prefix and its length. Also, a new macro OVL_XATTR_OPAQUE is introduced to replace old *ovl_opaque_xattr*.

Fix the length of "trusted.overlay." to *16*.

Signed-off-by hujianyang <[email protected]>
---
 fs/overlayfs/dir.c       |    4 ++--
 fs/overlayfs/inode.c     |    2 +-
 fs/overlayfs/overlayfs.h |    4 +++-
 fs/overlayfs/super.c     |    5 +----
 4 files changed, 7 insertions(+), 8 deletions(-)
fs/overlayfs/dir.c +2 -2
index 8ffc4b9..7da3f17 100644
@@ -118,14 +118,14 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 
 static int ovl_set_opaque(struct dentry *upperdentry)
 {
-	return ovl_do_setxattr(upperdentry, ovl_opaque_xattr, "y", 1, 0);
+	return ovl_do_setxattr(upperdentry, OVL_XATTR_OPAQUE, "y", 1, 0);
 }
 
 static void ovl_remove_opaque(struct dentry *upperdentry)
 {
 	int err;
 
-	err = ovl_do_removexattr(upperdentry, ovl_opaque_xattr);
+	err = ovl_do_removexattr(upperdentry, OVL_XATTR_OPAQUE);
 	if (err) {
 		pr_warn("overlayfs: failed to remove opaque from '%s' (%i)\n",
 			upperdentry->d_name.name, err);
fs/overlayfs/inode.c +1 -1
index 07d74b2..b072fb5 100644
@@ -205,7 +205,7 @@ static int ovl_readlink(struct dentry *dentry, char __user *buf, int bufsiz)
 
 static bool ovl_is_private_xattr(const char *name)
 {
-	return strncmp(name, "trusted.overlay.", 14) == 0;
+	return strncmp(name, OVL_XATTR_PRE_NAME, OVL_XATTR_PRE_LEN) == 0;
 }
 
 int ovl_setxattr(struct dentry *dentry, const char *name,
fs/overlayfs/overlayfs.h +3 -1
index 814bed3..285994f 100644
@@ -18,7 +18,9 @@ enum ovl_path_type {
 	OVL_PATH_LOWER,
 };
 
-extern const char *ovl_opaque_xattr;
+#define OVL_XATTR_PRE_NAME "trusted.overlay."
+#define OVL_XATTR_PRE_LEN  16
+#define OVL_XATTR_OPAQUE   OVL_XATTR_PRE_NAME"opaque"
 
 static inline int ovl_do_rmdir(struct inode *dir, struct dentry *dentry)
 {
fs/overlayfs/super.c +1 -4
index f16d318..80afc7f 100644
@@ -58,9 +58,6 @@ struct ovl_entry {
 	};
 };
 
-const char *ovl_opaque_xattr = "trusted.overlay.opaque";
-
-
 enum ovl_path_type ovl_path_type(struct dentry *dentry)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;

@@ -249,7 +246,7 @@ static bool ovl_is_opaquedir(struct dentry *dentry)
 	if (!S_ISDIR(inode->i_mode) || !inode->i_op->getxattr)
 		return false;
 
-	res = inode->i_op->getxattr(dentry, ovl_opaque_xattr, &val, 1);
+	res = inode->i_op->getxattr(dentry, OVL_XATTR_OPAQUE, &val, 1);
 	if (res == 1 && val == 'y')
 		return true;
Signature · 1 line
1.6.0.2
author
#2 lore raw

PATCH 1/1 ovl: Enable opauqe setting an removing in userspace

In reply to hujianyang

This patch is an attempt to enbale opauqe setting and removing in userspace. It may lead to some race conditions.

Signed-off-by hujianyang <[email protected]>
---
 fs/overlayfs/inode.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)
fs/overlayfs/inode.c +9 -8
index b072fb5..f9e3642 100644
@@ -208,18 +208,25 @@ static bool ovl_is_private_xattr(const char *name)
 	return strncmp(name, OVL_XATTR_PRE_NAME, OVL_XATTR_PRE_LEN) == 0;
 }
 
+static bool ovl_need_xattr_filter(struct dentry *dentry,
+				  enum ovl_path_type type)
+{
+	return type == OVL_PATH_UPPER && S_ISDIR(dentry->d_inode->i_mode);
+}
+
 int ovl_setxattr(struct dentry *dentry, const char *name,
 		 const void *value, size_t size, int flags)
 {
 	int err;
 	struct dentry *upperdentry;
+	enum ovl_path_type type = ovl_path_type(dentry);
 
 	err = ovl_want_write(dentry);
 	if (err)
 		goto out;
 
 	err = -EPERM;
-	if (ovl_is_private_xattr(name))
+	if (ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name))
 		goto out_drop_write;
 
 	err = ovl_copy_up(dentry);

@@ -235,12 +242,6 @@ out:
 	return err;
 }
 
-static bool ovl_need_xattr_filter(struct dentry *dentry,
-				  enum ovl_path_type type)
-{
-	return type == OVL_PATH_UPPER && S_ISDIR(dentry->d_inode->i_mode);
-}
-
 ssize_t ovl_getxattr(struct dentry *dentry, const char *name,
 		     void *value, size_t size)
 {

@@ -296,7 +297,7 @@ int ovl_removexattr(struct dentry *dentry, const char *name)
 		goto out;
 
 	err = -ENODATA;
-	if (ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name))
+	if (!ovl_need_xattr_filter(dentry, type) && ovl_is_private_xattr(name))
 		goto out_drop_write;
 
 	if (type == OVL_PATH_LOWER) {
Signature · 1 line
1.6.0.2
maintainer
#3 lore raw
In reply to hujianyang
Quoted 3 lines · hujianyang

I'd like to introduce some of my work.

1) Use macros to indicate overlayfs_private_xattr.

Okay.

Quoted 2 lines

2) Enable overlayfs_private_xattr setting an removing in userspace.
* I'm not clear with this one

No, I don't think this makes any sense. Maintenance can be done by unmounting the overlay and doing modification directly on the upper layer. After this is done (adding/removing opaque flag, adding removing whiteouts, etc) the overlay can be mounted again and the modifications will be applied.

I don't think online modification of the internal attributes of the overlay makes much sense.

Except maybe, maybe a privileged undelete, but even that one is
tricky: the whiteout was either from deletion of the lower layer or the upper layer file, the later not being undeleteable. But the two cases are currently indistinguishable, so the undelete would always recover the lower layer file, which is probably not what the user wants and might cause more harm than good.

Thanks,
Miklos

author
#4 lore raw
Quoted 6 lines · Miklos Szeredi

No, I don't think this makes any sense. Maintenance can be done by unmounting the overlay and doing modification directly on the upper layer. After this is done (adding/removing opaque flag, adding removing whiteouts, etc) the overlay can be mounted again and the modifications will be applied.

Yes, we can remove overlayfs_xattr directly on upper files.

Quoted 11 lines

I don't think online modification of the internal attributes of the overlay makes much sense.

Except maybe, maybe a privileged undelete, but even that one is
tricky: the whiteout was either from deletion of the lower layer or the upper layer file, the later not being undeleteable. But the two cases are currently indistinguishable, so the undelete would always recover the lower layer file, which is probably not what the user wants and might cause more harm than good.

Agree.

Thanks for your explanation. Now I know "trusted.overlay.opaque" can be set in userspace, but this xattr is set on files in upper filesystem, not directly on files in overlayfs.

Thanks~!

Hu