Re: [PATCH v2 4/8] ovl: use tmpfile_open() helper

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Sep 19, 2022 at 04:10:27PM +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>
> ---
>  fs/overlayfs/copy_up.c   | 49 ++++++++++++++++++++++------------------
>  fs/overlayfs/overlayfs.h | 12 ++++++----
>  fs/overlayfs/super.c     | 10 ++++----
>  3 files changed, 40 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index fdde6c56cc3d..ac087b48b5da 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -194,16 +194,16 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
>  }
>  
>  static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> -			    struct path *new, loff_t len)
> +			    struct path *new, struct file *new_file, loff_t len)
>  {

Ugh...  Lift opening into both callers and get rid of struct path *new,
please.  Would be much easier to follow that way...

> -	err = ovl_copy_up_inode(c, temp);
> +	err = ovl_copy_up_inode(c, temp, NULL);

FWIW, I would consider passing a struct file * in all cases, with O_PATH
for non-regular ones...

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

	Careful - that part essentially checks if we have a working
->tmpfile(), but we rely upon more than just having it - we want
dentry-based setxattr() and friends to work after O_TMPFILE.
Are we making that a requirement for ->tmpfile()?  I.e. that
after O_TMPFILE open notify_change() et.al. on its dentry
will work *without* corresponding struct file.  In particular,
fuse seems to check for ATTR_FILE...