[PATCH AUTOSEL 5.13 40/88] ovl: copy up sync/noatime fileattr flags

[PATCH AUTOSEL 5.13 40/88] ovl: copy up sync/noatime fileattr flags

[Sasha Levin]

From: Amir Goldstein <amir73il@gmail.com>

[ Upstream commit 72db82115d2bdfbfba8b15a92d91872cfe1b40c6 ]

When a lower file has sync/noatime fileattr flags, the behavior of
overlayfs post copy up is inconsistent.

Immediately after copy up, ovl inode still has the S_SYNC/S_NOATIME
inode flags copied from lower inode, so vfs code still treats the ovl
inode as sync/noatime.  After ovl inode evict or mount cycle,
the ovl inode does not have these inode flags anymore.

To fix this inconsistency, try to copy the fileattr flags on copy up
if the upper fs supports the fileattr_set() method.

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

We cannot copy up the immutable/append-only inode flags in a similar
manner, because immutable/append-only inodes cannot be linked and because
overlayfs will not be able to set overlay.* xattr on the upper inodes.

Those flags will be addressed by a followup patch.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 fs/overlayfs/copy_up.c   | 51 ++++++++++++++++++++++++++++++++++------
 fs/overlayfs/inode.c     | 44 ++++++++++++++++++++++++----------
 fs/overlayfs/overlayfs.h | 15 +++++++++++-
 3 files changed, 89 insertions(+), 21 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 2846b943e80c..5e7fb92f9edd 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -8,6 +8,7 @@
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/file.h>
+#include <linux/fileattr.h>
 #include <linux/splice.h>
 #include <linux/xattr.h>
 #include <linux/security.h>
@@ -130,6 +131,31 @@ int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
 	return error;
 }
 
+static int ovl_copy_fileattr(struct path *old, struct path *new)
+{
+	struct fileattr oldfa = { .flags_valid = true };
+	struct fileattr newfa = { .flags_valid = true };
+	int err;
+
+	err = ovl_real_fileattr_get(old, &oldfa);
+	if (err)
+		return err;
+
+	err = ovl_real_fileattr_get(new, &newfa);
+	if (err)
+		return err;
+
+	BUILD_BUG_ON(OVL_COPY_FS_FLAGS_MASK & ~FS_COMMON_FL);
+	newfa.flags &= ~OVL_COPY_FS_FLAGS_MASK;
+	newfa.flags |= (oldfa.flags & OVL_COPY_FS_FLAGS_MASK);
+
+	BUILD_BUG_ON(OVL_COPY_FSX_FLAGS_MASK & ~FS_XFLAG_COMMON);
+	newfa.fsx_xflags &= ~OVL_COPY_FSX_FLAGS_MASK;
+	newfa.fsx_xflags |= (oldfa.fsx_xflags & OVL_COPY_FSX_FLAGS_MASK);
+
+	return ovl_real_fileattr_set(new, &newfa);
+}
+
 static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
 			    struct path *new, loff_t len)
 {
@@ -493,20 +519,21 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
+	struct inode *inode = d_inode(c->dentry);
+	struct path upperpath, datapath;
 	int err;
 
+	ovl_path_upper(c->dentry, &upperpath);
+	if (WARN_ON(upperpath.dentry != NULL))
+		return -EIO;
+
+	upperpath.dentry = temp;
+
 	/*
 	 * Copy up data first and then xattrs. Writing data after
 	 * xattrs will remove security.capability xattr automatically.
 	 */
 	if (S_ISREG(c->stat.mode) && !c->metacopy) {
-		struct path upperpath, datapath;
-
-		ovl_path_upper(c->dentry, &upperpath);
-		if (WARN_ON(upperpath.dentry != NULL))
-			return -EIO;
-		upperpath.dentry = temp;
-
 		ovl_path_lowerdata(c->dentry, &datapath);
 		err = ovl_copy_up_data(ofs, &datapath, &upperpath,
 				       c->stat.size);
@@ -518,6 +545,16 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 	if (err)
 		return err;
 
+	if (inode->i_flags & OVL_COPY_I_FLAGS_MASK) {
+		/*
+		 * Copy the fileattr inode flags that are the source of already
+		 * copied i_flags
+		 */
+		err = ovl_copy_fileattr(&c->lowerpath, &upperpath);
+		if (err)
+			return err;
+	}
+
 	/*
 	 * Store identifier of lower inode in upper inode xattr to
 	 * allow lookup of the copy up origin inode.
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 5e828a1c98a8..b288843e6b42 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -503,16 +503,14 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
  * Introducing security_inode_fileattr_get/set() hooks would solve this issue
  * properly.
  */
-static int ovl_security_fileattr(struct dentry *dentry, struct fileattr *fa,
+static int ovl_security_fileattr(struct path *realpath, struct fileattr *fa,
 				 bool set)
 {
-	struct path realpath;
 	struct file *file;
 	unsigned int cmd;
 	int err;
 
-	ovl_path_real(dentry, &realpath);
-	file = dentry_open(&realpath, O_RDONLY, current_cred());
+	file = dentry_open(realpath, O_RDONLY, current_cred());
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
@@ -527,11 +525,22 @@ static int ovl_security_fileattr(struct dentry *dentry, struct fileattr *fa,
 	return err;
 }
 
+int ovl_real_fileattr_set(struct path *realpath, struct fileattr *fa)
+{
+	int err;
+
+	err = ovl_security_fileattr(realpath, fa, true);
+	if (err)
+		return err;
+
+	return vfs_fileattr_set(&init_user_ns, 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 +550,10 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
 
 	err = ovl_copy_up(dentry);
 	if (!err) {
-		upperdentry = ovl_dentry_upper(dentry);
+		ovl_path_real(dentry, &upperpath);
 
 		old_cred = ovl_override_creds(inode->i_sb);
-		err = ovl_security_fileattr(dentry, fa, true);
-		if (!err)
-			err = vfs_fileattr_set(&init_user_ns, upperdentry, fa);
+		err = ovl_real_fileattr_set(&upperpath, fa);
 		revert_creds(old_cred);
 		ovl_copyflags(ovl_inode_real(inode), inode);
 	}
@@ -555,17 +562,28 @@ int ovl_fileattr_set(struct user_namespace *mnt_userns,
 	return err;
 }
 
+int ovl_real_fileattr_get(struct path *realpath, struct fileattr *fa)
+{
+	int err;
+
+	err = ovl_security_fileattr(realpath, fa, false);
+	if (err)
+		return err;
+
+	return vfs_fileattr_get(realpath->dentry, fa);
+}
+
 int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
-	struct dentry *realdentry = ovl_dentry_real(dentry);
+	struct path realpath;
 	const struct cred *old_cred;
 	int err;
 
+	ovl_path_real(dentry, &realpath);
+
 	old_cred = ovl_override_creds(inode->i_sb);
-	err = ovl_security_fileattr(dentry, fa, false);
-	if (!err)
-		err = vfs_fileattr_get(realdentry, fa);
+	err = ovl_real_fileattr_get(&realpath, fa);
 	revert_creds(old_cred);
 
 	return err;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6ec73db4bf9e..8e94d9d0c919 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -518,9 +518,20 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 	i_size_write(to, i_size_read(from));
 }
 
+/* vfs inode flags copied from real to ovl inode */
+#define OVL_COPY_I_FLAGS_MASK	(S_SYNC | S_NOATIME | S_APPEND | S_IMMUTABLE)
+
+/*
+ * fileattr flags copied from lower to upper inode on copy up.
+ * We cannot copy immutable/append-only flags, because that would prevevnt
+ * linking temp inode to upper dir.
+ */
+#define OVL_COPY_FS_FLAGS_MASK	(FS_SYNC_FL | FS_NOATIME_FL)
+#define OVL_COPY_FSX_FLAGS_MASK	(FS_XFLAG_SYNC | FS_XFLAG_NOATIME)
+
 static inline void ovl_copyflags(struct inode *from, struct inode *to)
 {
-	unsigned int mask = S_SYNC | S_IMMUTABLE | S_APPEND | S_NOATIME;
+	unsigned int mask = OVL_COPY_I_FLAGS_MASK;
 
 	inode_set_flags(to, from->i_flags & mask, mask);
 }
@@ -548,6 +559,8 @@ struct dentry *ovl_create_temp(struct dentry *workdir, struct ovl_cattr *attr);
 extern const struct file_operations ovl_file_operations;
 int __init ovl_aio_request_cache_init(void);
 void ovl_aio_request_cache_destroy(void);
+int ovl_real_fileattr_get(struct path *realpath, struct fileattr *fa);
+int ovl_real_fileattr_set(struct path *realpath, struct fileattr *fa);
 int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa);
 int ovl_fileattr_set(struct user_namespace *mnt_userns,
 		     struct dentry *dentry, struct fileattr *fa);
-- 
2.30.2

[Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs

[Kevin Locke]

Hi all,

With 5.15-rc5 or torvalds master (d999ade1cc86), attempting to rename
a file fails with -EINVAL on an overlayfs mount with a lower
filesystem that returns -EINVAL for ioctl(FS_IOC_GETFLAGS).  For
example, with ntfs-3g:

    mkdir lower upper work overlay
    dd if=/dev/zero of=ntfs.raw bs=1M count=2
    mkntfs -F ntfs.raw
    mount ntfs.raw lower
    touch lower/file.txt
    mount -t overlay -o "lowerdir=$PWD/lower,upperdir=$PWD/upper,workdir=$PWD/work" - overlay
    mv overlay/file.txt overlay/file2.txt

mv fails and (misleadingly) prints

    mv: cannot move 'overlay/file.txt' to a subdirectory of itself, 'overlay/file2.txt'

which strace(1) reveals to be due to rename(2) returning -22
(-EINVAL).  A bit of digging revealed that -EINVAL is coming from
vfs_fileattr_get() with the following stack:

ovl_real_fileattr_get.cold+0x9/0x12 [overlay]
ovl_copy_up_inode+0x1b5/0x280 [overlay]
ovl_copy_up_one+0xaf1/0xee0 [overlay]
ovl_copy_up_flags+0xab/0xf0 [overlay]
ovl_rename+0x149/0x850 [overlay]
? privileged_wrt_inode_uidgid+0x47/0x60
? generic_permission+0x90/0x200
? ovl_permission+0x70/0x120 [overlay]
vfs_rename+0x619/0x9d0
do_renameat2+0x3c0/0x570
__x64_sys_renameat2+0x4b/0x60
do_syscall_64+0x3b/0xc0
entry_SYSCALL_64_after_hwframe+0x44/0xae

This issue does not occur on 5.14.  I've bisected the regression to
72db82115d2b.

Thanks,
Kevin

Re: [Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs

[Miklos Szeredi]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

On Sun, 17 Oct 2021 at 22:53, Kevin Locke <kevin@kevinlocke.name> wrote:
>
> Hi all,
>
> With 5.15-rc5 or torvalds master (d999ade1cc86), attempting to rename
> a file fails with -EINVAL on an overlayfs mount with a lower
> filesystem that returns -EINVAL for ioctl(FS_IOC_GETFLAGS).  For
> example, with ntfs-3g:
>
>     mkdir lower upper work overlay
>     dd if=/dev/zero of=ntfs.raw bs=1M count=2
>     mkntfs -F ntfs.raw
>     mount ntfs.raw lower
>     touch lower/file.txt
>     mount -t overlay -o "lowerdir=$PWD/lower,upperdir=$PWD/upper,workdir=$PWD/work" - overlay
>     mv overlay/file.txt overlay/file2.txt
>
> mv fails and (misleadingly) prints
>
>     mv: cannot move 'overlay/file.txt' to a subdirectory of itself, 'overlay/file2.txt'
>
> which strace(1) reveals to be due to rename(2) returning -22
> (-EINVAL).  A bit of digging revealed that -EINVAL is coming from
> vfs_fileattr_get() with the following stack:
>
> ovl_real_fileattr_get.cold+0x9/0x12 [overlay]
> ovl_copy_up_inode+0x1b5/0x280 [overlay]
> ovl_copy_up_one+0xaf1/0xee0 [overlay]
> ovl_copy_up_flags+0xab/0xf0 [overlay]
> ovl_rename+0x149/0x850 [overlay]
> ? privileged_wrt_inode_uidgid+0x47/0x60
> ? generic_permission+0x90/0x200
> ? ovl_permission+0x70/0x120 [overlay]
> vfs_rename+0x619/0x9d0
> do_renameat2+0x3c0/0x570
> __x64_sys_renameat2+0x4b/0x60
> do_syscall_64+0x3b/0xc0
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> This issue does not occur on 5.14.  I've bisected the regression to
> 72db82115d2b.

This is clearly a regression.  Not trivial how far the fix should go, though.

One option is to just ignore all errors from ovl_copy_fileattr(),
which would solve this and similar issues.  However that would result
in missing the cases when the attributes were really meant to be
copied up, but failed to do so for some reason.

If vfs_fileattr_get() fails with ENOIOCTLCMD or ENOTTY on lower, that
obviously means we need to return success (lower fs does not support
fileattr).   As ntfs-3g seems to return EINVAL that needs to be added
too.

More interesting question is what to do with get/set failures on
upper.   My feeling is that for now we should try to return errors
(even ENOTTY), but should print a warning in the kernel log.  If that
turns out to regress some use cases, then that needs to fixed as well.

Untested patch attached.

Thanks,
Miklos

[-- Attachment #2: ovl-fix-fileattr-copy-up-failure.patch --]
[-- Type: text/x-patch, Size: 1270 bytes --]

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 4e7d5bfa2949..ed0a0ce6deda 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -140,12 +140,21 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
 	int err;
 
 	err = ovl_real_fileattr_get(old, &oldfa);
-	if (err)
+	if (err) {
+		/* Ntfs-3g return -EINVAL for "no fileattr support" */
+		if (err == -ENOTTY || err == -EINVAL)
+			return 0;
+		pr_warn("failed to retrieve fileattr (%pd2, err=%i)\n",
+			old, err);
 		return err;
+	}
 
 	err = ovl_real_fileattr_get(new, &newfa);
-	if (err)
+	if (err) {
+		pr_warn("failed to retrieve fileattr (%pd2, err=%i)\n",
+			old, err);
 		return err;
+	}
 
 	/*
 	 * We cannot set immutable and append-only flags on upper inode,
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 832b17589733..1f36158c7dbe 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -610,7 +610,10 @@ int ovl_real_fileattr_get(struct path *realpath, struct fileattr *fa)
 	if (err)
 		return err;
 
-	return vfs_fileattr_get(realpath->dentry, fa);
+	err = vfs_fileattr_get(realpath->dentry, fa);
+	if (err == -ENOIOCTLCMD)
+		err = -ENOTTY;
+	return err;
 }
 
 int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)

Re: [Regression] ovl: rename(2) EINVAL if lower doesn't support fileattrs

[Kevin Locke]

On Mon, 2021-10-18 at 10:47 +0200, Miklos Szeredi wrote:
> On Sun, 17 Oct 2021 at 22:53, Kevin Locke <kevin@kevinlocke.name> wrote:
>> With 5.15-rc5 or torvalds master (d999ade1cc86), attempting to rename
>> a file fails with -EINVAL on an overlayfs mount with a lower
>> filesystem that returns -EINVAL for ioctl(FS_IOC_GETFLAGS).
>>
>> [...]
>>
>> This issue does not occur on 5.14.  I've bisected the regression to
>> 72db82115d2b.
> 
> This is clearly a regression.  Not trivial how far the fix should go, though.
> 
> One option is to just ignore all errors from ovl_copy_fileattr(),
> which would solve this and similar issues.  However that would result
> in missing the cases when the attributes were really meant to be
> copied up, but failed to do so for some reason.
> 
> If vfs_fileattr_get() fails with ENOIOCTLCMD or ENOTTY on lower, that
> obviously means we need to return success (lower fs does not support
> fileattr).   As ntfs-3g seems to return EINVAL that needs to be added
> too.

I agree.  I like your approach.  Especially for ENOIOCTLCMD/ENOTTY.
Since ntfs-3g returns EINVAL, that seems necessary to avoid logspam
for now.  Do you think it would make sense for me to suggest returning
ENOIOCTLCMD to the ntfs-3g project?  It seems more appropriate and
consistent with other filesystems to me, but I'm certainly not an
expert:
https://github.com/tuxera/ntfs-3g/blob/2021.8.22/libntfs-3g/ioctl.c#L415
I didn't see any other errors from in-tree filesystems, but who knows
errors other FUSE projects may return.

> More interesting question is what to do with get/set failures on
> upper.   My feeling is that for now we should try to return errors
> (even ENOTTY), but should print a warning in the kernel log.  If that
> turns out to regress some use cases, then that needs to fixed as well.

Does that mean all copy-up operations would fail for an upper which
does not support file attributes, or only ones where the file being
copied has attributes set?  I'm a little concerned about debugability,
since the failure modes may not be obvious to users.  Although the log
warnings would help with that.

Would it be possible to refuse to mount an upper which doesn't support
file attrs over a lower which does?  At least the failure would be
immediate and obvious.  If a use case is found, perhaps a mount option
to control file attr behavior cold be added at that point?  Just
thinking out loud.

> Untested patch attached.

Works great for me.

Tested-by: Kevin Locke <kevin@kevinlocke.name>

Thanks for the fast investigation and fix!
Kevin

[PATCH AUTOSEL 5.13 41/88] ovl: skip checking lower file's i_writecount on truncate

[Sasha Levin]

From: Chengguang Xu <cgxu519@mykernel.net>

[ Upstream commit b71759ef1e1730db81dab98e9dab9455e8c7f5a2 ]

It is possible that a directory tree is shared between multiple overlay
instances as a lower layer.  In this case when one instance executes a file
residing on the lower layer, the other instance denies a truncate(2) call
on this file.

This only happens for truncate(2) and not for open(2) with the O_TRUNC
flag.

Fix this interference and inconsistency by removing the preliminary
i_writecount check before copy-up.

This means that unlike on normal filesystems truncate(argv[0]) will now
succeed.  If this ever causes a regression in a real world use case this
needs to be revisited.

One way to fix this properly would be to keep a correct i_writecount in the
overlay inode, but that is difficult due to memory mapping code only
dealing with the real file/inode.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 Documentation/filesystems/overlayfs.rst | 3 +++
 fs/overlayfs/inode.c                    | 6 ------
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 455ca86eb4fc..7da6c30ed596 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -427,6 +427,9 @@ b) If a file residing on a lower layer is opened for read-only and then
 memory mapped with MAP_SHARED, then subsequent changes to the file are not
 reflected in the memory mapping.
 
+c) If a file residing on a lower layer is being executed, then opening that
+file for write or truncating the file will not be denied with ETXTBSY.
+
 The following options allow overlayfs to act more like a standards
 compliant filesystem:
 
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b288843e6b42..6566294c5fd6 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -33,12 +33,6 @@ int ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		goto out;
 
 	if (attr->ia_valid & ATTR_SIZE) {
-		struct inode *realinode = d_inode(ovl_dentry_real(dentry));
-
-		err = -ETXTBSY;
-		if (atomic_read(&realinode->i_writecount) < 0)
-			goto out_drop_write;
-
 		/* Truncate should trigger data copy up as well */
 		full_copy_up = true;
 	}
-- 
2.30.2