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;
