Unverified Commit 7ab96df8 authored by NeilBrown's avatar NeilBrown Committed by Christian Brauner
Browse files

VFS/nfsd/cachefiles/ovl: add start_creating() and end_creating() 



start_creating() is similar to simple_start_creating() but is not so
simple.
It takes a qstr for the name, includes permission checking, and does NOT
report an error if the name already exists, returning a positive dentry
instead.

This is currently used by nfsd, cachefiles, and overlayfs.

end_creating() is called after the dentry has been used.
end_creating() drops the reference to the dentry as it is generally no
longer needed.  This is exactly the first section of end_creating_path()
so that function is changed to call the new end_creating()

These calls help encapsulate locking rules so that directory locking can
be changed.

Occasionally this change means that the parent lock is held for a
shorter period of time, for example in cachefiles_commit_tmpfile().
As this function now unlocks after an unlink and before the following
lookup, it is possible that the lookup could again find a positive
dentry, so a while loop is introduced there.

In overlayfs the ovl_lookup_temp() function has ovl_tempname()
split out to be used in ovl_start_creating_temp().  The other use
of ovl_lookup_temp() is preparing for a rename.  When rename handling
is updated, ovl_lookup_temp() will be removed.

Reviewed-by: default avatarJeff Layton <jlayton@kernel.org>
Reviewed-by: default avatarAmir Goldstein <amir73il@gmail.com>
Signed-off-by: default avatarNeilBrown <neil@brown.name>
Link: https://patch.msgid.link/20251113002050.676694-5-neilb@ownmail.net


Tested-by: default avatar <syzbot@syzkaller.appspotmail.com>
Signed-off-by: default avatarChristian Brauner <brauner@kernel.org>
parent 3661a788

fs/cachefiles/namei.c

+21 −20
Original line number Diff line number Diff line
@@ -93,12 +93,11 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache, 
	_enter(",,%s", dirname);



	/* search the current directory for the element name */

	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);



retry:

	ret = cachefiles_inject_read_error();

	if (ret == 0)

		subdir = lookup_one(&nop_mnt_idmap, &QSTR(dirname), dir);

		subdir = start_creating(&nop_mnt_idmap, dir, &QSTR(dirname));

	else

		subdir = ERR_PTR(ret);

	trace_cachefiles_lookup(NULL, dir, subdir);
@@ -141,7 +140,7 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache, 
		trace_cachefiles_mkdir(dir, subdir);



		if (unlikely(d_unhashed(subdir) || d_is_negative(subdir))) {

			dput(subdir);

			end_creating(subdir, dir);

			goto retry;

		}

		ASSERT(d_backing_inode(subdir));
@@ -154,7 +153,8 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache, 


	/* Tell rmdir() it's not allowed to delete the subdir */

	inode_lock(d_inode(subdir));

	inode_unlock(d_inode(dir));

	dget(subdir);

	end_creating(subdir, dir);



	if (!__cachefiles_mark_inode_in_use(NULL, d_inode(subdir))) {

		pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n",
@@ -196,14 +196,11 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache, 
	return ERR_PTR(-EBUSY);



mkdir_error:

	inode_unlock(d_inode(dir));

	if (!IS_ERR(subdir))

		dput(subdir);

	end_creating(subdir, dir);

	pr_err("mkdir %s failed with error %d\n", dirname, ret);

	return ERR_PTR(ret);



lookup_error:

	inode_unlock(d_inode(dir));

	ret = PTR_ERR(subdir);

	pr_err("Lookup %s failed with error %d\n", dirname, ret);

	return ERR_PTR(ret);
@@ -679,36 +676,41 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache, 


	_enter(",%pD", object->file);



	inode_lock_nested(d_inode(fan), I_MUTEX_PARENT);

	ret = cachefiles_inject_read_error();

	if (ret == 0)

		dentry = lookup_one(&nop_mnt_idmap, &QSTR(object->d_name), fan);

		dentry = start_creating(&nop_mnt_idmap, fan, &QSTR(object->d_name));

	else

		dentry = ERR_PTR(ret);

	if (IS_ERR(dentry)) {

		trace_cachefiles_vfs_error(object, d_inode(fan), PTR_ERR(dentry),

					   cachefiles_trace_lookup_error);

		_debug("lookup fail %ld", PTR_ERR(dentry));

		goto out_unlock;

		goto out;

	}



	if (!d_is_negative(dentry)) {

	/*

	 * This loop will only execute more than once if some other thread

	 * races to create the object we are trying to create.

	 */

	while (!d_is_negative(dentry)) {

		ret = cachefiles_unlink(volume->cache, object, fan, dentry,

					FSCACHE_OBJECT_IS_STALE);

		if (ret < 0)

			goto out_dput;

			goto out_end;



		end_creating(dentry, fan);



		dput(dentry);

		ret = cachefiles_inject_read_error();

		if (ret == 0)

			dentry = lookup_one(&nop_mnt_idmap, &QSTR(object->d_name), fan);

			dentry = start_creating(&nop_mnt_idmap, fan,

						&QSTR(object->d_name));

		else

			dentry = ERR_PTR(ret);

		if (IS_ERR(dentry)) {

			trace_cachefiles_vfs_error(object, d_inode(fan), PTR_ERR(dentry),

						   cachefiles_trace_lookup_error);

			_debug("lookup fail %ld", PTR_ERR(dentry));

			goto out_unlock;

			goto out;

		}

	}
@@ -729,10 +731,9 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache, 
		success = true;

	}



out_dput:

	dput(dentry);

out_unlock:

	inode_unlock(d_inode(fan));

out_end:

	end_creating(dentry, fan);

out:

	_leave(" = %u", success);

	return success;

}

fs/namei.c

+28 −7
Original line number Diff line number Diff line
@@ -3221,6 +3221,33 @@ struct dentry *lookup_noperm_positive_unlocked(struct qstr *name, 
}

EXPORT_SYMBOL(lookup_noperm_positive_unlocked);



/**

 * start_creating - prepare to create a given name with permission checking

 * @idmap:  idmap of the mount

 * @parent: directory in which to prepare to create the name

 * @name:   the name to be created

 *

 * Locks are taken and a lookup is performed prior to creating

 * an object in a directory.  Permission checking (MAY_EXEC) is performed

 * against @idmap.

 *

 * If the name already exists, a positive dentry is returned, so

 * behaviour is similar to O_CREAT without O_EXCL, which doesn't fail

 * with -EEXIST.

 *

 * Returns: a negative or positive dentry, or an error.

 */

struct dentry *start_creating(struct mnt_idmap *idmap, struct dentry *parent,

			      struct qstr *name)

{

	int err = lookup_one_common(idmap, name, parent);



	if (err)

		return ERR_PTR(err);

	return start_dirop(parent, name, LOOKUP_CREATE);

}

EXPORT_SYMBOL(start_creating);



#ifdef CONFIG_UNIX98_PTYS

int path_pts(struct path *path)

{
@@ -4306,13 +4333,7 @@ EXPORT_SYMBOL(start_creating_path); 
 */

void end_creating_path(const struct path *path, struct dentry *dentry)

{

	if (IS_ERR(dentry))

		/* The parent is still locked despite the error from

		 * vfs_mkdir() - must unlock it.

		 */

		inode_unlock(path->dentry->d_inode);

	else

		end_dirop(dentry);

	end_creating(dentry, path->dentry);

	mnt_drop_write(path->mnt);

	path_put(path);

}

fs/nfsd/nfs3proc.c

+5 −9
Original line number Diff line number Diff line
@@ -281,14 +281,11 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, 
	if (host_err)

		return nfserrno(host_err);



	inode_lock_nested(inode, I_MUTEX_PARENT);



	child = lookup_one(&nop_mnt_idmap,

			   &QSTR_LEN(argp->name, argp->len),

			   parent);

	child = start_creating(&nop_mnt_idmap, parent,

			       &QSTR_LEN(argp->name, argp->len));

	if (IS_ERR(child)) {

		status = nfserrno(PTR_ERR(child));

		goto out;

		goto out_write;

	}



	if (d_really_is_negative(child)) {
@@ -367,9 +364,8 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, 
	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);



out:

	inode_unlock(inode);

	if (child && !IS_ERR(child))

		dput(child);

	end_creating(child, parent);

out_write:

	fh_drop_write(fhp);

	return status;

}

fs/nfsd/nfs4proc.c

+5 −9
Original line number Diff line number Diff line
@@ -264,14 +264,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, 
	if (is_create_with_attrs(open))

		nfsd4_acl_to_attr(NF4REG, open->op_acl, &attrs);



	inode_lock_nested(inode, I_MUTEX_PARENT);



	child = lookup_one(&nop_mnt_idmap,

			   &QSTR_LEN(open->op_fname, open->op_fnamelen),

			   parent);

	child = start_creating(&nop_mnt_idmap, parent,

			       &QSTR_LEN(open->op_fname, open->op_fnamelen));

	if (IS_ERR(child)) {

		status = nfserrno(PTR_ERR(child));

		goto out;

		goto out_write;

	}



	if (d_really_is_negative(child)) {
@@ -379,10 +376,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, 
	if (attrs.na_aclerr)

		open->op_bmval[0] &= ~FATTR4_WORD0_ACL;

out:

	inode_unlock(inode);

	end_creating(child, parent);

	nfsd_attrs_free(&attrs);

	if (child && !IS_ERR(child))

		dput(child);

out_write:

	fh_drop_write(fhp);

	return status;

}

fs/nfsd/nfs4recover.c

+6 −10
Original line number Diff line number Diff line
@@ -195,13 +195,11 @@ nfsd4_create_clid_dir(struct nfs4_client *clp) 
		goto out_creds;



	dir = nn->rec_file->f_path.dentry;

	/* lock the parent */

	inode_lock(d_inode(dir));



	dentry = lookup_one(&nop_mnt_idmap, &QSTR(dname), dir);

	dentry = start_creating(&nop_mnt_idmap, dir, &QSTR(dname));

	if (IS_ERR(dentry)) {

		status = PTR_ERR(dentry);

		goto out_unlock;

		goto out;

	}

	if (d_really_is_positive(dentry))

		/*
@@ -212,15 +210,13 @@ nfsd4_create_clid_dir(struct nfs4_client *clp) 
		 * In the 4.0 case, we should never get here; but we may

		 * as well be forgiving and just succeed silently.

		 */

		goto out_put;

		goto out_end;

	dentry = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), dentry, S_IRWXU);

	if (IS_ERR(dentry))

		status = PTR_ERR(dentry);

out_put:

	if (!status)

		dput(dentry);

out_unlock:

	inode_unlock(d_inode(dir));

out_end:

	end_creating(dentry, dir);

out:

	if (status == 0) {

		if (nn->in_grace)

			__nfsd4_create_reclaim_record_grace(clp, dname,