On Fri, Jul 11, 2025 at 10:25 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote:
> >
> > If ovl_copy_up_data() fails the error is not immediately handled but the
> > code continues on to call ovl_start_write() and lock_rename(),
> > presumably because both of these locks are needed for the cleanup.
> > On then (if the lock was successful) is the error checked.
> >
> > This makes the code a little hard to follow and could be fragile.
> >
> > This patch changes to handle the error immediately. A new
> > ovl_cleanup_unlocked() is created which takes the required directory
> > lock (though it doesn't take the write lock on the filesystem). This
> > will be used extensively in later patches.
> >
> > In general we need to check the parent is still correct after taking the
> > lock (as ovl_copy_up_workdir() does after a successful lock_rename()) so
> > that is included in ovl_cleanup_unlocked() using new lock_parent() and
> > unlock_parent() calls (it is planned to move this API into VFS code
> > eventually, though in a slightly different form).
>
> Since you are not planning to move it to VFS with this name
> AND since I assume you want to merge this ovl cleanup prior
> to the rest of of patches, please use an ovl helper without
> the ovl_ namespace prefix and you have a typo above
> its parent_lock() not lock_parent().
>
> And apropos lock helper names, at the tip of your branch
> the lock helpers used in ovl_cleanup() are named:
> lock_and_check_dentry()/dentry_unlock()
>
> I have multiple comments on your choice of names for those helpers:
> 1. Please use a consistent name pattern for lock/unlock.
> The pattern <obj-or-lock-type>_{lock,unlock}_* is far more common
> then the pattern lock_<obj-or-lock-type> in the kernel, but at least
> be consistent with dentry_lock_and_check() or better yet
> parent_lock() and later parent_lock_get_child()
> 2. dentry_unlock() is a very strange name for a helper that
> unlocks the parent. The fact that you document what it does
> in Kernel-doc does not stop people reading the code using it
> from being confused and writing bugs.
> 3. Why not call it parnet_unlock() like I suggested and like you
> used in this patch set and why not introduce it in VFS to begin with?
> For that matter parent_unlock_{put,return}_child() is more clear IMO.
> 4. The name dentry_unlock_rename(&rd) also does not balance nicely with
> the name lookup_and_lock_rename(&rd) and has nothing to do with the
> dentry_ prefix. How about lookup_done_and_unlock_rename(&rd)?
>
> Hope this is not too much complaining for review of a small cleanup patch :-p
>
> >
> > A fresh cleanup block is added which doesn't share code with other
> > cleanup blocks. It will get a new users in the next patch.
> >
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> > fs/overlayfs/copy_up.c | 12 ++++++++++--
> > fs/overlayfs/dir.c | 15 +++++++++++++++
> > fs/overlayfs/overlayfs.h | 6 ++++++
> > fs/overlayfs/util.c | 10 ++++++++++
> > 4 files changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 8a3c0d18ec2e..5d21b8d94a0a 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -794,6 +794,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > */
> > path.dentry = temp;
> > err = ovl_copy_up_data(c, &path);
> > + 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
> > @@ -809,8 +812,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > if (IS_ERR(trap))
> > goto out;
> > goto unlock;
> > - } else if (err) {
> > - goto cleanup;
> > }
> >
> > err = ovl_copy_up_metadata(c, temp);
> > @@ -857,6 +858,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > ovl_cleanup(ofs, wdir, temp);
> > dput(temp);
> > goto unlock;
> > +
> > +cleanup_need_write:
> > + ovl_start_write(c->dentry);
> > + ovl_cleanup_unlocked(ofs, c->workdir, temp);
> > + ovl_end_write(c->dentry);
> > + dput(temp);
> > + return err;
> > }
> >
>
> Sorry, I will not accept more messy goto routines.
> I rewrote your simplification based on the tip of your branch.
> Much simpler and no need for this extra routine.
> Just always use ovl_cleanup_unlocked() in this function and
> ovl_start_write() before goto cleanup_unlocked:
>
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -794,13 +794,16 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> */
> path.dentry = temp;
> err = ovl_copy_up_data(c, &path);
> + ovl_start_write(c->dentry);
> + if (err)
> + goto cleanup_unlocked;
> +
> /*
> * We cannot hold lock_rename() throughout this helper, because of
> * lock ordering with sb_writers, which shouldn't be held when calling
> * ovl_copy_up_data(), so lock workdir and destdir and make sure that
> * temp wasn't moved before copy up completion or cleanup.
> */
> - ovl_start_write(c->dentry);
> trap = lock_rename(c->workdir, c->destdir);
> if (trap || temp->d_parent != c->workdir) {
> /* temp or workdir moved underneath us? abort without cleanup */
> @@ -809,8 +812,6 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> if (IS_ERR(trap))
> goto out;
> goto unlock;
> - } else if (err) {
> - goto cleanup;
> }
>
> err = ovl_copy_up_metadata(c, temp);
> @@ -846,17 +847,17 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> ovl_inode_update(inode, temp);
> if (S_ISDIR(inode->i_mode))
> ovl_set_flag(OVL_WHITEOUTS, inode);
> -unlock:
> - unlock_rename(c->workdir, c->destdir);
> out:
> ovl_end_write(c->dentry);
>
> return err;
>
> cleanup:
> - ovl_cleanup(ofs, wdir, temp);
> + unlock_rename(c->workdir, c->destdir);
> +cleanup_unlocked:
> + ovl_cleanup_unlocked(ofs, wdir, temp);
> dput(temp);
> - goto unlock;
> + goto out;
> }
> ---
>
> > /* Copyup using O_TMPFILE which does not require cross dir locking */
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 4fc221ea6480..cee35d69e0e6 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -43,6 +43,21 @@ int ovl_cleanup(struct ovl_fs *ofs, struct inode *wdir, struct dentry *wdentry)
> > return err;
> > }
> >
> > +int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir,
> > + struct dentry *wdentry)
> > +{
> > + int err;
> > +
> > + err = parent_lock(workdir, wdentry);
> > + if (err)
> > + return err;
> > +
> > + ovl_cleanup(ofs, workdir->d_inode, wdentry);
> > + parent_unlock(workdir);
> > +
> > + return err;
> > +}
> > +
> > struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir)
> > {
> > struct dentry *temp;
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 42228d10f6b9..68dc78c712a8 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -416,6 +416,11 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
> > }
> >
> > /* util.c */
> > +int parent_lock(struct dentry *parent, struct dentry *child);
> > +static inline void parent_unlock(struct dentry *parent)
> > +{
> > + inode_unlock(parent->d_inode);
> > +}
>
> ovl_parent_unlock() or move to vfs please.
>
> > int ovl_get_write_access(struct dentry *dentry);
> > void ovl_put_write_access(struct dentry *dentry);
> > void ovl_start_write(struct dentry *dentry);
> > @@ -843,6 +848,7 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs,
> > struct inode *dir, struct dentry *newdentry,
> > struct ovl_cattr *attr);
> > int ovl_cleanup(struct ovl_fs *ofs, struct inode *dir, struct dentry *dentry);
> > +int ovl_cleanup_unlocked(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
> > struct dentry *ovl_lookup_temp(struct ovl_fs *ofs, struct dentry *workdir);
> > struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
> > struct ovl_cattr *attr);
> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> > index 2b4754c645ee..a5105d68f6b4 100644
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -1544,3 +1544,13 @@ void ovl_copyattr(struct inode *inode)
> > i_size_write(inode, i_size_read(realinode));
> > spin_unlock(&inode->i_lock);
> > }
> > +
> > +int parent_lock(struct dentry *parent, struct dentry *child)
> > +{
> > + inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> > + if (!child || child->d_parent == parent)
> > + return 0;
> > +
> > + inode_unlock(parent->d_inode);
> > + return -EINVAL;
> > +}
>
> ovl_parent_lock() or move to vfs please.
>
BTW, I prefer to define them in vfs if I wasn't clear (in a separate patch)
Where you can later rename them to:
parent_lock_get_child()/parent_unlock_put_child()
and fork the parallel lookup variants.
Thanks,
Amir.
