On Tue, 24 Feb 2026, Chris Mason wrote:
> NeilBrown <[email protected]> wrote:
> > From: NeilBrown <[email protected]>
> >
> > When ovl_create_real() is used to create a file on the upper filesystem
> > it needs to return the resulting dentry - positive and hashed.
> > It is usually the case the that dentry passed to the create function
> > (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> > filesystem may unhash that dentry forcing a repeat lookup next time the
> > name is wanted.
> >
>
> Hi everyone,
>
> Amir suggested I run these through, and this commit was flagged:
>
> commit 62d49d1e44667e4f93bec415faabec5526992ac0
> Author: NeilBrown <[email protected]>
>
> ovl: change ovl_create_real() to get a new lock when re-opening created file.
>
> This commit changes ovl_create_real() to drop the directory lock and
> reacquire a new lock for lookup when the created dentry is unhashed. It
> also removes ovl_lookup_upper() which is no longer used.
>
> Signed-off-by: NeilBrown <[email protected]>
>
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index c4feb89ad1e39..6285069ccc590 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct
> > dentry *dir,
> > }
> >
> > struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> > - struct dentry *newdentry, struct ovl_cattr *attr)
> > + struct dentry *newdentry, struct qstr *qname,
> > + struct ovl_cattr *attr)
> > {
> > struct inode *dir = parent->d_inode;
> > int err;
> > @@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs,
> > struct dentry *parent,
> > struct dentry *d;
> > /*
> > * Some filesystems (i.e. casefolded) may return an unhashed
> > - * negative dentry from the ovl_lookup_upper() call before
> > + * negative dentry from the ovl_start_creating_upper() call
> > before
> > * ovl_create_real().
> > * In that case, lookup again after making the newdentry
> > * positive, so ovl_create_upper() always returns a hashed
> > - * positive dentry.
> > + * positive dentry. We lookup using qname which should be
> > + * the same name as newentry, but is certain not to change.
> > + * As we have to drop the lock before the lookup a race
> > + * could result in a lookup failure. In that case we return
> > + * an error.
> > */
> > - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> > - newdentry->d_name.len);
> > - dput(newdentry);
> > - if (IS_ERR_OR_NULL(d))
> > + end_creating_keep(newdentry);
> > + d = ovl_start_creating_upper(ofs, parent, qname);
> > +
> > + if (IS_ERR_OR_NULL(d)) {
> > err = d ? PTR_ERR(d) : -ENOENT;
> > + } else if (d->d_inode != newdentry->d_inode) {
> > + err = -EIO;
> > + dput(newdentry);
> > + } else {
> > + dput(newdentry);
> > return d;
> > + }
> > + return ERR_PTR(err);
> > }
>
> Does this leak the newdentry reference when IS_ERR_OR_NULL(d) is true?
>
> The code now calls end_creating_keep(newdentry) which keeps the reference,
> then calls ovl_start_creating_upper(). If that returns an error or NULL,
> the function returns ERR_PTR(err) without cleaning up newdentry:
>
> end_creating_keep(newdentry);
> d = ovl_start_creating_upper(ofs, parent, qname);
>
> if (IS_ERR_OR_NULL(d)) {
> err = d ? PTR_ERR(d) : -ENOENT;
> } ...
> return ERR_PTR(err);
>
> The old code always went through the 'out:' label which called
> end_creating(newdentry) on error. The new code bypasses that cleanup.
>
>
> Does this leak both the parent lock and the dentry 'd' when the lookup
> returns a different inode?
>
> When d->d_inode != newdentry->d_inode, the code sets err = -EIO and calls
> dput(newdentry), but doesn't clean up 'd':
>
> } else if (d->d_inode != newdentry->d_inode) {
> err = -EIO;
> dput(newdentry);
> } ...
> return ERR_PTR(err);
>
> The dentry 'd' was returned by ovl_start_creating_upper() with the parent
> directory lock held. The code needs to call end_creating(d) to release the
> lock via end_dirop()->inode_unlock() and release the dentry reference via
> end_dirop()->dput().
>
>
Yes, that code is rather messed up - thanks.
I've made it:
end_creating_keep(newdentry);
d = ovl_start_creating_upper(ofs, parent, qname);
if (IS_ERR_OR_NULL(d)) {
err = d ? PTR_ERR(d) : -ENOENT;
} else if (d->d_inode != newdentry->d_inode) {
err = -EIO;
} else {
dput(newdentry);
return d;
}
end_creating(d);
dput(newdentry);
return ERR_PTR(err);
Thanks,
NeilBrown
