Re: [PATCH 8/8] NFSD: Instantiate a struct file when creating a regular NFSv4 file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, May 13, 2022 at 03:40:13PM -0400, Chuck Lever wrote:

> +/**
> + * dentry_create - Create and open a file
> + * @path: path to create
> + * @flags: O_ flags
> + * @mode: mode bits for new file
> + * @cred: credentials to use
> + *
> + * Caller must hold the parent directory's lock, and have prepared
> + * a negative dentry, placed in @path->dentry, for the new file.
> + *
> + * On success, returns a "struct file *". Otherwise a ERR_PTR
> + * is returned.
> + */
> +struct file *dentry_create(const struct path *path, int flags, umode_t mode,
> +			   const struct cred *cred)
> +{
> +	struct dentry *parent;
> +	struct file *f;
> +	int error;
> +
> +	validate_creds(cred);
> +	f = alloc_empty_file(flags, cred);
> +	if (IS_ERR(f))
> +		return f;
> +
> +	parent = dget_parent(path->dentry);
> +	error = vfs_create(mnt_user_ns(path->mnt), d_inode(parent),
> +			   path->dentry, mode, true);
> +	dput(parent);

Yuck.  dget_parent() is not entirely without valid uses, but this isn't
one.  It's for the cases when parent is *not* stable and you need to grab
what had been the parent at some point (even though it might not be the
parent anymore by the time dget_parent() returns).  Here you seriously
depend upon it remaining the parent of that sucker all the way through -
otherwise vfs_create() would break.  And you really, really depend upon
its survival - the caller is holding it locked, so they would better
have it pinned.

So this dget_parent() (and dput()) is pointless and confusing;
path->dentry->d_parent would suffice.  And document that path->dentry
must not be root and that its parent should match path->mnt.

> +	if (error) {
> +		fput(f);
> +		return ERR_PTR(error);
> +	}
> +
> +	error = vfs_open(path, f);
> +	if (error) {
> +		fput(f);
> +		return ERR_PTR(error);
> +	}
> +
> +	return f;

FWIW, I'd rather have it done as
	error = vfs_create(...);
	if (!error)
		error = vfs_open(path, f);

	if (unlikely(error)) {
		fput(f);
		return ERR_PTR(error);
	}
	return f;



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux