On Wed, Jun 25, 2025 at 5:44 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@xxxxxxxxxx> wrote:
> >
> > ovl currently locks a directory or two and then performs multiple actions
> > in one or both directories. This is incompatible with proposed changes
> > which will lock just the dentry of objects being acted on.
> >
> > This patch moves calls to ovl_create_temp() and ovl_create_index() out
> > of the locked region and has them take and release the relevant lock
> > themselves.
> >
> > The lock that was taken before these functions are called is now taken
> > after. This means that any code between where the lock was taken and
> > these calls is now unlocked. This necessitates the creation of
> > _unlocked() versions of ovl_cleanup() and ovl_lookup_upper(). These
> > will be used more widely in future patches.
> >
> > ovl_cleanup_unlocked() takes a dentry for the directory rather than an
> > inode as this simplifies calling slightly.
> >
> > Note that when we move a lookup or create out of a locked region in
> > which the dentry is acted on, we need to ensure after taking the lock
> > that the dentry is still in the directory we expect it to be in. It is
> > conceivable that it was moved.
> >
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> > fs/overlayfs/copy_up.c | 37 +++++++++++-------
> > fs/overlayfs/dir.c | 84 +++++++++++++++++++++++++---------------
> > fs/overlayfs/overlayfs.h | 10 +++++
> > fs/overlayfs/super.c | 7 ++--
> > 4 files changed, 88 insertions(+), 50 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 8a3c0d18ec2e..7a21ad1f2b6e 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -517,15 +517,12 @@ 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)
> > {
> > struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > struct dentry *indexdir = ovl_indexdir(dentry->d_sb);
> > - struct inode *dir = d_inode(indexdir);
> > struct dentry *index = NULL;
> > struct dentry *temp = NULL;
> > struct qstr name = { };
> > @@ -558,17 +555,21 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
> > err = ovl_set_upper_fh(ofs, upper, temp);
> > if (err)
> > goto out;
> > -
> > + lock_rename(indexdir, indexdir);
>
> This is a really strange hack.
> I assume your next patch set is going to remove this call, but I do not wish
> to merge this hack as is for an unknown period of time.
>
> Please introduce helpers {un,}lock_parent()
>
> > index = ovl_lookup_upper(ofs, name.name, indexdir, name.len);
> > if (IS_ERR(index)) {
> > err = PTR_ERR(index);
> > + } else if (temp->d_parent != indexdir) {
> > + err = -EINVAL;
> > + dput(index);
>
> This can be inside lock_parent(parent, child)
> where child is an optional arg.
>
> err = lock_parent(indexdir, temp);
> if (err)
> goto out;
>
> Because we should be checking this right after lock and
> not after ovl_lookup_upper().
>
> > } else {
> > err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
> > dput(index);
> > }
> > + unlock_rename(indexdir, indexdir);
> > out:
> > if (err)
> > - ovl_cleanup(ofs, dir, temp);
> > + ovl_cleanup_unlocked(ofs, indexdir, temp);
> > dput(temp);
> > free_name:
> > kfree(name.name);
> > @@ -779,9 +780,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > return err;
> >
> > ovl_start_write(c->dentry);
> > - inode_lock(wdir);
> > temp = ovl_create_temp(ofs, c->workdir, &cattr);
> > - inode_unlock(wdir);
> > ovl_end_write(c->dentry);
> > ovl_revert_cu_creds(&cc);
> >
> > @@ -794,6 +793,8 @@ 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_write_unlocked;
> > /*
> > * We cannot hold lock_rename() throughout this helper, because of
> > * lock ordering with sb_writers, which shouldn't be held when calling
> > @@ -801,6 +802,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > * temp wasn't moved before copy up completion or cleanup.
> > */
> > ovl_start_write(c->dentry);
> > +
> > + if (S_ISDIR(c->stat.mode) && c->indexed) {
> > + err = ovl_create_index(c->dentry, c->origin_fh, temp);
> > + if (err)
> > + goto cleanup_unlocked;
> > + }
> > +
> > trap = lock_rename(c->workdir, c->destdir);
> > if (trap || temp->d_parent != c->workdir) {
> > /* temp or workdir moved underneath us? abort without cleanup */
> > @@ -809,20 +817,12 @@ 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);
> > 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);
> > @@ -857,6 +857,13 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> > ovl_cleanup(ofs, wdir, temp);
> > dput(temp);
> > goto unlock;
> > +
> > +cleanup_write_unlocked:
> > + ovl_start_write(c->dentry);
> > +cleanup_unlocked:
> > + ovl_cleanup_unlocked(ofs, c->workdir, temp);
> > + dput(temp);
> > + goto out;
> > }
>
> Wow these various cleanup flows are quite hard to follow.
> This is a massive patch set which is very hard for me to review
> and it will also be hard for me to maintain the code as it is.
> We need to figure out a way to simplify the code flow
> either more re-factoring or using some scoped cleanup hooks.
> I am open to suggestions.
>
> Thanks,
> Amir.
>
> >
> > /* 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..a51a3dc02bf5 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;
> > +
> > + inode_lock_nested(workdir->d_inode, I_MUTEX_PARENT);
> > + if (wdentry->d_parent == workdir)
> > + ovl_cleanup(ofs, workdir->d_inode, wdentry);
> > + else
> > + err = -EINVAL;
> > + inode_unlock(workdir->d_inode);
> > +
> > + return err;
> > +}
> > +
Just to illustrate what I meant and how the flow of ovl_cleanup_unlocked()
and later on ovl_cleanup() would look simpler:
int lock_parent(struct dentry *parent, struct dentry *child)
{
int err;
inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
if (!child || child->d_parent == parent)
return 0;
inode_unlock(parent->d_inode);
return -EINVAL;
}
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 0;
}
Thanks,
Amir.
