[PATCH v3 0/3] Reduce impact of overlayfs backing files fake path

[PATCH v3 0/3] Reduce impact of overlayfs backing files fake path

[Amir Goldstein]

Hi Christian,

Following v3 addresses Al's review comments on v2.

---
This is another step in the multi step plan to reduce the vulnerability
of filesystems code to overlayfs backing files whose f_path and f_inode
are not on the same sb.

History:
--------
When overlayfs was first merged, overlayfs files of regular files and
directories, the ones that are installed in file table, had a "fake" path,
namely, f_path is the overlayfs path and f_inode is the "real" inode on
the underlying filesystem.

At that time, all filesystem syscalls, generic vfs code and any filesystem
code (among the fs that are supported as overlayfs underlying layers) had
to be aware of the possibility of a file with "fake" path and had to use
the helper file_dentry() to get the "real" dentry and the helper
locks_inode() to get to the overlayfs inode.
At that time, there was no way to get the "real" path.

This situation was fragile because many filesystem developers were not
aware of having to deal with file with "fake" path.
Admittedly, it wasn't very well documented either.

Backing files:
--------------
The first big step to reduce the impact of files with fake path was in
v4.19 that introduced d1d04ef8572b ("ovl: stack file ops").  Since
v4.19, f_inode of the overlayfs files that are installed in file table
are overlayfs inodes, so those files are no longer odd.

As a result of this change, a lot of syscalls code, which take fd as an
argument, is no longer exposed to files with "fake" path and the helper
file_locks() was no longer needed.

However, since v4.19, overlayfs uses internal backing files with "fake"
path, so generic vfs code that overlayfs calls with the backing files and
filesystem code still needs to be aware of files with "fake" path.

The one place where the internal backing files are "exposed" to users
is when users mmap overlayfs files, the backing file (and not the
overlayfs file) is stored in vm_file. The reason that overlayfs backing
files use a "fake" path is so that /proc/<pid>/maps will disaply the
overlayfs file path to users.

Recently:
---------
In v6.5, we took another small step by introducing of the backing_file
container and the file_real_path() helper.  This change allowed vfs and
filesystem code to get the "real" path of an overlayfs backing file.
With this change, we were able to make fsnotify work correctly and
report events on the "real" filesystem objects that were accessed via
overlayfs.

This method works fine, but it still leaves the vfs vulnerable to new
code that is not aware of files with fake path.  A recent example is
commit db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version").
This commit uses direct referencing to f_path in IMA code that otherwise
uses file_inode() and file_dentry() to reference the filesystem objects
that it is measuring.

Coming up:
----------
This patch set actually implements my initial proposal [1] that was
proposed for v6.5 - instead of having filesystem code opt-in to get the
"real" path, have generic code opt-in for the "fake" path in the few
places that it is needed.

It is possible that I missed a few places that need opt-in for "fake"
path, but in most likelihood, a missed opt-in to "fake" path would just
result in printing the "real" path to user and if %pD is used for
printing, that will probably make no difference anyway.

Is it far more likely that new filesystems code that does not use the
file_dentry() and file_real_path() helpers will end up causing crashes
or averting LSM/audit rules if we keep the "fake" path exposed by default.

Future:
-------
This change already makes file_dentry() moot, but for now we did not
change this helper just added a WARN_ON() in ovl_d_real() to catch if we
have made any wrong assumptions.

After the dust settles on this change, we can make file_dentry() a plain
accessor and we can drop the inode argument to ->d_real().

Testing:
--------
As usual, I ran fstests with overlayfs over xfs and all the LTS tests
that test fsnotify + overlayfs.

I have limited testing ability for audit/IMA/LSM modules, so we will
have to rely on other people and bots testing of linux-next.
Syzbot has been known to be very obsessive about reporting bugs of
overlayfs + IMA.

Merging:
--------
The branch that I tested [2] is based on both the stable vfs.mount.write
branch and vfs.misc of the moment.

If you agree to stage these patches in vfs tree, they would need
to be based on both vfs.mount.write and the file_free_rcu patches.
So perhaps split the file_free_rcu patches out of vfs.misc and into a
vfs.file topic branch that is based on the stable vfs.mount.write branch
and add my patches on top?

I don't need vfs.file to be stable, I understand that the file_free_rcu()
patches may still change, but since I would like to test overlayfs-next
together with the "fake" path patches, it would be nice if the vfs.file
branch would be more stable than vfs.misc usually is.

Thanks,
Amir.

Changes since v2:
- No need to check for NULL user_path->mnt (viro)
- Simplify the ovl_d_real() assertion of non-NULL inode (viro)
- Remove f_path() accessor (viro)

Changes since v1:
- backing_file (fake_file rebranded) container already merged
- fsnotify support for backing files already merged
- Take mnt_writers refcount on the "real" path
- Rename file_fake_path() => file_user_path()
- No need to use file_user_path() for exe_file path access
- No need to use file_user_path() in audit/LSM code
- Added file_user_path() in some tracing/debugging code

[1] https://lore.kernel.org/r/20230609073239.957184-1-amir73il@gmail.com/
[2] https://github.com/amir73il/linux/commits/ovl_fake_path

Amir Goldstein (3):
  fs: get mnt_writers count for an open backing file's real path
  fs: create helper file_user_path() for user displayed mapped file path
  fs: store real path instead of fake path in backing file f_path

 arch/arc/kernel/troubleshoot.c |  6 ++--
 fs/file_table.c                | 12 ++++----
 fs/internal.h                  | 11 +++++--
 fs/open.c                      | 52 +++++++++++++++++++++++-----------
 fs/overlayfs/super.c           | 16 ++++++++---
 fs/proc/base.c                 |  2 +-
 fs/proc/nommu.c                |  2 +-
 fs/proc/task_mmu.c             |  4 +--
 fs/proc/task_nommu.c           |  2 +-
 include/linux/fs.h             | 22 +++++++-------
 include/linux/fsnotify.h       |  3 +-
 kernel/trace/trace_output.c    |  2 +-
 12 files changed, 84 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] fs: get mnt_writers count for an open backing file's real path

[Amir Goldstein]

A writeable mapped backing file can perform writes to the real inode.
Therefore, the real path mount must be kept writable so long as the
writable map exists.

This may not be strictly needed for ovelrayfs private upper mount,
but it is correct to take the mnt_writers count in the vfs helper.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/internal.h | 11 +++++++++--
 fs/open.c     | 31 +++++++++++++++++++++++++------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/fs/internal.h b/fs/internal.h
index f08d8fe3ae5e..4e93a685bdaa 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -96,13 +96,20 @@ 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 release_empty_file(struct file *f);
 
+static inline void file_put_write_access(struct file *file)
+{
+	put_write_access(file->f_inode);
+	mnt_put_write_access(file->f_path.mnt);
+	if (unlikely(file->f_mode & FMODE_BACKING))
+		mnt_put_write_access(backing_file_real_path(file)->mnt);
+}
+
 static inline void put_file_access(struct file *file)
 {
 	if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
 		i_readcount_dec(file->f_inode);
 	} else if (file->f_mode & FMODE_WRITER) {
-		put_write_access(file->f_inode);
-		mnt_put_write_access(file->f_path.mnt);
+		file_put_write_access(file);
 	}
 }
 
diff --git a/fs/open.c b/fs/open.c
index a65ce47810cf..fe63e236da22 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -870,6 +870,30 @@ SYSCALL_DEFINE3(fchown, unsigned int, fd, uid_t, user, gid_t, group)
 	return ksys_fchown(fd, user, group);
 }
 
+static inline int file_get_write_access(struct file *f)
+{
+	int error;
+
+	error = get_write_access(f->f_inode);
+	if (unlikely(error))
+		return error;
+	error = mnt_get_write_access(f->f_path.mnt);
+	if (unlikely(error))
+		goto cleanup_inode;
+	if (unlikely(f->f_mode & FMODE_BACKING)) {
+		error = mnt_get_write_access(backing_file_real_path(f)->mnt);
+		if (unlikely(error))
+			goto cleanup_mnt;
+	}
+	return 0;
+
+cleanup_mnt:
+	mnt_put_write_access(f->f_path.mnt);
+cleanup_inode:
+	put_write_access(f->f_inode);
+	return error;
+}
+
 static int do_dentry_open(struct file *f,
 			  struct inode *inode,
 			  int (*open)(struct inode *, struct file *))
@@ -892,14 +916,9 @@ static int do_dentry_open(struct file *f,
 	if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
 		i_readcount_inc(inode);
 	} else if (f->f_mode & FMODE_WRITE && !special_file(inode->i_mode)) {
-		error = get_write_access(inode);
+		error = file_get_write_access(f);
 		if (unlikely(error))
 			goto cleanup_file;
-		error = mnt_get_write_access(f->f_path.mnt);
-		if (unlikely(error)) {
-			put_write_access(inode);
-			goto cleanup_file;
-		}
 		f->f_mode |= FMODE_WRITER;
 	}
 
-- 
2.34.1

[PATCH v3 2/3] fs: create helper file_user_path() for user displayed mapped file path

[Amir Goldstein]

Overlayfs uses backing files with "fake" overlayfs f_path and "real"
underlying f_inode, in order to use underlying inode aops for mapped
files and to display the overlayfs path in /proc/<pid>/maps.

In preparation for storing the overlayfs "fake" path instead of the
underlying "real" path in struct backing_file, define a noop helper
file_user_path() that returns f_path for now.

Use the new helper in procfs and kernel logs whenever a path of a
mapped file is displayed to users.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 arch/arc/kernel/troubleshoot.c |  6 ++++--
 fs/proc/base.c                 |  2 +-
 fs/proc/nommu.c                |  2 +-
 fs/proc/task_mmu.c             |  4 ++--
 fs/proc/task_nommu.c           |  2 +-
 include/linux/fs.h             | 14 ++++++++++++++
 kernel/trace/trace_output.c    |  2 +-
 7 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/arch/arc/kernel/troubleshoot.c b/arch/arc/kernel/troubleshoot.c
index d5b3ed2c58f5..c380d8c30704 100644
--- a/arch/arc/kernel/troubleshoot.c
+++ b/arch/arc/kernel/troubleshoot.c
@@ -90,10 +90,12 @@ static void show_faulting_vma(unsigned long address)
 	 */
 	if (vma) {
 		char buf[ARC_PATH_MAX];
-		char *nm = "?";
+		char *nm = "anon";
 
 		if (vma->vm_file) {
-			nm = file_path(vma->vm_file, buf, ARC_PATH_MAX-1);
+			/* XXX: can we use %pD below and get rid of buf? */
+			nm = d_path(file_user_path(vma->vm_file), buf,
+				    ARC_PATH_MAX-1);
 			if (IS_ERR(nm))
 				nm = "?";
 		}
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ffd54617c354..20695c928ee6 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2218,7 +2218,7 @@ static int map_files_get_link(struct dentry *dentry, struct path *path)
 	rc = -ENOENT;
 	vma = find_exact_vma(mm, vm_start, vm_end);
 	if (vma && vma->vm_file) {
-		*path = vma->vm_file->f_path;
+		*path = *file_user_path(vma->vm_file);
 		path_get(path);
 		rc = 0;
 	}
diff --git a/fs/proc/nommu.c b/fs/proc/nommu.c
index 4d3493579458..c6e7ebc63756 100644
--- a/fs/proc/nommu.c
+++ b/fs/proc/nommu.c
@@ -58,7 +58,7 @@ static int nommu_region_show(struct seq_file *m, struct vm_region *region)
 
 	if (file) {
 		seq_pad(m, ' ');
-		seq_file_path(m, file, "");
+		seq_path(m, file_user_path(file), "");
 	}
 
 	seq_putc(m, '\n');
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3dd5be96691b..1593940ca01e 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -296,7 +296,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma)
 		if (anon_name)
 			seq_printf(m, "[anon_shmem:%s]", anon_name->name);
 		else
-			seq_file_path(m, file, "\n");
+			seq_path(m, file_user_path(file), "\n");
 		goto done;
 	}
 
@@ -1967,7 +1967,7 @@ static int show_numa_map(struct seq_file *m, void *v)
 
 	if (file) {
 		seq_puts(m, " file=");
-		seq_file_path(m, file, "\n\t= ");
+		seq_path(m, file_user_path(file), "\n\t= ");
 	} else if (vma_is_initial_heap(vma)) {
 		seq_puts(m, " heap");
 	} else if (vma_is_initial_stack(vma)) {
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 7cebd397cc26..bce674533000 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -157,7 +157,7 @@ static int nommu_vma_show(struct seq_file *m, struct vm_area_struct *vma)
 
 	if (file) {
 		seq_pad(m, ' ');
-		seq_file_path(m, file, "");
+		seq_path(m, file_user_path(file), "");
 	} else if (mm && vma_is_initial_stack(vma)) {
 		seq_pad(m, ' ');
 		seq_puts(m, "[stack]");
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7f7e7d2efbeb..a8e4e1cac48e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2472,6 +2472,20 @@ static inline const struct path *file_real_path(struct file *f)
 	return &f->f_path;
 }
 
+/*
+ * file_user_path - get the path to display for memory mapped file
+ *
+ * When mmapping a file on a stackable filesystem (e.g., overlayfs), the file
+ * stored in ->vm_file is a backing file whose f_inode is on the underlying
+ * filesystem.  When the mapped file path is displayed to user (e.g. via
+ * /proc/<pid>/maps), this helper should be used to get the path to display
+ * to the user, which is the path of the fd that user has requested to map.
+ */
+static inline const struct path *file_user_path(struct file *f)
+{
+	return &f->f_path;
+}
+
 static inline struct file *file_clone_open(struct file *file)
 {
 	return dentry_open(&file->f_path, file->f_flags, file->f_cred);
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index db575094c498..d8b302d01083 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -404,7 +404,7 @@ static int seq_print_user_ip(struct trace_seq *s, struct mm_struct *mm,
 			vmstart = vma->vm_start;
 		}
 		if (file) {
-			ret = trace_seq_path(s, &file->f_path);
+			ret = trace_seq_path(s, file_user_path(file));
 			if (ret)
 				trace_seq_printf(s, "[+0x%lx]",
 						 ip - vmstart);
-- 
2.34.1

[PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Amir Goldstein]

A backing file struct stores two path's, one "real" path that is referring
to f_inode and one "fake" path, which should be displayed to users in
/proc/<pid>/maps.

There is a lot more potential code that needs to know the "real" path, then
code that needs to know the "fake" path.

Instead of code having to request the "real" path with file_real_path(),
store the "real" path in f_path and require code that needs to know the
"fake" path request it with file_user_path().
Replace the file_real_path() helper with a simple const accessor f_path().

After this change, file_dentry() is not expected to observe any files
with overlayfs f_path and real f_inode, so the call to ->d_real() should
not be needed.  Leave the ->d_real() call for now and add an assertion
in ovl_d_real() to catch if we made wrong assumptions.

Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
Link: https://lore.kernel.org/r/CAJfpegtt48eXhhjDFA1ojcHPNKj3Go6joryCPtEFAKpocyBsnw@mail.gmail.com/
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/file_table.c          | 12 ++++++------
 fs/internal.h            |  2 +-
 fs/open.c                | 23 +++++++++++------------
 fs/overlayfs/super.c     | 16 ++++++++++++----
 include/linux/fs.h       | 22 ++++------------------
 include/linux/fsnotify.h |  3 +--
 6 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 08fd1dd6d863..fa92743ba6a9 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -44,10 +44,10 @@ static struct kmem_cache *filp_cachep __read_mostly;
 
 static struct percpu_counter nr_files __cacheline_aligned_in_smp;
 
-/* Container for backing file with optional real path */
+/* Container for backing file with optional user path */
 struct backing_file {
 	struct file file;
-	struct path real_path;
+	struct path user_path;
 };
 
 static inline struct backing_file *backing_file(struct file *f)
@@ -55,11 +55,11 @@ static inline struct backing_file *backing_file(struct file *f)
 	return container_of(f, struct backing_file, file);
 }
 
-struct path *backing_file_real_path(struct file *f)
+struct path *backing_file_user_path(struct file *f)
 {
-	return &backing_file(f)->real_path;
+	return &backing_file(f)->user_path;
 }
-EXPORT_SYMBOL_GPL(backing_file_real_path);
+EXPORT_SYMBOL_GPL(backing_file_user_path);
 
 static inline void file_free(struct file *f)
 {
@@ -68,7 +68,7 @@ static inline void file_free(struct file *f)
 		percpu_counter_dec(&nr_files);
 	put_cred(f->f_cred);
 	if (unlikely(f->f_mode & FMODE_BACKING)) {
-		path_put(backing_file_real_path(f));
+		path_put(backing_file_user_path(f));
 		kfree(backing_file(f));
 	} else {
 		kmem_cache_free(filp_cachep, f);
diff --git a/fs/internal.h b/fs/internal.h
index 4e93a685bdaa..58e43341aebf 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -101,7 +101,7 @@ static inline void file_put_write_access(struct file *file)
 	put_write_access(file->f_inode);
 	mnt_put_write_access(file->f_path.mnt);
 	if (unlikely(file->f_mode & FMODE_BACKING))
-		mnt_put_write_access(backing_file_real_path(file)->mnt);
+		mnt_put_write_access(backing_file_user_path(file)->mnt);
 }
 
 static inline void put_file_access(struct file *file)
diff --git a/fs/open.c b/fs/open.c
index fe63e236da22..02dc608d40d8 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -881,7 +881,7 @@ static inline int file_get_write_access(struct file *f)
 	if (unlikely(error))
 		goto cleanup_inode;
 	if (unlikely(f->f_mode & FMODE_BACKING)) {
-		error = mnt_get_write_access(backing_file_real_path(f)->mnt);
+		error = mnt_get_write_access(backing_file_user_path(f)->mnt);
 		if (unlikely(error))
 			goto cleanup_mnt;
 	}
@@ -1182,20 +1182,19 @@ EXPORT_SYMBOL_GPL(kernel_file_open);
 
 /**
  * backing_file_open - open a backing file for kernel internal use
- * @path:	path of the file to open
+ * @user_path:	path that the user reuqested to open
  * @flags:	open flags
  * @real_path:	path of the backing file
  * @cred:	credentials for open
  *
  * Open a backing file for a stackable filesystem (e.g., overlayfs).
- * @path may be on the stackable filesystem and backing inode on the
- * underlying filesystem. In this case, we want to be able to return
- * the @real_path of the backing inode. This is done by embedding the
- * returned file into a container structure that also stores the path of
- * the backing inode on the underlying filesystem, which can be
- * retrieved using backing_file_real_path().
+ * @user_path may be on the stackable filesystem and @real_path on the
+ * underlying filesystem.  In this case, we want to be able to return the
+ * @user_path of the stackable filesystem. This is done by embedding the
+ * returned file into a container structure that also stores the stacked
+ * file's path, which can be retrieved using backing_file_user_path().
  */
-struct file *backing_file_open(const struct path *path, int flags,
+struct file *backing_file_open(const struct path *user_path, int flags,
 			       const struct path *real_path,
 			       const struct cred *cred)
 {
@@ -1206,9 +1205,9 @@ struct file *backing_file_open(const struct path *path, int flags,
 	if (IS_ERR(f))
 		return f;
 
-	f->f_path = *path;
-	path_get(real_path);
-	*backing_file_real_path(f) = *real_path;
+	path_get(user_path);
+	*backing_file_user_path(f) = *user_path;
+	f->f_path = *real_path;
 	error = do_dentry_open(f, d_inode(real_path->dentry), NULL);
 	if (error) {
 		fput(f);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 3fa2416264a4..400a925a8ad2 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -34,14 +34,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	struct dentry *real = NULL, *lower;
 	int err;
 
-	/* It's an overlay file */
+	/*
+	 * vfs is only expected to call d_real() with NULL from d_real_inode()
+	 * and with overlay inode from file_dentry() on an overlay file.
+	 *
+	 * TODO: remove @inode argument from d_real() API, remove code in this
+	 * function that deals with non-NULL @inode and remove d_real() call
+	 * from file_dentry().
+	 */
 	if (inode && d_inode(dentry) == inode)
 		return dentry;
+	else if (inode)
+		goto bug;
 
 	if (!d_is_reg(dentry)) {
-		if (!inode || inode == d_inode(dentry))
-			return dentry;
-		goto bug;
+		/* d_real_inode() is only relevant for regular files */
+		return dentry;
 	}
 
 	real = ovl_dentry_upper(dentry);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a8e4e1cac48e..b0624d83c2db 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2451,26 +2451,10 @@ struct file *dentry_open(const struct path *path, int flags,
 			 const struct cred *creds);
 struct file *dentry_create(const struct path *path, int flags, umode_t mode,
 			   const struct cred *cred);
-struct file *backing_file_open(const struct path *path, int flags,
+struct file *backing_file_open(const struct path *user_path, int flags,
 			       const struct path *real_path,
 			       const struct cred *cred);
-struct path *backing_file_real_path(struct file *f);
-
-/*
- * file_real_path - get the path corresponding to f_inode
- *
- * When opening a backing file for a stackable filesystem (e.g.,
- * overlayfs) f_path may be on the stackable filesystem and f_inode on
- * the underlying filesystem.  When the path associated with f_inode is
- * needed, this helper should be used instead of accessing f_path
- * directly.
-*/
-static inline const struct path *file_real_path(struct file *f)
-{
-	if (unlikely(f->f_mode & FMODE_BACKING))
-		return backing_file_real_path(f);
-	return &f->f_path;
-}
+struct path *backing_file_user_path(struct file *f);
 
 /*
  * file_user_path - get the path to display for memory mapped file
@@ -2483,6 +2467,8 @@ static inline const struct path *file_real_path(struct file *f)
  */
 static inline const struct path *file_user_path(struct file *f)
 {
+	if (unlikely(f->f_mode & FMODE_BACKING))
+		return backing_file_user_path(f);
 	return &f->f_path;
 }
 
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index ed48e4f1e755..bcb6609b54b3 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -96,8 +96,7 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
 	if (file->f_mode & FMODE_NONOTIFY)
 		return 0;
 
-	/* Overlayfs internal files have fake f_path */
-	path = file_real_path(file);
+	path = &file->f_path;
 	return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH);
 }
 
-- 
2.34.1

Re: [PATCH v3 0/3] Reduce impact of overlayfs backing files fake path

[Christian Brauner]

On Mon, 09 Oct 2023 18:37:09 +0300, Amir Goldstein wrote:
> Following v3 addresses Al's review comments on v2.
> 

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/3] fs: get mnt_writers count for an open backing file's real path
      https://git.kernel.org/vfs/vfs/c/90e168d5fa01
[2/3] fs: create helper file_user_path() for user displayed mapped file path
      https://git.kernel.org/vfs/vfs/c/842b845c7657
[3/3] fs: store real path instead of fake path in backing file f_path
      https://git.kernel.org/vfs/vfs/c/6b9503cf48c9

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Miklos Szeredi]

On Mon, 9 Oct 2023 at 17:37, Amir Goldstein <amir73il@gmail.com> wrote:

>  static inline void put_file_access(struct file *file)
> diff --git a/fs/open.c b/fs/open.c
> index fe63e236da22..02dc608d40d8 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -881,7 +881,7 @@ static inline int file_get_write_access(struct file *f)
>         if (unlikely(error))
>                 goto cleanup_inode;
>         if (unlikely(f->f_mode & FMODE_BACKING)) {
> -               error = mnt_get_write_access(backing_file_real_path(f)->mnt);
> +               error = mnt_get_write_access(backing_file_user_path(f)->mnt);
>                 if (unlikely(error))
>                         goto cleanup_mnt;
>         }

Do we really need write access on the overlay mount?

If so, should the order of getting write access not be the other way
round (overlay first, backing second)?

Thanks,
Miklos

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Amir Goldstein]

On Tue, Oct 10, 2023 at 2:59 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 9 Oct 2023 at 17:37, Amir Goldstein <amir73il@gmail.com> wrote:
>
> >  static inline void put_file_access(struct file *file)
> > diff --git a/fs/open.c b/fs/open.c
> > index fe63e236da22..02dc608d40d8 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -881,7 +881,7 @@ static inline int file_get_write_access(struct file *f)
> >         if (unlikely(error))
> >                 goto cleanup_inode;
> >         if (unlikely(f->f_mode & FMODE_BACKING)) {
> > -               error = mnt_get_write_access(backing_file_real_path(f)->mnt);
> > +               error = mnt_get_write_access(backing_file_user_path(f)->mnt);
> >                 if (unlikely(error))
> >                         goto cleanup_mnt;
> >         }
>
> Do we really need write access on the overlay mount?
>

I'd rather this vfs code be generic and not assume things about
ovl private mount.
These assumptions may not hold for fuse passthough backing files.

That said, if we have an open(O_RDWR),mmap(PROT_WRITE),close()
sequence on overlayfs, don't we need the write access on ovl_upper_mnt
in order to avoid upper sb from being remounted RO?

> If so, should the order of getting write access not be the other way
> round (overlay first, backing second)?
>

Why is the order important?
What am I missing?

Thanks,
Amir.

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Amir Goldstein]

On Tue, Oct 10, 2023 at 4:10 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Oct 10, 2023 at 2:59 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 9 Oct 2023 at 17:37, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > >  static inline void put_file_access(struct file *file)
> > > diff --git a/fs/open.c b/fs/open.c
> > > index fe63e236da22..02dc608d40d8 100644
> > > --- a/fs/open.c
> > > +++ b/fs/open.c
> > > @@ -881,7 +881,7 @@ static inline int file_get_write_access(struct file *f)
> > >         if (unlikely(error))
> > >                 goto cleanup_inode;
> > >         if (unlikely(f->f_mode & FMODE_BACKING)) {
> > > -               error = mnt_get_write_access(backing_file_real_path(f)->mnt);
> > > +               error = mnt_get_write_access(backing_file_user_path(f)->mnt);
> > >                 if (unlikely(error))
> > >                         goto cleanup_mnt;
> > >         }
> >
> > Do we really need write access on the overlay mount?
> >
>
> I'd rather this vfs code be generic and not assume things about
> ovl private mount.
> These assumptions may not hold for fuse passthough backing files.
>
> That said, if we have an open(O_RDWR),mmap(PROT_WRITE),close()
> sequence on overlayfs, don't we need the write access on ovl_upper_mnt
> in order to avoid upper sb from being remounted RO?
>

Sorry, you asked about ovl mount.
To me it makes sense that if users observe ovl paths in writable mapped
memory, that ovl should not be remounted RO.
Anyway, I don't see a good reason to allow remount RO for ovl in that case.
Is there?

> > If so, should the order of getting write access not be the other way
> > round (overlay first, backing second)?
> >
>
> Why is the order important?
> What am I missing?
>
> Thanks,
> Amir.

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Miklos Szeredi]

On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:

> Sorry, you asked about ovl mount.
> To me it makes sense that if users observe ovl paths in writable mapped
> memory, that ovl should not be remounted RO.
> Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> Is there?

Agreed.

But is preventing remount RO important enough to warrant special
casing of backing file in generic code?  I'm not convinced either
way...

Thanks,
Miklos

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Amir Goldstein]

On Tue, Oct 10, 2023 at 4:34 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > Sorry, you asked about ovl mount.
> > To me it makes sense that if users observe ovl paths in writable mapped
> > memory, that ovl should not be remounted RO.
> > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > Is there?
>
> Agreed.
>
> But is preventing remount RO important enough to warrant special
> casing of backing file in generic code?  I'm not convinced either
> way...

I prefer correctness and I doubt that the check
   if (unlikely(f->f_mode & FMODE_BACKING))
is worth optimizing.

but I will let Christian make the final call.

Thanks,
Amir.

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Al Viro]

On Tue, Oct 10, 2023 at 03:34:45PM +0200, Miklos Szeredi wrote:
> On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> > Sorry, you asked about ovl mount.
> > To me it makes sense that if users observe ovl paths in writable mapped
> > memory, that ovl should not be remounted RO.
> > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > Is there?
> 
> Agreed.
> 
> But is preventing remount RO important enough to warrant special
> casing of backing file in generic code?  I'm not convinced either
> way...

You definitely want to guarantee that remounting filesystem r/o
prevents the changes of visible contents; it's not just POSIX,
it's a fairly basic common assumption about any local filesystems.

Whether that should affect generic code...  You could do what CODA does,
I suppose; call ->mmap() of underlying file, then copy the resulting
->vm_ops into your private structure and override ->close() there
(keeping the original elsewhere in the same structure).  Then your
->close() would call the original and drop write access on the
ovl mount explicitly taken in your ->open().

*IF* we go that way, we probably ought to provide a ->get_path()
method for VMAs (NULL meaning "take ->vm_file->f_path") and use
that in procfs accesses.  That could reduce the impact on generic
code pretty much to zero - FMODE_BACKING included.

But it would cost you an allocation of vm_operations_struct per
mmap, most of them almost identical ;-/  And merging would not be
trivial - CODA stores a reference to original ->vm_file in
that structure, and uses container_of() to get to it in
their ->close().  AFAICS, there's no other safe place to stash
that information in anywhere in vm_area_struct.

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Al Viro]

On Tue, Oct 10, 2023 at 05:55:04PM +0100, Al Viro wrote:
> On Tue, Oct 10, 2023 at 03:34:45PM +0200, Miklos Szeredi wrote:
> > On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
> > 
> > > Sorry, you asked about ovl mount.
> > > To me it makes sense that if users observe ovl paths in writable mapped
> > > memory, that ovl should not be remounted RO.
> > > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > > Is there?
> > 
> > Agreed.
> > 
> > But is preventing remount RO important enough to warrant special
> > casing of backing file in generic code?  I'm not convinced either
> > way...
> 
> You definitely want to guarantee that remounting filesystem r/o
> prevents the changes of visible contents; it's not just POSIX,
> it's a fairly basic common assumption about any local filesystems.

Incidentally, could we simply keep a reference to original struct file
instead of messing with path?

The only caller of backing_file_open() gets &file->f_path as user_path; how
about passing file instead, and having backing_file_open() do get_file()
on it and stash the sucker into your object?

And have put_file_access() do
	if (unlikely(file->f_mode & FMODE_BACKING))
		fput(backing_file(file)->file);
in the end.

No need to mess with write access in any special way and it's closer
to the semantics we have for normal mmap(), after all - it keeps the
file we'd passed to it open as long as mapping is there.

Comments?

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Amir Goldstein]

On Tue, Oct 10, 2023 at 8:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Oct 10, 2023 at 05:55:04PM +0100, Al Viro wrote:
> > On Tue, Oct 10, 2023 at 03:34:45PM +0200, Miklos Szeredi wrote:
> > > On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > Sorry, you asked about ovl mount.
> > > > To me it makes sense that if users observe ovl paths in writable mapped
> > > > memory, that ovl should not be remounted RO.
> > > > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > > > Is there?
> > >
> > > Agreed.
> > >
> > > But is preventing remount RO important enough to warrant special
> > > casing of backing file in generic code?  I'm not convinced either
> > > way...
> >
> > You definitely want to guarantee that remounting filesystem r/o
> > prevents the changes of visible contents; it's not just POSIX,
> > it's a fairly basic common assumption about any local filesystems.
>
> Incidentally, could we simply keep a reference to original struct file
> instead of messing with path?
>
> The only caller of backing_file_open() gets &file->f_path as user_path; how
> about passing file instead, and having backing_file_open() do get_file()
> on it and stash the sucker into your object?
>
> And have put_file_access() do
>         if (unlikely(file->f_mode & FMODE_BACKING))
>                 fput(backing_file(file)->file);
> in the end.
>
> No need to mess with write access in any special way and it's closer
> to the semantics we have for normal mmap(), after all - it keeps the
> file we'd passed to it open as long as mapping is there.
>
> Comments?

Seems good to me.
It also shrinks backing_file by one pointer.

I think this patch can be an extra one after
"fs: store real path instead of fake path in backing file f_path"

Instead of changing storing of real_path to storing orig file in
one change?

If there are no objections, I will write it up.

Thanks,
Amir.

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Miklos Szeredi]

On Tue, 10 Oct 2023 at 19:41, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Oct 10, 2023 at 05:55:04PM +0100, Al Viro wrote:
> > On Tue, Oct 10, 2023 at 03:34:45PM +0200, Miklos Szeredi wrote:
> > > On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > Sorry, you asked about ovl mount.
> > > > To me it makes sense that if users observe ovl paths in writable mapped
> > > > memory, that ovl should not be remounted RO.
> > > > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > > > Is there?
> > >
> > > Agreed.
> > >
> > > But is preventing remount RO important enough to warrant special
> > > casing of backing file in generic code?  I'm not convinced either
> > > way...
> >
> > You definitely want to guarantee that remounting filesystem r/o
> > prevents the changes of visible contents; it's not just POSIX,
> > it's a fairly basic common assumption about any local filesystems.
>
> Incidentally, could we simply keep a reference to original struct file
> instead of messing with path?
>
> The only caller of backing_file_open() gets &file->f_path as user_path; how
> about passing file instead, and having backing_file_open() do get_file()
> on it and stash the sucker into your object?
>
> And have put_file_access() do
>         if (unlikely(file->f_mode & FMODE_BACKING))
>                 fput(backing_file(file)->file);
> in the end.

That's much nicer, I like it.

Thanks,
Miklos

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Al Viro]

On Tue, Oct 10, 2023 at 08:57:21PM +0300, Amir Goldstein wrote:
> On Tue, Oct 10, 2023 at 8:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Tue, Oct 10, 2023 at 05:55:04PM +0100, Al Viro wrote:
> > > On Tue, Oct 10, 2023 at 03:34:45PM +0200, Miklos Szeredi wrote:
> > > > On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > > Sorry, you asked about ovl mount.
> > > > > To me it makes sense that if users observe ovl paths in writable mapped
> > > > > memory, that ovl should not be remounted RO.
> > > > > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > > > > Is there?
> > > >
> > > > Agreed.
> > > >
> > > > But is preventing remount RO important enough to warrant special
> > > > casing of backing file in generic code?  I'm not convinced either
> > > > way...
> > >
> > > You definitely want to guarantee that remounting filesystem r/o
> > > prevents the changes of visible contents; it's not just POSIX,
> > > it's a fairly basic common assumption about any local filesystems.
> >
> > Incidentally, could we simply keep a reference to original struct file
> > instead of messing with path?
> >
> > The only caller of backing_file_open() gets &file->f_path as user_path; how
> > about passing file instead, and having backing_file_open() do get_file()
> > on it and stash the sucker into your object?
> >
> > And have put_file_access() do
> >         if (unlikely(file->f_mode & FMODE_BACKING))
> >                 fput(backing_file(file)->file);
> > in the end.
> >
> > No need to mess with write access in any special way and it's closer
> > to the semantics we have for normal mmap(), after all - it keeps the
> > file we'd passed to it open as long as mapping is there.
> >
> > Comments?
> 
> Seems good to me.
> It also shrinks backing_file by one pointer.
> 
> I think this patch can be an extra one after
> "fs: store real path instead of fake path in backing file f_path"
> 
> Instead of changing storing of real_path to storing orig file in
> one change?
> 
> If there are no objections, I will write it up.

Actually, now that I'd looked at it a bit more...  Look:
we don't need to do *anything* in put_file_access(); just
make file_free()
        if (unlikely(f->f_mode & FMODE_BACKING))
		fput(backing_file(f)->user_file);
instead of conditional path_put().  That + change of open_backing_file()
prototype + get_file() in there pretty much eliminates the work done
in 1/3 - you don't need to mess with {get,put}_file_write_access()
at all.

I'd start with this:

struct file *vm_user_file(struct vm_area_struct *vma)
{
	return vma->vm_file;
}
+ replace file = vma->vm_file; with file = vm_user_file(vma) in
the places affected by your 2/3.  That's the first (obviously
safe) commit.  Then the change of backing_file_open() combined
with making vm_user_file() do this:
	file = vma->vm_file;
	if (file && unlikely(file->f_mode & FMODE_BACKING))
		file = backing_file(file)->user_file;
	return file;

Voila.  Two-commit series, considerably smaller than your
variant...

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Amir Goldstein]

On Tue, Oct 10, 2023 at 9:21 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Oct 10, 2023 at 08:57:21PM +0300, Amir Goldstein wrote:
> > On Tue, Oct 10, 2023 at 8:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 05:55:04PM +0100, Al Viro wrote:
> > > > On Tue, Oct 10, 2023 at 03:34:45PM +0200, Miklos Szeredi wrote:
> > > > > On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > > Sorry, you asked about ovl mount.
> > > > > > To me it makes sense that if users observe ovl paths in writable mapped
> > > > > > memory, that ovl should not be remounted RO.
> > > > > > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > > > > > Is there?
> > > > >
> > > > > Agreed.
> > > > >
> > > > > But is preventing remount RO important enough to warrant special
> > > > > casing of backing file in generic code?  I'm not convinced either
> > > > > way...
> > > >
> > > > You definitely want to guarantee that remounting filesystem r/o
> > > > prevents the changes of visible contents; it's not just POSIX,
> > > > it's a fairly basic common assumption about any local filesystems.
> > >
> > > Incidentally, could we simply keep a reference to original struct file
> > > instead of messing with path?
> > >
> > > The only caller of backing_file_open() gets &file->f_path as user_path; how
> > > about passing file instead, and having backing_file_open() do get_file()
> > > on it and stash the sucker into your object?
> > >
> > > And have put_file_access() do
> > >         if (unlikely(file->f_mode & FMODE_BACKING))
> > >                 fput(backing_file(file)->file);
> > > in the end.
> > >
> > > No need to mess with write access in any special way and it's closer
> > > to the semantics we have for normal mmap(), after all - it keeps the
> > > file we'd passed to it open as long as mapping is there.
> > >
> > > Comments?
> >
> > Seems good to me.
> > It also shrinks backing_file by one pointer.
> >
> > I think this patch can be an extra one after
> > "fs: store real path instead of fake path in backing file f_path"
> >
> > Instead of changing storing of real_path to storing orig file in
> > one change?
> >
> > If there are no objections, I will write it up.
>
> Actually, now that I'd looked at it a bit more...  Look:
> we don't need to do *anything* in put_file_access(); just
> make file_free()
>         if (unlikely(f->f_mode & FMODE_BACKING))
>                 fput(backing_file(f)->user_file);
> instead of conditional path_put().  That + change of open_backing_file()
> prototype + get_file() in there pretty much eliminates the work done
> in 1/3 - you don't need to mess with {get,put}_file_write_access()
> at all.
>
> I'd start with this:
>
> struct file *vm_user_file(struct vm_area_struct *vma)
> {
>         return vma->vm_file;
> }
> + replace file = vma->vm_file; with file = vm_user_file(vma) in
> the places affected by your 2/3.  That's the first (obviously
> safe) commit.  Then the change of backing_file_open() combined
> with making vm_user_file() do this:
>         file = vma->vm_file;
>         if (file && unlikely(file->f_mode & FMODE_BACKING))
>                 file = backing_file(file)->user_file;
>         return file;
>
> Voila.  Two-commit series, considerably smaller than your
> variant...
>

Yap. looks very nice.
I will try that out tomorrow.

Anyway, it doesn't hurt to have the current version in linux-next
for the night to see if the change from fake f_path to real f_path
has any unexpected outcomes.

Thanks for the suggestions!
Amir.

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Al Viro]

On Tue, Oct 10, 2023 at 09:28:41PM +0300, Amir Goldstein wrote:
> > with making vm_user_file() do this:
> >         file = vma->vm_file;
> >         if (file && unlikely(file->f_mode & FMODE_BACKING))
> >                 file = backing_file(file)->user_file;
> >         return file;
> >
> > Voila.  Two-commit series, considerably smaller than your
> > variant...
> >
> 
> Yap. looks very nice.
> I will try that out tomorrow.
> 
> Anyway, it doesn't hurt to have the current version in linux-next
> for the night to see if the change from fake f_path to real f_path
> has any unexpected outcomes.

There is an obvious problem with that approach - refcounts don't
mix with loops ;-/  Sorry about the noise - this really won't work.

Re: [PATCH v3 3/3] fs: store real path instead of fake path in backing file f_path

[Al Viro]

On Tue, Oct 10, 2023 at 08:14:15PM +0200, Miklos Szeredi wrote:
> On Tue, 10 Oct 2023 at 19:41, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Tue, Oct 10, 2023 at 05:55:04PM +0100, Al Viro wrote:
> > > On Tue, Oct 10, 2023 at 03:34:45PM +0200, Miklos Szeredi wrote:
> > > > On Tue, 10 Oct 2023 at 15:17, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > > Sorry, you asked about ovl mount.
> > > > > To me it makes sense that if users observe ovl paths in writable mapped
> > > > > memory, that ovl should not be remounted RO.
> > > > > Anyway, I don't see a good reason to allow remount RO for ovl in that case.
> > > > > Is there?
> > > >
> > > > Agreed.
> > > >
> > > > But is preventing remount RO important enough to warrant special
> > > > casing of backing file in generic code?  I'm not convinced either
> > > > way...
> > >
> > > You definitely want to guarantee that remounting filesystem r/o
> > > prevents the changes of visible contents; it's not just POSIX,
> > > it's a fairly basic common assumption about any local filesystems.
> >
> > Incidentally, could we simply keep a reference to original struct file
> > instead of messing with path?
> >
> > The only caller of backing_file_open() gets &file->f_path as user_path; how
> > about passing file instead, and having backing_file_open() do get_file()
> > on it and stash the sucker into your object?
> >
> > And have put_file_access() do
> >         if (unlikely(file->f_mode & FMODE_BACKING))
> >                 fput(backing_file(file)->file);
> > in the end.
> 
> That's much nicer, I like it.

Won't work, unfortunately ;-/  We have the damn thing created on open();
it really can't pin the original file, or we'll never get to closing it.

I don't think this approach could be salvaged - we could make that
reference non-counting outside of mmap(), but we have no good way to
catch the moment when it should be dropped; not without intercepting
vm_ops->close() (and thus vm_ops->open() as well)...

Oh, well..