From 24f14009b8f1754ec2ae4c168940c01259b0f88a Mon Sep 17 00:00:00 2001 From: youngjun Date: Tue, 16 Jun 2020 17:30:43 +0900 Subject: ovl: inode reference leak in ovl_is_inuse true case. When "ovl_is_inuse" true case, trap inode reference not put. plus adding the comment explaining sequence of ovl_is_inuse after ovl_setup_trap. Fixes: 0be0bfd2de9d ("ovl: fix regression caused by overlapping layers detection") Cc: # v4.19+ Reviewed-by: Amir Goldstein Signed-off-by: youngjun Signed-off-by: Miklos Szeredi --- fs/overlayfs/super.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 91476bc422f9..498d49d5dd19 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1493,14 +1493,23 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs, if (err < 0) goto out; + /* + * Check if lower root conflicts with this overlay layers before + * checking if it is in-use as upperdir/workdir of "another" + * mount, because we do not bother to check in ovl_is_inuse() if + * the upperdir/workdir is in fact in-use by our + * upperdir/workdir. + */ err = ovl_setup_trap(sb, stack[i].dentry, &trap, "lowerdir"); if (err) goto out; if (ovl_is_inuse(stack[i].dentry)) { err = ovl_report_in_use(ofs, "lowerdir"); - if (err) + if (err) { + iput(trap); goto out; + } } mnt = clone_private_mount(&stack[i]); -- cgit v1.2.3 From 5ac8e8025aedd2ea832ae3fe3ca0f920697f45c7 Mon Sep 17 00:00:00 2001 From: youngjun Date: Sun, 21 Jun 2020 07:30:59 -0700 Subject: ovl: change ovl_copy_up_flags static "ovl_copy_up_flags" is used in copy_up.c. so, change it static. Signed-off-by: youngjun Signed-off-by: Miklos Szeredi --- fs/overlayfs/copy_up.c | 2 +- fs/overlayfs/overlayfs.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 79dd052c7dbf..5e0cde85bd6b 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -895,7 +895,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry, return err; } -int ovl_copy_up_flags(struct dentry *dentry, int flags) +static int ovl_copy_up_flags(struct dentry *dentry, int flags) { int err = 0; const struct cred *old_cred = ovl_override_creds(dentry->d_sb); diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index b725c7f15ff4..29bc1ec699e7 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -483,7 +483,6 @@ void ovl_aio_request_cache_destroy(void); /* copy_up.c */ 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); int ovl_copy_xattr(struct dentry *old, struct dentry *new); int ovl_set_attr(struct dentry *upper, struct kstat *stat); -- cgit v1.2.3 From d78a0dcf64cb94ffdd5253722caed3f044a92a78 Mon Sep 17 00:00:00 2001 From: youngjun Date: Sun, 21 Jun 2020 05:50:01 -0700 Subject: ovl: remove not used argument in ovl_check_origin ovl_check_origin outparam 'ctrp' argument not used by caller. So remove this argument. Signed-off-by: youngjun Reviewed-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/namei.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 3566282a9199..3cad68c3efb2 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -389,7 +389,7 @@ invalid: } static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry, - struct ovl_path **stackp, unsigned int *ctrp) + struct ovl_path **stackp) { struct ovl_fh *fh = ovl_get_fh(upperdentry, OVL_XATTR_ORIGIN); int err; @@ -406,10 +406,6 @@ static int ovl_check_origin(struct ovl_fs *ofs, struct dentry *upperdentry, return err; } - if (WARN_ON(*ctrp)) - return -EIO; - - *ctrp = 1; return 0; } @@ -861,8 +857,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, goto out; } if (upperdentry && !d.is_dir) { - unsigned int origin_ctr = 0; - /* * Lookup copy up origin by decoding origin file handle. * We may get a disconnected dentry, which is fine, @@ -873,8 +867,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, * number - it's the same as if we held a reference * to a dentry in lower layer that was moved under us. */ - err = ovl_check_origin(ofs, upperdentry, &origin_path, - &origin_ctr); + err = ovl_check_origin(ofs, upperdentry, &origin_path); if (err) goto out_put_upper; -- cgit v1.2.3 From 124c2de2c0aee96271e4ddab190083d8aa7aa71a Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Wed, 17 Jun 2020 09:57:11 +0300 Subject: ovl: relax WARN_ON() when decoding lower directory file handle Decoding a lower directory file handle to overlay path with cold inode/dentry cache may go as follows: 1. Decode real lower file handle to lower dir path 2. Check if lower dir is indexed (was copied up) 3. If indexed, get the upper dir path from index 4. Lookup upper dir path in overlay 5. If overlay path found, verify that overlay lower is the lower dir from step 1 On failure to verify step 5 above, user will get an ESTALE error and a WARN_ON will be printed. A mismatch in step 5 could be a result of lower directory that was renamed while overlay was offline, after that lower directory has been copied up and indexed. This is a scripted reproducer based on xfstest overlay/052: # Create lower subdir create_dirs create_test_files $lower/lowertestdir/subdir mount_dirs # Copy up lower dir and encode lower subdir file handle touch $SCRATCH_MNT/lowertestdir test_file_handles $SCRATCH_MNT/lowertestdir/subdir -p -o $tmp.fhandle # Rename lower dir offline unmount_dirs mv $lower/lowertestdir $lower/lowertestdir.new/ mount_dirs # Attempt to decode lower subdir file handle test_file_handles $SCRATCH_MNT -p -i $tmp.fhandle Since this WARN_ON() can be triggered by user we need to relax it. Fixes: 4b91c30a5a19 ("ovl: lookup connected ancestor of dir in inode cache") Cc: # v4.16+ Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/export.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c index 8f4286450f92..0e696f72cf65 100644 --- a/fs/overlayfs/export.c +++ b/fs/overlayfs/export.c @@ -476,7 +476,7 @@ static struct dentry *ovl_lookup_real_inode(struct super_block *sb, if (IS_ERR_OR_NULL(this)) return this; - if (WARN_ON(ovl_dentry_real_at(this, layer->idx) != real)) { + if (ovl_dentry_real_at(this, layer->idx) != real) { dput(this); this = ERR_PTR(-EIO); } -- cgit v1.2.3 From 20396365a16dae4b4e3967fc7115d5336feb30ee Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Sun, 21 Jun 2020 09:37:59 +0300 Subject: ovl: fix oops in ovl_indexdir_cleanup() with nfs_export=on Mounting with nfs_export=on, xfstests overlay/031 triggers a kernel panic since v5.8-rc1 overlayfs updates. overlayfs: orphan index entry (index/00fb1..., ftype=4000, nlink=2) BUG: kernel NULL pointer dereference, address: 0000000000000030 RIP: 0010:ovl_cleanup_and_whiteout+0x28/0x220 [overlay] Bisect point at commit c21c839b8448 ("ovl: whiteout inode sharing") Minimal reproducer: -------------------------------------------------- rm -rf l u w m mkdir -p l u w m mkdir -p l/testdir touch l/testdir/testfile mount -t overlay -o lowerdir=l,upperdir=u,workdir=w,nfs_export=on overlay m echo 1 > m/testdir/testfile umount m rm -rf u/testdir mount -t overlay -o lowerdir=l,upperdir=u,workdir=w,nfs_export=on overlay m umount m -------------------------------------------------- When mount with nfs_export=on, and fail to verify an orphan index, we're cleaning this index from indexdir by calling ovl_cleanup_and_whiteout(). This dereferences ofs->workdir, that was earlier set to NULL. The design was that ovl->workdir will point at ovl->indexdir, but we are assigning ofs->indexdir to ofs->workdir only after ovl_indexdir_cleanup(). There is no reason not to do it sooner, because once we get success from ofs->indexdir = ovl_workdir_create(... there is no turning back. Reported-and-tested-by: Murphy Zhou Fixes: c21c839b8448 ("ovl: whiteout inode sharing") Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/super.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 498d49d5dd19..31ef24dfcf88 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1354,6 +1354,12 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs, ofs->indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true); if (ofs->indexdir) { + /* index dir will act also as workdir */ + iput(ofs->workdir_trap); + ofs->workdir_trap = NULL; + dput(ofs->workdir); + ofs->workdir = dget(ofs->indexdir); + err = ovl_setup_trap(sb, ofs->indexdir, &ofs->indexdir_trap, "indexdir"); if (err) @@ -1852,20 +1858,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) sb->s_flags |= SB_RDONLY; if (!(ovl_force_readonly(ofs)) && ofs->config.index) { - /* index dir will act also as workdir */ - dput(ofs->workdir); - ofs->workdir = NULL; - iput(ofs->workdir_trap); - ofs->workdir_trap = NULL; - err = ovl_get_indexdir(sb, ofs, oe, &upperpath); if (err) goto out_free_oe; /* Force r/o mount with no index dir */ - if (ofs->indexdir) - ofs->workdir = dget(ofs->indexdir); - else + if (!ofs->indexdir) sb->s_flags |= SB_RDONLY; } -- cgit v1.2.3 From a888db310195400f050b89c47673f0f8babfbb41 Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Wed, 8 Jul 2020 16:16:13 +0300 Subject: ovl: fix regression with re-formatted lower squashfs Commit 9df085f3c9a2 ("ovl: relax requirement for non null uuid of lower fs") relaxed the requirement for non null uuid with single lower layer to allow enabling index and nfs_export features with single lower squashfs. Fabian reported a regression in a setup when overlay re-uses an existing upper layer and re-formats the lower squashfs image. Because squashfs has no uuid, the origin xattr in upper layer are decoded from the new lower layer where they may resolve to a wrong origin file and user may get an ESTALE or EIO error on lookup. To avoid the reported regression while still allowing the new features with single lower squashfs, do not allow decoding origin with lower null uuid unless user opted-in to one of the new features that require following the lower inode of non-dir upper (index, xino, metacopy). Reported-by: Fabian Link: https://lore.kernel.org/linux-unionfs/32532923.JtPX5UtSzP@fgdesktop/ Fixes: 9df085f3c9a2 ("ovl: relax requirement for non null uuid of lower fs") Cc: stable@vger.kernel.org # v4.20+ Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/super.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 31ef24dfcf88..94ca095b2a9e 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1402,6 +1402,18 @@ static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid) if (!ofs->config.nfs_export && !ovl_upper_mnt(ofs)) return true; + /* + * We allow using single lower with null uuid for index and nfs_export + * for example to support those features with single lower squashfs. + * To avoid regressions in setups of overlay with re-formatted lower + * squashfs, do not allow decoding origin with lower null uuid unless + * user opted-in to one of the new features that require following the + * lower inode of non-dir upper. + */ + if (!ofs->config.index && !ofs->config.metacopy && !ofs->config.xino && + uuid_is_null(uuid)) + return false; + for (i = 0; i < ofs->numfs; i++) { /* * We use uuid to associate an overlay lower file handle with a -- cgit v1.2.3 From 470c15636125d1591bc901f6aa2d8e4a60ed2190 Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Mon, 13 Jul 2020 17:19:43 +0300 Subject: ovl: force read-only sb on failure to create index dir With index feature enabled, on failure to create index dir, overlay is being mounted read-only. However, we do not forbid user to remount overlay read-write. Fix that by setting ofs->workdir to NULL, which prevents remount read-write. Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/super.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 94ca095b2a9e..f41ef1dd384c 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1352,12 +1352,13 @@ static int ovl_get_indexdir(struct super_block *sb, struct ovl_fs *ofs, goto out; } + /* index dir will act also as workdir */ + iput(ofs->workdir_trap); + ofs->workdir_trap = NULL; + dput(ofs->workdir); + ofs->workdir = NULL; ofs->indexdir = ovl_workdir_create(ofs, OVL_INDEXDIR_NAME, true); if (ofs->indexdir) { - /* index dir will act also as workdir */ - iput(ofs->workdir_trap); - ofs->workdir_trap = NULL; - dput(ofs->workdir); ofs->workdir = dget(ofs->indexdir); err = ovl_setup_trap(sb, ofs->indexdir, &ofs->indexdir_trap, @@ -1869,7 +1870,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) if (!ovl_upper_mnt(ofs)) sb->s_flags |= SB_RDONLY; - if (!(ovl_force_readonly(ofs)) && ofs->config.index) { + if (!ovl_force_readonly(ofs) && ofs->config.index) { err = ovl_get_indexdir(sb, ofs, oe, &upperpath); if (err) goto out_free_oe; -- cgit v1.2.3 From f0e1266ed24b8a69655e1ab7fb618dbf6730879b Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Mon, 13 Jul 2020 17:19:44 +0300 Subject: ovl: fix mount option checks for nfs_export with no upperdir Without upperdir mount option, there is no index dir and the dependency checks nfs_export => index for mount options parsing are incorrect. Allow the combination nfs_export=on,index=off with no upperdir and move the check for dependency redirect_dir=nofollow for non-upper mount case to mount options parsing. Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- Documentation/filesystems/overlayfs.rst | 4 ++-- fs/overlayfs/super.c | 31 ++++++++++++++++++------------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst index 660dbaf0b9b8..fcda5d6ba9ac 100644 --- a/Documentation/filesystems/overlayfs.rst +++ b/Documentation/filesystems/overlayfs.rst @@ -560,8 +560,8 @@ When the NFS export feature is enabled, all directory index entries are verified on mount time to check that upper file handles are not stale. This verification may cause significant overhead in some cases. -Note: the mount options index=off,nfs_export=on are conflicting and will -result in an error. +Note: the mount options index=off,nfs_export=on are conflicting for a +read-write mount and will result in an error. Testsuite diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index f41ef1dd384c..4b38141c2985 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -580,12 +580,19 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) } } - /* Workdir is useless in non-upper mount */ - if (!config->upperdir && config->workdir) { - pr_info("option \"workdir=%s\" is useless in a non-upper mount, ignore\n", - config->workdir); - kfree(config->workdir); - config->workdir = NULL; + /* Workdir/index are useless in non-upper mount */ + if (!config->upperdir) { + if (config->workdir) { + pr_info("option \"workdir=%s\" is useless in a non-upper mount, ignore\n", + config->workdir); + kfree(config->workdir); + config->workdir = NULL; + } + if (config->index && index_opt) { + pr_info("option \"index=on\" is useless in a non-upper mount, ignore\n"); + index_opt = false; + } + config->index = false; } err = ovl_parse_redirect_mode(config, config->redirect_mode); @@ -622,11 +629,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) /* Resolve nfs_export -> index dependency */ if (config->nfs_export && !config->index) { - if (nfs_export_opt && index_opt) { + if (!config->upperdir && config->redirect_follow) { + pr_info("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n"); + config->nfs_export = false; + } else if (nfs_export_opt && index_opt) { pr_err("conflicting options: nfs_export=on,index=off\n"); return -EINVAL; - } - if (index_opt) { + } else if (index_opt) { /* * There was an explicit index=off that resulted * in this conflict. @@ -1603,10 +1612,6 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb, if (!ofs->config.upperdir && numlower == 1) { pr_err("at least 2 lowerdir are needed while upperdir nonexistent\n"); return ERR_PTR(-EINVAL); - } else if (!ofs->config.upperdir && ofs->config.nfs_export && - ofs->config.redirect_follow) { - pr_warn("NFS export requires \"redirect_dir=nofollow\" on non-upper mount, falling back to nfs_export=off.\n"); - ofs->config.nfs_export = false; } stack = kcalloc(numlower, sizeof(struct path), GFP_KERNEL); -- cgit v1.2.3 From 81a33c1ee941c3bb9ffc6bac8f676be13351344e Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Thu, 18 Jun 2020 18:43:53 +0300 Subject: ovl: fix unneeded call to ovl_change_flags() The check if user has changed the overlay file was wrong, causing unneeded call to ovl_change_flags() including taking f_lock on every file access. Fixes: d989903058a8 ("ovl: do not generate duplicate fsnotify events for "fake" path") Cc: # v4.19+ Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/file.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 01820e654a21..0d940e29d62b 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -33,13 +33,16 @@ static char ovl_whatisit(struct inode *inode, struct inode *realinode) return 'm'; } +/* No atime modificaton nor notify on underlying */ +#define OVL_OPEN_FLAGS (O_NOATIME | FMODE_NONOTIFY) + static struct file *ovl_open_realfile(const struct file *file, struct inode *realinode) { struct inode *inode = file_inode(file); struct file *realfile; const struct cred *old_cred; - int flags = file->f_flags | O_NOATIME | FMODE_NONOTIFY; + int flags = file->f_flags | OVL_OPEN_FLAGS; int acc_mode = ACC_MODE(flags); int err; @@ -72,8 +75,7 @@ static int ovl_change_flags(struct file *file, unsigned int flags) struct inode *inode = file_inode(file); int err; - /* No atime modificaton on underlying */ - flags |= O_NOATIME | FMODE_NONOTIFY; + flags |= OVL_OPEN_FLAGS; /* If some flag changed that cannot be changed then something's amiss */ if (WARN_ON((file->f_flags ^ flags) & ~OVL_SETFL_MASK)) @@ -126,7 +128,7 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real, } /* Did the flags change since open? */ - if (unlikely((file->f_flags ^ real->file->f_flags) & ~O_NOATIME)) + if (unlikely((file->f_flags ^ real->file->f_flags) & ~OVL_OPEN_FLAGS)) return ovl_change_flags(real->file, file->f_flags); return 0; -- cgit v1.2.3 From 4518dfcf761e3c44632855abcf433236cf7ab6c6 Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Wed, 15 Jul 2020 16:38:08 +0300 Subject: ovl: fix lookup of indexed hardlinks with metacopy We recently moved setting inode flag OVL_UPPERDATA to ovl_lookup(). When looking up an overlay dentry, upperdentry may be found by index and not by name. In that case, we fail to read the metacopy xattr and falsly set the OVL_UPPERDATA on the overlay inode. This caused a regression in xfstest overlay/033 when run with OVERLAY_MOUNT_OPTIONS="-o metacopy=on". Fixes: 28166ab3c875 ("ovl: initialize OVL_UPPERDATA in ovl_lookup()") Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/namei.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 3cad68c3efb2..f7d4358db637 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -1066,6 +1066,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, upperredirect = NULL; goto out_free_oe; } + err = ovl_check_metacopy_xattr(upperdentry); + if (err < 0) + goto out_free_oe; + uppermetacopy = err; } if (upperdentry || ctr) { -- cgit v1.2.3