On Fri, Jun 12, 2020 at 3:57 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> The core routine get_unmapped_area will call a filesystem specific version
> of get_unmapped_area if it exists in file operations. If a file is on a
> union/overlay, overlayfs does not contain a get_unmapped_area f_op and the
> underlying filesystem routine may be ignored. Add an overlayfs f_op to call
> the underlying f_op if it exists.
>
> The routine is_file_hugetlbfs() is used to determine if a file is on
> hugetlbfs. This is determined by f_mode & FMODE_HUGETLBFS. Copy the mode
> to the overlayfs file during open so that is_file_hugetlbfs() will work as
> intended.
>
> These two issues can result in the BUG as shown in [1].
>
> [1] https://lore.kernel.org/linux-mm/000000000000b4684e05a2968ca6@xxxxxxxxxx/
>
> Reported-by: syzbot+d6ec23007e951dadf3de@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Miklos Szeredi <miklos@xxxxxxxxxx>
> Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> ---
> fs/overlayfs/file.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 87c362f65448..41e5746ba3c6 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -124,6 +124,8 @@ static int ovl_real_fdget(const struct file *file, struct fd *real)
> return ovl_real_fdget_meta(file, real, false);
> }
>
> +#define OVL_F_MODE_TO_UPPER (FMODE_HUGETLBFS)
> +
> static int ovl_open(struct inode *inode, struct file *file)
> {
> struct file *realfile;
> @@ -140,6 +142,9 @@ static int ovl_open(struct inode *inode, struct file *file)
> if (IS_ERR(realfile))
> return PTR_ERR(realfile);
>
> + /* Copy modes from underlying file */
> + file->f_mode |= (realfile->f_mode & OVL_F_MODE_TO_UPPER);
> +
The name OVL_F_MODE_TO_UPPER is strange because
ovl_open() may be opening a file from lower or from upper.
Please see attached patches.
They are not enough to solve the syzbot repro, but if you do want to
fix hugetlb/overlay interop I think they will be necessary.
Thanks,
Amir.
From 691915fd8d4bb2c0f944b7d8e504a323b8e53b69 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Sun, 14 Jun 2020 13:51:30 +0300
Subject: [PATCH 2/2] ovl: warn about unsupported file operations on upper fs
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@xxxxxxxxx>
---
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
From bd4e44245f2f162da37b02dc1e0e7458cd28a5de Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Sun, 14 Jun 2020 10:44:09 +0300
Subject: [PATCH 1/2] ovl: inherit supported ops f_mode flags from final real
file
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@xxxxxxxxxx>
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
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
