two questiones about overlayfs

two questiones about overlayfs

[zhangyi (F)]

Hi,all:
I found two problemes about overlayfs, please let me know what you think:

1. Cannot update upper dir's access time.
Reproduce:
# mkdir lower upper worker merger
# mkdir upper/dir
# mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
# ls mrger/dir
# stat mrger/dir
  Access: 2017-08-07 11:03:56.364771584 -0400
  Modify: 2017-08-07 11:01:52.319771584 -0400
  Change: 2017-08-07 11:01:52.319771584 -0400
# touch mrger/dir/aa
# stat mrger/dir
  Access: 2017-08-07 11:03:56.364771584 -0400
  Modify: 2017-08-07 11:05:33.969771584 -0400
  Change: 2017-08-07 11:05:33.969771584 -0400
# ls mrger/dir
# stat mrger/dir
  Access: 2017-08-07 11:03:56.364771584 -0400
  Modify: 2017-08-07 11:05:33.969771584 -0400
  Change: 2017-08-07 11:05:33.969771584 -0400

I find the reason of this problem is in update_ovl_inode_times():
(598e3c8f7 "vfs: update ovl inode before relatime check")

static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
			       bool rcu)
{
	if (!rcu) {
		struct inode *realinode = d_real_inode(dentry);

	*d_real_inode() cannot get dir's real inode, so i_mtime always equal to i_ctime*
	*an not updated*

		if (unlikely(inode != realinode) &&
		    (!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
		     !timespec_equal(&inode->i_ctime, &realinode->i_ctime))) {
			inode->i_mtime = realinode->i_mtime;
			inode->i_ctime = realinode->i_ctime;
		}
	}
}

2. Chattr will modify lower file's attributes directly.
Reproduce:
# mkdir lower upper worker merger
# touch lower/aa
# lsattr -p lower/aa
    0 --------------e---- lower/aa
# mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
# chattr -p 123 merger/aa             #set project id
# lsattr -p lower/aa
  123 --------------e---- lower/aa

If we try to set "immutable" or any other attributes, the result are consistent.
Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
FS_IOC_SETFLAGS ioctl will get the lower inode and modify it. By the way,
ovl's directory not support ioctl now, so we fail to change dir's attributes.

Do you think these two "problemes" need to fix or should avoid ?

Thanks,
ZhangYi.

Re: two questiones about overlayfs

[Amir Goldstein]

On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Hi,all:
> I found two problemes about overlayfs, please let me know what you think:
>
> 1. Cannot update upper dir's access time.
> Reproduce:
> # mkdir lower upper worker merger
> # mkdir upper/dir
> # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> # ls mrger/dir
> # stat mrger/dir
>   Access: 2017-08-07 11:03:56.364771584 -0400
>   Modify: 2017-08-07 11:01:52.319771584 -0400
>   Change: 2017-08-07 11:01:52.319771584 -0400
> # touch mrger/dir/aa
> # stat mrger/dir
>   Access: 2017-08-07 11:03:56.364771584 -0400
>   Modify: 2017-08-07 11:05:33.969771584 -0400
>   Change: 2017-08-07 11:05:33.969771584 -0400
> # ls mrger/dir
> # stat mrger/dir
>   Access: 2017-08-07 11:03:56.364771584 -0400
>   Modify: 2017-08-07 11:05:33.969771584 -0400
>   Change: 2017-08-07 11:05:33.969771584 -0400
>
> I find the reason of this problem is in update_ovl_inode_times():
> (598e3c8f7 "vfs: update ovl inode before relatime check")
>
> static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
>                                bool rcu)
> {
>         if (!rcu) {
>                 struct inode *realinode = d_real_inode(dentry);
>
>         *d_real_inode() cannot get dir's real inode, so i_mtime always equal to i_ctime*
>         *an not updated*
>

Technically, I think this could be changed to check
if unlikely(dentry->d_flags & DCACHE_OP_REAL) and call
vfs_getattr(&path, &stat, STATX_MTIME | STATX_CTIME, AT_STATX_SYNC_AS_STAT)
to get the really real inode times also for directory.

But I think that would be considered a hack and the proper way to solve this
would be to introduce a new dentry op ->d_real_inode().
Unlike d_real_inode() which returns the inode used for open,
->d_real_inode() would return the inode used for stat.

If the helper d_real_inode() would call the new op,
the only other user of d_real_inode() (check_conflicting_open) would have to
open code inode_is_open_for_write(d_real(dentry)->d_inode)

>                 if (unlikely(inode != realinode) &&
>                     (!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
>                      !timespec_equal(&inode->i_ctime, &realinode->i_ctime))) {
>                         inode->i_mtime = realinode->i_mtime;
>                         inode->i_ctime = realinode->i_ctime;
>                 }
>         }
> }
>
> 2. Chattr will modify lower file's attributes directly.
> Reproduce:
> # mkdir lower upper worker merger
> # touch lower/aa
> # lsattr -p lower/aa
>     0 --------------e---- lower/aa
> # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> # chattr -p 123 merger/aa             #set project id
> # lsattr -p lower/aa
>   123 --------------e---- lower/aa
>
> If we try to set "immutable" or any other attributes, the result are consistent.
> Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
> FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.

Ouch! I guess it's a "known to some" issue.
Fixing this would be a pain (intercept ioctl and whitelisting readonly
fs specific ioctls).
Hopefully, at least sepolicy would prevent these unauthorized changes
to lower if
selinux is used??

Amir.

Re: two questiones about overlayfs

[Vivek Goyal]

On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:

[CC Dan Walsh]

[..]
> > 2. Chattr will modify lower file's attributes directly.
> > Reproduce:
> > # mkdir lower upper worker merger
> > # touch lower/aa
> > # lsattr -p lower/aa
> >     0 --------------e---- lower/aa
> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> > # chattr -p 123 merger/aa             #set project id
> > # lsattr -p lower/aa
> >   123 --------------e---- lower/aa
> >
> > If we try to set "immutable" or any other attributes, the result are consistent.
> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
> 
> Ouch! I guess it's a "known to some" issue.
> Fixing this would be a pain (intercept ioctl and whitelisting readonly
> fs specific ioctls).
> Hopefully, at least sepolicy would prevent these unauthorized changes
> to lower if
> selinux is used??

I tried it in a docker container on Fedora 26 host and selinux seemed to
block it.

[root@947c53c7d69a bin]# lsattr -p zcat
    0 ------------------- zcat

[root@947c53c7d69a bin]# chattr -p 123 zcat
chattr: Permission denied while setting flags on zcat

Aug 08 16:15:37  audit[24865]: AVC avc:  denied  { setattr } for
pid=24865 comm="chattr" path="/usr/bin/zcat" dev="dm-0" ino=17358919
scontext=system_u:system_r:container_t:s0:c403,c847
tcontext=system_u:object_r:container_share_t:s0 tclass=file permissive=0

But this should probably be properly fixed.

Vivek

Re: two questiones about overlayfs

[Daniel Walsh]

On 08/08/2017 04:21 PM, Vivek Goyal wrote:
> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
>
> [CC Dan Walsh]
>
> [..]
>>> 2. Chattr will modify lower file's attributes directly.
>>> Reproduce:
>>> # mkdir lower upper worker merger
>>> # touch lower/aa
>>> # lsattr -p lower/aa
>>>      0 --------------e---- lower/aa
>>> # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>>> # chattr -p 123 merger/aa             #set project id
>>> # lsattr -p lower/aa
>>>    123 --------------e---- lower/aa
>>>
>>> If we try to set "immutable" or any other attributes, the result are consistent.
>>> Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
>>> FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
>> Ouch! I guess it's a "known to some" issue.
>> Fixing this would be a pain (intercept ioctl and whitelisting readonly
>> fs specific ioctls).
>> Hopefully, at least sepolicy would prevent these unauthorized changes
>> to lower if
>> selinux is used??
> I tried it in a docker container on Fedora 26 host and selinux seemed to
> block it.
>
> [root@947c53c7d69a bin]# lsattr -p zcat
>      0 ------------------- zcat
>
> [root@947c53c7d69a bin]# chattr -p 123 zcat
> chattr: Permission denied while setting flags on zcat
>
> Aug 08 16:15:37  audit[24865]: AVC avc:  denied  { setattr } for
> pid=24865 comm="chattr" path="/usr/bin/zcat" dev="dm-0" ino=17358919
> scontext=system_u:system_r:container_t:s0:c403,c847
> tcontext=system_u:object_r:container_share_t:s0 tclass=file permissive=0
>
> But this should probably be properly fixed.
>
> Vivek

Yes SELinux will block all changes to the lower level.

Re: two questiones about overlayfs

[Miklos Szeredi]

On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> > Hi,all:
> > I found two problemes about overlayfs, please let me know what you think:
> >
> > 1. Cannot update upper dir's access time.
> > Reproduce:
> > # mkdir lower upper worker merger
> > # mkdir upper/dir
> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> > # ls mrger/dir
> > # stat mrger/dir
> >   Access: 2017-08-07 11:03:56.364771584 -0400
> >   Modify: 2017-08-07 11:01:52.319771584 -0400
> >   Change: 2017-08-07 11:01:52.319771584 -0400
> > # touch mrger/dir/aa
> > # stat mrger/dir
> >   Access: 2017-08-07 11:03:56.364771584 -0400
> >   Modify: 2017-08-07 11:05:33.969771584 -0400
> >   Change: 2017-08-07 11:05:33.969771584 -0400
> > # ls mrger/dir
> > # stat mrger/dir
> >   Access: 2017-08-07 11:03:56.364771584 -0400
> >   Modify: 2017-08-07 11:05:33.969771584 -0400
> >   Change: 2017-08-07 11:05:33.969771584 -0400
> >
> > I find the reason of this problem is in update_ovl_inode_times():
> > (598e3c8f7 "vfs: update ovl inode before relatime check")
> >
> > static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
> >                                bool rcu)
> > {
> >         if (!rcu) {
> >                 struct inode *realinode = d_real_inode(dentry);
> >
> >         *d_real_inode() cannot get dir's real inode, so i_mtime always equal to i_ctime*
> >         *an not updated*
> >
> 
> Technically, I think this could be changed to check
> if unlikely(dentry->d_flags & DCACHE_OP_REAL) and call
> vfs_getattr(&path, &stat, STATX_MTIME | STATX_CTIME, AT_STATX_SYNC_AS_STAT)
> to get the really real inode times also for directory.
> 
> But I think that would be considered a hack and the proper way to solve this
> would be to introduce a new dentry op ->d_real_inode().
> Unlike d_real_inode() which returns the inode used for open,
> ->d_real_inode() would return the inode used for stat.
> 
> If the helper d_real_inode() would call the new op,
> the only other user of d_real_inode() (check_conflicting_open) would have to
> open code inode_is_open_for_write(d_real(dentry)->d_inode)

Better just use the already existing ->d_real() with a special flags value to
indicate we want the real dentry for stat, not for open.

Something like this:

diff --git a/fs/inode.c b/fs/inode.c
index 50370599e371..8e526c7a1173 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1570,7 +1570,7 @@ static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
 			       bool rcu)
 {
 	if (!rcu) {
-		struct inode *realinode = d_real_inode(dentry);
+		struct inode *realinode = d_inode(d_real(dentry, NULL, -1));
 
 		if (unlikely(inode != realinode) &&
 		    (!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d86e89f97201..792dc043f7b4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -76,6 +76,10 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	int err;
 
 	if (!d_is_reg(dentry)) {
+		/* Special case -1 to mean query for stat not for open */
+		BUILD_BUG_ON((unsigned int) -1 ==  VALID_OPEN_FLAGS);
+		if (open_flags == (unsigned int) -1)
+			return ovl_dentry_real(dentry);
 		if (!inode || inode == d_inode(dentry))
 			return dentry;
 		goto bug;

Re: two questiones about overlayfs

[Miklos Szeredi]

On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:

[snip]

> > 2. Chattr will modify lower file's attributes directly.
> > Reproduce:
> > # mkdir lower upper worker merger
> > # touch lower/aa
> > # lsattr -p lower/aa
> >     0 --------------e---- lower/aa
> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> > # chattr -p 123 merger/aa             #set project id
> > # lsattr -p lower/aa
> >   123 --------------e---- lower/aa
> >
> > If we try to set "immutable" or any other attributes, the result are consistent.
> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
> 
> Ouch! I guess it's a "known to some" issue.
> Fixing this would be a pain (intercept ioctl and whitelisting readonly
> fs specific ioctls).

Fixing ioctl properly would be a pain.  But we can hack around the issue, and
just deny it for now.

See patch below

(Side note: probably better just add another argument to ->d_real() instead of
trying to cram everyting into the open_flags arg).

Thanks,
Miklos

diff --git a/fs/internal.h b/fs/internal.h
index 9676fe11c093..60cdbcd2887b 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -71,8 +71,10 @@ extern void __init mnt_init(void);
 
 extern int __mnt_want_write(struct vfsmount *);
 extern int __mnt_want_write_file(struct file *);
+extern int mnt_want_write_file_path(struct file *);
 extern void __mnt_drop_write(struct vfsmount *);
 extern void __mnt_drop_write_file(struct file *);
+extern void mnt_drop_write_file_path(struct file *);
 
 /*
  * fs_struct.c
diff --git a/fs/namespace.c b/fs/namespace.c
index f8893dc6a989..22a492332ec4 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -431,13 +431,18 @@ int __mnt_want_write_file(struct file *file)
 }
 
 /**
- * mnt_want_write_file - get write access to a file's mount
+ * mnt_want_write_file_path - get write access to a file's mount
  * @file: the file who's mount on which to take a write
  *
  * This is like mnt_want_write, but it takes a file and can
  * do some optimisations if the file is open for write already
+ *
+ * Called by the vfs for cases when we have an open file at hand, but will do an
+ * inode operation on it (important distinction for files opened on overlayfs,
+ * since the file operations will come from the real underlying file, while
+ * inode operations come from the overlay).
  */
-int mnt_want_write_file(struct file *file)
+int mnt_want_write_file_path(struct file *file)
 {
 	int ret;
 
@@ -447,6 +452,53 @@ int mnt_want_write_file(struct file *file)
 		sb_end_write(file->f_path.mnt->mnt_sb);
 	return ret;
 }
+
+static inline int may_write_real(struct file *file)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct dentry *upperdentry;
+
+	/* Writable file? */
+	if (file->f_mode & FMODE_WRITER)
+		return 0;
+
+	/* Not overlayfs? */
+	if (likely(!(dentry->d_flags & DCACHE_OP_REAL)))
+		return 0;
+
+	/* File refers to upper, writable layer? */
+	upperdentry = d_real(dentry, NULL, -2);
+	if (upperdentry && file_inode(file) == d_inode(upperdentry))
+		return 0;
+
+	/* Lower layer: can't write to real file, sorry... */
+	return -EINVAL;
+}
+
+/**
+ * mnt_want_write_file - get write access to a file's mount
+ * @file: the file who's mount on which to take a write
+ *
+ * This is like mnt_want_write, but it takes a file and can
+ * do some optimisations if the file is open for write already
+ *
+ * Mostly called by filesystems from their ioctl operation before performing
+ * modification.  On overlayfs this needs to check if the file is on a read-only
+ * lower layer and deny access in that case.
+ */
+int mnt_want_write_file(struct file *file)
+{
+	int ret;
+
+	ret = may_write_real(file);
+	if (!ret) {
+		sb_start_write(file_inode(file)->i_sb);
+		ret = __mnt_want_write_file(file);
+		if (ret)
+			sb_end_write(file_inode(file)->i_sb);
+	}
+	return ret;
+}
 EXPORT_SYMBOL_GPL(mnt_want_write_file);
 
 /**
@@ -484,10 +536,16 @@ void __mnt_drop_write_file(struct file *file)
 	__mnt_drop_write(file->f_path.mnt);
 }
 
-void mnt_drop_write_file(struct file *file)
+void mnt_drop_write_file_path(struct file *file)
 {
 	mnt_drop_write(file->f_path.mnt);
 }
+
+void mnt_drop_write_file(struct file *file)
+{
+	__mnt_drop_write(file->f_path.mnt);
+	sb_end_write(file_inode(file)->i_sb);
+}
 EXPORT_SYMBOL(mnt_drop_write_file);
 
 static int mnt_make_readonly(struct mount *mnt)
diff --git a/fs/open.c b/fs/open.c
index 35bb784763a4..c53a2315df98 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -670,12 +670,12 @@ SYSCALL_DEFINE3(fchown, unsigned int, fd, uid_t, user, gid_t, group)
 	if (!f.file)
 		goto out;
 
-	error = mnt_want_write_file(f.file);
+	error = mnt_want_write_file_path(f.file);
 	if (error)
 		goto out_fput;
 	audit_file(f.file);
 	error = chown_common(&f.file->f_path, user, group);
-	mnt_drop_write_file(f.file);
+	mnt_drop_write_file_path(f.file);
 out_fput:
 	fdput(f);
 out:
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d86e89f97201..95e1271cc90f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -75,6 +75,10 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	struct dentry *real;
 	int err;
 
+	BUILD_BUG_ON(!((unsigned int) -2 & ~VALID_OPEN_FLAGS));
+	if (open_flags == (unsigned int) -2)
+		return ovl_dentry_upper(dentry);
+
 	if (!d_is_reg(dentry)) {
 		if (!inode || inode == d_inode(dentry))
 			return dentry;
diff --git a/fs/xattr.c b/fs/xattr.c
index 464c94bf65f9..d7c2cf7817bb 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -23,6 +23,7 @@
 #include <linux/posix_acl_xattr.h>
 
 #include <linux/uaccess.h>
+#include "internal.h"
 
 static const char *
 strcmp_prefix(const char *a, const char *a_prefix)
@@ -496,10 +497,10 @@ SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
 	if (!f.file)
 		return error;
 	audit_file(f.file);
-	error = mnt_want_write_file(f.file);
+	error = mnt_want_write_file_path(f.file);
 	if (!error) {
 		error = setxattr(f.file->f_path.dentry, name, value, size, flags);
-		mnt_drop_write_file(f.file);
+		mnt_drop_write_file_path(f.file);
 	}
 	fdput(f);
 	return error;
@@ -728,10 +729,10 @@ SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
 	if (!f.file)
 		return error;
 	audit_file(f.file);
-	error = mnt_want_write_file(f.file);
+	error = mnt_want_write_file_path(f.file);
 	if (!error) {
 		error = removexattr(f.file->f_path.dentry, name);
-		mnt_drop_write_file(f.file);
+		mnt_drop_write_file_path(f.file);
 	}
 	fdput(f);
 	return error;

Re: two questiones about overlayfs

[Amir Goldstein]

On Tue, Aug 15, 2017 at 3:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
>> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> [snip]
>
>> > 2. Chattr will modify lower file's attributes directly.
>> > Reproduce:
>> > # mkdir lower upper worker merger
>> > # touch lower/aa
>> > # lsattr -p lower/aa
>> >     0 --------------e---- lower/aa
>> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>> > # chattr -p 123 merger/aa             #set project id
>> > # lsattr -p lower/aa
>> >   123 --------------e---- lower/aa
>> >
>> > If we try to set "immutable" or any other attributes, the result are consistent.
>> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
>> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
>>
>> Ouch! I guess it's a "known to some" issue.
>> Fixing this would be a pain (intercept ioctl and whitelisting readonly
>> fs specific ioctls).
>
> Fixing ioctl properly would be a pain.  But we can hack around the issue, and
> just deny it for now.
>
> See patch below

I like this, but it will require good test coverage of fs specific ioctls.
The list of filesystems that call  mnt_want_write_file() for ioctl is not short.

>
> (Side note: probably better just add another argument to ->d_real() instead of
> trying to cram everyting into the open_flags arg).
>

I was going to say that the -1 flags hack is not pretty and suggest O_REAL_INODE
open flag. With the addition of -2 I think another interenal_flags
argument is in order,
which will also replace the implicit modes of d_real() with explicit modes:
D_REAL_COPYUP, D_REAL_MATCH_INODE, D_REAL_RDONLY and now also
D_REAL_STAT and D_REAL_UPPER.

This patch makes me wonder about O_PATH.
It looks strange that f->f_inode and f->f_mapping are being set at all
for files open
with O_PATH. Indeed file_inode(f) could sometimes be used for files
open with O_PATH
(e.g. fchdir() -> inode_permission() ), but it may make more sense to
use a helper
file_path_inode(f.file) (same as locks_inode()) for those cases
instead of relying on
f->f_inode being set. No?

Amir.

Re: two questiones about overlayfs

[Miklos Szeredi]

On Tue, Aug 15, 2017 at 4:52 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Aug 15, 2017 at 3:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
>>> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>
>> [snip]
>>
>>> > 2. Chattr will modify lower file's attributes directly.
>>> > Reproduce:
>>> > # mkdir lower upper worker merger
>>> > # touch lower/aa
>>> > # lsattr -p lower/aa
>>> >     0 --------------e---- lower/aa
>>> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>>> > # chattr -p 123 merger/aa             #set project id
>>> > # lsattr -p lower/aa
>>> >   123 --------------e---- lower/aa
>>> >
>>> > If we try to set "immutable" or any other attributes, the result are consistent.
>>> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
>>> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
>>>
>>> Ouch! I guess it's a "known to some" issue.
>>> Fixing this would be a pain (intercept ioctl and whitelisting readonly
>>> fs specific ioctls).
>>
>> Fixing ioctl properly would be a pain.  But we can hack around the issue, and
>> just deny it for now.
>>
>> See patch below
>
> I like this, but it will require good test coverage of fs specific ioctls.
> The list of filesystems that call  mnt_want_write_file() for ioctl is not short.

If it's called from within the filesystem, then the new behavior is
certainly the correct one.  IOW filesystems that can be part of an
overlay should never look at f_path.dentry directly, hence they will
not use mnt_want_write_file() in the sense that fsetxattr() and
fchown() do.

For non-overlayfs uses the old and new should be equivalent.

>
>>
>> (Side note: probably better just add another argument to ->d_real() instead of
>> trying to cram everyting into the open_flags arg).
>>
>
> I was going to say that the -1 flags hack is not pretty and suggest O_REAL_INODE
> open flag. With the addition of -2 I think another interenal_flags
> argument is in order,
> which will also replace the implicit modes of d_real() with explicit modes:
> D_REAL_COPYUP, D_REAL_MATCH_INODE, D_REAL_RDONLY and now also
> D_REAL_STAT and D_REAL_UPPER.
>
> This patch makes me wonder about O_PATH.
> It looks strange that f->f_inode and f->f_mapping are being set at all
> for files open
> with O_PATH. Indeed file_inode(f) could sometimes be used for files
> open with O_PATH
> (e.g. fchdir() -> inode_permission() ), but it may make more sense to
> use a helper
> file_path_inode(f.file) (same as locks_inode()) for those cases
> instead of relying on
> f->f_inode being set. No?

I understand your position, but with f_inode and f_mapping left NULL,
we are setting up ourselves for oopsing bugs, instead of much more
benign bugs (or non-bugs).

Thanks,
Miklos

Re: two questiones about overlayfs

[Amir Goldstein]

On Tue, Aug 15, 2017 at 5:06 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Aug 15, 2017 at 4:52 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, Aug 15, 2017 at 3:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
>>>> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>
>>> [snip]
>>>
>>>> > 2. Chattr will modify lower file's attributes directly.
>>>> > Reproduce:
>>>> > # mkdir lower upper worker merger
>>>> > # touch lower/aa
>>>> > # lsattr -p lower/aa
>>>> >     0 --------------e---- lower/aa
>>>> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>>>> > # chattr -p 123 merger/aa             #set project id
>>>> > # lsattr -p lower/aa
>>>> >   123 --------------e---- lower/aa
>>>> >
>>>> > If we try to set "immutable" or any other attributes, the result are consistent.
>>>> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
>>>> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
>>>>
>>>> Ouch! I guess it's a "known to some" issue.
>>>> Fixing this would be a pain (intercept ioctl and whitelisting readonly
>>>> fs specific ioctls).
>>>
>>> Fixing ioctl properly would be a pain.  But we can hack around the issue, and
>>> just deny it for now.
>>>
>>> See patch below
>>
>> I like this, but it will require good test coverage of fs specific ioctls.
>> The list of filesystems that call  mnt_want_write_file() for ioctl is not short.
>
> If it's called from within the filesystem, then the new behavior is
> certainly the correct one.

It certainly is. It doesn't mean that fixing incorrect behavior won't
lead to unacceptable regressions, which may require explicit
d_real() call from filesystem to be fixed.

Side note: IMO may_write_real() should return -EPERM instead
of -EINVAL, same behavior as IS_IMMUTABLE, i.e. there is nothing
invalid about the parameters of the SETFLAGS ioctl as far as the user
is concerned. You could also go with -EROFS, which makes more sence,
but that may be a bit more surprising to user.

Amir.

Re: two questiones about overlayfs

[Miklos Szeredi]

On Tue, Aug 15, 2017 at 5:28 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Aug 15, 2017 at 5:06 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Aug 15, 2017 at 4:52 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Tue, Aug 15, 2017 at 3:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
>>>>> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>>>>
>>>> [snip]
>>>>
>>>>> > 2. Chattr will modify lower file's attributes directly.
>>>>> > Reproduce:
>>>>> > # mkdir lower upper worker merger
>>>>> > # touch lower/aa
>>>>> > # lsattr -p lower/aa
>>>>> >     0 --------------e---- lower/aa
>>>>> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>>>>> > # chattr -p 123 merger/aa             #set project id
>>>>> > # lsattr -p lower/aa
>>>>> >   123 --------------e---- lower/aa
>>>>> >
>>>>> > If we try to set "immutable" or any other attributes, the result are consistent.
>>>>> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
>>>>> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
>>>>>
>>>>> Ouch! I guess it's a "known to some" issue.
>>>>> Fixing this would be a pain (intercept ioctl and whitelisting readonly
>>>>> fs specific ioctls).
>>>>
>>>> Fixing ioctl properly would be a pain.  But we can hack around the issue, and
>>>> just deny it for now.
>>>>
>>>> See patch below
>>>
>>> I like this, but it will require good test coverage of fs specific ioctls.
>>> The list of filesystems that call  mnt_want_write_file() for ioctl is not short.
>>
>> If it's called from within the filesystem, then the new behavior is
>> certainly the correct one.
>
> It certainly is. It doesn't mean that fixing incorrect behavior won't
> lead to unacceptable regressions, which may require explicit
> d_real() call from filesystem to be fixed.

I don't get it.  The only possible regression is denying modification
on lower layer where previously was allowed.  But anybody relying on
that would be pretty crazy.

>
> Side note: IMO may_write_real() should return -EPERM instead
> of -EINVAL, same behavior as IS_IMMUTABLE, i.e. there is nothing
> invalid about the parameters of the SETFLAGS ioctl as far as the user
> is concerned. You could also go with -EROFS, which makes more sence,
> but that may be a bit more surprising to user.

Yeah, EPERM sounds better.

Thanks,
Miklos

Re: two questiones about overlayfs

[Amir Goldstein]

On Tue, Aug 15, 2017 at 5:33 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Aug 15, 2017 at 5:28 PM, Amir Goldstein <amir73il@gmail.com> wrote:

>> It certainly is. It doesn't mean that fixing incorrect behavior won't
>> lead to unacceptable regressions, which may require explicit
>> d_real() call from filesystem to be fixed.
>
> I don't get it.  The only possible regression is denying modification
> on lower layer where previously was allowed.  But anybody relying on
> that would be pretty crazy.
>

Agreed that is pretty crazy, but consider:
_strong_open_rdwr:
- remove immutable flag
- open file RDWR

That would work on current kernels in spite of possibly changing lower
immutable flag, but may break with this patch.
If this is done by a sufficiently common app, fixing the breakage may require
some explicit copy up somewhere...
Note that the app is not crazy to require changing lower, the app is perfectly
sane to require being able to remove the immutable flag before trying
to open rdrw.

But I guess the only way to know is make the change and wait for the reports.

Amir.

Re: two questiones about overlayfs

[Vivek Goyal]

On Tue, Aug 15, 2017 at 05:33:01PM +0200, Miklos Szeredi wrote:
> On Tue, Aug 15, 2017 at 5:28 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Tue, Aug 15, 2017 at 5:06 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >> On Tue, Aug 15, 2017 at 4:52 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> >>> On Tue, Aug 15, 2017 at 3:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >>>> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
> >>>>> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> >>>>
> >>>> [snip]
> >>>>
> >>>>> > 2. Chattr will modify lower file's attributes directly.
> >>>>> > Reproduce:
> >>>>> > # mkdir lower upper worker merger
> >>>>> > # touch lower/aa
> >>>>> > # lsattr -p lower/aa
> >>>>> >     0 --------------e---- lower/aa
> >>>>> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> >>>>> > # chattr -p 123 merger/aa             #set project id
> >>>>> > # lsattr -p lower/aa
> >>>>> >   123 --------------e---- lower/aa
> >>>>> >
> >>>>> > If we try to set "immutable" or any other attributes, the result are consistent.
> >>>>> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
> >>>>> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
> >>>>>
> >>>>> Ouch! I guess it's a "known to some" issue.
> >>>>> Fixing this would be a pain (intercept ioctl and whitelisting readonly
> >>>>> fs specific ioctls).
> >>>>
> >>>> Fixing ioctl properly would be a pain.  But we can hack around the issue, and
> >>>> just deny it for now.
> >>>>
> >>>> See patch below
> >>>
> >>> I like this, but it will require good test coverage of fs specific ioctls.
> >>> The list of filesystems that call  mnt_want_write_file() for ioctl is not short.
> >>
> >> If it's called from within the filesystem, then the new behavior is
> >> certainly the correct one.
> >
> > It certainly is. It doesn't mean that fixing incorrect behavior won't
> > lead to unacceptable regressions, which may require explicit
> > d_real() call from filesystem to be fixed.
> 
> I don't get it.  The only possible regression is denying modification
> on lower layer where previously was allowed.  But anybody relying on
> that would be pretty crazy.

Hi Miklos,

IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not
been copied up yet).

IOW, on overlayfs, will it be responsibility of user space to make
sure file has been copied up, for chattr operation to succeed? Does that
mean we need to modify chattr to open file for WRITE instead of READ.

Vivek

Re: two questiones about overlayfs

[Amir Goldstein]

On Tue, Aug 15, 2017 at 5:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Aug 15, 2017 at 05:33:01PM +0200, Miklos Szeredi wrote:
>> On Tue, Aug 15, 2017 at 5:28 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> > On Tue, Aug 15, 2017 at 5:06 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> >> On Tue, Aug 15, 2017 at 4:52 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> >>> On Tue, Aug 15, 2017 at 3:35 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> >>>> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote:
>> >>>>> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> >>>>
>> >>>> [snip]
>> >>>>
>> >>>>> > 2. Chattr will modify lower file's attributes directly.
>> >>>>> > Reproduce:
>> >>>>> > # mkdir lower upper worker merger
>> >>>>> > # touch lower/aa
>> >>>>> > # lsattr -p lower/aa
>> >>>>> >     0 --------------e---- lower/aa
>> >>>>> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>> >>>>> > # chattr -p 123 merger/aa             #set project id
>> >>>>> > # lsattr -p lower/aa
>> >>>>> >   123 --------------e---- lower/aa
>> >>>>> >
>> >>>>> > If we try to set "immutable" or any other attributes, the result are consistent.
>> >>>>> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
>> >>>>> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it.
>> >>>>>
>> >>>>> Ouch! I guess it's a "known to some" issue.
>> >>>>> Fixing this would be a pain (intercept ioctl and whitelisting readonly
>> >>>>> fs specific ioctls).
>> >>>>
>> >>>> Fixing ioctl properly would be a pain.  But we can hack around the issue, and
>> >>>> just deny it for now.
>> >>>>
>> >>>> See patch below
>> >>>
>> >>> I like this, but it will require good test coverage of fs specific ioctls.
>> >>> The list of filesystems that call  mnt_want_write_file() for ioctl is not short.
>> >>
>> >> If it's called from within the filesystem, then the new behavior is
>> >> certainly the correct one.
>> >
>> > It certainly is. It doesn't mean that fixing incorrect behavior won't
>> > lead to unacceptable regressions, which may require explicit
>> > d_real() call from filesystem to be fixed.
>>
>> I don't get it.  The only possible regression is denying modification
>> on lower layer where previously was allowed.  But anybody relying on
>> that would be pretty crazy.
>
> Hi Miklos,
>
> IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not
> been copied up yet).
>

Yap.

> IOW, on overlayfs, will it be responsibility of user space to make
> sure file has been copied up, for chattr operation to succeed? Does that
> mean we need to modify chattr to open file for WRITE instead of READ.
>

I guess that would make sense.
I only wonder what was the reason for chattr to open RDONLY in
the first place (cc Ted)??

Amir.

Re: two questiones about overlayfs

[Miklos Szeredi]

On Tue, Aug 15, 2017 at 6:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Aug 15, 2017 at 5:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:

>> IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not
>> been copied up yet).
>>
>
> Yap.
>
>> IOW, on overlayfs, will it be responsibility of user space to make
>> sure file has been copied up, for chattr operation to succeed? Does that
>> mean we need to modify chattr to open file for WRITE instead of READ.
>>
>
> I guess that would make sense.
> I only wonder what was the reason for chattr to open RDONLY in
> the first place (cc Ted)??

What about copy up of flags?  Should we?  Does reflink copy the flags?

Thanks,
Miklos

Re: two questiones about overlayfs

[Miklos Szeredi]

On Wed, Aug 16, 2017 at 12:19 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Aug 15, 2017 at 6:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, Aug 15, 2017 at 5:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>
>>> IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not
>>> been copied up yet).
>>>
>>
>> Yap.
>>
>>> IOW, on overlayfs, will it be responsibility of user space to make
>>> sure file has been copied up, for chattr operation to succeed? Does that
>>> mean we need to modify chattr to open file for WRITE instead of READ.
>>>
>>
>> I guess that would make sense.
>> I only wonder what was the reason for chattr to open RDONLY in
>> the first place (cc Ted)??
>
> What about copy up of flags?  Should we?  Does reflink copy the flags?

That last one is a stupid question, reflink is a data copying
operation, not an inode copying one...

Thanks,
Miklos

Re: two questiones about overlayfs

[Amir Goldstein]

On Wed, Aug 16, 2017 at 12:19 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Tue, Aug 15, 2017 at 6:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Tue, Aug 15, 2017 at 5:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>
>>> IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not
>>> been copied up yet).
>>>
>>
>> Yap.
>>
>>> IOW, on overlayfs, will it be responsibility of user space to make
>>> sure file has been copied up, for chattr operation to succeed? Does that
>>> mean we need to modify chattr to open file for WRITE instead of READ.
>>>
>>
>> I guess that would make sense.
>> I only wonder what was the reason for chattr to open RDONLY in
>> the first place (cc Ted)??
>
> What about copy up of flags?  Should we?

IMO we should start with copying KSTAT_ATTR_FS_IOC_FLAGS,
which can already be available in kstat.
Other "standard" FS_IOC_FLAGS can be copied up when statx
gets support for those flags.

FYI, chattr [+/-]<flags> does:
- fgetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_GETFLAGS}
- change flags
- fsetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_SETFLAGS}

OPEN_FLAGS is defined independently in lib/e2p/f{get,set}flags.c as
(O_RDONLY|O_NONBLOCK|O_LARGEFILE), so technically all it
takes is to change the defined in  lib/e2p/fsetflags.c to O_RDWR
and then chattr [+/-]<flags> can be used for preemptive "copy up flags"
by userspace (*) to work around issues which require flags copy up.

(*) At least with ext4 chattr would only "copy up" the
EXT4_FL_USER_MODIFIABLE flags, which is probably for the best.

Amir.

Re: two questiones about overlayfs

[Vivek Goyal]

On Wed, Aug 16, 2017 at 12:19:22PM +0200, Miklos Szeredi wrote:
> On Tue, Aug 15, 2017 at 6:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Tue, Aug 15, 2017 at 5:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> >> IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not
> >> been copied up yet).
> >>
> >
> > Yap.
> >
> >> IOW, on overlayfs, will it be responsibility of user space to make
> >> sure file has been copied up, for chattr operation to succeed? Does that
> >> mean we need to modify chattr to open file for WRITE instead of READ.
> >>
> >
> > I guess that would make sense.
> > I only wonder what was the reason for chattr to open RDONLY in
> > the first place (cc Ted)??
> 
> What about copy up of flags?  Should we?  Does reflink copy the flags?

Oh yes, copy up of flags seem to be an issue too. I have a file on lower
with project id 123 and once that file gets copied up, project id goes
back to 0.

[merged]# lsattr -p foo.txt 
  123 ------------------- foo.txt

[merged]# touch foo.txt

[merged]# lsattr -p foo.txt 
    0 ------------------- foo.txt

Vivek

Re: two questiones about overlayfs

[Amir Goldstein]

On Wed, Aug 16, 2017 at 3:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Aug 16, 2017 at 12:19:22PM +0200, Miklos Szeredi wrote:
...
>>
>> What about copy up of flags?  Should we?  Does reflink copy the flags?
>
> Oh yes, copy up of flags seem to be an issue too. I have a file on lower
> with project id 123 and once that file gets copied up, project id goes
> back to 0.
>
> [merged]# lsattr -p foo.txt
>   123 ------------------- foo.txt
>
> [merged]# touch foo.txt
>
> [merged]# lsattr -p foo.txt
>     0 ------------------- foo.txt
>

Vivek,

This specific case I would tag as "desired behavior".
This is the behavior that allowed us to implement container storage quota
for docker using overlayfs + project quota set on overlay upper dir.
Copy up file inherits its project quota id from upper dir recursively, which
at least for docker use case is "desired behavior"

IMO, for the KSTAT_ATTR_FS_IOC_FLAGS use case, copy up flags
(compressed, encrypted, immutable, append, nodump) all make sense,
although immutable/append should be set after copying data while
encrypted/compressed should probably be set before copying data??

Amir.

Re: two questiones about overlayfs

[Vivek Goyal]

On Wed, Aug 16, 2017 at 06:12:57PM +0200, Amir Goldstein wrote:
> On Wed, Aug 16, 2017 at 3:52 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Aug 16, 2017 at 12:19:22PM +0200, Miklos Szeredi wrote:
> ...
> >>
> >> What about copy up of flags?  Should we?  Does reflink copy the flags?
> >
> > Oh yes, copy up of flags seem to be an issue too. I have a file on lower
> > with project id 123 and once that file gets copied up, project id goes
> > back to 0.
> >
> > [merged]# lsattr -p foo.txt
> >   123 ------------------- foo.txt
> >
> > [merged]# touch foo.txt
> >
> > [merged]# lsattr -p foo.txt
> >     0 ------------------- foo.txt
> >
> 
> Vivek,
> 
> This specific case I would tag as "desired behavior".
> This is the behavior that allowed us to implement container storage quota
> for docker using overlayfs + project quota set on overlay upper dir.
> Copy up file inherits its project quota id from upper dir recursively, which
> at least for docker use case is "desired behavior"

Hi Amir,

I am not quota expert but it kind of makes sense to me. So when a file is
copied up it is a newly created file which will inherit its project id from its
parent directory (or ancestor), instead of trying to retain project id
from lower/.

> 
> IMO, for the KSTAT_ATTR_FS_IOC_FLAGS use case, copy up flags
> (compressed, encrypted, immutable, append, nodump) all make sense,
> although immutable/append should be set after copying data while
> encrypted/compressed should probably be set before copying data??

Sounds reasonable to me.

Vivek

Re: two questiones about overlayfs

[zhangyi (F)]

On 2017/8/16 19:10, Amir Goldstein Wrote:
> On Wed, Aug 16, 2017 at 12:19 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Tue, Aug 15, 2017 at 6:16 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Tue, Aug 15, 2017 at 5:56 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>>>> IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not
>>>> been copied up yet).
>>>>
>>>
>>> Yap.
>>>
>>>> IOW, on overlayfs, will it be responsibility of user space to make
>>>> sure file has been copied up, for chattr operation to succeed? Does that
>>>> mean we need to modify chattr to open file for WRITE instead of READ.
>>>>
>>>
>>> I guess that would make sense.
>>> I only wonder what was the reason for chattr to open RDONLY in
>>> the first place (cc Ted)??
>>
>> What about copy up of flags?  Should we?
> 
> IMO we should start with copying KSTAT_ATTR_FS_IOC_FLAGS,
> which can already be available in kstat.
> Other "standard" FS_IOC_FLAGS can be copied up when statx
> gets support for those flags.
> 
> FYI, chattr [+/-]<flags> does:
> - fgetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_GETFLAGS}
> - change flags
> - fsetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_SETFLAGS}
> 
> OPEN_FLAGS is defined independently in lib/e2p/f{get,set}flags.c as
> (O_RDONLY|O_NONBLOCK|O_LARGEFILE), so technically all it
> takes is to change the defined in  lib/e2p/fsetflags.c to O_RDWR
> and then chattr [+/-]<flags> can be used for preemptive "copy up flags"
> by userspace (*) to work around issues which require flags copy up.
> 

Hi Amir:
I notice that RDONLY is required if file is immutable or append-only,
chattr will fail with EPERM return value if we use O_RDWR open flag to open
a immutable or append-only file. So I think we cannot simply change
to O_RDWR in lib/e2p/fsetflags.c, we should take care about this special,
please see check in may_open() and __inode_permission().

Thanks,
Zhangyi.

Re: two questiones about overlayfs

[Amir Goldstein]

On Thu, Aug 17, 2017 at 4:55 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/8/16 19:10, Amir Goldstein Wrote:
...
>>>> I guess that would make sense.
>>>> I only wonder what was the reason for chattr to open RDONLY in
>>>> the first place (cc Ted)??
>>>
>>> What about copy up of flags?  Should we?
>>
>> IMO we should start with copying KSTAT_ATTR_FS_IOC_FLAGS,
>> which can already be available in kstat.
>> Other "standard" FS_IOC_FLAGS can be copied up when statx
>> gets support for those flags.
>>
>> FYI, chattr [+/-]<flags> does:
>> - fgetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_GETFLAGS}
>> - change flags
>> - fsetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_SETFLAGS}
>>
>> OPEN_FLAGS is defined independently in lib/e2p/f{get,set}flags.c as
>> (O_RDONLY|O_NONBLOCK|O_LARGEFILE), so technically all it
>> takes is to change the defined in  lib/e2p/fsetflags.c to O_RDWR
>> and then chattr [+/-]<flags> can be used for preemptive "copy up flags"
>> by userspace (*) to work around issues which require flags copy up.
>>
>
> Hi Amir:
> I notice that RDONLY is required if file is immutable or append-only,
> chattr will fail with EPERM return value if we use O_RDWR open flag to open
> a immutable or append-only file. So I think we cannot simply change
> to O_RDWR in lib/e2p/fsetflags.c, we should take care about this special,
> please see check in may_open() and __inode_permission().
>

Zhangyi,

You are right about the fact that we cannot "just change" OPEN_FLAGS
to O_RDWR, because it would return EPERM for non-overlayfs case and
also in some cases for overlayfs upper file.
However, the workaround for userspace to open O_RDWR to trigger
copy up before chattr should still work because may_open() has no
knowledge of the real inode flags (open may fail but still copy up). See
commit b0990fbbbd14 ("ovl: check IS_APPEND() on real upper inode")
Note that this commit does NOT prevent opening a lower file
O_RDWR even if that file is IS_APPEND or IS_IMMUTABLE
as mentioned in the commit message and that the reason this
commit only speaks about IS_APPEND() is that IS_MMUTABLE
is also checked later with inode_permission(MAY_WRITE).

(*) Like my answer before, this is all in theory, so I may be missing
something... again.

Amir.

Re: two questiones about overlayfs

[Amir Goldstein]

On Mon, Aug 7, 2017 at 10:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> Hi,all:
> I found two problemes about overlayfs, please let me know what you think:
>
> 1. Cannot update upper dir's access time.
> Reproduce:
> # mkdir lower upper worker merger
> # mkdir upper/dir
> # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> # ls mrger/dir
> # stat mrger/dir
>   Access: 2017-08-07 11:03:56.364771584 -0400
>   Modify: 2017-08-07 11:01:52.319771584 -0400
>   Change: 2017-08-07 11:01:52.319771584 -0400
> # touch mrger/dir/aa
> # stat mrger/dir
>   Access: 2017-08-07 11:03:56.364771584 -0400
>   Modify: 2017-08-07 11:05:33.969771584 -0400
>   Change: 2017-08-07 11:05:33.969771584 -0400
> # ls mrger/dir
> # stat mrger/dir
>   Access: 2017-08-07 11:03:56.364771584 -0400
>   Modify: 2017-08-07 11:05:33.969771584 -0400
>   Change: 2017-08-07 11:05:33.969771584 -0400
>
> I find the reason of this problem is in update_ovl_inode_times():
> (598e3c8f7 "vfs: update ovl inode before relatime check")
>
> static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
>                                bool rcu)
> {
>         if (!rcu) {
>                 struct inode *realinode = d_real_inode(dentry);
>
>         *d_real_inode() cannot get dir's real inode, so i_mtime always equal to i_ctime*
>         *an not updated*
>
>                 if (unlikely(inode != realinode) &&
>                     (!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
>                      !timespec_equal(&inode->i_ctime, &realinode->i_ctime))) {
>                         inode->i_mtime = realinode->i_mtime;
>                         inode->i_ctime = realinode->i_ctime;
>                 }
>         }
> }
>
> 2. Chattr will modify lower file's attributes directly.
> Reproduce:
> # mkdir lower upper worker merger
> # touch lower/aa
> # lsattr -p lower/aa
>     0 --------------e---- lower/aa
> # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
> # chattr -p 123 merger/aa             #set project id
> # lsattr -p lower/aa
>   123 --------------e---- lower/aa
>
> If we try to set "immutable" or any other attributes, the result are consistent.
> Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
> FS_IOC_SETFLAGS ioctl will get the lower inode and modify it. By the way,
> ovl's directory not support ioctl now, so we fail to change dir's attributes.
>
> Do you think these two "problemes" need to fix or should avoid ?
>

ZhangYi,

Now that fixes for the 2 issues you reported are in overlayfs-next,
could you send those 2 reproducers as xfstests?

Thanks,
Amir.

Re: two questiones about overlayfs

[zhangyi (F)]

On 2017/9/11 21:34, Amir Goldstein Wrote:
> On Mon, Aug 7, 2017 at 10:57 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
>> Hi,all:
>> I found two problemes about overlayfs, please let me know what you think:
>>
>> 1. Cannot update upper dir's access time.
>> Reproduce:
>> # mkdir lower upper worker merger
>> # mkdir upper/dir
>> # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>> # ls mrger/dir
>> # stat mrger/dir
>>   Access: 2017-08-07 11:03:56.364771584 -0400
>>   Modify: 2017-08-07 11:01:52.319771584 -0400
>>   Change: 2017-08-07 11:01:52.319771584 -0400
>> # touch mrger/dir/aa
>> # stat mrger/dir
>>   Access: 2017-08-07 11:03:56.364771584 -0400
>>   Modify: 2017-08-07 11:05:33.969771584 -0400
>>   Change: 2017-08-07 11:05:33.969771584 -0400
>> # ls mrger/dir
>> # stat mrger/dir
>>   Access: 2017-08-07 11:03:56.364771584 -0400
>>   Modify: 2017-08-07 11:05:33.969771584 -0400
>>   Change: 2017-08-07 11:05:33.969771584 -0400
>>
>> I find the reason of this problem is in update_ovl_inode_times():
>> (598e3c8f7 "vfs: update ovl inode before relatime check")
>>
>> static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
>>                                bool rcu)
>> {
>>         if (!rcu) {
>>                 struct inode *realinode = d_real_inode(dentry);
>>
>>         *d_real_inode() cannot get dir's real inode, so i_mtime always equal to i_ctime*
>>         *an not updated*
>>
>>                 if (unlikely(inode != realinode) &&
>>                     (!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
>>                      !timespec_equal(&inode->i_ctime, &realinode->i_ctime))) {
>>                         inode->i_mtime = realinode->i_mtime;
>>                         inode->i_ctime = realinode->i_ctime;
>>                 }
>>         }
>> }
>>
>> 2. Chattr will modify lower file's attributes directly.
>> Reproduce:
>> # mkdir lower upper worker merger
>> # touch lower/aa
>> # lsattr -p lower/aa
>>     0 --------------e---- lower/aa
>> # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger
>> # chattr -p 123 merger/aa             #set project id
>> # lsattr -p lower/aa
>>   123 --------------e---- lower/aa
>>
>> If we try to set "immutable" or any other attributes, the result are consistent.
>> Because chattr open file in RDONLY mode, so it will not trigger copyup, and then,
>> FS_IOC_SETFLAGS ioctl will get the lower inode and modify it. By the way,
>> ovl's directory not support ioctl now, so we fail to change dir's attributes.
>>
>> Do you think these two "problemes" need to fix or should avoid ?
>>
> 
> ZhangYi,
> 
> Now that fixes for the 2 issues you reported are in overlayfs-next,
> could you send those 2 reproducers as xfstests?
>

Yes, I saw the patches and looks good to me. I will send those two reproducers soon, thanks.

Thanks,
ZhangYi.