[PATCH v3 0/9] fuse tmpfile

[PATCH v3 0/9] fuse tmpfile

[Miklos Szeredi]

Al,

This should address your comments.

No xfstests regressions on xfs or overlayfs.  Also tested overlayfs on
fuse.

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#fuse-tmpfile-v3

V3:
 - add bits to Documentation/
 - add hugetlbfs cleanup
 - overlayfs copy-up: move opening target file to caller 

V2:
 - rename finish_tmpfile() to finish_open_simple()
 - fix warning reported by kernel test robot
 - patch description improvements

---
Al Viro (1):
  hugetlbfs: cleanup mknod and tmpfile

Miklos Szeredi (8):
  cachefiles: tmpfile error handling cleanup
  vfs: add tmpfile_open() helper
  cachefiles: use tmpfile_open() helper
  ovl: use tmpfile_open() helper
  vfs: make vfs_tmpfile() static
  vfs: move open right after ->tmpfile()
  vfs: open inside ->tmpfile()
  fuse: implement ->tmpfile()

 Documentation/filesystems/locking.rst |   3 +-
 Documentation/filesystems/porting.rst |  10 +++
 Documentation/filesystems/vfs.rst     |   6 +-
 fs/bad_inode.c                        |   2 +-
 fs/btrfs/inode.c                      |   8 +-
 fs/cachefiles/namei.c                 |  67 +++++++---------
 fs/dcache.c                           |   4 +-
 fs/ext2/namei.c                       |   6 +-
 fs/ext4/namei.c                       |   6 +-
 fs/f2fs/namei.c                       |  13 ++--
 fs/fuse/dir.c                         |  25 +++++-
 fs/fuse/fuse_i.h                      |   3 +
 fs/hugetlbfs/inode.c                  |  42 ++++------
 fs/minix/namei.c                      |   6 +-
 fs/namei.c                            |  84 ++++++++++++--------
 fs/overlayfs/copy_up.c                | 108 ++++++++++++++------------
 fs/overlayfs/overlayfs.h              |  14 ++--
 fs/overlayfs/super.c                  |  10 ++-
 fs/overlayfs/util.c                   |   2 +-
 fs/ramfs/inode.c                      |   6 +-
 fs/ubifs/dir.c                        |   7 +-
 fs/udf/namei.c                        |   6 +-
 fs/xfs/xfs_iops.c                     |  16 ++--
 include/linux/dcache.h                |   3 +-
 include/linux/fs.h                    |  16 +++-
 include/uapi/linux/fuse.h             |   6 +-
 mm/shmem.c                            |   6 +-
 27 files changed, 279 insertions(+), 206 deletions(-)

-- 
2.37.3

[PATCH v3 1/9] cachefiles: tmpfile error handling cleanup

[Miklos Szeredi]

Separate the error labels from the success path and use 'ret' to store the
error value before jumping to the error label.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/cachefiles/namei.c | 55 ++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 29 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index facf2ebe464b..d3a5884fe5c9 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -460,31 +460,27 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
 
 	path.mnt = cache->mnt;
 	ret = cachefiles_inject_write_error();
-	if (ret == 0)
+	if (ret == 0) {
 		path.dentry = vfs_tmpfile(&init_user_ns, fan, S_IFREG, O_RDWR);
-	else
-		path.dentry = ERR_PTR(ret);
-	if (IS_ERR(path.dentry)) {
-		trace_cachefiles_vfs_error(object, d_inode(fan), PTR_ERR(path.dentry),
+		ret = PTR_ERR_OR_ZERO(path.dentry);
+	}
+	if (ret) {
+		trace_cachefiles_vfs_error(object, d_inode(fan), ret,
 					   cachefiles_trace_tmpfile_error);
-		if (PTR_ERR(path.dentry) == -EIO)
+		if (ret == -EIO)
 			cachefiles_io_error_obj(object, "Failed to create tmpfile");
-		file = ERR_CAST(path.dentry);
-		goto out;
+		goto err;
 	}
 
 	trace_cachefiles_tmpfile(object, d_backing_inode(path.dentry));
 
-	if (!cachefiles_mark_inode_in_use(object, path.dentry)) {
-		file = ERR_PTR(-EBUSY);
-		goto out_dput;
-	}
+	ret = -EBUSY;
+	if (!cachefiles_mark_inode_in_use(object, path.dentry))
+		goto err_dput;
 
 	ret = cachefiles_ondemand_init_object(object);
-	if (ret < 0) {
-		file = ERR_PTR(ret);
-		goto out_unuse;
-	}
+	if (ret < 0)
+		goto err_unuse;
 
 	ni_size = object->cookie->object_size;
 	ni_size = round_up(ni_size, CACHEFILES_DIO_BLOCK_SIZE);
@@ -499,36 +495,37 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
 			trace_cachefiles_vfs_error(
 				object, d_backing_inode(path.dentry), ret,
 				cachefiles_trace_trunc_error);
-			file = ERR_PTR(ret);
-			goto out_unuse;
+			goto err_unuse;
 		}
 	}
 
 	file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
 				   d_backing_inode(path.dentry), cache->cache_cred);
+	ret = PTR_ERR(file);
 	if (IS_ERR(file)) {
 		trace_cachefiles_vfs_error(object, d_backing_inode(path.dentry),
-					   PTR_ERR(file),
-					   cachefiles_trace_open_error);
-		goto out_unuse;
+					   ret, cachefiles_trace_open_error);
+		goto err_unuse;
 	}
+	ret = -EINVAL;
 	if (unlikely(!file->f_op->read_iter) ||
 	    unlikely(!file->f_op->write_iter)) {
 		fput(file);
 		pr_notice("Cache does not support read_iter and write_iter\n");
-		file = ERR_PTR(-EINVAL);
-		goto out_unuse;
+		goto err_unuse;
 	}
-
-	goto out_dput;
-
-out_unuse:
-	cachefiles_do_unmark_inode_in_use(object, path.dentry);
-out_dput:
 	dput(path.dentry);
 out:
 	cachefiles_end_secure(cache, saved_cred);
 	return file;
+
+err_unuse:
+	cachefiles_do_unmark_inode_in_use(object, path.dentry);
+err_dput:
+	dput(path.dentry);
+err:
+	file = ERR_PTR(ret);
+	goto out;
 }
 
 /*
-- 
2.37.3

[PATCH v3 2/9] hugetlbfs: cleanup mknod and tmpfile

[Miklos Szeredi]

From: Al Viro <viro@zeniv.linux.org.uk>

Duplicate the few lines that are shared between hugetlbfs_mknod() and
hugetlbfs_tmpfile().

This is a prerequisite for sanely changing the signature of ->tmpfile().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/hugetlbfs/inode.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index f7a5b5124d8a..0b458beb318c 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -885,33 +885,18 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 /*
  * File creation. Allocate an inode, and we're done..
  */
-static int do_hugetlbfs_mknod(struct inode *dir,
-			struct dentry *dentry,
-			umode_t mode,
-			dev_t dev,
-			bool tmpfile)
+static int hugetlbfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
+			   struct dentry *dentry, umode_t mode, dev_t dev)
 {
 	struct inode *inode;
-	int error = -ENOSPC;
 
 	inode = hugetlbfs_get_inode(dir->i_sb, dir, mode, dev);
-	if (inode) {
-		dir->i_ctime = dir->i_mtime = current_time(dir);
-		if (tmpfile) {
-			d_tmpfile(dentry, inode);
-		} else {
-			d_instantiate(dentry, inode);
-			dget(dentry);/* Extra count - pin the dentry in core */
-		}
-		error = 0;
-	}
-	return error;
-}
-
-static int hugetlbfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
-			   struct dentry *dentry, umode_t mode, dev_t dev)
-{
-	return do_hugetlbfs_mknod(dir, dentry, mode, dev, false);
+	if (!inode)
+		return -ENOSPC;
+	dir->i_ctime = dir->i_mtime = current_time(dir);
+	d_instantiate(dentry, inode);
+	dget(dentry);/* Extra count - pin the dentry in core */
+	return 0;
 }
 
 static int hugetlbfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
@@ -935,7 +920,14 @@ static int hugetlbfs_tmpfile(struct user_namespace *mnt_userns,
 			     struct inode *dir, struct dentry *dentry,
 			     umode_t mode)
 {
-	return do_hugetlbfs_mknod(dir, dentry, mode | S_IFREG, 0, true);
+	struct inode *inode;
+
+	inode = hugetlbfs_get_inode(dir->i_sb, dir, mode | S_IFREG, 0);
+	if (!inode)
+		return -ENOSPC;
+	dir->i_ctime = dir->i_mtime = current_time(dir);
+	d_tmpfile(dentry, inode);
+	return 0;
 }
 
 static int hugetlbfs_symlink(struct user_namespace *mnt_userns,
-- 
2.37.3

Re: [PATCH v3 2/9] hugetlbfs: cleanup mknod and tmpfile

[Christian Brauner]

On Tue, Sep 20, 2022 at 09:36:25PM +0200, Miklos Szeredi wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> Duplicate the few lines that are shared between hugetlbfs_mknod() and
> hugetlbfs_tmpfile().
> 
> This is a prerequisite for sanely changing the signature of ->tmpfile().
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

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

[Miklos Szeredi]

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);
+
 static int do_tmpfile(struct nameidata *nd, unsigned flags,
 		const struct open_flags *op,
 		struct file *file)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9eced4cc286e..34e7a189565b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2007,6 +2007,10 @@ static inline int vfs_whiteout(struct user_namespace *mnt_userns,
 struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
 			   struct dentry *dentry, umode_t mode, int open_flag);
 
+struct file *tmpfile_open(struct user_namespace *mnt_userns,
+			  const struct path *parentpath,
+			  umode_t mode, int open_flag, const struct cred *cred);
+
 int vfs_mkobj(struct dentry *, umode_t,
 		int (*f)(struct dentry *, umode_t, void *),
 		void *);
-- 
2.37.3

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

[Christian Brauner]

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);

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

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 10:09, Christian Brauner <brauner@kernel.org> wrote:
>
> 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);

Okay, but this code gets completely rewritten in 7/9, so maybe not
worth bothering about.

Thanks,
Miklos

[PATCH v3 4/9] cachefiles: use tmpfile_open() helper

[Miklos Szeredi]

Use the tmpfile_open() helper instead of doing tmpfile creation and opening
separately.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/cachefiles/namei.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index d3a5884fe5c9..44f575328af4 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -451,18 +451,19 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
 	const struct cred *saved_cred;
 	struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash];
 	struct file *file;
-	struct path path;
+	struct path path = { .mnt = cache->mnt, .dentry = fan };
 	uint64_t ni_size;
 	long ret;
 
 
 	cachefiles_begin_secure(cache, &saved_cred);
 
-	path.mnt = cache->mnt;
 	ret = cachefiles_inject_write_error();
 	if (ret == 0) {
-		path.dentry = vfs_tmpfile(&init_user_ns, fan, S_IFREG, O_RDWR);
-		ret = PTR_ERR_OR_ZERO(path.dentry);
+		file = tmpfile_open(&init_user_ns, &path, S_IFREG,
+				    O_RDWR | O_LARGEFILE | O_DIRECT,
+				    cache->cache_cred);
+		ret = PTR_ERR_OR_ZERO(file);
 	}
 	if (ret) {
 		trace_cachefiles_vfs_error(object, d_inode(fan), ret,
@@ -471,12 +472,14 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
 			cachefiles_io_error_obj(object, "Failed to create tmpfile");
 		goto err;
 	}
+	/* From now path refers to the tmpfile */
+	path.dentry = file->f_path.dentry;
 
 	trace_cachefiles_tmpfile(object, d_backing_inode(path.dentry));
 
 	ret = -EBUSY;
 	if (!cachefiles_mark_inode_in_use(object, path.dentry))
-		goto err_dput;
+		goto err_fput;
 
 	ret = cachefiles_ondemand_init_object(object);
 	if (ret < 0)
@@ -499,14 +502,6 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
 		}
 	}
 
-	file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
-				   d_backing_inode(path.dentry), cache->cache_cred);
-	ret = PTR_ERR(file);
-	if (IS_ERR(file)) {
-		trace_cachefiles_vfs_error(object, d_backing_inode(path.dentry),
-					   ret, cachefiles_trace_open_error);
-		goto err_unuse;
-	}
 	ret = -EINVAL;
 	if (unlikely(!file->f_op->read_iter) ||
 	    unlikely(!file->f_op->write_iter)) {
@@ -514,15 +509,14 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
 		pr_notice("Cache does not support read_iter and write_iter\n");
 		goto err_unuse;
 	}
-	dput(path.dentry);
 out:
 	cachefiles_end_secure(cache, saved_cred);
 	return file;
 
 err_unuse:
 	cachefiles_do_unmark_inode_in_use(object, path.dentry);
-err_dput:
-	dput(path.dentry);
+err_fput:
+	fput(file);
 err:
 	file = ERR_PTR(ret);
 	goto out;
-- 
2.37.3

Re: [PATCH v3 4/9] cachefiles: use tmpfile_open() helper

[Christian Brauner]

On Tue, Sep 20, 2022 at 09:36:27PM +0200, Miklos Szeredi wrote:
> Use the tmpfile_open() helper instead of doing tmpfile creation and opening
> separately.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/cachefiles/namei.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index d3a5884fe5c9..44f575328af4 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -451,18 +451,19 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
>  	const struct cred *saved_cred;
>  	struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash];
>  	struct file *file;
> -	struct path path;
> +	struct path path = { .mnt = cache->mnt, .dentry = fan };
>  	uint64_t ni_size;
>  	long ret;

Maybe we shouldn't use struct path to first refer to the parent path and
then to the tmp path to avoid any potential confusion and instead rely
on a compount initializer for the tmpfile_open() call (__not tested__)?:

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 44f575328af4..979b2f173ac3 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -451,7 +451,7 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
        const struct cred *saved_cred;
        struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash];
        struct file *file;
-       struct path path = { .mnt = cache->mnt, .dentry = fan };
+       struct path path;
        uint64_t ni_size;
        long ret;

@@ -460,8 +460,10 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)

        ret = cachefiles_inject_write_error();
        if (ret == 0) {
-               file = tmpfile_open(&init_user_ns, &path, S_IFREG,
-                                   O_RDWR | O_LARGEFILE | O_DIRECT,
+               file = tmpfile_open(&init_user_ns,
+                                   &{const struct path} {.mnt = cache->mnt,
+                                                         .dentry = fan},
+                                   S_IFREG, O_RDWR | O_LARGEFILE | O_DIRECT,
                                    cache->cache_cred);
                ret = PTR_ERR_OR_ZERO(file);
        }
@@ -472,7 +474,9 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
                        cachefiles_io_error_obj(object, "Failed to create tmpfile");
                goto err;
        }
-       /* From now path refers to the tmpfile */
+
+       /* prepare tmp path */
+       path.mnt = cache->mnt;
        path.dentry = file->f_path.dentry;

        trace_cachefiles_tmpfile(object, d_backing_inode(path.dentry));

Re: [PATCH v3 4/9] cachefiles: use tmpfile_open() helper

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 10:27, Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, Sep 20, 2022 at 09:36:27PM +0200, Miklos Szeredi wrote:
> > Use the tmpfile_open() helper instead of doing tmpfile creation and opening
> > separately.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/cachefiles/namei.c | 26 ++++++++++----------------
> >  1 file changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index d3a5884fe5c9..44f575328af4 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -451,18 +451,19 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
> >       const struct cred *saved_cred;
> >       struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash];
> >       struct file *file;
> > -     struct path path;
> > +     struct path path = { .mnt = cache->mnt, .dentry = fan };
> >       uint64_t ni_size;
> >       long ret;
>
> Maybe we shouldn't use struct path to first refer to the parent path and
> then to the tmp path to avoid any potential confusion and instead rely
> on a compount initializer for the tmpfile_open() call (__not tested__)?:
>
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index 44f575328af4..979b2f173ac3 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -451,7 +451,7 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
>         const struct cred *saved_cred;
>         struct dentry *fan = volume->fanout[(u8)object->cookie->key_hash];
>         struct file *file;
> -       struct path path = { .mnt = cache->mnt, .dentry = fan };
> +       struct path path;
>         uint64_t ni_size;
>         long ret;
>
> @@ -460,8 +460,10 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
>
>         ret = cachefiles_inject_write_error();
>         if (ret == 0) {
> -               file = tmpfile_open(&init_user_ns, &path, S_IFREG,
> -                                   O_RDWR | O_LARGEFILE | O_DIRECT,
> +               file = tmpfile_open(&init_user_ns,
> +                                   &{const struct path} {.mnt = cache->mnt,
> +                                                         .dentry = fan},

This doesn't look nice.   I fixed it with a separate "parentpath" variable.

Thanks,
Miklos

Re: [PATCH v3 4/9] cachefiles: use tmpfile_open() helper

[Al Viro]

On Wed, Sep 21, 2022 at 10:26:12AM +0200, Christian Brauner wrote:
> -       /* From now path refers to the tmpfile */
> +
> +       /* prepare tmp path */
> +       path.mnt = cache->mnt;
>         path.dentry = file->f_path.dentry;

Do we even want that struct path from that point on?  Look:

	d_backing_inode(path.dentry) is a weird way to spell file_inode(file).

	cachefiles_mark_inode_in_use() is an overkill here - it *can't* fail
here, so all we want is
	inode_lock(inode);
	inode->i_flags |= S_KERNEL_FILE;
	trace_cachefiles_mark_active(object, inode);
	inode_unlock(inode);
where inode is, again, file_inode(file).

	cachefiles_do_unmark_inode_in_use() uses only inode.

	vfs_truncate() could use &file->f_path, but there's a potentially
nastier problem - theoretically, there are filesystems where we might
want struct file available for operation, especially for opened-and-unlinked
equivalents.  In any case, &file->f_path would do just as well as its copy.

Re: [PATCH v3 4/9] cachefiles: use tmpfile_open() helper

[Christian Brauner]

On Wed, Sep 21, 2022 at 08:46:20PM +0100, Al Viro wrote:
> On Wed, Sep 21, 2022 at 10:26:12AM +0200, Christian Brauner wrote:
> > -       /* From now path refers to the tmpfile */
> > +
> > +       /* prepare tmp path */
> > +       path.mnt = cache->mnt;
> >         path.dentry = file->f_path.dentry;
> 
> Do we even want that struct path from that point on?  Look:
> 
> 	d_backing_inode(path.dentry) is a weird way to spell file_inode(file).
> 
> 	cachefiles_mark_inode_in_use() is an overkill here - it *can't* fail
> here, so all we want is
> 	inode_lock(inode);
> 	inode->i_flags |= S_KERNEL_FILE;
> 	trace_cachefiles_mark_active(object, inode);
> 	inode_unlock(inode);
> where inode is, again, file_inode(file).
> 
> 	cachefiles_do_unmark_inode_in_use() uses only inode.
> 
> 	vfs_truncate() could use &file->f_path, but there's a potentially
> nastier problem - theoretically, there are filesystems where we might
> want struct file available for operation, especially for opened-and-unlinked
> equivalents.  In any case, &file->f_path would do just as well as its copy.

Yeah, sounds reasonable.

[PATCH v3 5/9] ovl: use tmpfile_open() helper

[Miklos Szeredi]

If tmpfile is used for copy up, then use this helper to create the tmpfile
and open it at the same time.  This will later allow filesystems such as
fuse to do this operation atomically.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c   | 108 +++++++++++++++++++++------------------
 fs/overlayfs/overlayfs.h |  14 ++---
 fs/overlayfs/super.c     |  10 ++--
 fs/overlayfs/util.c      |   2 +-
 4 files changed, 72 insertions(+), 62 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index fdde6c56cc3d..62a63e9ca57d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -193,11 +193,11 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
 	return ovl_real_fileattr_set(new, &newfa);
 }
 
-static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
-			    struct path *new, loff_t len)
+static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
+			    struct file *new_file, loff_t len)
 {
+	struct path datapath;
 	struct file *old_file;
-	struct file *new_file;
 	loff_t old_pos = 0;
 	loff_t new_pos = 0;
 	loff_t cloned;
@@ -206,23 +206,18 @@ static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
 	bool skip_hole = false;
 	int error = 0;
 
-	if (len == 0)
-		return 0;
+	ovl_path_lowerdata(dentry, &datapath);
+	if (WARN_ON(datapath.dentry == NULL))
+		return -EIO;
 
-	old_file = ovl_path_open(old, O_LARGEFILE | O_RDONLY);
+	old_file = ovl_path_open(&datapath, O_LARGEFILE | O_RDONLY);
 	if (IS_ERR(old_file))
 		return PTR_ERR(old_file);
 
-	new_file = ovl_path_open(new, O_LARGEFILE | O_WRONLY);
-	if (IS_ERR(new_file)) {
-		error = PTR_ERR(new_file);
-		goto out_fput;
-	}
-
 	/* Try to use clone_file_range to clone up within the same fs */
 	cloned = do_clone_file_range(old_file, 0, new_file, 0, len, 0);
 	if (cloned == len)
-		goto out;
+		goto out_fput;
 	/* Couldn't clone, so now we try to copy the data */
 
 	/* Check if lower fs supports seek operation */
@@ -282,10 +277,8 @@ static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
 
 		len -= bytes;
 	}
-out:
 	if (!error && ovl_should_sync(ofs))
 		error = vfs_fsync(new_file, 0);
-	fput(new_file);
 out_fput:
 	fput(old_file);
 	return error;
@@ -556,30 +549,31 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 	return err;
 }
 
-static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
+static int ovl_copy_up_data(struct ovl_copy_up_ctx *c, const struct path *temp)
 {
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
-	struct inode *inode = d_inode(c->dentry);
-	struct path upperpath, datapath;
+	struct file *new_file;
 	int err;
 
-	ovl_path_upper(c->dentry, &upperpath);
-	if (WARN_ON(upperpath.dentry != NULL))
-		return -EIO;
+	if (!S_ISREG(c->stat.mode) || c->metacopy || !c->stat.size)
+		return 0;
 
-	upperpath.dentry = temp;
+	new_file = ovl_path_open(temp, O_LARGEFILE | O_WRONLY);
+	if (IS_ERR(new_file))
+		return PTR_ERR(new_file);
 
-	/*
-	 * Copy up data first and then xattrs. Writing data after
-	 * xattrs will remove security.capability xattr automatically.
-	 */
-	if (S_ISREG(c->stat.mode) && !c->metacopy) {
-		ovl_path_lowerdata(c->dentry, &datapath);
-		err = ovl_copy_up_data(ofs, &datapath, &upperpath,
-				       c->stat.size);
-		if (err)
-			return err;
-	}
+	err = ovl_copy_up_file(ofs, c->dentry, new_file, c->stat.size);
+	fput(new_file);
+
+	return err;
+}
+
+static int ovl_copy_up_metadata(struct ovl_copy_up_ctx *c, struct dentry *temp)
+{
+	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
+	struct inode *inode = d_inode(c->dentry);
+	struct path upperpath = { .mnt = ovl_upper_mnt(ofs), .dentry = temp };
+	int err;
 
 	err = ovl_copy_xattr(c->dentry->d_sb, &c->lowerpath, temp);
 	if (err)
@@ -662,6 +656,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	struct inode *inode;
 	struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
+	struct path path = { .mnt = ovl_upper_mnt(ofs) };
 	struct dentry *temp, *upper;
 	struct ovl_cu_creds cc;
 	int err;
@@ -688,7 +683,16 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	if (IS_ERR(temp))
 		goto unlock;
 
-	err = ovl_copy_up_inode(c, temp);
+	/*
+	 * Copy up data first and then xattrs. Writing data after
+	 * xattrs will remove security.capability xattr automatically.
+	 */
+	path.dentry = temp;
+	err = ovl_copy_up_data(c, &path);
+	if (err)
+		goto cleanup;
+
+	err = ovl_copy_up_metadata(c, temp);
 	if (err)
 		goto cleanup;
 
@@ -732,6 +736,7 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	struct inode *udir = d_inode(c->destdir);
 	struct dentry *temp, *upper;
+	struct file *tmpfile;
 	struct ovl_cu_creds cc;
 	int err;
 
@@ -739,15 +744,22 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
 	if (err)
 		return err;
 
-	temp = ovl_do_tmpfile(ofs, c->workdir, c->stat.mode);
+	tmpfile = ovl_do_tmpfile(ofs, c->workdir, c->stat.mode);
 	ovl_revert_cu_creds(&cc);
 
-	if (IS_ERR(temp))
-		return PTR_ERR(temp);
+	if (IS_ERR(tmpfile))
+		return PTR_ERR(tmpfile);
 
-	err = ovl_copy_up_inode(c, temp);
+	temp = tmpfile->f_path.dentry;
+	if (!c->metacopy && c->stat.size) {
+		err = ovl_copy_up_file(ofs, c->dentry, tmpfile, c->stat.size);
+		if (err)
+			return err;
+	}
+
+	err = ovl_copy_up_metadata(c, temp);
 	if (err)
-		goto out_dput;
+		goto out_fput;
 
 	inode_lock_nested(udir, I_MUTEX_PARENT);
 
@@ -761,16 +773,14 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
 	inode_unlock(udir);
 
 	if (err)
-		goto out_dput;
+		goto out_fput;
 
 	if (!c->metacopy)
 		ovl_set_upperdata(d_inode(c->dentry));
-	ovl_inode_update(d_inode(c->dentry), temp);
+	ovl_inode_update(d_inode(c->dentry), dget(temp));
 
-	return 0;
-
-out_dput:
-	dput(temp);
+out_fput:
+	fput(tmpfile);
 	return err;
 }
 
@@ -899,7 +909,7 @@ static ssize_t ovl_getxattr_value(struct path *path, char *name, char **value)
 static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 {
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
-	struct path upperpath, datapath;
+	struct path upperpath;
 	int err;
 	char *capability = NULL;
 	ssize_t cap_size;
@@ -908,10 +918,6 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 	if (WARN_ON(upperpath.dentry == NULL))
 		return -EIO;
 
-	ovl_path_lowerdata(c->dentry, &datapath);
-	if (WARN_ON(datapath.dentry == NULL))
-		return -EIO;
-
 	if (c->stat.size) {
 		err = cap_size = ovl_getxattr_value(&upperpath, XATTR_NAME_CAPS,
 						    &capability);
@@ -919,7 +925,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 			goto out;
 	}
 
-	err = ovl_copy_up_data(ofs, &datapath, &upperpath, c->stat.size);
+	err = ovl_copy_up_data(c, &upperpath);
 	if (err)
 		goto out_free;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 87759165d32b..ca64085ddc5f 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -310,14 +310,16 @@ static inline int ovl_do_whiteout(struct ovl_fs *ofs,
 	return err;
 }
 
-static inline struct dentry *ovl_do_tmpfile(struct ovl_fs *ofs,
-					    struct dentry *dentry, umode_t mode)
+static inline struct file *ovl_do_tmpfile(struct ovl_fs *ofs,
+					  struct dentry *dentry, umode_t mode)
 {
-	struct dentry *ret = vfs_tmpfile(ovl_upper_mnt_userns(ofs), dentry, mode, 0);
-	int err = PTR_ERR_OR_ZERO(ret);
+	struct path path = { .mnt = ovl_upper_mnt(ofs), .dentry = dentry };
+	struct file *file = tmpfile_open(ovl_upper_mnt_userns(ofs), &path, mode,
+					O_LARGEFILE | O_WRONLY, current_cred());
+	int err = PTR_ERR_OR_ZERO(file);
 
 	pr_debug("tmpfile(%pd2, 0%o) = %i\n", dentry, mode, err);
-	return ret;
+	return file;
 }
 
 static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
@@ -401,7 +403,7 @@ void ovl_inode_update(struct inode *inode, struct dentry *upperdentry);
 void ovl_dir_modified(struct dentry *dentry, bool impurity);
 u64 ovl_dentry_version_get(struct dentry *dentry);
 bool ovl_is_whiteout(struct dentry *dentry);
-struct file *ovl_path_open(struct path *path, int flags);
+struct file *ovl_path_open(const struct path *path, int flags);
 int ovl_copy_up_start(struct dentry *dentry, int flags);
 void ovl_copy_up_end(struct dentry *dentry);
 bool ovl_already_copied_up(struct dentry *dentry, int flags);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ec746d447f1b..7837223689c1 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -15,6 +15,7 @@
 #include <linux/seq_file.h>
 #include <linux/posix_acl_xattr.h>
 #include <linux/exportfs.h>
+#include <linux/file.h>
 #include "overlayfs.h"
 
 MODULE_AUTHOR("Miklos Szeredi <miklos@szeredi.hu>");
@@ -1356,7 +1357,8 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 			    struct path *workpath)
 {
 	struct vfsmount *mnt = ovl_upper_mnt(ofs);
-	struct dentry *temp, *workdir;
+	struct dentry *workdir;
+	struct file *tmpfile;
 	bool rename_whiteout;
 	bool d_type;
 	int fh_type;
@@ -1392,10 +1394,10 @@ static int ovl_make_workdir(struct super_block *sb, struct ovl_fs *ofs,
 		pr_warn("upper fs needs to support d_type.\n");
 
 	/* Check if upper/work fs supports O_TMPFILE */
-	temp = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
-	ofs->tmpfile = !IS_ERR(temp);
+	tmpfile = ovl_do_tmpfile(ofs, ofs->workdir, S_IFREG | 0);
+	ofs->tmpfile = !IS_ERR(tmpfile);
 	if (ofs->tmpfile)
-		dput(temp);
+		fput(tmpfile);
 	else
 		pr_warn("upper fs does not support tmpfile.\n");
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 87f811c089e4..968926c0c7ab 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -490,7 +490,7 @@ bool ovl_is_whiteout(struct dentry *dentry)
 	return inode && IS_WHITEOUT(inode);
 }
 
-struct file *ovl_path_open(struct path *path, int flags)
+struct file *ovl_path_open(const struct path *path, int flags)
 {
 	struct inode *inode = d_inode(path->dentry);
 	struct user_namespace *real_mnt_userns = mnt_user_ns(path->mnt);
-- 
2.37.3

Re: [PATCH v3 5/9] ovl: use tmpfile_open() helper

[Christian Brauner]

On Tue, Sep 20, 2022 at 09:36:28PM +0200, Miklos Szeredi wrote:
> If tmpfile is used for copy up, then use this helper to create the tmpfile
> and open it at the same time.  This will later allow filesystems such as
> fuse to do this operation atomically.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

[PATCH v3 6/9] vfs: make vfs_tmpfile() static

[Miklos Szeredi]

No callers outside of fs/namei.c anymore.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namei.c         | 3 +--
 include/linux/fs.h | 3 ---
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 5e4a0c59eef6..652d09ae66fb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3583,7 +3583,7 @@ static int do_open(struct nameidata *nd,
  * On non-idmapped mounts or if permission checking is to be performed on the
  * raw inode simply passs init_user_ns.
  */
-struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
+static struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
 			   struct dentry *dentry, umode_t mode, int open_flag)
 {
 	struct dentry *child = NULL;
@@ -3622,7 +3622,6 @@ struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
 	dput(child);
 	return ERR_PTR(error);
 }
-EXPORT_SYMBOL(vfs_tmpfile);
 
 /**
  * tmpfile_open - open a tmpfile for kernel internal use
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 34e7a189565b..a445da4842e0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2004,9 +2004,6 @@ static inline int vfs_whiteout(struct user_namespace *mnt_userns,
 			 WHITEOUT_DEV);
 }
 
-struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
-			   struct dentry *dentry, umode_t mode, int open_flag);
-
 struct file *tmpfile_open(struct user_namespace *mnt_userns,
 			  const struct path *parentpath,
 			  umode_t mode, int open_flag, const struct cred *cred);
-- 
2.37.3

Re: [PATCH v3 6/9] vfs: make vfs_tmpfile() static

[Christian Brauner]

On Tue, Sep 20, 2022 at 09:36:29PM +0200, Miklos Szeredi wrote:
> No callers outside of fs/namei.c anymore.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

Looks good to me,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

[PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Miklos Szeredi]

Create a helper finish_open_simple() that opens the file with the original
dentry.  Handle the error case here as well to simplify callers.

Call this helper right after ->tmpfile() is called.

Next patch will change the tmpfile API and move this call into tmpfile
instances.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namei.c         | 79 ++++++++++++++++++----------------------------
 include/linux/fs.h |  9 ++++++
 2 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 652d09ae66fb..4faf7e743664 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3583,44 +3583,43 @@ static int do_open(struct nameidata *nd,
  * On non-idmapped mounts or if permission checking is to be performed on the
  * raw inode simply passs init_user_ns.
  */
-static struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
-			   struct dentry *dentry, umode_t mode, int open_flag)
+static int vfs_tmpfile(struct user_namespace *mnt_userns,
+		       const struct path *parentpath,
+		       struct file *file, umode_t mode)
 {
-	struct dentry *child = NULL;
-	struct inode *dir = dentry->d_inode;
+	struct dentry *child;
+	struct inode *dir = d_inode(parentpath->dentry);
 	struct inode *inode;
 	int error;
 
 	/* we want directory to be writable */
 	error = inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC);
 	if (error)
-		goto out_err;
-	error = -EOPNOTSUPP;
+		return error;
 	if (!dir->i_op->tmpfile)
-		goto out_err;
-	error = -ENOMEM;
-	child = d_alloc(dentry, &slash_name);
+		return -EOPNOTSUPP;
+	child = d_alloc(parentpath->dentry, &slash_name);
 	if (unlikely(!child))
-		goto out_err;
+		return -ENOMEM;
+	file->f_path.mnt = parentpath->mnt;
+	file->f_path.dentry = child;
 	mode = vfs_prepare_mode(mnt_userns, dir, mode, mode, mode);
 	error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
+	error = finish_open_simple(file, error);
+	dput(child);
 	if (error)
-		goto out_err;
-	error = -ENOENT;
+		return error;
+	error = may_open(mnt_userns, &file->f_path, 0, file->f_flags);
+	if (error)
+		return error;
 	inode = child->d_inode;
-	if (unlikely(!inode))
-		goto out_err;
-	if (!(open_flag & O_EXCL)) {
+	if (!(file->f_flags & O_EXCL)) {
 		spin_lock(&inode->i_lock);
 		inode->i_state |= I_LINKABLE;
 		spin_unlock(&inode->i_lock);
 	}
 	ima_post_create_tmpfile(mnt_userns, inode);
-	return child;
-
-out_err:
-	dput(child);
-	return ERR_PTR(error);
+	return 0;
 }
 
 /**
@@ -3641,25 +3640,15 @@ struct file *tmpfile_open(struct user_namespace *mnt_userns,
 {
 	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);
 
+	file = alloc_empty_file_noaccount(open_flag, cred);
+	if (!IS_ERR(file)) {
+		error = vfs_tmpfile(mnt_userns, parentpath, file, mode);
+		if (error) {
+			fput(file);
+			file = ERR_PTR(error);
+		}
+	}
 	return file;
 }
 EXPORT_SYMBOL(tmpfile_open);
@@ -3669,26 +3658,20 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
 		struct file *file)
 {
 	struct user_namespace *mnt_userns;
-	struct dentry *child;
 	struct path path;
 	int error = path_lookupat(nd, flags | LOOKUP_DIRECTORY, &path);
+
 	if (unlikely(error))
 		return error;
 	error = mnt_want_write(path.mnt);
 	if (unlikely(error))
 		goto out;
 	mnt_userns = mnt_user_ns(path.mnt);
-	child = vfs_tmpfile(mnt_userns, path.dentry, op->mode, op->open_flag);
-	error = PTR_ERR(child);
-	if (IS_ERR(child))
+	error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
+	if (error)
 		goto out2;
-	dput(path.dentry);
-	path.dentry = child;
-	audit_inode(nd->name, child, 0);
+	audit_inode(nd->name, file->f_path.dentry, 0);
 	/* Don't check for other permissions, the inode was just created */
-	error = may_open(mnt_userns, &path, 0, op->open_flag);
-	if (!error)
-		error = vfs_open(&path, file);
 out2:
 	mnt_drop_write(path.mnt);
 out:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a445da4842e0..f0d17eefb966 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2780,6 +2780,15 @@ extern int finish_open(struct file *file, struct dentry *dentry,
 			int (*open)(struct inode *, struct file *));
 extern int finish_no_open(struct file *file, struct dentry *dentry);
 
+/* Helper for the simple case when original dentry is used */
+static inline int finish_open_simple(struct file *file, int error)
+{
+	if (error)
+		return error;
+
+	return finish_open(file, file->f_path.dentry, NULL);
+}
+
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
 extern void __init vfs_caches_init(void);
-- 
2.37.3

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Al Viro]

On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:

>  	inode = child->d_inode;

Better
	inode = file_inode(file);

so that child would be completely ignored after dput().

> +	error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
> +	if (error)
>  		goto out2;
> -	dput(path.dentry);
> -	path.dentry = child;
> -	audit_inode(nd->name, child, 0);
> +	audit_inode(nd->name, file->f_path.dentry, 0);
>  	/* Don't check for other permissions, the inode was just created */
> -	error = may_open(mnt_userns, &path, 0, op->open_flag);

Umm...  I'm not sure that losing it is the right thing - it might
be argued that ->permission(..., MAY_OPEN) is to be ignored for
tmpfile (and the only thing checking for MAY_OPEN is nfs, which is
*not* going to grow tmpfile any time soon - certainly not with these
calling conventions), but you are also dropping the call of
security_inode_permission(inode, MAY_OPEN) and that's a change
compared to what LSM crowd used to get...

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Miklos Szeredi]

On Tue, 20 Sept 2022 at 22:57, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:
>
> >       inode = child->d_inode;
>
> Better
>         inode = file_inode(file);
>
> so that child would be completely ignored after dput().
>
> > +     error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
> > +     if (error)
> >               goto out2;
> > -     dput(path.dentry);
> > -     path.dentry = child;
> > -     audit_inode(nd->name, child, 0);
> > +     audit_inode(nd->name, file->f_path.dentry, 0);
> >       /* Don't check for other permissions, the inode was just created */
> > -     error = may_open(mnt_userns, &path, 0, op->open_flag);
>
> Umm...  I'm not sure that losing it is the right thing - it might
> be argued that ->permission(..., MAY_OPEN) is to be ignored for
> tmpfile (and the only thing checking for MAY_OPEN is nfs, which is
> *not* going to grow tmpfile any time soon - certainly not with these
> calling conventions), but you are also dropping the call of
> security_inode_permission(inode, MAY_OPEN) and that's a change
> compared to what LSM crowd used to get...

Not losing it, just moving it into vfs_tmpfile().

Thanks,
Miklos

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Christian Brauner]

On Wed, Sep 21, 2022 at 05:06:57AM +0200, Miklos Szeredi wrote:
> On Tue, 20 Sept 2022 at 22:57, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:
> >
> > >       inode = child->d_inode;
> >
> > Better
> >         inode = file_inode(file);
> >
> > so that child would be completely ignored after dput().
> >
> > > +     error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
> > > +     if (error)
> > >               goto out2;
> > > -     dput(path.dentry);
> > > -     path.dentry = child;
> > > -     audit_inode(nd->name, child, 0);
> > > +     audit_inode(nd->name, file->f_path.dentry, 0);
> > >       /* Don't check for other permissions, the inode was just created */
> > > -     error = may_open(mnt_userns, &path, 0, op->open_flag);
> >
> > Umm...  I'm not sure that losing it is the right thing - it might
> > be argued that ->permission(..., MAY_OPEN) is to be ignored for
> > tmpfile (and the only thing checking for MAY_OPEN is nfs, which is
> > *not* going to grow tmpfile any time soon - certainly not with these
> > calling conventions), but you are also dropping the call of
> > security_inode_permission(inode, MAY_OPEN) and that's a change
> > compared to what LSM crowd used to get...
> 
> Not losing it, just moving it into vfs_tmpfile().

Afaict, we haven't called may_open() for tmpfile creation in either
cachefiles or overlayfs before. So from that perspective I wonder if
there's a good reason for us to do it now.

The fact that we don't account these kernel internal tmpfiles feels like
another exemption that points to them being internal files that don't
need to be subject to common restrictions.

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 10:54, Christian Brauner <brauner@kernel.org> wrote:
>
> On Wed, Sep 21, 2022 at 05:06:57AM +0200, Miklos Szeredi wrote:
> > On Tue, 20 Sept 2022 at 22:57, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:
> > >
> > > >       inode = child->d_inode;
> > >
> > > Better
> > >         inode = file_inode(file);
> > >
> > > so that child would be completely ignored after dput().
> > >
> > > > +     error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
> > > > +     if (error)
> > > >               goto out2;
> > > > -     dput(path.dentry);
> > > > -     path.dentry = child;
> > > > -     audit_inode(nd->name, child, 0);
> > > > +     audit_inode(nd->name, file->f_path.dentry, 0);
> > > >       /* Don't check for other permissions, the inode was just created */
> > > > -     error = may_open(mnt_userns, &path, 0, op->open_flag);
> > >
> > > Umm...  I'm not sure that losing it is the right thing - it might
> > > be argued that ->permission(..., MAY_OPEN) is to be ignored for
> > > tmpfile (and the only thing checking for MAY_OPEN is nfs, which is
> > > *not* going to grow tmpfile any time soon - certainly not with these
> > > calling conventions), but you are also dropping the call of
> > > security_inode_permission(inode, MAY_OPEN) and that's a change
> > > compared to what LSM crowd used to get...
> >
> > Not losing it, just moving it into vfs_tmpfile().
>
> Afaict, we haven't called may_open() for tmpfile creation in either
> cachefiles or overlayfs before. So from that perspective I wonder if
> there's a good reason for us to do it now.

For overlayfs we did check MAY_WRITE | MAY_OPEN through
ovl_path_open().  Just checking MAY_OPEN relaxes this, but it's in
line with the overlay model of checking the same permissions as if the
operation was invoked directly.

For cachefiles no permission was checked before this patch, so in
theory it could change behavior.  Moving the permission check back out
to callers would fix this, but I'm not entirely sure that that is the
best way forward.

David, what is the model for cachefiles?  Is this okay to check for
permissions on underlying ops, or that must be avoided?

Thanks,
Miklos

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Al Viro]

On Wed, Sep 21, 2022 at 05:06:57AM +0200, Miklos Szeredi wrote:
> On Tue, 20 Sept 2022 at 22:57, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:
> >
> > >       inode = child->d_inode;
> >
> > Better
> >         inode = file_inode(file);
> >
> > so that child would be completely ignored after dput().
> >
> > > +     error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
> > > +     if (error)
> > >               goto out2;
> > > -     dput(path.dentry);
> > > -     path.dentry = child;
> > > -     audit_inode(nd->name, child, 0);
> > > +     audit_inode(nd->name, file->f_path.dentry, 0);
> > >       /* Don't check for other permissions, the inode was just created */
> > > -     error = may_open(mnt_userns, &path, 0, op->open_flag);
> >
> > Umm...  I'm not sure that losing it is the right thing - it might
> > be argued that ->permission(..., MAY_OPEN) is to be ignored for
> > tmpfile (and the only thing checking for MAY_OPEN is nfs, which is
> > *not* going to grow tmpfile any time soon - certainly not with these
> > calling conventions), but you are also dropping the call of
> > security_inode_permission(inode, MAY_OPEN) and that's a change
> > compared to what LSM crowd used to get...
> 
> Not losing it, just moving it into vfs_tmpfile().

Sorry, missed that bit...

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Christian Brauner]

On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:
> Create a helper finish_open_simple() that opens the file with the original
> dentry.  Handle the error case here as well to simplify callers.
> 
> Call this helper right after ->tmpfile() is called.
> 
> Next patch will change the tmpfile API and move this call into tmpfile
> instances.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/namei.c         | 79 ++++++++++++++++++----------------------------
>  include/linux/fs.h |  9 ++++++
>  2 files changed, 40 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 652d09ae66fb..4faf7e743664 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3583,44 +3583,43 @@ static int do_open(struct nameidata *nd,
>   * On non-idmapped mounts or if permission checking is to be performed on the
>   * raw inode simply passs init_user_ns.
>   */
> -static struct dentry *vfs_tmpfile(struct user_namespace *mnt_userns,
> -			   struct dentry *dentry, umode_t mode, int open_flag)
> +static int vfs_tmpfile(struct user_namespace *mnt_userns,
> +		       const struct path *parentpath,
> +		       struct file *file, umode_t mode)
>  {
> -	struct dentry *child = NULL;
> -	struct inode *dir = dentry->d_inode;
> +	struct dentry *child;
> +	struct inode *dir = d_inode(parentpath->dentry);
>  	struct inode *inode;
>  	int error;
>  
>  	/* we want directory to be writable */
>  	error = inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC);
>  	if (error)
> -		goto out_err;
> -	error = -EOPNOTSUPP;
> +		return error;
>  	if (!dir->i_op->tmpfile)
> -		goto out_err;
> -	error = -ENOMEM;
> -	child = d_alloc(dentry, &slash_name);
> +		return -EOPNOTSUPP;
> +	child = d_alloc(parentpath->dentry, &slash_name);
>  	if (unlikely(!child))
> -		goto out_err;
> +		return -ENOMEM;
> +	file->f_path.mnt = parentpath->mnt;
> +	file->f_path.dentry = child;
>  	mode = vfs_prepare_mode(mnt_userns, dir, mode, mode, mode);
>  	error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
> +	error = finish_open_simple(file, error);
> +	dput(child);
>  	if (error)
> -		goto out_err;
> -	error = -ENOENT;
> +		return error;
> +	error = may_open(mnt_userns, &file->f_path, 0, file->f_flags);
> +	if (error)
> +		return error;
>  	inode = child->d_inode;
> -	if (unlikely(!inode))
> -		goto out_err;
> -	if (!(open_flag & O_EXCL)) {
> +	if (!(file->f_flags & O_EXCL)) {
>  		spin_lock(&inode->i_lock);
>  		inode->i_state |= I_LINKABLE;
>  		spin_unlock(&inode->i_lock);
>  	}
>  	ima_post_create_tmpfile(mnt_userns, inode);
> -	return child;
> -
> -out_err:
> -	dput(child);
> -	return ERR_PTR(error);
> +	return 0;
>  }
>  
>  /**
> @@ -3641,25 +3640,15 @@ struct file *tmpfile_open(struct user_namespace *mnt_userns,
>  {
>  	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);
>  
> +	file = alloc_empty_file_noaccount(open_flag, cred);
> +	if (!IS_ERR(file)) {
> +		error = vfs_tmpfile(mnt_userns, parentpath, file, mode);
> +		if (error) {
> +			fput(file);
> +			file = ERR_PTR(error);
> +		}
> +	}
>  	return file;
>  }
>  EXPORT_SYMBOL(tmpfile_open);
> @@ -3669,26 +3658,20 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
>  		struct file *file)
>  {
>  	struct user_namespace *mnt_userns;
> -	struct dentry *child;
>  	struct path path;
>  	int error = path_lookupat(nd, flags | LOOKUP_DIRECTORY, &path);
> +
>  	if (unlikely(error))
>  		return error;
>  	error = mnt_want_write(path.mnt);
>  	if (unlikely(error))
>  		goto out;
>  	mnt_userns = mnt_user_ns(path.mnt);
> -	child = vfs_tmpfile(mnt_userns, path.dentry, op->mode, op->open_flag);
> -	error = PTR_ERR(child);
> -	if (IS_ERR(child))
> +	error = vfs_tmpfile(mnt_userns, &path, file, op->mode);
> +	if (error)
>  		goto out2;
> -	dput(path.dentry);
> -	path.dentry = child;
> -	audit_inode(nd->name, child, 0);
> +	audit_inode(nd->name, file->f_path.dentry, 0);
>  	/* Don't check for other permissions, the inode was just created */
> -	error = may_open(mnt_userns, &path, 0, op->open_flag);
> -	if (!error)
> -		error = vfs_open(&path, file);
>  out2:
>  	mnt_drop_write(path.mnt);
>  out:
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a445da4842e0..f0d17eefb966 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2780,6 +2780,15 @@ extern int finish_open(struct file *file, struct dentry *dentry,
>  			int (*open)(struct inode *, struct file *));
>  extern int finish_no_open(struct file *file, struct dentry *dentry);
>  
> +/* Helper for the simple case when original dentry is used */
> +static inline int finish_open_simple(struct file *file, int error)

It would be nice if the new helpers would would be called
vfs_finish_open()/vfs_finish_open_simple() and vfs_tmpfile_open() to
stick with our vfs_* prefix convention.

It is extremely helpful when looking/grepping for helpers and the
consistency we have gained there in recent years is pretty good.

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 11:03, Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:


> > +/* Helper for the simple case when original dentry is used */
> > +static inline int finish_open_simple(struct file *file, int error)
>
> It would be nice if the new helpers would would be called
> vfs_finish_open()/vfs_finish_open_simple() and vfs_tmpfile_open() to
> stick with our vfs_* prefix convention.
>
> It is extremely helpful when looking/grepping for helpers and the
> consistency we have gained there in recent years is pretty good.

Agreed.  However only finish_open_simple() is the new one, and naming
it vfs_finish_open_simple() makes it inconsistent with
finish_open_simple().    I'd just leave this renaming to a separate
patchset and discussion, as it's hard enough to make progress with the
current one without expanding its scope.

Thanks,
Miklos

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Christian Brauner]

On Wed, Sep 21, 2022 at 04:56:01PM +0200, Miklos Szeredi wrote:
> On Wed, 21 Sept 2022 at 11:03, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Tue, Sep 20, 2022 at 09:36:30PM +0200, Miklos Szeredi wrote:
> 
> 
> > > +/* Helper for the simple case when original dentry is used */
> > > +static inline int finish_open_simple(struct file *file, int error)
> >
> > It would be nice if the new helpers would would be called
> > vfs_finish_open()/vfs_finish_open_simple() and vfs_tmpfile_open() to
> > stick with our vfs_* prefix convention.
> >
> > It is extremely helpful when looking/grepping for helpers and the
> > consistency we have gained there in recent years is pretty good.
> 
> Agreed.  However only finish_open_simple() is the new one, and naming
> it vfs_finish_open_simple() makes it inconsistent with
> finish_open_simple().    I'd just leave this renaming to a separate
> patchset and discussion, as it's hard enough to make progress with the
> current one without expanding its scope.

Ok, the finish_open* thing can be left for later. But vfs_tmpfile_open()
should be doable for this patchset already.

Re: [PATCH v3 7/9] vfs: move open right after ->tmpfile()

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 17:09, Christian Brauner <brauner@kernel.org> wrote:

> Ok, the finish_open* thing can be left for later. But vfs_tmpfile_open()
> should be doable for this patchset already.

Fixed.

Thanks,
Miklos

[PATCH v3 8/9] vfs: open inside ->tmpfile()

[Miklos Szeredi]

This is in preparation for adding tmpfile support to fuse, which requires
that the tmpfile creation and opening are done as a single operation.

Replace the 'struct dentry *' argument of i_op->tmpfile with
'struct file *'.

Call finish_open_simple() as the last thing in ->tmpfile() instances (may
be omitted in the error case).

Change d_tmpfile() argument to 'struct file *' as well to make callers more
readable.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 Documentation/filesystems/locking.rst |  3 ++-
 Documentation/filesystems/porting.rst | 10 ++++++++++
 Documentation/filesystems/vfs.rst     |  6 ++++--
 fs/bad_inode.c                        |  2 +-
 fs/btrfs/inode.c                      |  8 ++++----
 fs/dcache.c                           |  4 +++-
 fs/ext2/namei.c                       |  6 +++---
 fs/ext4/namei.c                       |  6 +++---
 fs/f2fs/namei.c                       | 13 ++++++++-----
 fs/hugetlbfs/inode.c                  |  6 +++---
 fs/minix/namei.c                      |  6 +++---
 fs/namei.c                            |  3 +--
 fs/ramfs/inode.c                      |  6 +++---
 fs/ubifs/dir.c                        |  7 ++++---
 fs/udf/namei.c                        |  6 +++---
 fs/xfs/xfs_iops.c                     | 16 +++++++++-------
 include/linux/dcache.h                |  3 ++-
 include/linux/fs.h                    |  2 +-
 mm/shmem.c                            |  6 +++---
 19 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index 4bb2627026ec..8f737e76935c 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -79,7 +79,8 @@ prototypes::
 	int (*atomic_open)(struct inode *, struct dentry *,
 				struct file *, unsigned open_flag,
 				umode_t create_mode);
-	int (*tmpfile) (struct inode *, struct dentry *, umode_t);
+	int (*tmpfile) (struct user_namespace *, struct inode *,
+			struct file *, umode_t);
 	int (*fileattr_set)(struct user_namespace *mnt_userns,
 			    struct dentry *dentry, struct fileattr *fa);
 	int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index aee9aaf9f3df..af138241bb4b 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -922,3 +922,13 @@ is provided - file_open_root_mnt().  In-tree users adjusted.
 no_llseek is gone; don't set .llseek to that - just leave it NULL instead.
 Checks for "does that file have llseek(2), or should it fail with ESPIPE"
 should be done by looking at FMODE_LSEEK in file->f_mode.
+
+---
+
+**mandatory**
+
+Calling conventions for ->tmpfile() have changed.  It now takes a struct
+file pointer instead of struct dentry pointer.  d_tmpfile() is similarly
+changed to simplify callers.  The passed file is in a non-open state and on
+success must be opened before returning (e.g. by calling
+finish_open_simple()).
diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
index 6cd6953e175b..71b0b8114b18 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -439,7 +439,7 @@ As of kernel 2.6.22, the following members are defined:
 		void (*update_time)(struct inode *, struct timespec *, int);
 		int (*atomic_open)(struct inode *, struct dentry *, struct file *,
 				   unsigned open_flag, umode_t create_mode);
-		int (*tmpfile) (struct user_namespace *, struct inode *, struct dentry *, umode_t);
+		int (*tmpfile) (struct user_namespace *, struct inode *, struct file *, umode_t);
 	        int (*set_acl)(struct user_namespace *, struct inode *, struct posix_acl *, int);
 		int (*fileattr_set)(struct user_namespace *mnt_userns,
 				    struct dentry *dentry, struct fileattr *fa);
@@ -589,7 +589,9 @@ otherwise noted.
 ``tmpfile``
 	called in the end of O_TMPFILE open().  Optional, equivalent to
 	atomically creating, opening and unlinking a file in given
-	directory.
+	directory.  On success needs to return with the file already
+	open; this can be done by calling finish_open_simple() right at
+	the end.
 
 ``fileattr_get``
 	called on ioctl(FS_IOC_GETFLAGS) and ioctl(FS_IOC_FSGETXATTR) to
diff --git a/fs/bad_inode.c b/fs/bad_inode.c
index 12b8fdcc445b..9d1cde8066cf 100644
--- a/fs/bad_inode.c
+++ b/fs/bad_inode.c
@@ -147,7 +147,7 @@ static int bad_inode_atomic_open(struct inode *inode, struct dentry *dentry,
 }
 
 static int bad_inode_tmpfile(struct user_namespace *mnt_userns,
-			     struct inode *inode, struct dentry *dentry,
+			     struct inode *inode, struct file *file,
 			     umode_t mode)
 {
 	return -EIO;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1372210869b1..416373721085 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -10168,7 +10168,7 @@ static int btrfs_permission(struct user_namespace *mnt_userns,
 }
 
 static int btrfs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-			 struct dentry *dentry, umode_t mode)
+			 struct file *file, umode_t mode)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(dir->i_sb);
 	struct btrfs_trans_handle *trans;
@@ -10176,7 +10176,7 @@ static int btrfs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 	struct inode *inode;
 	struct btrfs_new_inode_args new_inode_args = {
 		.dir = dir,
-		.dentry = dentry,
+		.dentry = file->f_path.dentry,
 		.orphan = true,
 	};
 	unsigned int trans_num_items;
@@ -10213,7 +10213,7 @@ static int btrfs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 	set_nlink(inode, 1);
 
 	if (!ret) {
-		d_tmpfile(dentry, inode);
+		d_tmpfile(file, inode);
 		unlock_new_inode(inode);
 		mark_inode_dirty(inode);
 	}
@@ -10225,7 +10225,7 @@ static int btrfs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 out_inode:
 	if (ret)
 		iput(inode);
-	return ret;
+	return finish_open_simple(file, ret);
 }
 
 void btrfs_set_range_writeback(struct btrfs_inode *inode, u64 start, u64 end)
diff --git a/fs/dcache.c b/fs/dcache.c
index bb0c4d0038db..89dc61389102 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3258,8 +3258,10 @@ void d_genocide(struct dentry *parent)
 
 EXPORT_SYMBOL(d_genocide);
 
-void d_tmpfile(struct dentry *dentry, struct inode *inode)
+void d_tmpfile(struct file *file, struct inode *inode)
 {
+	struct dentry *dentry = file->f_path.dentry;
+
 	inode_dec_link_count(inode);
 	BUG_ON(dentry->d_name.name != dentry->d_iname ||
 		!hlist_unhashed(&dentry->d_u.d_alias) ||
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 5fd9a22d2b70..9125eab85146 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -120,7 +120,7 @@ static int ext2_create (struct user_namespace * mnt_userns,
 }
 
 static int ext2_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-			struct dentry *dentry, umode_t mode)
+			struct file *file, umode_t mode)
 {
 	struct inode *inode = ext2_new_inode(dir, mode, NULL);
 	if (IS_ERR(inode))
@@ -128,9 +128,9 @@ static int ext2_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 
 	ext2_set_file_ops(inode);
 	mark_inode_dirty(inode);
-	d_tmpfile(dentry, inode);
+	d_tmpfile(file, inode);
 	unlock_new_inode(inode);
-	return 0;
+	return finish_open_simple(file, 0);
 }
 
 static int ext2_mknod (struct user_namespace * mnt_userns, struct inode * dir,
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 3a31b662f661..9c3fde633a6e 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2849,7 +2849,7 @@ static int ext4_mknod(struct user_namespace *mnt_userns, struct inode *dir,
 }
 
 static int ext4_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-			struct dentry *dentry, umode_t mode)
+			struct file *file, umode_t mode)
 {
 	handle_t *handle;
 	struct inode *inode;
@@ -2871,7 +2871,7 @@ static int ext4_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 		inode->i_op = &ext4_file_inode_operations;
 		inode->i_fop = &ext4_file_operations;
 		ext4_set_aops(inode);
-		d_tmpfile(dentry, inode);
+		d_tmpfile(file, inode);
 		err = ext4_orphan_add(handle, inode);
 		if (err)
 			goto err_unlock_inode;
@@ -2882,7 +2882,7 @@ static int ext4_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 		ext4_journal_stop(handle);
 	if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries))
 		goto retry;
-	return err;
+	return finish_open_simple(file, err);
 err_unlock_inode:
 	ext4_journal_stop(handle);
 	unlock_new_inode(inode);
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index bf00d5057abb..d5065a5af1f8 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -845,7 +845,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
 }
 
 static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-			  struct dentry *dentry, umode_t mode, bool is_whiteout,
+			  struct file *file, umode_t mode, bool is_whiteout,
 			  struct inode **new_inode)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
@@ -892,8 +892,8 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 		inode->i_state |= I_LINKABLE;
 		spin_unlock(&inode->i_lock);
 	} else {
-		if (dentry)
-			d_tmpfile(dentry, inode);
+		if (file)
+			d_tmpfile(file, inode);
 		else
 			f2fs_i_links_write(inode, false);
 	}
@@ -915,16 +915,19 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 }
 
 static int f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-			struct dentry *dentry, umode_t mode)
+			struct file *file, umode_t mode)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(dir);
+	int err;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
 	if (!f2fs_is_checkpoint_ready(sbi))
 		return -ENOSPC;
 
-	return __f2fs_tmpfile(mnt_userns, dir, dentry, mode, false, NULL);
+	err = __f2fs_tmpfile(mnt_userns, dir, file, mode, false, NULL);
+
+	return finish_open_simple(file, err);
 }
 
 static int f2fs_create_whiteout(struct user_namespace *mnt_userns,
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 0b458beb318c..026daa8fc221 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -917,7 +917,7 @@ static int hugetlbfs_create(struct user_namespace *mnt_userns,
 }
 
 static int hugetlbfs_tmpfile(struct user_namespace *mnt_userns,
-			     struct inode *dir, struct dentry *dentry,
+			     struct inode *dir, struct file *file,
 			     umode_t mode)
 {
 	struct inode *inode;
@@ -926,8 +926,8 @@ static int hugetlbfs_tmpfile(struct user_namespace *mnt_userns,
 	if (!inode)
 		return -ENOSPC;
 	dir->i_ctime = dir->i_mtime = current_time(dir);
-	d_tmpfile(dentry, inode);
-	return 0;
+	d_tmpfile(file, inode);
+	return finish_open_simple(file, 0);
 }
 
 static int hugetlbfs_symlink(struct user_namespace *mnt_userns,
diff --git a/fs/minix/namei.c b/fs/minix/namei.c
index 937fa5fae2b8..8afdc408ca4f 100644
--- a/fs/minix/namei.c
+++ b/fs/minix/namei.c
@@ -53,16 +53,16 @@ static int minix_mknod(struct user_namespace *mnt_userns, struct inode *dir,
 }
 
 static int minix_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-			 struct dentry *dentry, umode_t mode)
+			 struct file *file, umode_t mode)
 {
 	int error;
 	struct inode *inode = minix_new_inode(dir, mode, &error);
 	if (inode) {
 		minix_set_inode(inode, 0);
 		mark_inode_dirty(inode);
-		d_tmpfile(dentry, inode);
+		d_tmpfile(file, inode);
 	}
-	return error;
+	return finish_open_simple(file, error);
 }
 
 static int minix_create(struct user_namespace *mnt_userns, struct inode *dir,
diff --git a/fs/namei.c b/fs/namei.c
index 4faf7e743664..ef001cc46ae5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3604,8 +3604,7 @@ static int vfs_tmpfile(struct user_namespace *mnt_userns,
 	file->f_path.mnt = parentpath->mnt;
 	file->f_path.dentry = child;
 	mode = vfs_prepare_mode(mnt_userns, dir, mode, mode, mode);
-	error = dir->i_op->tmpfile(mnt_userns, dir, child, mode);
-	error = finish_open_simple(file, error);
+	error = dir->i_op->tmpfile(mnt_userns, dir, file, mode);
 	dput(child);
 	if (error)
 		return error;
diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index bc66d0173e33..b3257e852820 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -146,15 +146,15 @@ static int ramfs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
 }
 
 static int ramfs_tmpfile(struct user_namespace *mnt_userns,
-			 struct inode *dir, struct dentry *dentry, umode_t mode)
+			 struct inode *dir, struct file *file, umode_t mode)
 {
 	struct inode *inode;
 
 	inode = ramfs_get_inode(dir->i_sb, dir, mode, 0);
 	if (!inode)
 		return -ENOSPC;
-	d_tmpfile(dentry, inode);
-	return 0;
+	d_tmpfile(file, inode);
+	return finish_open_simple(file, 0);
 }
 
 static const struct inode_operations ramfs_dir_inode_operations = {
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 86151889548e..f59acd6a3615 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -424,8 +424,9 @@ static void unlock_2_inodes(struct inode *inode1, struct inode *inode2)
 }
 
 static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-			 struct dentry *dentry, umode_t mode)
+			 struct file *file, umode_t mode)
 {
+	struct dentry *dentry = file->f_path.dentry;
 	struct inode *inode;
 	struct ubifs_info *c = dir->i_sb->s_fs_info;
 	struct ubifs_budget_req req = { .new_ino = 1, .new_dent = 1,
@@ -475,7 +476,7 @@ static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 
 	mutex_lock(&ui->ui_mutex);
 	insert_inode_hash(inode);
-	d_tmpfile(dentry, inode);
+	d_tmpfile(file, inode);
 	ubifs_assert(c, ui->dirty);
 
 	instantiated = 1;
@@ -489,7 +490,7 @@ static int ubifs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 
 	ubifs_release_budget(c, &req);
 
-	return 0;
+	return finish_open_simple(file, 0);
 
 out_cancel:
 	unlock_2_inodes(dir, inode);
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index b3d5f97f16cd..fb4c30e05245 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -626,7 +626,7 @@ static int udf_create(struct user_namespace *mnt_userns, struct inode *dir,
 }
 
 static int udf_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-		       struct dentry *dentry, umode_t mode)
+		       struct file *file, umode_t mode)
 {
 	struct inode *inode = udf_new_inode(dir, mode);
 
@@ -640,9 +640,9 @@ static int udf_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 	inode->i_op = &udf_file_inode_operations;
 	inode->i_fop = &udf_file_operations;
 	mark_inode_dirty(inode);
-	d_tmpfile(dentry, inode);
+	d_tmpfile(file, inode);
 	unlock_new_inode(inode);
-	return 0;
+	return finish_open_simple(file, 0);
 }
 
 static int udf_mknod(struct user_namespace *mnt_userns, struct inode *dir,
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 45518b8c613c..764409c466fd 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -167,7 +167,7 @@ xfs_generic_create(
 	struct dentry	*dentry,
 	umode_t		mode,
 	dev_t		rdev,
-	bool		tmpfile)	/* unnamed file */
+	struct file	*tmpfile)	/* unnamed file */
 {
 	struct inode	*inode;
 	struct xfs_inode *ip = NULL;
@@ -234,7 +234,7 @@ xfs_generic_create(
 		 * d_tmpfile can immediately set it back to zero.
 		 */
 		set_nlink(inode, 1);
-		d_tmpfile(dentry, inode);
+		d_tmpfile(tmpfile, inode);
 	} else
 		d_instantiate(dentry, inode);
 
@@ -261,7 +261,7 @@ xfs_vn_mknod(
 	umode_t			mode,
 	dev_t			rdev)
 {
-	return xfs_generic_create(mnt_userns, dir, dentry, mode, rdev, false);
+	return xfs_generic_create(mnt_userns, dir, dentry, mode, rdev, NULL);
 }
 
 STATIC int
@@ -272,7 +272,7 @@ xfs_vn_create(
 	umode_t			mode,
 	bool			flags)
 {
-	return xfs_generic_create(mnt_userns, dir, dentry, mode, 0, false);
+	return xfs_generic_create(mnt_userns, dir, dentry, mode, 0, NULL);
 }
 
 STATIC int
@@ -283,7 +283,7 @@ xfs_vn_mkdir(
 	umode_t			mode)
 {
 	return xfs_generic_create(mnt_userns, dir, dentry, mode | S_IFDIR, 0,
-				  false);
+				  NULL);
 }
 
 STATIC struct dentry *
@@ -1080,10 +1080,12 @@ STATIC int
 xfs_vn_tmpfile(
 	struct user_namespace	*mnt_userns,
 	struct inode		*dir,
-	struct dentry		*dentry,
+	struct file		*file,
 	umode_t			mode)
 {
-	return xfs_generic_create(mnt_userns, dir, dentry, mode, 0, true);
+	int err = xfs_generic_create(mnt_userns, dir, file->f_path.dentry, mode, 0, file);
+
+	return finish_open_simple(file, err);
 }
 
 static const struct inode_operations xfs_inode_operations = {
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 92c78ed02b54..bde9f8ff8869 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -16,6 +16,7 @@
 #include <linux/wait.h>
 
 struct path;
+struct file;
 struct vfsmount;
 
 /*
@@ -250,7 +251,7 @@ extern struct dentry * d_make_root(struct inode *);
 /* <clickety>-<click> the ramfs-type tree */
 extern void d_genocide(struct dentry *);
 
-extern void d_tmpfile(struct dentry *, struct inode *);
+extern void d_tmpfile(struct file *, struct inode *);
 
 extern struct dentry *d_find_alias(struct inode *);
 extern void d_prune_aliases(struct inode *);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f0d17eefb966..e08efecd2644 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2168,7 +2168,7 @@ struct inode_operations {
 			   struct file *, unsigned open_flag,
 			   umode_t create_mode);
 	int (*tmpfile) (struct user_namespace *, struct inode *,
-			struct dentry *, umode_t);
+			struct file *, umode_t);
 	int (*set_acl)(struct user_namespace *, struct inode *,
 		       struct posix_acl *, int);
 	int (*fileattr_set)(struct user_namespace *mnt_userns,
diff --git a/mm/shmem.c b/mm/shmem.c
index 42e5888bf84d..f63c51bc373e 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2912,7 +2912,7 @@ shmem_mknod(struct user_namespace *mnt_userns, struct inode *dir,
 
 static int
 shmem_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
-	      struct dentry *dentry, umode_t mode)
+	      struct file *file, umode_t mode)
 {
 	struct inode *inode;
 	int error = -ENOSPC;
@@ -2927,9 +2927,9 @@ shmem_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 		error = simple_acl_create(dir, inode);
 		if (error)
 			goto out_iput;
-		d_tmpfile(dentry, inode);
+		d_tmpfile(file, inode);
 	}
-	return error;
+	return finish_open_simple(file, error);
 out_iput:
 	iput(inode);
 	return error;
-- 
2.37.3

Re: [PATCH v3 8/9] vfs: open inside ->tmpfile()

[Christian Brauner]

On Tue, Sep 20, 2022 at 09:36:31PM +0200, Miklos Szeredi wrote:
> This is in preparation for adding tmpfile support to fuse, which requires
> that the tmpfile creation and opening are done as a single operation.
> 
> Replace the 'struct dentry *' argument of i_op->tmpfile with
> 'struct file *'.
> 
> Call finish_open_simple() as the last thing in ->tmpfile() instances (may
> be omitted in the error case).
> 
> Change d_tmpfile() argument to 'struct file *' as well to make callers more
> readable.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

Seems fine to me. Fwiw, it feels like all the file->f_path.dentry derefs
could be wrapped in a helper similar to file_inode(). I know we have
file_dentry() but that calls d_real() so not sure if that'll be correct
for all updated callers,

Re: [PATCH v3 8/9] vfs: open inside ->tmpfile()

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 11:08, Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, Sep 20, 2022 at 09:36:31PM +0200, Miklos Szeredi wrote:
> > This is in preparation for adding tmpfile support to fuse, which requires
> > that the tmpfile creation and opening are done as a single operation.
> >
> > Replace the 'struct dentry *' argument of i_op->tmpfile with
> > 'struct file *'.
> >
> > Call finish_open_simple() as the last thing in ->tmpfile() instances (may
> > be omitted in the error case).
> >
> > Change d_tmpfile() argument to 'struct file *' as well to make callers more
> > readable.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
>
> Seems fine to me. Fwiw, it feels like all the file->f_path.dentry derefs
> could be wrapped in a helper similar to file_inode(). I know we have
> file_dentry() but that calls d_real() so not sure if that'll be correct
> for all updated callers,

I don't think file_dentry() should be used for this.

file_dentry() is basically a hack for overlayfs's "fake path" thing.
It should only be used where strictly necessary.  At one point it
would be good to look again at cleaning this mess up.

Thanks,
Miklos

Re: [PATCH v3 8/9] vfs: open inside ->tmpfile()

[Christian Brauner]

On Wed, Sep 21, 2022 at 04:58:38PM +0200, Miklos Szeredi wrote:
> On Wed, 21 Sept 2022 at 11:08, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Tue, Sep 20, 2022 at 09:36:31PM +0200, Miklos Szeredi wrote:
> > > This is in preparation for adding tmpfile support to fuse, which requires
> > > that the tmpfile creation and opening are done as a single operation.
> > >
> > > Replace the 'struct dentry *' argument of i_op->tmpfile with
> > > 'struct file *'.
> > >
> > > Call finish_open_simple() as the last thing in ->tmpfile() instances (may
> > > be omitted in the error case).
> > >
> > > Change d_tmpfile() argument to 'struct file *' as well to make callers more
> > > readable.
> > >
> > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > > ---
> >
> > Seems fine to me. Fwiw, it feels like all the file->f_path.dentry derefs
> > could be wrapped in a helper similar to file_inode(). I know we have
> > file_dentry() but that calls d_real() so not sure if that'll be correct
> > for all updated callers,
> 
> I don't think file_dentry() should be used for this.
> 
> file_dentry() is basically a hack for overlayfs's "fake path" thing.
> It should only be used where strictly necessary.  At one point it
> would be good to look again at cleaning this mess up.

Yeah, that's what I was getting at. The file_dentry() helper would
ideally just be as simple as file_inode() and then we'd have
file_dentry_real() for the stacking filesystem scenarios.

Re: [PATCH v3 8/9] vfs: open inside ->tmpfile()

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 17:08, Christian Brauner <brauner@kernel.org> wrote:
>
> On Wed, Sep 21, 2022 at 04:58:38PM +0200, Miklos Szeredi wrote:

> > file_dentry() is basically a hack for overlayfs's "fake path" thing.
> > It should only be used where strictly necessary.  At one point it
> > would be good to look again at cleaning this mess up.
>
> Yeah, that's what I was getting at. The file_dentry() helper would
> ideally just be as simple as file_inode() and then we'd have
> file_dentry_real() for the stacking filesystem scenarios.

Sure.  But that again belongs to a separate discussion, imo.

Thanks,
Miklos

Re: [PATCH v3 8/9] vfs: open inside ->tmpfile()

[Christian Brauner]

On Wed, Sep 21, 2022 at 05:27:18PM +0200, Miklos Szeredi wrote:
> On Wed, 21 Sept 2022 at 17:08, Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Wed, Sep 21, 2022 at 04:58:38PM +0200, Miklos Szeredi wrote:
> 
> > > file_dentry() is basically a hack for overlayfs's "fake path" thing.
> > > It should only be used where strictly necessary.  At one point it
> > > would be good to look again at cleaning this mess up.
> >
> > Yeah, that's what I was getting at. The file_dentry() helper would
> > ideally just be as simple as file_inode() and then we'd have
> > file_dentry_real() for the stacking filesystem scenarios.
> 
> Sure.  But that again belongs to a separate discussion, imo.

Separate patchset for sure.

Re: [PATCH v3 8/9] vfs: open inside ->tmpfile()

[Al Viro]

On Wed, Sep 21, 2022 at 05:07:50PM +0200, Christian Brauner wrote:

> > I don't think file_dentry() should be used for this.
> > 
> > file_dentry() is basically a hack for overlayfs's "fake path" thing.
> > It should only be used where strictly necessary.  At one point it
> > would be good to look again at cleaning this mess up.
> 
> Yeah, that's what I was getting at. The file_dentry() helper would
> ideally just be as simple as file_inode() and then we'd have
> file_dentry_real() for the stacking filesystem scenarios.

	I would rather minimize the number of places where we access
file->f_path.dentry in the first place.  Any of those is asking for
confusion and overlayfs-triggered bugs.

	A helper for that would invite bugs where it gets used in
places of file_dentry() and vice versa; sure, the same bugs are
possible for open-coded variants (and we had such bugs), but I would
rather have fewer places doing that to start with (don't get me
started on the debugfs design.  Please.)

[PATCH v3 9/9] fuse: implement ->tmpfile()

[Miklos Szeredi]

This is basically equivalent to the FUSE_CREATE operation which creates and
opens a regular file.

Add a new FUSE_TMPFILE operation, otherwise just reuse the protocol and the
code for FUSE_CREATE.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dir.c             | 25 ++++++++++++++++++++++---
 fs/fuse/fuse_i.h          |  3 +++
 include/uapi/linux/fuse.h |  6 +++++-
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index b585b04e815e..01b2d5c5a64a 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -529,7 +529,7 @@ static int get_security_context(struct dentry *entry, umode_t mode,
  */
 static int fuse_create_open(struct inode *dir, struct dentry *entry,
 			    struct file *file, unsigned int flags,
-			    umode_t mode)
+			    umode_t mode, u32 opcode)
 {
 	int err;
 	struct inode *inode;
@@ -573,7 +573,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 		inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID;
 	}
 
-	args.opcode = FUSE_CREATE;
+	args.opcode = opcode;
 	args.nodeid = get_node_id(dir);
 	args.in_numargs = 2;
 	args.in_args[0].size = sizeof(inarg);
@@ -676,7 +676,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 	if (fc->no_create)
 		goto mknod;
 
-	err = fuse_create_open(dir, entry, file, flags, mode);
+	err = fuse_create_open(dir, entry, file, flags, mode, FUSE_CREATE);
 	if (err == -ENOSYS) {
 		fc->no_create = 1;
 		goto mknod;
@@ -802,6 +802,24 @@ static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
 	return fuse_mknod(&init_user_ns, dir, entry, mode, 0);
 }
 
+static int fuse_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
+			struct file *file, umode_t mode)
+{
+	struct fuse_conn *fc = get_fuse_conn(dir);
+	int err;
+
+	if (fc->no_tmpfile)
+		goto no_tmpfile;
+
+	err = fuse_create_open(dir, file->f_path.dentry, file, file->f_flags, mode, FUSE_TMPFILE);
+	if (err == -ENOSYS) {
+		fc->no_tmpfile = 1;
+no_tmpfile:
+		err = -EOPNOTSUPP;
+	}
+	return err;
+}
+
 static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 		      struct dentry *entry, umode_t mode)
 {
@@ -1913,6 +1931,7 @@ static const struct inode_operations fuse_dir_inode_operations = {
 	.setattr	= fuse_setattr,
 	.create		= fuse_create,
 	.atomic_open	= fuse_atomic_open,
+	.tmpfile	= fuse_tmpfile,
 	.mknod		= fuse_mknod,
 	.permission	= fuse_permission,
 	.getattr	= fuse_getattr,
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 488b460e046f..98a9cf531873 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -784,6 +784,9 @@ struct fuse_conn {
 	/* Does the filesystem support per inode DAX? */
 	unsigned int inode_dax:1;
 
+	/* Is tmpfile not implemented by fs? */
+	unsigned int no_tmpfile:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index d6ccee961891..76ee8f9e024a 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -194,6 +194,9 @@
  *  - add FUSE_SECURITY_CTX init flag
  *  - add security context to create, mkdir, symlink, and mknod requests
  *  - add FUSE_HAS_INODE_DAX, FUSE_ATTR_DAX
+ *
+ *  7.37
+ *  - add FUSE_TMPFILE
  */
 
 #ifndef _LINUX_FUSE_H
@@ -229,7 +232,7 @@
 #define FUSE_KERNEL_VERSION 7
 
 /** Minor version number of this interface */
-#define FUSE_KERNEL_MINOR_VERSION 36
+#define FUSE_KERNEL_MINOR_VERSION 37
 
 /** The node ID of the root inode */
 #define FUSE_ROOT_ID 1
@@ -537,6 +540,7 @@ enum fuse_opcode {
 	FUSE_SETUPMAPPING	= 48,
 	FUSE_REMOVEMAPPING	= 49,
 	FUSE_SYNCFS		= 50,
+	FUSE_TMPFILE		= 51,
 
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
-- 
2.37.3

Re: [PATCH v3 9/9] fuse: implement ->tmpfile()

[Christian Brauner]

On Tue, Sep 20, 2022 at 09:36:32PM +0200, Miklos Szeredi wrote:
> This is basically equivalent to the FUSE_CREATE operation which creates and
> opens a regular file.
> 
> Add a new FUSE_TMPFILE operation, otherwise just reuse the protocol and the
> code for FUSE_CREATE.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/dir.c             | 25 ++++++++++++++++++++++---
>  fs/fuse/fuse_i.h          |  3 +++
>  include/uapi/linux/fuse.h |  6 +++++-
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index b585b04e815e..01b2d5c5a64a 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -529,7 +529,7 @@ static int get_security_context(struct dentry *entry, umode_t mode,
>   */
>  static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  			    struct file *file, unsigned int flags,
> -			    umode_t mode)
> +			    umode_t mode, u32 opcode)
>  {
>  	int err;
>  	struct inode *inode;
> @@ -573,7 +573,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  		inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID;
>  	}
>  
> -	args.opcode = FUSE_CREATE;
> +	args.opcode = opcode;
>  	args.nodeid = get_node_id(dir);
>  	args.in_numargs = 2;
>  	args.in_args[0].size = sizeof(inarg);
> @@ -676,7 +676,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  	if (fc->no_create)
>  		goto mknod;
>  
> -	err = fuse_create_open(dir, entry, file, flags, mode);
> +	err = fuse_create_open(dir, entry, file, flags, mode, FUSE_CREATE);
>  	if (err == -ENOSYS) {
>  		fc->no_create = 1;
>  		goto mknod;
> @@ -802,6 +802,24 @@ static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
>  	return fuse_mknod(&init_user_ns, dir, entry, mode, 0);
>  }
>  
> +static int fuse_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
> +			struct file *file, umode_t mode)
> +{
> +	struct fuse_conn *fc = get_fuse_conn(dir);
> +	int err;
> +
> +	if (fc->no_tmpfile)
> +		goto no_tmpfile;
> +
> +	err = fuse_create_open(dir, file->f_path.dentry, file, file->f_flags, mode, FUSE_TMPFILE);
> +	if (err == -ENOSYS) {
> +		fc->no_tmpfile = 1;
> +no_tmpfile:
> +		err = -EOPNOTSUPP;
> +	}
> +	return err;
> +}

Hm, seems like this could avoid the goto into an if-block:

static int fuse_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
			struct file *file, umode_t mode)
{
	struct fuse_conn *fc = get_fuse_conn(dir);
	int err;

	if (fc->no_tmpfile)
		return -EOPNOTSUPP;

	err = fuse_create_open(dir, file->f_path.dentry, file, file->f_flags, mode, FUSE_TMPFILE);
	if (err == -ENOSYS) {
		fc->no_tmpfile = 1;
		err = -EOPNOTSUPP;
	}
	return err;
}

> +
>  static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>  		      struct dentry *entry, umode_t mode)
>  {
> @@ -1913,6 +1931,7 @@ static const struct inode_operations fuse_dir_inode_operations = {
>  	.setattr	= fuse_setattr,
>  	.create		= fuse_create,
>  	.atomic_open	= fuse_atomic_open,
> +	.tmpfile	= fuse_tmpfile,
>  	.mknod		= fuse_mknod,
>  	.permission	= fuse_permission,
>  	.getattr	= fuse_getattr,
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 488b460e046f..98a9cf531873 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -784,6 +784,9 @@ struct fuse_conn {
>  	/* Does the filesystem support per inode DAX? */
>  	unsigned int inode_dax:1;
>  
> +	/* Is tmpfile not implemented by fs? */
> +	unsigned int no_tmpfile:1;

Just a nit, it might be nicer to turn this into a positive, i.e.,
unsigned int has_tmpfile:1. Easier to understand as people usually
aren't great at processing negations.

> +
>  	/** The number of requests waiting for completion */
>  	atomic_t num_waiting;
>  
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index d6ccee961891..76ee8f9e024a 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -194,6 +194,9 @@
>   *  - add FUSE_SECURITY_CTX init flag
>   *  - add security context to create, mkdir, symlink, and mknod requests
>   *  - add FUSE_HAS_INODE_DAX, FUSE_ATTR_DAX
> + *
> + *  7.37
> + *  - add FUSE_TMPFILE
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -229,7 +232,7 @@
>  #define FUSE_KERNEL_VERSION 7
>  
>  /** Minor version number of this interface */
> -#define FUSE_KERNEL_MINOR_VERSION 36
> +#define FUSE_KERNEL_MINOR_VERSION 37
>  
>  /** The node ID of the root inode */
>  #define FUSE_ROOT_ID 1
> @@ -537,6 +540,7 @@ enum fuse_opcode {
>  	FUSE_SETUPMAPPING	= 48,
>  	FUSE_REMOVEMAPPING	= 49,
>  	FUSE_SYNCFS		= 50,
> +	FUSE_TMPFILE		= 51,
>  
>  	/* CUSE specific operations */
>  	CUSE_INIT		= 4096,
> -- 
> 2.37.3
> 

Re: [PATCH v3 9/9] fuse: implement ->tmpfile()

[Miklos Szeredi]

On Wed, 21 Sept 2022 at 11:17, Christian Brauner <brauner@kernel.org> wrote:

>
> Hm, seems like this could avoid the goto into an if-block:
>
> static int fuse_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
>                         struct file *file, umode_t mode)
> {
>         struct fuse_conn *fc = get_fuse_conn(dir);
>         int err;
>
>         if (fc->no_tmpfile)
>                 return -EOPNOTSUPP;
>
>         err = fuse_create_open(dir, file->f_path.dentry, file, file->f_flags, mode, FUSE_TMPFILE);
>         if (err == -ENOSYS) {
>                 fc->no_tmpfile = 1;
>                 err = -EOPNOTSUPP;
>         }
>         return err;
> }

Okay.

> > +
> >  static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> >                     struct dentry *entry, umode_t mode)
> >  {
> > @@ -1913,6 +1931,7 @@ static const struct inode_operations fuse_dir_inode_operations = {
> >       .setattr        = fuse_setattr,
> >       .create         = fuse_create,
> >       .atomic_open    = fuse_atomic_open,
> > +     .tmpfile        = fuse_tmpfile,
> >       .mknod          = fuse_mknod,
> >       .permission     = fuse_permission,
> >       .getattr        = fuse_getattr,
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 488b460e046f..98a9cf531873 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -784,6 +784,9 @@ struct fuse_conn {
> >       /* Does the filesystem support per inode DAX? */
> >       unsigned int inode_dax:1;
> >
> > +     /* Is tmpfile not implemented by fs? */
> > +     unsigned int no_tmpfile:1;
>
> Just a nit, it might be nicer to turn this into a positive, i.e.,
> unsigned int has_tmpfile:1. Easier to understand as people usually
> aren't great at processing negations.

Fuse has zillions of these no_foobar flags.  Turning this single one
into a positive would be much more confusing IMO.

Thanks,
Miklos