On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> ovl currently locks a directory or two and then performs multiple actions
> in one or both directories. This is incompatible with proposed changes
> which will lock just the dentry of objects being acted on.
>
> This patch moves calls to ovl_create_temp() out of the locked regions and
> has it take and release the relevant lock itself.
>
> The lock that was taken before this function was called is now taken
> after. This means that any code between where the lock was taken and
> ovl_create_temp() is now unlocked. This necessitates the use of
> ovl_cleanup_unlocked() and the creation of ovl_lookup_upper_unlocked().
> These will be used more widely in future patches.
>
> Now that the file is created before the lock is taken for rename, we
> need to ensure the parent wasn't changed before the lock was gained.
> ovl_lock_rename_workdir() is changed to optionally receive the dentries
> that will be involved in the rename. If either is present but has the
> wrong parent, an error is returned.
>
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> ---
> fs/overlayfs/copy_up.c | 5 ---
> fs/overlayfs/dir.c | 67 ++++++++++++++++++++--------------------
> fs/overlayfs/overlayfs.h | 12 ++++++-
> fs/overlayfs/super.c | 11 ++++---
> fs/overlayfs/util.c | 7 ++++-
> 5 files changed, 58 insertions(+), 44 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 25be0b80a40b..eafb46686854 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -523,7 +523,6 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> {
> struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
> - struct inode *dir = d_inode(indexdir);
> struct dentry *index = NULL;
> struct dentry *temp = NULL;
> struct qstr name = { };
> @@ -549,9 +548,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> return err;
>
> ovl_start_write(dentry);
> - inode_lock(dir);
> temp = ovl_create_temp(ofs, indexdir, OVL_CATTR(S_IFDIR | 0));
> - inode_unlock(dir);
> err = PTR_ERR(temp);
> if (IS_ERR(temp))
> goto free_name;
> @@ -785,9 +782,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> return err;
>
> ovl_start_write(c->dentry);
> - inode_lock(wdir);
> temp = ovl_create_temp(ofs, c->workdir, &cattr);
> - inode_unlock(wdir);
> ovl_end_write(c->dentry);
> ovl_revert_cu_creds(&cc);
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index cee35d69e0e6..144e1753d0c9 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -214,8 +214,12 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct inode *dir,
> struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
> struct ovl_cattr *attr)
> {
> - return ovl_create_real(ofs, d_inode(workdir),
> - ovl_lookup_temp(ofs, workdir), attr);
> + struct dentry *ret;
> + inode_lock(workdir->d_inode);
> + ret = ovl_create_real(ofs, d_inode(workdir),
> + ovl_lookup_temp(ofs, workdir), attr);
> + inode_unlock(workdir->d_inode);
> + return ret;
> }
>
> static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
> @@ -353,7 +357,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> struct dentry *workdir = ovl_workdir(dentry);
> struct inode *wdir = workdir->d_inode;
> struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
> - struct inode *udir = upperdir->d_inode;
> struct path upperpath;
> struct dentry *upper;
> struct dentry *opaquedir;
> @@ -363,28 +366,25 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> if (WARN_ON(!workdir))
> return ERR_PTR(-EROFS);
>
> - err = ovl_lock_rename_workdir(workdir, upperdir);
> - if (err)
> - goto out;
> -
> ovl_path_upper(dentry, &upperpath);
> err = vfs_getattr(&upperpath, &stat,
> STATX_BASIC_STATS, AT_STATX_SYNC_AS_STAT);
> if (err)
> - goto out_unlock;
> + goto out;
>
> err = -ESTALE;
> if (!S_ISDIR(stat.mode))
> - goto out_unlock;
> + goto out;
> upper = upperpath.dentry;
> - if (upper->d_parent->d_inode != udir)
> - goto out_unlock;
>
> opaquedir = ovl_create_temp(ofs, workdir, OVL_CATTR(stat.mode));
> err = PTR_ERR(opaquedir);
> if (IS_ERR(opaquedir))
> - goto out_unlock;
> -
> + /* workdir was unlocked, no upperdir */
> + goto out;
Strong lint error here. Don't use multi lines (inc. comments) without {}
TBH this comment adds no clarity for me. I would remove it.
> + err = ovl_lock_rename_workdir(workdir, opaquedir, upperdir, upper);
> + if (err)
> + goto out_cleanup_unlocked;
Nit: please keep the empty line after goto as it was in the code before.
I removed this empty line in other patches as well and it hurts my eyes.
I know we do not have a 100% consistent style in that regard in overlayfs code
(e.g. S_ISDIR check above), but please try to avoid changing the existing
style of code in that regard.
> err = ovl_copy_xattr(dentry->d_sb, &upperpath, opaquedir);
> if (err)
> goto out_cleanup;
> @@ -413,10 +413,10 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
> return opaquedir;
>
> out_cleanup:
> - ovl_cleanup(ofs, wdir, opaquedir);
> - dput(opaquedir);
> -out_unlock:
> unlock_rename(workdir, upperdir);
> +out_cleanup_unlocked:
> + ovl_cleanup_unlocked(ofs, workdir, opaquedir);
> + dput(opaquedir);
> out:
> return ERR_PTR(err);
> }
> @@ -454,15 +454,11 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> return err;
> }
>
> - err = ovl_lock_rename_workdir(workdir, upperdir);
> - if (err)
> - goto out;
> -
> - upper = ovl_lookup_upper(ofs, dentry->d_name.name, upperdir,
> - dentry->d_name.len);
> + upper = ovl_lookup_upper_unlocked(ofs, dentry->d_name.name, upperdir,
> + dentry->d_name.len);
> err = PTR_ERR(upper);
> if (IS_ERR(upper))
> - goto out_unlock;
> + goto out;
>
> err = -ESTALE;
> if (d_is_negative(upper) || !ovl_upper_is_whiteout(ofs, upper))
> @@ -473,6 +469,10 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> if (IS_ERR(newdentry))
> goto out_dput;
>
> + err = ovl_lock_rename_workdir(workdir, newdentry, upperdir, upper);
> + if (err)
> + goto out_cleanup;
> +
goto out_cleanup_unlocked here please
and leave the rest of the goto cleanup be
just like you did in ovl_clear_empty().
This looks way better than v1 patch 2 that overflowed my review context stack.
With minor nits above fixed, feel free to add:
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
Thanks,
Amir.
