Xattr issues with overlayfs

Xattr issues with overlayfs

[hujianyang]

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

[PATCH] ovl: Use macros to present ovl_xattr

[hujianyang]

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 <hujianyang@huawei.com>
---
 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(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 8ffc4b9..7da3f17 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -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);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 07d74b2..b072fb5 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -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,
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 814bed3..285994f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -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)
 {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f16d318..80afc7f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -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;

-- 
1.6.0.2

[PATCH] ovl: Enable opauqe setting an removing in userspace

[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 <hujianyang@huawei.com>
---
 fs/overlayfs/inode.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b072fb5..f9e3642 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -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) {
-- 
1.6.0.2

Re: Xattr issues with overlayfs

[Miklos Szeredi]

On Mon, Nov 24, 2014 at 11:16 AM, hujianyang <hujianyang@huawei.com> wrote:

> I'd like to introduce some of my work.
>
> 1) Use macros to indicate overlayfs_private_xattr.

Okay.

> 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

Re: Xattr issues with overlayfs

[hujianyang]

On 2014/11/25 22:41, Miklos Szeredi wrote:
> 
> 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.

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