diff mbox series

[v3,1/5] ovl: do not open non-data lower file for fsync

Message ID 20241007141925.327055-2-amir73il@gmail.com (mailing list archive)
State New
Headers show
Series Store overlay real upper file in ovl_file | expand

Commit Message

Amir Goldstein Oct. 7, 2024, 2:19 p.m. UTC 
ovl_fsync() with !datasync opens a backing file from the top most dentry
in the stack, checks if this dentry is non-upper and skips the fsync.

In case of an overlay dentry stack with lower data and lower metadata
above it, but without an upper metadata above it, the backing file is
opened from the top most lower metadata dentry and never used.

Refactor the helper ovl_real_fdget_meta() into ovl_real_fdget_path() and
ovl_upper_fdget() where the latter returns an empty struct fd in that
case to avoid the unneeded backing file open.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/file.c | 71 +++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 28 deletions(-)

Comments

Al Viro Oct. 7, 2024, 2:39 p.m. UTC | #1 
On Mon, Oct 07, 2024 at 04:19:21PM +0200, Amir Goldstein wrote:
> +static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> +{
> +	struct dentry *dentry = file_dentry(file);
> +	struct path realpath;
> +	enum ovl_path_type type;
> +
> +	if (data)
> +		type = ovl_path_realdata(dentry, &realpath);
> +	else
> +		type = ovl_path_real(dentry, &realpath);
> +
> +	real->word = 0;
> +	/* Not interested in lower nor in upper meta if data was requested */
> +	if (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type)))
> +		return 0;
> +
> +	return ovl_real_fdget_path(file, real, &realpath);
>  }
>  
>  static int ovl_open(struct inode *inode, struct file *file)
> @@ -394,16 +411,14 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  	if (ret <= 0)
>  		return ret;
>  
> -	ret = ovl_real_fdget_meta(file, &real, !datasync);
> -	if (ret)
> +	/* Don't sync lower file for fear of receiving EROFS error */
> +	ret = ovl_upper_fdget(file, &real, datasync);
> +	if (ret || fd_empty(real))
>  		return ret;

Is there any real point in keeping ovl_upper_fdget() separate from the
only caller?  Note that the checks for type make a lot more sense
in ovl_fsync() than buried in a separate helper and this fd_empty()
is a "do we have the wrong type?" check in disguise.

Why not just expand it at the call site?
Amir Goldstein Oct. 7, 2024, 3:56 p.m. UTC | #2 
On Mon, Oct 7, 2024 at 4:39 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Oct 07, 2024 at 04:19:21PM +0200, Amir Goldstein wrote:
> > +static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> > +{
> > +     struct dentry *dentry = file_dentry(file);
> > +     struct path realpath;
> > +     enum ovl_path_type type;
> > +
> > +     if (data)
> > +             type = ovl_path_realdata(dentry, &realpath);
> > +     else
> > +             type = ovl_path_real(dentry, &realpath);
> > +
> > +     real->word = 0;
> > +     /* Not interested in lower nor in upper meta if data was requested */
> > +     if (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type)))
> > +             return 0;
> > +
> > +     return ovl_real_fdget_path(file, real, &realpath);
> >  }
> >
> >  static int ovl_open(struct inode *inode, struct file *file)
> > @@ -394,16 +411,14 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >       if (ret <= 0)
> >               return ret;
> >
> > -     ret = ovl_real_fdget_meta(file, &real, !datasync);
> > -     if (ret)
> > +     /* Don't sync lower file for fear of receiving EROFS error */
> > +     ret = ovl_upper_fdget(file, &real, datasync);
> > +     if (ret || fd_empty(real))
> >               return ret;
>
> Is there any real point in keeping ovl_upper_fdget() separate from the
> only caller?  Note that the checks for type make a lot more sense
> in ovl_fsync() than buried in a separate helper and this fd_empty()
> is a "do we have the wrong type?" check in disguise.
>
> Why not just expand it at the call site?

You are right (again) I open code it, it looks much better:

        /* Don't sync lower file for fear of receiving EROFS error */
-       upperfile = ovl_upper_file(file, datasync);
-       if (IS_ERR_OR_NULL(upperfile))
+       type = ovl_path_type(dentry);
+       if (!OVL_TYPE_UPPER(type) || (datasync && OVL_TYPE_MERGE(type)))
+               return 0;
+
+       ovl_path_upper(dentry, &upperpath);
+       upperfile = ovl_real_file_path(file, &upperpath);
+       if (IS_ERR(upperfile))
                return PTR_ERR(upperfile);


Thanks,
Amir.
Al Viro Oct. 7, 2024, 4:55 p.m. UTC | #3 
On Mon, Oct 07, 2024 at 04:19:21PM +0200, Amir Goldstein wrote:
> +static int ovl_real_fdget_path(const struct file *file, struct fd *real,
> +			       struct path *realpath)

> -	if (allow_meta) {
> -		ovl_path_real(dentry, &realpath);
> -	} else {
> -		/* lazy lookup and verify of lowerdata */
> -		err = ovl_verify_lowerdata(dentry);
This check went
> -		if (err)
> -			return err;
> -
> -		ovl_path_realdata(dentry, &realpath);
> -	}

> @@ -138,7 +129,33 @@ static int ovl_real_fdget(const struct file *file, struct fd *real)
>  		return 0;
>  	}
>  
> -	return ovl_real_fdget_meta(file, real, false);
> +	/* lazy lookup and verify of lowerdata */
> +	err = ovl_verify_lowerdata(dentry);

... here

> +	if (err)
> +		return err;
> +
> +	ovl_path_realdata(dentry, &realpath);

> +static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> +{
> +	struct dentry *dentry = file_dentry(file);
> +	struct path realpath;
> +	enum ovl_path_type type;
> +
> +	if (data)
> +		type = ovl_path_realdata(dentry, &realpath);

... but not here.

I can see the point of not doing that in ->fsync() after we'd already
done ovl_verify_lowerdata() at open time, but what's different about
->read_iter() and friends that also come only after ->open()?
IOW, why is fdatasync() different from other data-access cases?
Al Viro Oct. 7, 2024, 5:04 p.m. UTC | #4 
On Mon, Oct 07, 2024 at 05:56:51PM +0200, Amir Goldstein wrote:

> You are right (again) I open code it, it looks much better:
> 
>         /* Don't sync lower file for fear of receiving EROFS error */
> -       upperfile = ovl_upper_file(file, datasync);
> -       if (IS_ERR_OR_NULL(upperfile))
> +       type = ovl_path_type(dentry);
> +       if (!OVL_TYPE_UPPER(type) || (datasync && OVL_TYPE_MERGE(type)))
> +               return 0;
> +
> +       ovl_path_upper(dentry, &upperpath);

OK, this answers my other question (re why skipping ovl_verify_lowerdata()
in ovl_upper_file() is OK); you don't want to mess with the lowerdata
anyway, so...
Al Viro Oct. 7, 2024, 5:13 p.m. UTC | #5 
On Mon, Oct 07, 2024 at 05:55:40PM +0100, Al Viro wrote:

> > +static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> > +{
> > +	struct dentry *dentry = file_dentry(file);
> > +	struct path realpath;
> > +	enum ovl_path_type type;
> > +
> > +	if (data)
> > +		type = ovl_path_realdata(dentry, &realpath);
> 
> ... but not here.
> 
> I can see the point of not doing that in ->fsync() after we'd already
> done ovl_verify_lowerdata() at open time, but what's different about
> ->read_iter() and friends that also come only after ->open()?
> IOW, why is fdatasync() different from other data-access cases?

Nevermind that one - the answer is that ovl_path_realdata()
calls ovl_path_lowerdata() only in case when it sees
!OVL_TYPE_UPPER(type) || OVL_TYPE_MERGE(type), which guarantees
that the type check below that if (data) will fail anyway
(check being (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type))).

So this reduces the fdatasync case to
	if (!OVL_TYPE_UPPER(type) || OVL_TYPE_MERGE(type))
		fail;
	ovl_path_upper(dentry, &realpath);
just as the fsync case reduces to
	if (!OVL_TYPE_UPPER(type))
		fail;
	ovl_path_upper(dentry, &realpath);

making any lowerpath-related stuff irrelevant.
diff mbox series

Patch

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 4504493b20be..f5d0498355d0 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -89,32 +89,19 @@  static int ovl_change_flags(struct file *file, unsigned int flags)
 	return 0;
 }
 
-static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
-			       bool allow_meta)
+static int ovl_real_fdget_path(const struct file *file, struct fd *real,
+			       struct path *realpath)
 {
-	struct dentry *dentry = file_dentry(file);
 	struct file *realfile = file->private_data;
-	struct path realpath;
-	int err;
 
 	real->word = (unsigned long)realfile;
 
-	if (allow_meta) {
-		ovl_path_real(dentry, &realpath);
-	} else {
-		/* lazy lookup and verify of lowerdata */
-		err = ovl_verify_lowerdata(dentry);
-		if (err)
-			return err;
-
-		ovl_path_realdata(dentry, &realpath);
-	}
-	if (!realpath.dentry)
+	if (WARN_ON_ONCE(!realpath->dentry))
 		return -EIO;
 
 	/* Has it been copied up since we'd opened it? */
-	if (unlikely(file_inode(realfile) != d_inode(realpath.dentry))) {
-		struct file *f = ovl_open_realfile(file, &realpath);
+	if (unlikely(file_inode(realfile) != d_inode(realpath->dentry))) {
+		struct file *f = ovl_open_realfile(file, realpath);
 		if (IS_ERR(f))
 			return PTR_ERR(f);
 		real->word = (unsigned long)f | FDPUT_FPUT;
@@ -130,7 +117,11 @@  static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 
 static int ovl_real_fdget(const struct file *file, struct fd *real)
 {
-	if (d_is_dir(file_dentry(file))) {
+	struct dentry *dentry = file_dentry(file);
+	struct path realpath;
+	int err;
+
+	if (d_is_dir(dentry)) {
 		struct file *f = ovl_dir_real_file(file, false);
 		if (IS_ERR(f))
 			return PTR_ERR(f);
@@ -138,7 +129,33 @@  static int ovl_real_fdget(const struct file *file, struct fd *real)
 		return 0;
 	}
 
-	return ovl_real_fdget_meta(file, real, false);
+	/* lazy lookup and verify of lowerdata */
+	err = ovl_verify_lowerdata(dentry);
+	if (err)
+		return err;
+
+	ovl_path_realdata(dentry, &realpath);
+
+	return ovl_real_fdget_path(file, real, &realpath);
+}
+
+static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
+{
+	struct dentry *dentry = file_dentry(file);
+	struct path realpath;
+	enum ovl_path_type type;
+
+	if (data)
+		type = ovl_path_realdata(dentry, &realpath);
+	else
+		type = ovl_path_real(dentry, &realpath);
+
+	real->word = 0;
+	/* Not interested in lower nor in upper meta if data was requested */
+	if (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type)))
+		return 0;
+
+	return ovl_real_fdget_path(file, real, &realpath);
 }
 
 static int ovl_open(struct inode *inode, struct file *file)
@@ -394,16 +411,14 @@  static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	if (ret <= 0)
 		return ret;
 
-	ret = ovl_real_fdget_meta(file, &real, !datasync);
-	if (ret)
+	/* Don't sync lower file for fear of receiving EROFS error */
+	ret = ovl_upper_fdget(file, &real, datasync);
+	if (ret || fd_empty(real))
 		return ret;
 
-	/* Don't sync lower file for fear of receiving EROFS error */
-	if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
-		old_cred = ovl_override_creds(file_inode(file)->i_sb);
-		ret = vfs_fsync_range(fd_file(real), start, end, datasync);
-		revert_creds(old_cred);
-	}
+	old_cred = ovl_override_creds(file_inode(file)->i_sb);
+	ret = vfs_fsync_range(fd_file(real), start, end, datasync);
+	revert_creds(old_cred);
 
 	fdput(real);