Re: [PATCH v3 3/9] vfs: add tmpfile_open() helper

[Christian Brauner <brauner@kernel.org>]

On Tue, Sep 20, 2022 at 09:36:26PM +0200, Miklos Szeredi wrote:
> This helper unifies tmpfile creation with opening.
> 
> Existing vfs_tmpfile() callers outside of fs/namei.c will be converted to
> using this helper.  There are two such callers: cachefile and overlayfs.
> 
> The cachefiles code currently uses the open_with_fake_path() helper to open
> the tmpfile, presumably to disable accounting of the open file.  Overlayfs
> uses tmpfile for copy_up, which means these struct file instances will be
> short lived, hence it doesn't really matter if they are accounted or not.
> Disable accounting in this helper to, which should be okay for both caller.
> 
> Add MAY_OPEN permission checking for consistency.  Like for create(2)
> read/write permissions are not checked.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/namei.c         | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |  4 ++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 53b4bc094db2..5e4a0c59eef6 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3624,6 +3624,47 @@ struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
>  }
>  EXPORT_SYMBOL(vfs_tmpfile);
>  
> +/**
> + * tmpfile_open - open a tmpfile for kernel internal use
> + * @mnt_userns:	user namespace of the mount the inode was found from
> + * @parentpath:	path of the base directory
> + * @mode:	mode of the new tmpfile
> + * @open_flag:	flags
> + * @cred:	credentials for open
> + *
> + * Create and open a temporary file.  The file is not accounted in nr_files,
> + * hence this is only for kernel internal use, and must not be installed into
> + * file tables or such.
> + */
> +struct file *tmpfile_open(struct user_namespace *mnt_userns,
> +			  const struct path *parentpath,
> +			  umode_t mode, int open_flag, const struct cred *cred)
> +{
> +	struct file *file;
> +	int error;
> +	struct path path = { .mnt = parentpath->mnt };
> +
> +	path.dentry = vfs_tmpfile(mnt_userns, parentpath->dentry, mode, open_flag);
> +	if (IS_ERR(path.dentry))
> +		return ERR_CAST(path.dentry);
> +
> +	error = may_open(mnt_userns, &path, 0, open_flag);
> +	file = ERR_PTR(error);
> +	if (error)
> +		goto out_dput;
> +
> +	/*
> +	 * This relies on the "noaccount" property of fake open, otherwise
> +	 * equivalent to dentry_open().
> +	 */
> +	file = open_with_fake_path(&path, open_flag, d_inode(path.dentry), cred);
> +out_dput:
> +	dput(path.dentry);
> +
> +	return file;
> +}
> +EXPORT_SYMBOL(tmpfile_open);

Feels like this could be simplified while being equally legible to
something like:

/**
 * tmpfile_open - open a tmpfile for kernel internal use
 * @mnt_userns:	user namespace of the mount the inode was found from
 * @parentpath:	path of the base directory
 * @mode:	mode of the new tmpfile
 * @open_flag:	flags
 * @cred:	credentials for open
 *
 * Create and open a temporary file.  The file is not accounted in nr_files,
 * hence this is only for kernel internal use, and must not be installed into
 * file tables or such.
 *
 * The helper relies on the "noaccount" property of open_with_fake_path().
 * Otherwise it is equivalent to dentry_open().
 *
 * Return: Opened tmpfile on success, error pointer on failure.
 */
struct file *tmpfile_open(struct user_namespace *mnt_userns,
			  const struct path *parentpath,
			  umode_t mode, int open_flag, const struct cred *cred)
{
	struct file *file;
	int error;
	struct path path = { .mnt = parentpath->mnt };

	path.dentry = vfs_tmpfile(mnt_userns, parentpath->dentry, mode, open_flag);
	if (IS_ERR(path.dentry))
		return ERR_CAST(path.dentry);

	error = may_open(mnt_userns, &path, 0, open_flag);
	if (!error)
		file = open_with_fake_path(&path, open_flag, d_inode(path.dentry), cred);
	else
		file = ERR_PTR(error);
	dput(path.dentry);
	return file;
}
EXPORT_SYMBOL(tmpfile_open);