On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> Rather than locking the directory(s) before calling
> ovl_cleanup_and_whiteout(), change it (and ovl_whiteout()) to do the
> locking, so the locking can be fine grained as will be needed for
> proposed locking changes.
>
> Sometimes this is called to whiteout something in the index dir, in
> which case only that dir must be locked. In one case it is called on
> something in an upperdir, so two directories must be locked. We use
> ovl_lock_rename_workdir() for this and remove the restriction that
> upperdir cannot be indexdir - because now sometimes it is.
>
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
Thanks,
Amir.
> ---
> fs/overlayfs/dir.c | 20 +++++++++-----------
> fs/overlayfs/readdir.c | 3 ---
> fs/overlayfs/util.c | 7 -------
> 3 files changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 8580cd5c61e4..086719129be3 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -77,7 +77,6 @@ struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
> return temp;
> }
>
> -/* caller holds i_mutex on workdir */
> static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> {
> int err;
> @@ -85,6 +84,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> struct dentry *workdir = ofs->workdir;
> struct inode *wdir = workdir->d_inode;
>
> + inode_lock_nested(wdir, I_MUTEX_PARENT);
> if (!ofs->whiteout) {
> whiteout = ovl_lookup_temp(ofs, workdir);
> if (IS_ERR(whiteout))
> @@ -118,14 +118,13 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> whiteout = ofs->whiteout;
> ofs->whiteout = NULL;
> out:
> + inode_unlock(wdir);
> return whiteout;
> }
>
> -/* Caller must hold i_mutex on both workdir and dir */
> int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> struct dentry *dentry)
> {
> - struct inode *wdir = ofs->workdir->d_inode;
> struct dentry *whiteout;
> int err;
> int flags = 0;
> @@ -138,18 +137,22 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> if (d_is_dir(dentry))
> flags = RENAME_EXCHANGE;
>
> - err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
> + err = ovl_lock_rename_workdir(ofs->workdir, whiteout, dir, dentry);
> + if (!err) {
> + err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
> + unlock_rename(ofs->workdir, dir);
> + }
> if (err)
> goto kill_whiteout;
> if (flags)
> - ovl_cleanup(ofs, wdir, dentry);
> + ovl_cleanup_unlocked(ofs, ofs->workdir, dentry);
>
> out:
> dput(whiteout);
> return err;
>
> kill_whiteout:
> - ovl_cleanup(ofs, wdir, whiteout);
> + ovl_cleanup_unlocked(ofs, ofs->workdir, whiteout);
> goto out;
> }
>
> @@ -782,10 +785,6 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> goto out_dput_upper;
> }
>
> - err = ovl_lock_rename_workdir(workdir, NULL, upperdir, upper);
> - if (err)
> - goto out_dput_upper;
> -
> err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
> if (err)
> goto out_d_drop;
> @@ -793,7 +792,6 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> ovl_dir_modified(dentry->d_parent, true);
> out_d_drop:
> d_drop(dentry);
> - unlock_rename(workdir, upperdir);
> out_dput_upper:
> dput(upper);
> out_dput:
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 6cc5f885e036..4127d1f160b3 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1179,7 +1179,6 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> int err;
> struct dentry *indexdir = ofs->workdir;
> struct dentry *index = NULL;
> - struct inode *dir = indexdir->d_inode;
> struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = indexdir };
> LIST_HEAD(list);
> struct ovl_cache_entry *p;
> @@ -1231,9 +1230,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> * Whiteout orphan index to block future open by
> * handle after overlay nlink dropped to zero.
> */
> - inode_lock_nested(dir, I_MUTEX_PARENT);
> err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
> - inode_unlock(dir);
> } else {
> /* Cleanup orphan index entries */
> err = ovl_cleanup_unlocked(ofs, indexdir, index);
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 7369193b11ec..5218a477551b 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1071,7 +1071,6 @@ static void ovl_cleanup_index(struct dentry *dentry)
> {
> struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
> - struct inode *dir = indexdir->d_inode;
> struct dentry *lowerdentry = ovl_dentry_lower(dentry);
> struct dentry *upperdentry = ovl_dentry_upper(dentry);
> struct dentry *index = NULL;
> @@ -1113,10 +1112,8 @@ static void ovl_cleanup_index(struct dentry *dentry)
> index = NULL;
> } else if (ovl_index_all(dentry->d_sb)) {
> /* Whiteout orphan index to block future open by handle */
> - inode_lock_nested(dir, I_MUTEX_PARENT);
> err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
> indexdir, index);
> - inode_unlock(dir);
> } else {
> /* Cleanup orphan index entries */
> err = ovl_cleanup_unlocked(ofs, indexdir, index);
> @@ -1224,10 +1221,6 @@ int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> {
> struct dentry *trap;
>
> - /* Workdir should not be the same as upperdir */
> - if (workdir == upperdir)
> - goto err;
> -
> /* Workdir should not be subdir of upperdir and vice versa */
> trap = lock_rename(workdir, upperdir);
> if (IS_ERR(trap))
> --
> 2.49.0
>
