[PATCH 0/2] Propagate overlay f_mode from underlying file

[PATCH 0/2] Propagate overlay f_mode from underlying file

[Amir Goldstein]

Miklos,

Following the discussions on hugetlbfs-overlayfs, I wanted to see
if we can do better to emulate the underlying file supported ops.

Those patches don't actually help the hugetlbfs issue at all and
I am not sure they improve anything, but I though it would be better
if you had a look to say if there is anything interesting we can
get from this.

Thanks,
Amir.

Amir Goldstein (2):
  ovl: inherit supported ops f_mode flags from final real file
  ovl: warn about unsupported file operations on upper fs

 fs/overlayfs/copy_up.c   | 11 ++++++++
 fs/overlayfs/file.c      | 11 ++++++++
 fs/overlayfs/overlayfs.h |  5 ++++
 fs/overlayfs/super.c     | 60 ++++++++++++++++++++++++++--------------
 4 files changed, 66 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH 1/2] ovl: inherit supported ops f_mode flags from final real file

[Amir Goldstein]

Mike Kravetz reported that when hugetlbfs is used as overlayfs upper
layer, writes do not fail, but result in writing to lower layer.

This is surprising because hugeltbfs file does not have write() nor
write_iter() method.

Regardless of the question whether or not this type of filesystem should
be allowed as overlayfs upper layer, overlayfs file should emulate the
supported ops of the underlying files, so at least in the case where
underlying file ops cannot change as result of copy up, the overlayfs
file should inherit the f_mode flags indicating the supported ops of
the underlying file.

Reported-by: Mike Kravetz <mike.kravetz@oracle.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   | 11 +++++++++++
 fs/overlayfs/file.c      | 11 +++++++++++
 fs/overlayfs/overlayfs.h |  5 +++++
 3 files changed, 27 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 79dd052c7dbf..424f2a170f11 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -953,6 +953,17 @@ static bool ovl_open_need_copy_up(struct dentry *dentry, int flags)
 	return true;
 }
 
+/* May need copy up in the future? */
+bool ovl_may_need_copy_up(struct dentry *dentry)
+{
+	int flags = O_RDONLY;
+
+	if (ovl_upper_mnt(OVL_FS(dentry->d_sb)))
+		flags = O_RDWR;
+
+	return ovl_open_need_copy_up(dentry, flags);
+}
+
 int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 {
 	int err = 0;
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 01820e654a21..01dd3ed723df 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -153,6 +153,17 @@ static int ovl_open(struct inode *inode, struct file *file)
 	if (IS_ERR(realfile))
 		return PTR_ERR(realfile);
 
+	/*
+	 * Overlay file supported ops are a super set of the underlying file
+	 * supported ops and we do not change them when file is copied up.
+	 * But if file cannot be copied up, then there is no need to advertize
+	 * more supported ops than underlying file actually has.
+	 */
+	if (!ovl_may_need_copy_up(file_dentry(file))) {
+		file->f_mode &= ~OVL_UPPER_FMODE_MASK;
+		file->f_mode |= realfile->f_mode & OVL_UPPER_FMODE_MASK;
+	}
+
 	file->private_data = realfile;
 
 	return 0;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index b725c7f15ff4..6748c28ff477 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -110,6 +110,10 @@ struct ovl_fh {
 #define OVL_FH_FID_OFFSET	(OVL_FH_WIRE_OFFSET + \
 				 offsetof(struct ovl_fb, fid))
 
+/* f_mode bits expected to be set on an upper file */
+#define OVL_UPPER_FMODE_MASK (FMODE_CAN_READ | FMODE_CAN_WRITE | \
+			      FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE)
+
 static inline int ovl_do_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	int err = vfs_rmdir(dir, dentry);
@@ -485,6 +489,7 @@ int ovl_copy_up(struct dentry *dentry);
 int ovl_copy_up_with_data(struct dentry *dentry);
 int ovl_copy_up_flags(struct dentry *dentry, int flags);
 int ovl_maybe_copy_up(struct dentry *dentry, int flags);
+bool ovl_may_need_copy_up(struct dentry *dentry);
 int ovl_copy_xattr(struct dentry *old, struct dentry *new);
 int ovl_set_attr(struct dentry *upper, struct kstat *stat);
 struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper);
-- 
2.17.1

[PATCH 2/2] ovl: warn about unsupported file operations on upper fs

[Amir Goldstein]

syzbot has reported a crash in a test case where overlayfs uses
hugetlbfs as upper fs.

Since hugetlbfs has no write() nor write_iter() file ops, there seems
to be little value in supporting this configuration, however, we do not
want to regress strange use cases that me be using such configuration.

As a minimum, check for this case and warn about it on mount time as
well as adding this check for the strict requirements set of remote
upper fs.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 60 ++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 21 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 91476bc422f9..5bee70eb8f64 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -6,6 +6,7 @@
 
 #include <uapi/linux/magic.h>
 #include <linux/fs.h>
+#include <linux/file.h>
 #include <linux/namei.h>
 #include <linux/xattr.h>
 #include <linux/mount.h>
@@ -1131,42 +1132,61 @@ static int ovl_get_upper(struct super_block *sb, struct ovl_fs *ofs,
 }
 
 /*
- * Returns 1 if RENAME_WHITEOUT is supported, 0 if not supported and
+ * Returns 1 if required ops are supported, 0 if not supported and
  * negative values if error is encountered.
  */
-static int ovl_check_rename_whiteout(struct dentry *workdir)
+static int ovl_check_supported_ops(struct ovl_fs *ofs)
 {
-	struct inode *dir = d_inode(workdir);
-	struct dentry *temp;
+	struct inode *dir = d_inode(ofs->workdir);
+	struct dentry *temp = NULL;
 	struct dentry *dest;
 	struct dentry *whiteout;
 	struct name_snapshot name;
+	struct path path;
+	struct file *file;
+	unsigned int unsupported;
 	int err;
 
 	inode_lock_nested(dir, I_MUTEX_PARENT);
 
-	temp = ovl_create_temp(workdir, OVL_CATTR(S_IFREG | 0));
-	err = PTR_ERR(temp);
-	if (IS_ERR(temp))
+	path.mnt = ovl_upper_mnt(ofs);
+	path.dentry = ovl_create_temp(ofs->workdir, OVL_CATTR(S_IFREG | 0));
+	err = PTR_ERR(path.dentry);
+	if (IS_ERR(path.dentry))
 		goto out_unlock;
 
-	dest = ovl_lookup_temp(workdir);
-	err = PTR_ERR(dest);
-	if (IS_ERR(dest)) {
-		dput(temp);
+	temp = path.dentry;
+	file = dentry_open(&path, O_RDWR, current_cred());
+	err = PTR_ERR(file);
+	if (IS_ERR(file))
+		goto out_unlock;
+
+	unsupported = OVL_UPPER_FMODE_MASK & ~file->f_mode;
+	fput(file);
+	if (unsupported) {
+		err = 0;
+		pr_warn("upper fs does not support required file operations (%x).\n",
+			unsupported);
 		goto out_unlock;
 	}
 
+	dest = ovl_lookup_temp(ofs->workdir);
+	err = PTR_ERR(dest);
+	if (IS_ERR(dest))
+		goto out_unlock;
+
 	/* Name is inline and stable - using snapshot as a copy helper */
 	take_dentry_name_snapshot(&name, temp);
 	err = ovl_do_rename(dir, temp, dir, dest, RENAME_WHITEOUT);
 	if (err) {
-		if (err == -EINVAL)
+		if (err == -EINVAL) {
 			err = 0;
+			pr_warn("upper fs does not support RENAME_WHITEOUT.\n");
+		}
 		goto cleanup_temp;
 	}
 
-	whiteout = lookup_one_len(name.name.name, workdir, name.name.len);
+	whiteout = lookup_one_len(name.name.name, ofs->workdir, name.name.len);
 	err = PTR_ERR(whiteout);
 	if (IS_ERR(whiteout))
 		goto cleanup_temp;
@@ -1181,10 +1201,10 @@ static int ovl_check_rename_whiteout(struct dentry *workdir)
 cleanup_temp:
 	ovl_cleanup(dir, temp);
 	release_dentry_name_snapshot(&name);
-	dput(temp);
 	dput(dest);
 
 out_unlock:
+	dput(temp);
 	inode_unlock(dir);
 
 	return err;
@@ -1195,7 +1215,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 {
 	struct vfsmount *mnt = ovl_upper_mnt(ofs);
 	struct dentry *temp;
-	bool rename_whiteout;
+	bool supported_ops;
 	bool d_type;
 	int fh_type;
 	int err;
@@ -1235,14 +1255,12 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 		pr_warn("upper fs does not support tmpfile.\n");
 
 
-	/* Check if upper/work fs supports RENAME_WHITEOUT */
-	err = ovl_check_rename_whiteout(ofs->workdir);
+	/* Check if upper/work fs supports required ops */
+	err = ovl_check_supported_ops(ofs);
 	if (err < 0)
 		goto out;
 
-	rename_whiteout = err;
-	if (!rename_whiteout)
-		pr_warn("upper fs does not support RENAME_WHITEOUT.\n");
+	supported_ops = err;
 
 	/*
 	 * Check if upper/work fs supports trusted.overlay.* xattr
@@ -1264,7 +1282,7 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 	 * we can enforce strict requirements for remote upper fs.
 	 */
 	if (ovl_dentry_remote(ofs->workdir) &&
-	    (!d_type || !rename_whiteout || ofs->noxattr)) {
+	    (!d_type || !supported_ops || ofs->noxattr)) {
 		pr_err("upper fs missing required features.\n");
 		err = -EINVAL;
 		goto out;
-- 
2.17.1