On Fri, 11 Jul 2025, Amir Goldstein wrote:
> 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>
Done - thanks.
NeilBrown
>
> 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
> >
>
