| Message ID | 20200612004644.255692-2-mike.kravetz@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] hugetlb: use f_mode & FMODE_HUGETLBFS to identify hugetlbfs files | expand |
On Fri, Jun 12, 2020 at 3:57 AM Mike Kravetz <mike.kravetz@oracle.com> 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@google.com/ > > Reported-by: syzbot+d6ec23007e951dadf3de@syzkaller.appspotmail.com > Signed-off-by: Miklos Szeredi <miklos@szeredi.hu> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com> > --- > 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@gmail.com> 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@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;
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); + file->private_data = realfile; return 0; @@ -757,6 +762,21 @@ static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in, remap_flags, op); } +#ifdef CONFIG_MMU +static unsigned long ovl_get_unmapped_area(struct file *file, + unsigned long uaddr, unsigned long len, + unsigned long pgoff, unsigned long flags) +{ + struct file *realfile = file->private_data; + + return (realfile->f_op->get_unmapped_area ?: + current->mm->get_unmapped_area)(realfile, + uaddr, len, pgoff, flags); +} +#else +#define ovl_get_unmapped_area NULL +#endif + const struct file_operations ovl_file_operations = { .open = ovl_open, .release = ovl_release, @@ -774,6 +794,7 @@ const struct file_operations ovl_file_operations = { .copy_file_range = ovl_copy_file_range, .remap_file_range = ovl_remap_file_range, + .get_unmapped_area = ovl_get_unmapped_area, }; int __init ovl_aio_request_cache_init(void)