Re: [PATCH 09/11] VFS/ovl/smb: introduce start_renaming_dentry()

[NeilBrown <neilb@ownmail.net>]

On Tue, 30 Sep 2025, Amir Goldstein wrote:
> On Fri, Sep 26, 2025 at 4:51 AM NeilBrown <neilb@ownmail.net> wrote:
> > + * On success the found dentry is stored in @rd.new_dentry and
> > + * @rd.old_parent is confirmed to be the parent of @old_dentry.  If it
> > + * was originally %NULL, it is set.  In either case a refernence is taken.
> 
> Typo: %NULL, typo: refernence

%NULL isn't a typo.  As documented in kernel-doc.rst

   %CONST
   should be used to give appropriate formatting to constant.

Thanks for the refernence fix!

NeilBrown


> 
> > + *
> > + * References and the lock can be dropped with end_renaming()
> > + *
> > + * The passed in qstr need not have the hash calculated, and basic
> > + * eXecute permission checking is performed against @rd.mnt_idmap.
> > + *
> > + * Returns: zero or an error.
> > + */
> > +int start_renaming_dentry(struct renamedata *rd, int lookup_flags,
> > +                         struct dentry *old_dentry, struct qstr *new_last)
> > +{
> > +       int err;
> > +
> > +       err = lookup_one_common(rd->mnt_idmap, new_last, rd->new_parent);
> > +       if (err)
> > +               return err;
> > +       return __start_renaming_dentry(rd, lookup_flags, old_dentry, new_last);
> > +}
> > +
> >  void end_renaming(struct renamedata *rd)
> >  {
> >         unlock_rename(rd->old_parent, rd->new_parent);
> >         dput(rd->old_dentry);
> >         dput(rd->new_dentry);
> > +       dput(rd->old_parent);
> >  }
> >  EXPORT_SYMBOL(end_renaming);
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 6a31ea34ff80..3f19548b5d48 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -523,8 +523,8 @@ 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 dentry *index = NULL;
> >         struct dentry *temp = NULL;
> > +       struct renamedata rd = {};
> >         struct qstr name = { };
> >         int err;
> >
> > @@ -556,17 +556,15 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> >         if (err)
> >                 goto out;
> >
> > -       err = ovl_parent_lock(indexdir, temp);
> > +       rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> > +       rd.old_parent = indexdir;
> > +       rd.new_parent = indexdir;
> > +       err = start_renaming_dentry(&rd, 0, temp, &name);
> >         if (err)
> >                 goto out;
> > -       index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
> > -       if (IS_ERR(index)) {
> > -               err = PTR_ERR(index);
> > -       } else {
> > -               err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> > -               dput(index);
> > -       }
> > -       ovl_parent_unlock(indexdir);
> > +
> > +       err = ovl_do_rename_rd(&rd);
> > +       end_renaming(&rd);
> >  out:
> >         if (err)
> >                 ovl_cleanup(ofs, indexdir, temp);
> > @@ -763,7 +761,8 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >         struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
> >         struct inode *inode;
> >         struct path path = { .mnt = ovl_upper_mnt(ofs) };
> > -       struct dentry *temp, *upper, *trap;
> > +       struct renamedata rd = {};
> > +       struct dentry *temp;
> >         struct ovl_cu_creds cc;
> >         int err;
> >         struct ovl_cattr cattr = {
> > @@ -807,29 +806,27 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >          * ovl_copy_up_data(), so lock workdir and destdir and make sure that
> >          * temp wasn't moved before copy up completion or cleanup.
> >          */
> > -       trap = lock_rename(c->workdir, c->destdir);
> > -       if (trap || temp->d_parent != c->workdir) {
> > +       rd.mnt_idmap = ovl_upper_mnt_idmap(ofs);
> > +       rd.old_parent = c->workdir;
> > +       rd.new_parent = c->destdir;
> > +       rd.flags = 0;
> > +       err = start_renaming_dentry(&rd, 0, temp,
> > +                                   &QSTR_LEN(c->destname.name, c->destname.len));
> > +       if (err == -EINVAL || err == -EXDEV) {
> 
> This error code whitelist is not needed and is too fragile anyway.
> After your commit
> 9d23967b18c64 ("ovl: simplify an error path in ovl_copy_up_workdir()")
> any locking error is treated the same - it does not matter what the
> reason for lock_rename() or start_renaming_dentry() is.
> 
> >                 /* temp or workdir moved underneath us? abort without cleanup */
> >                 dput(temp);
> >                 err = -EIO;
> > -               if (!IS_ERR(trap))
> > -                       unlock_rename(c->workdir, c->destdir);
> >                 goto out;
> >         }
> 
> Frankly, we could get rid of the "abort without cleanup"
> comment and instead: err = -EIO; goto cleanup_unlocked;
> because before cleanup_unlocked, cleanup was relying on the
> lock_rename() to take the lock for the cleanup, but we don't need
> that anymore.
> 
> To be clear, I don't think is it important to goto cleanup_unlocked,
> leaving goto out is fine because we are not very sympathetic
> to changes to underlying layers while ovl is mounted, so we should
> not really care about this cleanup, but for the sake of simpler code
> I wouldn't mind the goto cleanup_unlocked.
> 
> > -
> > -       err = ovl_copy_up_metadata(c, temp);
> >         if (err)
> >                 goto cleanup;
> 
> Is this right? should we be calling end_renaming() on error?
> 
> Thanks,
> Amir.
>