Re: [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.

On Tue, 24 Feb 2026, Chris Mason wrote:

NeilBrown [off-list ref] wrote:

quoted

From: NeilBrown <neil@brown.name>

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 [off-list ref]

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 <neil@brown.name>

quoted

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