On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> ovl_copy_up_workdir() currently take a rename lock on two directories,
> then use the lock to both create a file in one directory, perform a
> rename, and possibly unlink the file for cleanup. This is incompatible
> with proposed changes which will lock just the dentry of objects being
> acted on.
>
> This patch moves the call to ovl_create_index() earlier in
> ovl_copy_up_workdir() to before the lock is taken, and also before write
> access to the filesystem is gained (this last is not strictly necessary
> but seems cleaner).
With my proposed change to patch 1, ovl_create_index() will be
called with ovl_start_write() held so you wont need to add it.
>
> ovl_create_index() then take the requires locks and drops them before
> returning.
>
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
With that fixed, feel free to add:
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
Thanks,
Amir.
> ---
> fs/overlayfs/copy_up.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 5d21b8d94a0a..25be0b80a40b 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -517,8 +517,6 @@ static int ovl_set_upper_fh(struct ovl_fs *ofs, struct dentry *upper,
>
> /*
> * Create and install index entry.
> - *
> - * Caller must hold i_mutex on indexdir.
> */
> static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> struct dentry *upper)
> @@ -550,7 +548,10 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> if (err)
> 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;
> @@ -559,6 +560,9 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> if (err)
> goto out;
>
> + err = parent_lock(indexdir, temp);
> + if (err)
> + goto out;
> index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
> if (IS_ERR(index)) {
> err = PTR_ERR(index);
> @@ -566,9 +570,11 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> dput(index);
> }
> + parent_unlock(indexdir);
> out:
> if (err)
> - ovl_cleanup(ofs, dir, temp);
> + ovl_cleanup_unlocked(ofs, indexdir, temp);
> + ovl_end_write(dentry);
> dput(temp);
> free_name:
> kfree(name.name);
> @@ -797,6 +803,12 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (err)
> goto cleanup_need_write;
>
> + if (S_ISDIR(c->stat.mode) && c->indexed) {
> + err = ovl_create_index(c->dentry, c->origin_fh, temp);
> + if (err)
> + goto cleanup_need_write;
> + }
> +
> /*
> * We cannot hold lock_rename() throughout this helper, because of
> * lock ordering with sb_writers, which shouldn't be held when calling
> @@ -818,12 +830,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (err)
> goto cleanup;
>
> - if (S_ISDIR(c->stat.mode) && c->indexed) {
> - err = ovl_create_index(c->dentry, c->origin_fh, temp);
> - if (err)
> - goto cleanup;
> - }
> -
> upper = ovl_lookup_upper(ofs, c->destname.name, c->destdir,
> c->destname.len);
> err = PTR_ERR(upper);
> --
> 2.49.0
>
