[PATCH v2 0/4] Stash overlay real upper file in backing_file

[PATCH v2 0/4] Stash overlay real upper file in backing_file

[Amir Goldstein]

Hi all,

This is v2 of the code to avoid temporary backing file opens in
overlayfs, taking into account Al's comments on v1 [1].

Miklos,

The implementation of ovl_real_file_path() helper is roughly based on
ovl_real_dir_file().

do you see any problems with this approach or any races not handled?
Note that I did have a logical bug in v1 (always choosing the stashed
upperfile if it exists), so there may be more.

Thanks,
Amir.

Changes since v1:
- Use helpers ovl_real_file() and ovl_upper_file() to express that
  ovl_real_file() cannot return NULL
- Fix readability and bug is code to select and store stashed upperfile

[1] https://lore.kernel.org/linux-fsdevel/20241004102342.179434-1-amir73il@gmail.com/

Amir Goldstein (4):
  ovl: do not open non-data lower file for fsync
  ovl: stash upper real file in backing_file struct
  ovl: convert ovl_real_fdget_path() callers to ovl_real_file_path()
  ovl: convert ovl_real_fdget() callers to ovl_real_file()

 fs/file_table.c     |   7 ++
 fs/internal.h       |   6 +
 fs/overlayfs/file.c | 288 ++++++++++++++++++++++++++------------------
 3 files changed, 182 insertions(+), 119 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] ovl: do not open non-data lower file for fsync

[Amir Goldstein]

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 | 61 +++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 4504493b20be..d40c10a6bfac 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,8 +411,8 @@ 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)
+	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 */
-- 
2.34.1

[PATCH v2 2/4] ovl: stash upper real file in backing_file struct

[Amir Goldstein]

When an overlayfs file is opened as lower and then the file is copied up,
every operation on the overlayfs open file will open a temporary backing
file to the upper dentry and close it at the end of the operation.

The original (lower) real file is stored in file->private_data pointer.
We could have allocated another container struct for file->private_data
to potentially store two backing files, the lower and the upper.

However the original backing file struct is not very space optimized
(and it has no memcache pool), so add a private_data pointer to the
backing_file struct and store the optional second backing upper file in
there instead of opening a temporary upper file on every operation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/file_table.c     |  7 +++++
 fs/internal.h       |  6 ++++
 fs/overlayfs/file.c | 70 +++++++++++++++++++++++++++++++++++++++------
 3 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index eed5ffad9997..1c2c08a5a66a 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -47,6 +47,7 @@ static struct percpu_counter nr_files __cacheline_aligned_in_smp;
 struct backing_file {
 	struct file file;
 	struct path user_path;
+	void *private_data;
 };
 
 static inline struct backing_file *backing_file(struct file *f)
@@ -60,6 +61,12 @@ struct path *backing_file_user_path(struct file *f)
 }
 EXPORT_SYMBOL_GPL(backing_file_user_path);
 
+void **backing_file_private_ptr(struct file *f)
+{
+	return &backing_file(f)->private_data;
+}
+EXPORT_SYMBOL_GPL(backing_file_private_ptr);
+
 static inline void file_free(struct file *f)
 {
 	security_file_free(f);
diff --git a/fs/internal.h b/fs/internal.h
index 8c1b7acbbe8f..b1152a3e8ba2 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -100,6 +100,12 @@ extern void chroot_fs_refs(const struct path *, const struct path *);
 struct file *alloc_empty_file(int flags, const struct cred *cred);
 struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred);
 struct file *alloc_empty_backing_file(int flags, const struct cred *cred);
+void **backing_file_private_ptr(struct file *f);
+
+static inline void *backing_file_private(struct file *f)
+{
+	return READ_ONCE(*backing_file_private_ptr(f));
+}
 
 static inline void file_put_write_access(struct file *file)
 {
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index d40c10a6bfac..42f9bbdd65b4 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -14,6 +14,8 @@
 #include <linux/backing-file.h>
 #include "overlayfs.h"
 
+#include "../internal.h"	/* for backing_file_private{,_ptr}() */
+
 static char ovl_whatisit(struct inode *inode, struct inode *realinode)
 {
 	if (realinode != ovl_inode_upper(inode))
@@ -89,29 +91,70 @@ static int ovl_change_flags(struct file *file, unsigned int flags)
 	return 0;
 }
 
+static bool ovl_is_real_file(const struct file *realfile,
+			     const struct path *realpath)
+{
+	return file_inode(realfile) == d_inode(realpath->dentry);
+}
+
 static int ovl_real_fdget_path(const struct file *file, struct fd *real,
 			       struct path *realpath)
 {
 	struct file *realfile = file->private_data;
+	struct file *upperfile = backing_file_private(realfile);
 
-	real->word = (unsigned long)realfile;
+	real->word = 0;
 
 	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 (IS_ERR(f))
-			return PTR_ERR(f);
-		real->word = (unsigned long)f | FDPUT_FPUT;
-		return 0;
+	/*
+	 * Usually, if we operated on a stashed upperfile once, all following
+	 * operations will operate on the stashed upperfile, but there is one
+	 * exception - ovl_fsync(datasync = false) can populate the stashed
+	 * upperfile to perform fsync on upper metadata inode.  In this case,
+	 * following read/write operations will not use the stashed upperfile.
+	 */
+	if (upperfile && likely(ovl_is_real_file(upperfile, realpath))) {
+		realfile = upperfile;
+		goto checkflags;
 	}
 
+	/*
+	 * If realfile is lower and has been copied up since we'd opened it,
+	 * open the real upper file and stash it in backing_file_private().
+	 */
+	if (unlikely(!ovl_is_real_file(realfile, realpath))) {
+		struct file *old;
+
+		/* Either stashed realfile or upperfile must match realinode */
+		if (WARN_ON_ONCE(upperfile))
+			return -EIO;
+
+		upperfile = ovl_open_realfile(file, realpath);
+		if (IS_ERR(upperfile))
+			return PTR_ERR(upperfile);
+
+		old = cmpxchg_release(backing_file_private_ptr(realfile), NULL,
+				      upperfile);
+		if (old) {
+			fput(upperfile);
+			upperfile = old;
+		}
+
+		/* Stashed upperfile that won the race must match realinode */
+		if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
+			return -EIO;
+
+		realfile = upperfile;
+	}
+
+checkflags:
 	/* Did the flags change since open? */
 	if (unlikely((file->f_flags ^ realfile->f_flags) & ~OVL_OPEN_FLAGS))
 		return ovl_change_flags(realfile, file->f_flags);
 
+	real->word = (unsigned long)realfile;
 	return 0;
 }
 
@@ -192,7 +235,16 @@ static int ovl_open(struct inode *inode, struct file *file)
 
 static int ovl_release(struct inode *inode, struct file *file)
 {
-	fput(file->private_data);
+	struct file *realfile = file->private_data;
+	struct file *upperfile = backing_file_private(realfile);
+
+	fput(realfile);
+	/*
+	 * If realfile is lower and file was copied up and accessed, we need
+	 * to put reference also on the stashed real upperfile.
+	 */
+	if (upperfile)
+		fput(upperfile);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2 2/4] ovl: stash upper real file in backing_file struct

[Al Viro]

On Sun, Oct 06, 2024 at 10:23:57AM +0200, Amir Goldstein wrote:
> +	/*
> +	 * Usually, if we operated on a stashed upperfile once, all following
> +	 * operations will operate on the stashed upperfile, but there is one
> +	 * exception - ovl_fsync(datasync = false) can populate the stashed
> +	 * upperfile to perform fsync on upper metadata inode.  In this case,
> +	 * following read/write operations will not use the stashed upperfile.
> +	 */
> +	if (upperfile && likely(ovl_is_real_file(upperfile, realpath))) {
> +		realfile = upperfile;
> +		goto checkflags;
>  	}
>  
> +	/*
> +	 * If realfile is lower and has been copied up since we'd opened it,
> +	 * open the real upper file and stash it in backing_file_private().
> +	 */
> +	if (unlikely(!ovl_is_real_file(realfile, realpath))) {
> +		struct file *old;
> +
> +		/* Either stashed realfile or upperfile must match realinode */
> +		if (WARN_ON_ONCE(upperfile))
> +			return -EIO;
> +
> +		upperfile = ovl_open_realfile(file, realpath);
> +		if (IS_ERR(upperfile))
> +			return PTR_ERR(upperfile);
> +
> +		old = cmpxchg_release(backing_file_private_ptr(realfile), NULL,
> +				      upperfile);
> +		if (old) {
> +			fput(upperfile);
> +			upperfile = old;
> +		}
> +
> +		/* Stashed upperfile that won the race must match realinode */
> +		if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
> +			return -EIO;
> +
> +		realfile = upperfile;
> +	}
> +
> +checkflags:

Hmm...  That still feels awkward.  Question: can we reach that code with
	* non-NULL upperfile
	* false ovl_is_real_file(upperfile, realpath)
	* true ovl_is_real_file(realfile, realpath)
Is that really possible?

Re: [PATCH v2 2/4] ovl: stash upper real file in backing_file struct

[Al Viro]

On Sun, Oct 06, 2024 at 10:04:26PM +0100, Al Viro wrote:
> On Sun, Oct 06, 2024 at 10:23:57AM +0200, Amir Goldstein wrote:
> > +	/*
> > +	 * Usually, if we operated on a stashed upperfile once, all following
> > +	 * operations will operate on the stashed upperfile, but there is one
> > +	 * exception - ovl_fsync(datasync = false) can populate the stashed
> > +	 * upperfile to perform fsync on upper metadata inode.  In this case,
> > +	 * following read/write operations will not use the stashed upperfile.
> > +	 */
> > +	if (upperfile && likely(ovl_is_real_file(upperfile, realpath))) {
> > +		realfile = upperfile;
> > +		goto checkflags;
> >  	}
> >  
> > +	/*
> > +	 * If realfile is lower and has been copied up since we'd opened it,
> > +	 * open the real upper file and stash it in backing_file_private().
> > +	 */
> > +	if (unlikely(!ovl_is_real_file(realfile, realpath))) {
> > +		struct file *old;
> > +
> > +		/* Either stashed realfile or upperfile must match realinode */
> > +		if (WARN_ON_ONCE(upperfile))
> > +			return -EIO;
> > +
> > +		upperfile = ovl_open_realfile(file, realpath);
> > +		if (IS_ERR(upperfile))
> > +			return PTR_ERR(upperfile);
> > +
> > +		old = cmpxchg_release(backing_file_private_ptr(realfile), NULL,
> > +				      upperfile);
> > +		if (old) {
> > +			fput(upperfile);
> > +			upperfile = old;
> > +		}
> > +
> > +		/* Stashed upperfile that won the race must match realinode */
> > +		if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
> > +			return -EIO;
> > +
> > +		realfile = upperfile;
> > +	}
> > +
> > +checkflags:
> 
> Hmm...  That still feels awkward.  Question: can we reach that code with
> 	* non-NULL upperfile
> 	* false ovl_is_real_file(upperfile, realpath)
> 	* true ovl_is_real_file(realfile, realpath)
> Is that really possible?

read() from metacopied file after fsync(), with the data still in lower
layer?  Or am I misreading that?

Re: [PATCH v2 2/4] ovl: stash upper real file in backing_file struct

[Al Viro]

On Mon, Oct 07, 2024 at 04:03:13AM +0100, Al Viro wrote:

> > Hmm...  That still feels awkward.  Question: can we reach that code with
> > 	* non-NULL upperfile
> > 	* false ovl_is_real_file(upperfile, realpath)
> > 	* true ovl_is_real_file(realfile, realpath)
> > Is that really possible?
> 
> read() from metacopied file after fsync(), with the data still in lower
> layer?  Or am I misreading that?

Unless I'm misreading that thing, the logics would be

	If what we are asked for is where the data used to be at open time
		just use what we'd opened back then and be done with that.

	Either we'd been copied up since open, or it's a metadata fsync of
	a metacopied file; it doesn't matter which, since the upper layer
	file will be the same either way.

	If it hadn't been opened and stashed into the backing_file, do so.

	If we end up using the reference stashed by somebody else (either
	by finding it there in the first place, or by having cmpxchg tell
	us we'd lost the race), verify that it _is_ in the right place;
	it really should be, short of an equivalent of fs corruption
	(== somebody fucking around with the upper layer under us).

Is that what's going on there?  If so, I think your current version is
correct, but I'd probably put it in a different way:

        if (!ovl_is_real_file(realfile, realpath) {
		/*
		 * the place we want is not where the data used to be at
		 * open time; either we'd been copied up, or it's an fsync
		 * of metacopied file.  Should be the same location either
		 * way...
		 */
	        struct file *upperfile = backing_file_private(realfile);
		struct file *old;

		if (!upperfile) { /* nobody opened it yet */
			upperfile = ovl_open_realfile(file, realpath);
			if (IS_ERR(upperfile))
				return upperfile;
			old = cmpxchg_release(backing_file_private_ptr(realfile),
					      NULL, upperfile);
			if (old) { /* but they did while we were opening it */
				fput(upperfile);
				upperfile = old;
			}
		}
		/*
		 * stashed file must have been from the right place, unless
		 * someone's been corrupting the upper layer.
		 */
		if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
			return ERR_PTR(-EIO);
		realfile = upperfile;
	}

Re: [PATCH v2 2/4] ovl: stash upper real file in backing_file struct

[Amir Goldstein]

On Mon, Oct 7, 2024 at 5:42 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Oct 07, 2024 at 04:03:13AM +0100, Al Viro wrote:
>
> > > Hmm...  That still feels awkward.  Question: can we reach that code with
> > >     * non-NULL upperfile
> > >     * false ovl_is_real_file(upperfile, realpath)
> > >     * true ovl_is_real_file(realfile, realpath)
> > > Is that really possible?
> >
> > read() from metacopied file after fsync(), with the data still in lower
> > layer?  Or am I misreading that?
>
> Unless I'm misreading that thing, the logics would be
>
>         If what we are asked for is where the data used to be at open time
>                 just use what we'd opened back then and be done with that.
>
>         Either we'd been copied up since open, or it's a metadata fsync of
>         a metacopied file; it doesn't matter which, since the upper layer
>         file will be the same either way.
>
>         If it hadn't been opened and stashed into the backing_file, do so.
>
>         If we end up using the reference stashed by somebody else (either
>         by finding it there in the first place, or by having cmpxchg tell
>         us we'd lost the race), verify that it _is_ in the right place;
>         it really should be, short of an equivalent of fs corruption
>         (== somebody fucking around with the upper layer under us).
>
> Is that what's going on there?  If so, I think your current version is
> correct, but I'd probably put it in a different way:
>
>         if (!ovl_is_real_file(realfile, realpath) {
>                 /*
>                  * the place we want is not where the data used to be at
>                  * open time; either we'd been copied up, or it's an fsync
>                  * of metacopied file.  Should be the same location either
>                  * way...
>                  */
>                 struct file *upperfile = backing_file_private(realfile);
>                 struct file *old;
>
>                 if (!upperfile) { /* nobody opened it yet */
>                         upperfile = ovl_open_realfile(file, realpath);
>                         if (IS_ERR(upperfile))
>                                 return upperfile;
>                         old = cmpxchg_release(backing_file_private_ptr(realfile),
>                                               NULL, upperfile);
>                         if (old) { /* but they did while we were opening it */
>                                 fput(upperfile);
>                                 upperfile = old;
>                         }
>                 }
>                 /*
>                  * stashed file must have been from the right place, unless
>                  * someone's been corrupting the upper layer.
>                  */
>                 if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
>                         return ERR_PTR(-EIO);
>                 realfile = upperfile;
>         }

I like. I will use that.

Thanks,
Amir.

[PATCH v2 3/4] ovl: convert ovl_real_fdget_path() callers to ovl_real_file_path()

[Amir Goldstein]

Stop using struct fd to return a real file from ovl_real_fdget_path(),
because we no longer return a temporary file object and the callers
always get a borrowed file reference.

Rename the helper to ovl_real_file_path(), return a borrowed reference
of the real file that is referenced from the overlayfs file or an error.

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

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 42f9bbdd65b4..ead805e9f2d6 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -97,16 +97,14 @@ static bool ovl_is_real_file(const struct file *realfile,
 	return file_inode(realfile) == d_inode(realpath->dentry);
 }
 
-static int ovl_real_fdget_path(const struct file *file, struct fd *real,
-			       struct path *realpath)
+static struct file *ovl_real_file_path(const struct file *file,
+				       struct path *realpath)
 {
 	struct file *realfile = file->private_data;
 	struct file *upperfile = backing_file_private(realfile);
 
-	real->word = 0;
-
 	if (WARN_ON_ONCE(!realpath->dentry))
-		return -EIO;
+		return ERR_PTR(-EIO);
 
 	/*
 	 * Usually, if we operated on a stashed upperfile once, all following
@@ -129,11 +127,11 @@ static int ovl_real_fdget_path(const struct file *file, struct fd *real,
 
 		/* Either stashed realfile or upperfile must match realinode */
 		if (WARN_ON_ONCE(upperfile))
-			return -EIO;
+			return ERR_PTR(-EIO);
 
 		upperfile = ovl_open_realfile(file, realpath);
 		if (IS_ERR(upperfile))
-			return PTR_ERR(upperfile);
+			return upperfile;
 
 		old = cmpxchg_release(backing_file_private_ptr(realfile), NULL,
 				      upperfile);
@@ -144,21 +142,24 @@ static int ovl_real_fdget_path(const struct file *file, struct fd *real,
 
 		/* Stashed upperfile that won the race must match realinode */
 		if (WARN_ON_ONCE(!ovl_is_real_file(upperfile, realpath)))
-			return -EIO;
+			return ERR_PTR(-EIO);
 
 		realfile = upperfile;
 	}
 
 checkflags:
 	/* Did the flags change since open? */
-	if (unlikely((file->f_flags ^ realfile->f_flags) & ~OVL_OPEN_FLAGS))
-		return ovl_change_flags(realfile, file->f_flags);
+	if (unlikely((file->f_flags ^ realfile->f_flags) & ~OVL_OPEN_FLAGS)) {
+		int err = ovl_change_flags(realfile, file->f_flags);
 
-	real->word = (unsigned long)realfile;
-	return 0;
+		if (err)
+			return ERR_PTR(err);
+	}
+
+	return realfile;
 }
 
-static int ovl_real_fdget(const struct file *file, struct fd *real)
+static struct file *ovl_real_file(const struct file *file)
 {
 	struct dentry *dentry = file_dentry(file);
 	struct path realpath;
@@ -166,23 +167,33 @@ static int ovl_real_fdget(const struct file *file, struct fd *real)
 
 	if (d_is_dir(dentry)) {
 		struct file *f = ovl_dir_real_file(file, false);
-		if (IS_ERR(f))
-			return PTR_ERR(f);
-		real->word = (unsigned long)f;
-		return 0;
+
+		if (WARN_ON_ONCE(!f))
+			return ERR_PTR(-EIO);
+		return f;
 	}
 
 	/* lazy lookup and verify of lowerdata */
 	err = ovl_verify_lowerdata(dentry);
 	if (err)
-		return err;
+		return ERR_PTR(err);
 
 	ovl_path_realdata(dentry, &realpath);
 
-	return ovl_real_fdget_path(file, real, &realpath);
+	return ovl_real_file_path(file, &realpath);
+}
+
+static int ovl_real_fdget(const struct file *file, struct fd *real)
+{
+	struct file *f = ovl_real_file(file);
+
+	if (IS_ERR(f))
+		return PTR_ERR(f);
+	real->word = (unsigned long)f;
+	return 0;
 }
 
-static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
+static struct file *ovl_upper_file(const struct file *file, bool data)
 {
 	struct dentry *dentry = file_dentry(file);
 	struct path realpath;
@@ -193,12 +204,11 @@ static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
 	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 NULL;
 
-	return ovl_real_fdget_path(file, real, &realpath);
+	return ovl_real_file_path(file, &realpath);
 }
 
 static int ovl_open(struct inode *inode, struct file *file)
@@ -455,7 +465,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 
 static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 {
-	struct fd real;
+	struct file *realfile;
 	const struct cred *old_cred;
 	int ret;
 
@@ -463,19 +473,17 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	if (ret <= 0)
 		return ret;
 
-	ret = ovl_upper_fdget(file, &real, datasync);
-	if (ret || fd_empty(real))
-		return ret;
+	realfile = ovl_upper_file(file, datasync);
+	if (IS_ERR_OR_NULL(realfile))
+		return PTR_ERR(realfile);
 
 	/* Don't sync lower file for fear of receiving EROFS error */
-	if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
+	if (file_inode(realfile) == 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);
+		ret = vfs_fsync_range(realfile, start, end, datasync);
 		revert_creds(old_cred);
 	}
 
-	fdput(real);
-
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH v2 3/4] ovl: convert ovl_real_fdget_path() callers to ovl_real_file_path()

[Al Viro]

On Sun, Oct 06, 2024 at 10:23:58AM +0200, Amir Goldstein wrote:
> Stop using struct fd to return a real file from ovl_real_fdget_path(),
> because we no longer return a temporary file object and the callers
> always get a borrowed file reference.
> 
> Rename the helper to ovl_real_file_path(), return a borrowed reference
> of the real file that is referenced from the overlayfs file or an error.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/file.c | 70 +++++++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 42f9bbdd65b4..ead805e9f2d6 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c

> +static struct file *ovl_upper_file(const struct file *file, bool data)
>  {
>  	struct dentry *dentry = file_dentry(file);
>  	struct path realpath;
> @@ -193,12 +204,11 @@ static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
>  	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 NULL;
>  
> -	return ovl_real_fdget_path(file, real, &realpath);
> +	return ovl_real_file_path(file, &realpath);

AFAICS, we should never get NULL from ovl_real_file_path() now.

>  static int ovl_open(struct inode *inode, struct file *file)
> @@ -455,7 +465,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
>  
>  static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  {
> -	struct fd real;
> +	struct file *realfile;
>  	const struct cred *old_cred;
>  	int ret;
>  
> @@ -463,19 +473,17 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  	if (ret <= 0)
>  		return ret;
>  
> -	ret = ovl_upper_fdget(file, &real, datasync);
> -	if (ret || fd_empty(real))
> -		return ret;
> +	realfile = ovl_upper_file(file, datasync);
> +	if (IS_ERR_OR_NULL(realfile))
> +		return PTR_ERR(realfile);

... if so, the only source of NULL here would be the checks for OVL_TYPE_...
in ovl_upper_file().  Which has no other callers...

>  	/* Don't sync lower file for fear of receiving EROFS error */
> -	if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
> +	if (file_inode(realfile) == ovl_inode_upper(file_inode(file))) {

Can that _not_ be true after the same checks in ovl_upper_file()?

Re: [PATCH v2 3/4] ovl: convert ovl_real_fdget_path() callers to ovl_real_file_path()

[Amir Goldstein]

On Mon, Oct 7, 2024 at 5:12 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sun, Oct 06, 2024 at 10:23:58AM +0200, Amir Goldstein wrote:
> > Stop using struct fd to return a real file from ovl_real_fdget_path(),
> > because we no longer return a temporary file object and the callers
> > always get a borrowed file reference.
> >
> > Rename the helper to ovl_real_file_path(), return a borrowed reference
> > of the real file that is referenced from the overlayfs file or an error.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/file.c | 70 +++++++++++++++++++++++++--------------------
> >  1 file changed, 39 insertions(+), 31 deletions(-)
> >
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index 42f9bbdd65b4..ead805e9f2d6 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
>
> > +static struct file *ovl_upper_file(const struct file *file, bool data)
> >  {
> >       struct dentry *dentry = file_dentry(file);
> >       struct path realpath;
> > @@ -193,12 +204,11 @@ static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> >       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 NULL;
> >
> > -     return ovl_real_fdget_path(file, real, &realpath);
> > +     return ovl_real_file_path(file, &realpath);
>
> AFAICS, we should never get NULL from ovl_real_file_path() now.
>
> >  static int ovl_open(struct inode *inode, struct file *file)
> > @@ -455,7 +465,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
> >
> >  static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >  {
> > -     struct fd real;
> > +     struct file *realfile;
> >       const struct cred *old_cred;
> >       int ret;
> >
> > @@ -463,19 +473,17 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >       if (ret <= 0)
> >               return ret;
> >
> > -     ret = ovl_upper_fdget(file, &real, datasync);
> > -     if (ret || fd_empty(real))
> > -             return ret;
> > +     realfile = ovl_upper_file(file, datasync);
> > +     if (IS_ERR_OR_NULL(realfile))
> > +             return PTR_ERR(realfile);
>
> ... if so, the only source of NULL here would be the checks for OVL_TYPE_...
> in ovl_upper_file().  Which has no other callers...
>
> >       /* Don't sync lower file for fear of receiving EROFS error */
> > -     if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
> > +     if (file_inode(realfile) == ovl_inode_upper(file_inode(file))) {
>
> Can that _not_ be true after the same checks in ovl_upper_file()?

It should always be true. I will remove the check and rename the variable to
upperfile to clarify further.

Thanks for the thorough review!

Amir.

[PATCH v2 4/4] ovl: convert ovl_real_fdget() callers to ovl_real_file()

[Amir Goldstein]

Stop using struct fd to return a real file from ovl_real_fdget(),
because we no longer return a temporary file object and the callers
always get a borrowed file reference.

Rename the helper to ovl_real_file(), return a borrowed reference of
the real file that is referenced from the overlayfs file or an error.

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

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index ead805e9f2d6..5e3f688ae908 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -183,16 +183,6 @@ static struct file *ovl_real_file(const struct file *file)
 	return ovl_real_file_path(file, &realpath);
 }
 
-static int ovl_real_fdget(const struct file *file, struct fd *real)
-{
-	struct file *f = ovl_real_file(file);
-
-	if (IS_ERR(f))
-		return PTR_ERR(f);
-	real->word = (unsigned long)f;
-	return 0;
-}
-
 static struct file *ovl_upper_file(const struct file *file, bool data)
 {
 	struct dentry *dentry = file_dentry(file);
@@ -262,7 +252,7 @@ static int ovl_release(struct inode *inode, struct file *file)
 static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 {
 	struct inode *inode = file_inode(file);
-	struct fd real;
+	struct file *realfile;
 	const struct cred *old_cred;
 	loff_t ret;
 
@@ -278,9 +268,9 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 			return vfs_setpos(file, 0, 0);
 	}
 
-	ret = ovl_real_fdget(file, &real);
-	if (ret)
-		return ret;
+	realfile = ovl_real_file(file);
+	if (IS_ERR(realfile))
+		return PTR_ERR(realfile);
 
 	/*
 	 * Overlay file f_pos is the master copy that is preserved
@@ -290,17 +280,15 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 	 * files, so we use the real file to perform seeks.
 	 */
 	ovl_inode_lock(inode);
-	fd_file(real)->f_pos = file->f_pos;
+	realfile->f_pos = file->f_pos;
 
 	old_cred = ovl_override_creds(inode->i_sb);
-	ret = vfs_llseek(fd_file(real), offset, whence);
+	ret = vfs_llseek(realfile, offset, whence);
 	revert_creds(old_cred);
 
-	file->f_pos = fd_file(real)->f_pos;
+	file->f_pos = realfile->f_pos;
 	ovl_inode_unlock(inode);
 
-	fdput(real);
-
 	return ret;
 }
 
@@ -341,8 +329,7 @@ static void ovl_file_accessed(struct file *file)
 static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
-	struct fd real;
-	ssize_t ret;
+	struct file *realfile;
 	struct backing_file_ctx ctx = {
 		.cred = ovl_creds(file_inode(file)->i_sb),
 		.user_file = file,
@@ -352,22 +339,19 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	if (!iov_iter_count(iter))
 		return 0;
 
-	ret = ovl_real_fdget(file, &real);
-	if (ret)
-		return ret;
-
-	ret = backing_file_read_iter(fd_file(real), iter, iocb, iocb->ki_flags,
-				     &ctx);
-	fdput(real);
+	realfile = ovl_real_file(file);
+	if (IS_ERR(realfile))
+		return PTR_ERR(realfile);
 
-	return ret;
+	return backing_file_read_iter(realfile, iter, iocb, iocb->ki_flags,
+				      &ctx);
 }
 
 static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(file);
-	struct fd real;
+	struct file *realfile;
 	ssize_t ret;
 	int ifl = iocb->ki_flags;
 	struct backing_file_ctx ctx = {
@@ -383,8 +367,9 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	/* Update mode */
 	ovl_copyattr(inode);
 
-	ret = ovl_real_fdget(file, &real);
-	if (ret)
+	realfile = ovl_real_file(file);
+	ret = PTR_ERR(realfile);
+	if (IS_ERR(realfile))
 		goto out_unlock;
 
 	if (!ovl_should_sync(OVL_FS(inode->i_sb)))
@@ -395,8 +380,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	 * this property in case it is set by the issuer.
 	 */
 	ifl &= ~IOCB_DIO_CALLER_COMP;
-	ret = backing_file_write_iter(fd_file(real), iter, iocb, ifl, &ctx);
-	fdput(real);
+	ret = backing_file_write_iter(realfile, iter, iocb, ifl, &ctx);
 
 out_unlock:
 	inode_unlock(inode);
@@ -408,28 +392,24 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
 			       struct pipe_inode_info *pipe, size_t len,
 			       unsigned int flags)
 {
-	struct fd real;
-	ssize_t ret;
+	struct file *realfile;
 	struct backing_file_ctx ctx = {
 		.cred = ovl_creds(file_inode(in)->i_sb),
 		.user_file = in,
 		.accessed = ovl_file_accessed,
 	};
 
-	ret = ovl_real_fdget(in, &real);
-	if (ret)
-		return ret;
-
-	ret = backing_file_splice_read(fd_file(real), ppos, pipe, len, flags, &ctx);
-	fdput(real);
+	realfile = ovl_real_file(in);
+	if (IS_ERR(realfile))
+		return PTR_ERR(realfile);
 
-	return ret;
+	return backing_file_splice_read(realfile, ppos, pipe, len, flags, &ctx);
 }
 
 /*
  * Calling iter_file_splice_write() directly from overlay's f_op may deadlock
  * due to lock order inversion between pipe->mutex in iter_file_splice_write()
- * and file_start_write(fd_file(real)) in ovl_write_iter().
+ * and file_start_write(realfile) in ovl_write_iter().
  *
  * So do everything ovl_write_iter() does and call iter_file_splice_write() on
  * the real file.
@@ -437,7 +417,7 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
 static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 				loff_t *ppos, size_t len, unsigned int flags)
 {
-	struct fd real;
+	struct file *realfile;
 	struct inode *inode = file_inode(out);
 	ssize_t ret;
 	struct backing_file_ctx ctx = {
@@ -450,12 +430,12 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 	/* Update mode */
 	ovl_copyattr(inode);
 
-	ret = ovl_real_fdget(out, &real);
-	if (ret)
+	realfile = ovl_real_file(out);
+	ret = PTR_ERR(realfile);
+	if (IS_ERR(realfile))
 		goto out_unlock;
 
-	ret = backing_file_splice_write(pipe, fd_file(real), ppos, len, flags, &ctx);
-	fdput(real);
+	ret = backing_file_splice_write(pipe, realfile, ppos, len, flags, &ctx);
 
 out_unlock:
 	inode_unlock(inode);
@@ -502,7 +482,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
 static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
 	struct inode *inode = file_inode(file);
-	struct fd real;
+	struct file *realfile;
 	const struct cred *old_cred;
 	int ret;
 
@@ -513,19 +493,18 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 	if (ret)
 		goto out_unlock;
 
-	ret = ovl_real_fdget(file, &real);
-	if (ret)
+	realfile = ovl_real_file(file);
+	ret = PTR_ERR(realfile);
+	if (IS_ERR(realfile))
 		goto out_unlock;
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
-	ret = vfs_fallocate(fd_file(real), mode, offset, len);
+	ret = vfs_fallocate(realfile, mode, offset, len);
 	revert_creds(old_cred);
 
 	/* Update size */
 	ovl_file_modified(file);
 
-	fdput(real);
-
 out_unlock:
 	inode_unlock(inode);
 
@@ -534,20 +513,18 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 
 static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 {
-	struct fd real;
+	struct file *realfile;
 	const struct cred *old_cred;
 	int ret;
 
-	ret = ovl_real_fdget(file, &real);
-	if (ret)
-		return ret;
+	realfile = ovl_real_file(file);
+	if (IS_ERR(realfile))
+		return PTR_ERR(realfile);
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
-	ret = vfs_fadvise(fd_file(real), offset, len, advice);
+	ret = vfs_fadvise(realfile, offset, len, advice);
 	revert_creds(old_cred);
 
-	fdput(real);
-
 	return ret;
 }
 
@@ -562,7 +539,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 			    loff_t len, unsigned int flags, enum ovl_copyop op)
 {
 	struct inode *inode_out = file_inode(file_out);
-	struct fd real_in, real_out;
+	struct file *realfile_in, *realfile_out;
 	const struct cred *old_cred;
 	loff_t ret;
 
@@ -575,31 +552,31 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 			goto out_unlock;
 	}
 
-	ret = ovl_real_fdget(file_out, &real_out);
-	if (ret)
+	realfile_out = ovl_real_file(file_out);
+	ret = PTR_ERR(realfile_out);
+	if (IS_ERR(realfile_out))
 		goto out_unlock;
 
-	ret = ovl_real_fdget(file_in, &real_in);
-	if (ret) {
-		fdput(real_out);
+	realfile_in = ovl_real_file(file_in);
+	ret = PTR_ERR(realfile_in);
+	if (IS_ERR(realfile_in))
 		goto out_unlock;
-	}
 
 	old_cred = ovl_override_creds(file_inode(file_out)->i_sb);
 	switch (op) {
 	case OVL_COPY:
-		ret = vfs_copy_file_range(fd_file(real_in), pos_in,
-					  fd_file(real_out), pos_out, len, flags);
+		ret = vfs_copy_file_range(realfile_in, pos_in,
+					  realfile_out, pos_out, len, flags);
 		break;
 
 	case OVL_CLONE:
-		ret = vfs_clone_file_range(fd_file(real_in), pos_in,
-					   fd_file(real_out), pos_out, len, flags);
+		ret = vfs_clone_file_range(realfile_in, pos_in,
+					   realfile_out, pos_out, len, flags);
 		break;
 
 	case OVL_DEDUPE:
-		ret = vfs_dedupe_file_range_one(fd_file(real_in), pos_in,
-						fd_file(real_out), pos_out, len,
+		ret = vfs_dedupe_file_range_one(realfile_in, pos_in,
+						realfile_out, pos_out, len,
 						flags);
 		break;
 	}
@@ -608,9 +585,6 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 	/* Update size */
 	ovl_file_modified(file_out);
 
-	fdput(real_in);
-	fdput(real_out);
-
 out_unlock:
 	inode_unlock(inode_out);
 
@@ -654,20 +628,19 @@ static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
 
 static int ovl_flush(struct file *file, fl_owner_t id)
 {
-	struct fd real;
+	struct file *realfile;
 	const struct cred *old_cred;
-	int err;
+	int err = 0;
 
-	err = ovl_real_fdget(file, &real);
-	if (err)
-		return err;
+	realfile = ovl_real_file(file);
+	if (IS_ERR(realfile))
+		return PTR_ERR(realfile);
 
-	if (fd_file(real)->f_op->flush) {
+	if (realfile->f_op->flush) {
 		old_cred = ovl_override_creds(file_inode(file)->i_sb);
-		err = fd_file(real)->f_op->flush(fd_file(real), id);
+		err = realfile->f_op->flush(realfile, id);
 		revert_creds(old_cred);
 	}
-	fdput(real);
 
 	return err;
 }
-- 
2.34.1

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Miklos Szeredi]

On Sun, 6 Oct 2024 at 10:24, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Hi all,
>
> This is v2 of the code to avoid temporary backing file opens in
> overlayfs, taking into account Al's comments on v1 [1].
>
> Miklos,
>
> The implementation of ovl_real_file_path() helper is roughly based on
> ovl_real_dir_file().
>
> do you see any problems with this approach or any races not handled?
> Note that I did have a logical bug in v1 (always choosing the stashed
> upperfile if it exists), so there may be more.

Stashing the upper file pointer in the lower file's backing struct
feels like a layering violation.

Wouldn't it be cleaner to just do what directory files do and link
both upper and lower backing files from the ovl file?

Thanks,
Miklos

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Amir Goldstein]

On Mon, Oct 7, 2024 at 11:35 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, 6 Oct 2024 at 10:24, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Hi all,
> >
> > This is v2 of the code to avoid temporary backing file opens in
> > overlayfs, taking into account Al's comments on v1 [1].
> >
> > Miklos,
> >
> > The implementation of ovl_real_file_path() helper is roughly based on
> > ovl_real_dir_file().
> >
> > do you see any problems with this approach or any races not handled?
> > Note that I did have a logical bug in v1 (always choosing the stashed
> > upperfile if it exists), so there may be more.
>
> Stashing the upper file pointer in the lower file's backing struct
> feels like a layering violation.
>
> Wouldn't it be cleaner to just do what directory files do and link
> both upper and lower backing files from the ovl file?

Maybe it is more straightforward, I can go with that, but it
feels like a waste not to use the space in backing_file,
so let me first try to convince you otherwise.

IMO, this is not a layer violation at all.
The way I perceive struct backing_file is as an inheritance from struct file,
similar to the way that ovl_inode is an inheritance from vfs_inode.

The difference being that backing_file is generic (to be used by ovl/fuse)
and does not have per-fs constructor/destructor/user_path() methods,
because that would have been over design.

You can say that backing_file_user_path() is the layer violation, having
the vfs peek into the ovl layer above it, but backing_file_private_ptr()
is the opposite - it is used only by the layer that allocated backing_file,
so it is just like saying that a struct file has a single private_data, while
the inherited generic backing_file can store two private_data pointers.

What's wrong with that?

Thanks,
Amir.

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Miklos Szeredi]

On Mon, 7 Oct 2024 at 12:22, Amir Goldstein <amir73il@gmail.com> wrote:

> Maybe it is more straightforward, I can go with that, but it
> feels like a waste not to use the space in backing_file,
> so let me first try to convince you otherwise.

Is it not a much bigger waste to allocate backing_file with kmalloc()
instead of kmem_cache_alloc()?

> IMO, this is not a layer violation at all.
> The way I perceive struct backing_file is as an inheritance from struct file,
> similar to the way that ovl_inode is an inheritance from vfs_inode.

That sounds about right.

> You can say that backing_file_user_path() is the layer violation, having
> the vfs peek into the ovl layer above it, but backing_file_private_ptr()
> is the opposite - it is used only by the layer that allocated backing_file,
> so it is just like saying that a struct file has a single private_data, while
> the inherited generic backing_file can store two private_data pointers.
>
> What's wrong with that?

It feels wrong to me, because lowerfile's backing_file is just a
convenient place to stash a completely unrelated pointer into.

Furthermore private_data pointers lack type safety with all the
problems that entails.

Thanks,
Miklos

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Amir Goldstein]

On Mon, Oct 7, 2024 at 12:38 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 7 Oct 2024 at 12:22, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > Maybe it is more straightforward, I can go with that, but it
> > feels like a waste not to use the space in backing_file,
> > so let me first try to convince you otherwise.
>
> Is it not a much bigger waste to allocate backing_file with kmalloc()
> instead of kmem_cache_alloc()?

Yes, much bigger...

Christian is still moving things around wrt lifetime of file and
backing_file, so I do not want to intervene in the file_table.c space.

>
> > IMO, this is not a layer violation at all.
> > The way I perceive struct backing_file is as an inheritance from struct file,
> > similar to the way that ovl_inode is an inheritance from vfs_inode.
>
> That sounds about right.
>
> > You can say that backing_file_user_path() is the layer violation, having
> > the vfs peek into the ovl layer above it, but backing_file_private_ptr()
> > is the opposite - it is used only by the layer that allocated backing_file,
> > so it is just like saying that a struct file has a single private_data, while
> > the inherited generic backing_file can store two private_data pointers.
> >
> > What's wrong with that?
>
> It feels wrong to me, because lowerfile's backing_file is just a
> convenient place to stash a completely unrelated pointer into.

Funny, that's like saying that a ->next member in a struct is
completely unrelated.

What I see after my patch is that ->private_data points to a singly
linked list of length 1 to 2 of backing files.

>
> Furthermore private_data pointers lack type safety with all the
> problems that entails.

Well, this is not any worth that current ->private_data, but I could
also make it, if you like it better:

 struct backing_file {
        struct file file;
        struct path user_path;
+       struct file *next;
 };

+struct file **backing_file_private_ptr(struct file *f)
+{
+       return &backing_file(f)->next;
+}
+EXPORT_SYMBOL_GPL(backing_file_next_ptr);

Again, I am not terribly opposed to allocating struct ovl_file as we do
with directory - it is certainly more straight forward to read, so that
is a good enough argument in itself, and "personal dislike" is also a fair
argument, just arguing for the sake of argument so you understand my POV.

Let me know your decision.

Thanks,
Amir.

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Miklos Szeredi]

On Mon, 7 Oct 2024 at 13:02, Amir Goldstein <amir73il@gmail.com> wrote:

> What I see after my patch is that ->private_data points to a singly
> linked list of length 1 to 2 of backing files.

Well, yeah.

Still, it's adding (arguably minimal) data and code to backing_file,
that is overlay specific.   If you show how this is relevant to fuse's
use of backing files, then that's a much stronger argument in favor.

> Well, this is not any worth that current ->private_data, but I could
> also make it, if you like it better:
>
>  struct backing_file {
>         struct file file;
>         struct path user_path;
> +       struct file *next;
>  };
>
> +struct file **backing_file_private_ptr(struct file *f)
> +{
> +       return &backing_file(f)->next;
> +}
> +EXPORT_SYMBOL_GPL(backing_file_next_ptr);

Yeah, that would solve type safety, but would make the infrastructure
less generic.

> Again, I am not terribly opposed to allocating struct ovl_file as we do
> with directory - it is certainly more straight forward to read, so that
> is a good enough argument in itself, and "personal dislike" is also a fair
> argument, just arguing for the sake of argument so you understand my POV.

I think readability is more important here than savings on memory or CPU.

Thanks,
Miklos

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Amir Goldstein]

On Mon, Oct 7, 2024 at 1:16 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 7 Oct 2024 at 13:02, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > What I see after my patch is that ->private_data points to a singly
> > linked list of length 1 to 2 of backing files.
>
> Well, yeah.
>
> Still, it's adding (arguably minimal) data and code to backing_file,
> that is overlay specific.   If you show how this is relevant to fuse's
> use of backing files, then that's a much stronger argument in favor.
>
> > Well, this is not any worth that current ->private_data, but I could
> > also make it, if you like it better:
> >
> >  struct backing_file {
> >         struct file file;
> >         struct path user_path;
> > +       struct file *next;
> >  };
> >
> > +struct file **backing_file_private_ptr(struct file *f)
> > +{
> > +       return &backing_file(f)->next;
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_next_ptr);
>
> Yeah, that would solve type safety, but would make the infrastructure
> less generic.
>
> > Again, I am not terribly opposed to allocating struct ovl_file as we do
> > with directory - it is certainly more straight forward to read, so that
> > is a good enough argument in itself, and "personal dislike" is also a fair
> > argument, just arguing for the sake of argument so you understand my POV.
>
> I think readability is more important here than savings on memory or CPU.
>

Will do.

Thanks,
Amir.

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Christian Brauner]

On Mon, Oct 07, 2024 at 01:01:56PM GMT, Amir Goldstein wrote:
> On Mon, Oct 7, 2024 at 12:38 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 7 Oct 2024 at 12:22, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > Maybe it is more straightforward, I can go with that, but it
> > > feels like a waste not to use the space in backing_file,
> > > so let me first try to convince you otherwise.
> >
> > Is it not a much bigger waste to allocate backing_file with kmalloc()
> > instead of kmem_cache_alloc()?
> 
> Yes, much bigger...
> 
> Christian is still moving things around wrt lifetime of file and
> backing_file, so I do not want to intervene in the file_table.c space.

My plan was to just snatch your series on top of things once it's ready.
Sorry, I didn't get around to take a look. It seems even just closing
your eyes for a weekend to a computer screen is like opening flood
gates...

Re: [PATCH v2 0/4] Stash overlay real upper file in backing_file

[Amir Goldstein]

On Mon, Oct 7, 2024 at 4:12 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Oct 07, 2024 at 01:01:56PM GMT, Amir Goldstein wrote:
> > On Mon, Oct 7, 2024 at 12:38 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Mon, 7 Oct 2024 at 12:22, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > Maybe it is more straightforward, I can go with that, but it
> > > > feels like a waste not to use the space in backing_file,
> > > > so let me first try to convince you otherwise.
> > >
> > > Is it not a much bigger waste to allocate backing_file with kmalloc()
> > > instead of kmem_cache_alloc()?
> >
> > Yes, much bigger...
> >
> > Christian is still moving things around wrt lifetime of file and
> > backing_file, so I do not want to intervene in the file_table.c space.
>
> My plan was to just snatch your series on top of things once it's ready.
> Sorry, I didn't get around to take a look. It seems even just closing
> your eyes for a weekend to a computer screen is like opening flood
> gates...

Well I just posted v3 and it leaves backing_file alone
which I also now agree with Miklos is looking much nicer.

So whatever changes in lifetime of backing_file that you wanted to make
feel free to make them.

Thanks,
Amir.