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.
Thanks,
Amir.
