From 34b4540e6646577d02afacfcbc08e563b7d69a9f Mon Sep 17 00:00:00 2001 From: Haifeng Xu Date: Tue, 30 Jul 2024 12:20:08 +0800 Subject: [PATCH 1/4] ovl: don't set the superblock's errseq_t manually Since commit 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs"), the return value from sync_fs callback can be seen in sync_filesystem(). Thus the errseq_set opreation can be removed here. Depends-on: commit 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs") Signed-off-by: Haifeng Xu Reviewed-by: Amir Goldstein Signed-off-by: Amir Goldstein --- fs/overlayfs/super.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 06a231970cb5..fe511192f83c 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -202,15 +202,9 @@ static int ovl_sync_fs(struct super_block *sb, int wait) int ret; ret = ovl_sync_status(ofs); - /* - * We have to always set the err, because the return value isn't - * checked in syncfs, and instead indirectly return an error via - * the sb's writeback errseq, which VFS inspects after this call. - */ - if (ret < 0) { - errseq_set(&sb->s_wb_err, -EIO); + + if (ret < 0) return -EIO; - } if (!ret) return ret; From 7d6899fb69d25e1bc6f4700b7c1d92e6b608593d Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Thu, 29 Aug 2024 17:51:08 +0200 Subject: [PATCH 2/4] ovl: fsync after metadata copy-up For upper filesystems which do not use strict ordering of persisting metadata changes (e.g. ubifs), when overlayfs file is modified for the first time, copy up will create a copy of the lower file and its parent directories in the upper layer. Permission lost of the new upper parent directory was observed during power-cut stress test. Fix by moving the fsync call to after metadata copy to make sure that the metadata copied up directory and files persists to disk before renaming from tmp to final destination. With metacopy enabled, this change will hurt performance of workloads such as chown -R, so we keep the legacy behavior of fsync only on copyup of data. Link: https://lore.kernel.org/linux-unionfs/CAOQ4uxj-pOvmw1-uXR3qVdqtLjSkwcR9nVKcNU_vC10Zyf2miQ@mail.gmail.com/ Reported-and-tested-by: Fei Lv Signed-off-by: Amir Goldstein --- fs/overlayfs/copy_up.c | 43 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index a5ef2005a2cc..051a802893a1 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -243,8 +243,24 @@ static int ovl_verify_area(loff_t pos, loff_t pos2, loff_t len, loff_t totlen) return 0; } +static int ovl_sync_file(struct path *path) +{ + struct file *new_file; + int err; + + new_file = ovl_path_open(path, O_LARGEFILE | O_RDONLY); + if (IS_ERR(new_file)) + return PTR_ERR(new_file); + + err = vfs_fsync(new_file, 0); + fput(new_file); + + return err; +} + static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, - struct file *new_file, loff_t len) + struct file *new_file, loff_t len, + bool datasync) { struct path datapath; struct file *old_file; @@ -342,7 +358,8 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, len -= bytes; } - if (!error && ovl_should_sync(ofs)) + /* call fsync once, either now or later along with metadata */ + if (!error && ovl_should_sync(ofs) && datasync) error = vfs_fsync(new_file, 0); out_fput: fput(old_file); @@ -574,6 +591,7 @@ struct ovl_copy_up_ctx { bool indexed; bool metacopy; bool metacopy_digest; + bool metadata_fsync; }; static int ovl_link_up(struct ovl_copy_up_ctx *c) @@ -634,7 +652,8 @@ static int ovl_copy_up_data(struct ovl_copy_up_ctx *c, const struct path *temp) if (IS_ERR(new_file)) return PTR_ERR(new_file); - err = ovl_copy_up_file(ofs, c->dentry, new_file, c->stat.size); + err = ovl_copy_up_file(ofs, c->dentry, new_file, c->stat.size, + !c->metadata_fsync); fput(new_file); return err; @@ -701,6 +720,10 @@ static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp) err = ovl_set_attr(ofs, temp, &c->stat); inode_unlock(temp->d_inode); + /* fsync metadata before moving it into upper dir */ + if (!err && ovl_should_sync(ofs) && c->metadata_fsync) + err = ovl_sync_file(&upperpath); + return err; } @@ -860,7 +883,8 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c) temp = tmpfile->f_path.dentry; if (!c->metacopy && c->stat.size) { - err = ovl_copy_up_file(ofs, c->dentry, tmpfile, c->stat.size); + err = ovl_copy_up_file(ofs, c->dentry, tmpfile, c->stat.size, + !c->metadata_fsync); if (err) goto out_fput; } @@ -1135,6 +1159,17 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry, !kgid_has_mapping(current_user_ns(), ctx.stat.gid)) return -EOVERFLOW; + /* + * With metacopy disabled, we fsync after final metadata copyup, for + * both regular files and directories to get atomic copyup semantics + * on filesystems that do not use strict metadata ordering (e.g. ubifs). + * + * With metacopy enabled we want to avoid fsync on all meta copyup + * that will hurt performance of workloads such as chown -R, so we + * only fsync on data copyup as legacy behavior. + */ + ctx.metadata_fsync = !OVL_FS(dentry->d_sb)->config.metacopy && + (S_ISREG(ctx.stat.mode) || S_ISDIR(ctx.stat.mode)); ctx.metacopy = ovl_need_meta_copy_up(dentry, ctx.stat.mode, flags); if (parent) { From 930b7c32ea2b514fb2c37aa3d4b946d954ee7fa2 Mon Sep 17 00:00:00 2001 From: Yuriy Belikov Date: Wed, 4 Sep 2024 19:54:29 +0300 Subject: [PATCH 3/4] overlayfs.rst: update metacopy section in overlayfs documentation - Provide info about trusted.overlay.metacopy extended attribute - Minor rephrasing regarding copy-up operation with metacopy=on Signed-off-by: Yuriy Belikov Signed-off-by: Amir Goldstein --- Documentation/filesystems/overlayfs.rst | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst index 165514401441..343644712340 100644 --- a/Documentation/filesystems/overlayfs.rst +++ b/Documentation/filesystems/overlayfs.rst @@ -367,8 +367,11 @@ Metadata only copy up When the "metacopy" feature is enabled, overlayfs will only copy up metadata (as opposed to whole file), when a metadata specific operation -like chown/chmod is performed. Full file will be copied up later when -file is opened for WRITE operation. +like chown/chmod is performed. An upper file in this state is marked with +"trusted.overlayfs.metacopy" xattr which indicates that the upper file +contains no data. The data will be copied up later when file is opened for +WRITE operation. After the lower file's data is copied up, +the "trusted.overlayfs.metacopy" xattr is removed from the upper file. In other words, this is delayed data copy up operation and data is copied up when there is a need to actually modify data. From 6c4a5f96450415735c31ed70ff354f0ee5cbf67b Mon Sep 17 00:00:00 2001 From: Mike Baynton Date: Wed, 10 Jul 2024 22:52:04 -0500 Subject: [PATCH 4/4] ovl: fail if trusted xattrs are needed but caller lacks permission Some overlayfs features require permission to read/write trusted.* xattrs. These include redirect_dir, verity, metacopy, and data-only layers. This patch adds additional validations at mount time to stop overlays from mounting in certain cases where the resulting mount would not function according to the user's expectations because they lack permission to access trusted.* xattrs (for example, not global root.) Similar checks in ovl_make_workdir() that disable features instead of failing are still relevant and used in cases where the resulting mount can still work "reasonably well." Generally, if the feature was enabled through kernel config or module option, any mount that worked before will still work the same; this applies to redirect_dir and metacopy. The user must explicitly request these features in order to generate a mount failure. Verity and data-only layers on the other hand must be explictly requested and have no "reasonable" disabled or degraded alternative, so mounts attempting either always fail. "lower data-only dirs require metacopy support" moved down in case userxattr is set, which disables metacopy. Cc: stable@vger.kernel.org # v6.6+ Signed-off-by: Mike Baynton Signed-off-by: Amir Goldstein --- fs/overlayfs/params.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c index d0568c091341..e42546c6c5df 100644 --- a/fs/overlayfs/params.c +++ b/fs/overlayfs/params.c @@ -755,11 +755,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx, { struct ovl_opt_set set = ctx->set; - if (ctx->nr_data > 0 && !config->metacopy) { - pr_err("lower data-only dirs require metacopy support.\n"); - return -EINVAL; - } - /* Workdir/index are useless in non-upper mount */ if (!config->upperdir) { if (config->workdir) { @@ -911,6 +906,39 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx, config->metacopy = false; } + /* + * Fail if we don't have trusted xattr capability and a feature was + * explicitly requested that requires them. + */ + if (!config->userxattr && !capable(CAP_SYS_ADMIN)) { + if (set.redirect && + config->redirect_mode != OVL_REDIRECT_NOFOLLOW) { + pr_err("redirect_dir requires permission to access trusted xattrs\n"); + return -EPERM; + } + if (config->metacopy && set.metacopy) { + pr_err("metacopy requires permission to access trusted xattrs\n"); + return -EPERM; + } + if (config->verity_mode) { + pr_err("verity requires permission to access trusted xattrs\n"); + return -EPERM; + } + if (ctx->nr_data > 0) { + pr_err("lower data-only dirs require permission to access trusted xattrs\n"); + return -EPERM; + } + /* + * Other xattr-dependent features should be disabled without + * great disturbance to the user in ovl_make_workdir(). + */ + } + + if (ctx->nr_data > 0 && !config->metacopy) { + pr_err("lower data-only dirs require metacopy support.\n"); + return -EINVAL; + } + return 0; }