[PATCH v2 0/3] Handle notifications on overlayfs fake path files

[PATCH v2 0/3] Handle notifications on overlayfs fake path files

[Amir Goldstein]

Miklos,

The first solution that we discussed for removing FMODE_NONOTIFY
from overlayfs real files using file_fake container got complicated.

This alternative solution is less intrusive to vfs and all the vfs
code should remian unaffected expect for the special fsnotify case
that we want to fix.

Thanks,
Amir.

Changes since v1:
- Drop the file_fake container
- Leave f_path fake and special case only fsnotify

[1] https://github.com/amir73il/linux/commits/ovl_real_path

Amir Goldstein (3):
  fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL
  fs: introduce f_real_path() helper
  ovl: enable fsnotify events on underlying real files

 Documentation/filesystems/locking.rst |  3 ++-
 Documentation/filesystems/vfs.rst     |  3 ++-
 fs/file_table.c                       | 29 ++++++++++++++++++++++++---
 fs/internal.h                         |  5 +++--
 fs/namei.c                            |  2 +-
 fs/open.c                             |  2 +-
 fs/overlayfs/file.c                   |  4 ++--
 fs/overlayfs/super.c                  | 27 ++++++++++++++++---------
 include/linux/dcache.h                | 11 ++++++----
 include/linux/fs.h                    |  8 +++++---
 include/linux/fsnotify.h              |  6 ++++--
 11 files changed, 71 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL

[Amir Goldstein]

Rename the flag FMODE_NOACCOUNT that is used to mark internal files of
overlayfs and cachefiles to the more generic name FMODE_INTERNAL, which
also indicates that the file's f_path is possibly "fake".

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/file_table.c    | 6 +++---
 fs/internal.h      | 5 +++--
 fs/namei.c         | 2 +-
 fs/open.c          | 2 +-
 include/linux/fs.h | 4 ++--
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 372653b92617..d64d3933f3e4 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -55,7 +55,7 @@ static void file_free_rcu(struct rcu_head *head)
 static inline void file_free(struct file *f)
 {
 	security_file_free(f);
-	if (!(f->f_mode & FMODE_NOACCOUNT))
+	if (!(f->f_mode & FMODE_INTERNAL))
 		percpu_counter_dec(&nr_files);
 	call_rcu(&f->f_rcuhead, file_free_rcu);
 }
@@ -205,12 +205,12 @@ struct file *alloc_empty_file(int flags, const struct cred *cred)
  *
  * Should not be used unless there's a very good reason to do so.
  */
-struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
+struct file *alloc_empty_file_internal(int flags, const struct cred *cred)
 {
 	struct file *f = __alloc_file(flags, cred);
 
 	if (!IS_ERR(f))
-		f->f_mode |= FMODE_NOACCOUNT;
+		f->f_mode |= FMODE_INTERNAL;
 
 	return f;
 }
diff --git a/fs/internal.h b/fs/internal.h
index bd3b2810a36b..018605caf597 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -97,8 +97,9 @@ extern void chroot_fs_refs(const struct path *, const struct path *);
 /*
  * file_table.c
  */
-extern struct file *alloc_empty_file(int, const struct cred *);
-extern struct file *alloc_empty_file_noaccount(int, const struct cred *);
+extern struct file *alloc_empty_file(int flags, const struct cred *cred);
+extern struct file *alloc_empty_file_internal(int flags,
+					      const struct cred *cred);
 
 static inline void put_file_access(struct file *file)
 {
diff --git a/fs/namei.c b/fs/namei.c
index e4fe0879ae55..167f5acb0acf 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3721,7 +3721,7 @@ struct file *vfs_tmpfile_open(struct mnt_idmap *idmap,
 	struct file *file;
 	int error;
 
-	file = alloc_empty_file_noaccount(open_flag, cred);
+	file = alloc_empty_file_internal(open_flag, cred);
 	if (!IS_ERR(file)) {
 		error = vfs_tmpfile(idmap, parentpath, file, mode);
 		if (error) {
diff --git a/fs/open.c b/fs/open.c
index 81444ebf6091..23f862708a4f 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1124,7 +1124,7 @@ EXPORT_SYMBOL(dentry_create);
 struct file *open_with_fake_path(const struct path *path, int flags,
 				struct inode *inode, const struct cred *cred)
 {
-	struct file *f = alloc_empty_file_noaccount(flags, cred);
+	struct file *f = alloc_empty_file_internal(flags, cred);
 	if (!IS_ERR(f)) {
 		int error;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21a981680856..13eec1e8ca86 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -180,8 +180,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 /* File represents mount that needs unmounting */
 #define FMODE_NEED_UNMOUNT	((__force fmode_t)0x10000000)
 
-/* File does not contribute to nr_files count */
-#define FMODE_NOACCOUNT		((__force fmode_t)0x20000000)
+/* File is kernel internal does not contribute to nr_files count */
+#define FMODE_INTERNAL		((__force fmode_t)0x20000000)
 
 /* File supports async buffered reads */
 #define FMODE_BUF_RASYNC	((__force fmode_t)0x40000000)
-- 
2.34.1

Re: [PATCH v2 1/3] fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL

[Christoph Hellwig]

On Sun, Jun 11, 2023 at 04:27:30PM +0300, Amir Goldstein wrote:
> Rename the flag FMODE_NOACCOUNT that is used to mark internal files of
> overlayfs and cachefiles to the more generic name FMODE_INTERNAL, which
> also indicates that the file's f_path is possibly "fake".

FMODE_INTERNAL is completely meaningless.  Plase come up with a name
that actually explain what is special about these files. 

Re: [PATCH v2 1/3] fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL

[Amir Goldstein]

On Mon, Jun 12, 2023 at 7:27 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sun, Jun 11, 2023 at 04:27:30PM +0300, Amir Goldstein wrote:
> > Rename the flag FMODE_NOACCOUNT that is used to mark internal files of
> > overlayfs and cachefiles to the more generic name FMODE_INTERNAL, which
> > also indicates that the file's f_path is possibly "fake".
>
> FMODE_INTERNAL is completely meaningless.  Plase come up with a name
> that actually explain what is special about these files.
>

Well, I am not sure if FMODE_FAKE_PATH in v3 is a better name,
because you did rightfully say that "fake path" is not that descriptive,
but I will think of a better way to describe "fake path" and match the
flag to the file container name.

Thanks,
Amir.

Re: [PATCH v2 1/3] fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL

[Christoph Hellwig]

On Mon, Jun 12, 2023 at 09:08:37AM +0300, Amir Goldstein wrote:
> Well, I am not sure if FMODE_FAKE_PATH in v3 is a better name,
> because you did rightfully say that "fake path" is not that descriptive,
> but I will think of a better way to describe "fake path" and match the
> flag to the file container name.

I suspect the just claling it out what it is and naming it
FMODE_OVERLAYFS might be a good idea.  We'd just need to make sure not
to set it for the cachefiles use case, which is probably a good idea
anyway.

Re: [PATCH v2 1/3] fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL

[Christoph Hellwig]

On Sun, Jun 11, 2023 at 11:11:25PM -0700, Christoph Hellwig wrote:
> On Mon, Jun 12, 2023 at 09:08:37AM +0300, Amir Goldstein wrote:
> > Well, I am not sure if FMODE_FAKE_PATH in v3 is a better name,
> > because you did rightfully say that "fake path" is not that descriptive,
> > but I will think of a better way to describe "fake path" and match the
> > flag to the file container name.
> 
> I suspect the just claling it out what it is and naming it
> FMODE_OVERLAYFS might be a good idea.  We'd just need to make sure not
> to set it for the cachefiles use case, which is probably a good idea
> anyway.

Adding Dave:

not sure if I'm missing something, but is there any good reason
cachefs doesn't juse use dentry_open() ?

Re: [PATCH v2 1/3] fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL

[Amir Goldstein]

On Mon, Jun 12, 2023 at 9:11 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jun 12, 2023 at 09:08:37AM +0300, Amir Goldstein wrote:
> > Well, I am not sure if FMODE_FAKE_PATH in v3 is a better name,
> > because you did rightfully say that "fake path" is not that descriptive,
> > but I will think of a better way to describe "fake path" and match the
> > flag to the file container name.
>
> I suspect the just claling it out what it is and naming it
> FMODE_OVERLAYFS might be a good idea.  We'd just need to make sure not
> to set it for the cachefiles use case, which is probably a good idea
> anyway.

Agree to both.
As I told Christian, I was reluctant to use the last available flag bit
(although you did free up a couple of flags:)), but making
FMODE_OVERLAYFS overlayfs only and keeping cachefiles with
FMODE_NOACCOUNT would be the cleaner thing to do.

Thanks,
Amir.

Re: [PATCH v2 1/3] fs: rename FMODE_NOACCOUNT to FMODE_INTERNAL

[Christoph Hellwig]

On Mon, Jun 12, 2023 at 09:32:05AM +0300, Amir Goldstein wrote:
> Agree to both.
> As I told Christian, I was reluctant to use the last available flag bit
> (although you did free up a couple of flags:)), but making
> FMODE_OVERLAYFS overlayfs only and keeping cachefiles with
> FMODE_NOACCOUNT would be the cleaner thing to do.

Please go ahead with the bit.  In addition to the block series I plan
to move all static flags from f_mode to a new f_op.flags field in
the next merge window, which should help with avaiability.

[PATCH v2 2/3] fs: introduce f_real_path() helper

[Amir Goldstein]

Overlayfs knows the real path of underlying dentries.  Add an optional
struct vfsmount out argument to ->d_real(), so callers could compose the
real path.

Add a helper f_real_path() that uses this new interface to return the
real path of f_inode, for overlayfs internal files whose f_path if a
"fake" overlayfs path and f_inode is the underlying real inode.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/locking.rst |  3 ++-
 Documentation/filesystems/vfs.rst     |  3 ++-
 fs/file_table.c                       | 23 +++++++++++++++++++++++
 fs/overlayfs/super.c                  | 27 ++++++++++++++++++---------
 include/linux/dcache.h                | 11 +++++++----
 include/linux/fs.h                    |  4 +++-
 6 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index aa1a233b0fa8..a6063b0c79fd 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -29,7 +29,8 @@ prototypes::
 	char *(*d_dname)((struct dentry *dentry, char *buffer, int buflen);
 	struct vfsmount *(*d_automount)(struct path *path);
 	int (*d_manage)(const struct path *, bool);
-	struct dentry *(*d_real)(struct dentry *, const struct inode *);
+	struct dentry *(*d_real)(struct dentry *, const struct inode *,
+				 struct vfsmount **pmnt);
 
 locking rules:
 
diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
index 769be5230210..edafe824fca4 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -1264,7 +1264,8 @@ defined:
 		char *(*d_dname)(struct dentry *, char *, int);
 		struct vfsmount *(*d_automount)(struct path *);
 		int (*d_manage)(const struct path *, bool);
-		struct dentry *(*d_real)(struct dentry *, const struct inode *);
+		struct dentry *(*d_real)(struct dentry *, const struct inode *,
+					 struct vfsmount **pmnt);
 	};
 
 ``d_revalidate``
diff --git a/fs/file_table.c b/fs/file_table.c
index d64d3933f3e4..fa187ceb54d6 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -215,6 +215,29 @@ struct file *alloc_empty_file_internal(int flags, const struct cred *cred)
 	return f;
 }
 
+/**
+ * f_real_path - return the real path of an internal file with fake path
+ *
+ * @file: The file to query
+ *
+ * If f_path is on a union/overlay and f_inode is not, then return the
+ * underlying real path of f_inode.
+ * Otherwise return f_path (by value).
+ */
+struct path f_real_path(const struct file *f)
+{
+	struct path path;
+
+	if (!(f->f_mode & FMODE_INTERNAL) ||
+	    (d_inode(f->f_path.dentry) == f->f_inode))
+		return f->f_path;
+
+	path.mnt = f->f_path.mnt;
+	path.dentry = d_real(f->f_path.dentry, f->f_inode, &path.mnt);
+	return path;
+}
+EXPORT_SYMBOL(f_real_path);
+
 /**
  * alloc_file - allocate and initialize a 'struct file'
  *
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index d9be5d318e1b..591c77b33ff3 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -60,9 +60,11 @@ MODULE_PARM_DESC(metacopy,
 		 "Default to on or off for the metadata only copy up feature");
 
 static struct dentry *ovl_d_real(struct dentry *dentry,
-				 const struct inode *inode)
+				 const struct inode *inode,
+				 struct vfsmount **pmnt)
 {
 	struct dentry *real = NULL, *lower;
+	struct path realpath;
 
 	/* It's an overlay file */
 	if (inode && d_inode(dentry) == inode)
@@ -74,12 +76,13 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 		goto bug;
 	}
 
-	real = ovl_dentry_upper(dentry);
+	ovl_path_upper(dentry, &realpath);
+	real = realpath.dentry;
 	if (real && (inode == d_inode(real)))
-		return real;
+		goto found;
 
 	if (real && !inode && ovl_has_upperdata(d_inode(dentry)))
-		return real;
+		goto found;
 
 	/*
 	 * Best effort lazy lookup of lowerdata for !inode case to return
@@ -90,16 +93,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	 * when setting the uprobe.
 	 */
 	ovl_maybe_lookup_lowerdata(dentry);
-	lower = ovl_dentry_lowerdata(dentry);
+	ovl_path_lowerdata(dentry, &realpath);
+	lower = realpath.dentry;
 	if (!lower)
 		goto bug;
-	real = lower;
 
 	/* Handle recursion */
-	real = d_real(real, inode);
+	real = d_real(lower, inode, &realpath.mnt);
+
+	if (inode && inode != d_inode(real))
+		goto bug;
+
+found:
+	if (pmnt)
+		*pmnt = realpath.mnt;
+	return real;
 
-	if (!inode || inode == d_inode(real))
-		return real;
 bug:
 	WARN(1, "%s(%pd4, %s:%lu): real dentry (%p/%lu) not found\n",
 	     __func__, dentry, inode ? inode->i_sb->s_id : "NULL",
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 6b351e009f59..78a54d175662 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -139,7 +139,8 @@ struct dentry_operations {
 	char *(*d_dname)(struct dentry *, char *, int);
 	struct vfsmount *(*d_automount)(struct path *);
 	int (*d_manage)(const struct path *, bool);
-	struct dentry *(*d_real)(struct dentry *, const struct inode *);
+	struct dentry *(*d_real)(struct dentry *, const struct inode *,
+				 struct vfsmount **);
 } ____cacheline_aligned;
 
 /*
@@ -564,6 +565,7 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper)
  * d_real - Return the real dentry
  * @dentry: the dentry to query
  * @inode: inode to select the dentry from multiple layers (can be NULL)
+ * @pmnt: returns the real mnt in case @dentry is not real
  *
  * If dentry is on a union/overlay, then return the underlying, real dentry.
  * Otherwise return the dentry itself.
@@ -571,10 +573,11 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper)
  * See also: Documentation/filesystems/vfs.rst
  */
 static inline struct dentry *d_real(struct dentry *dentry,
-				    const struct inode *inode)
+				    const struct inode *inode,
+				    struct vfsmount **pmnt)
 {
 	if (unlikely(dentry->d_flags & DCACHE_OP_REAL))
-		return dentry->d_op->d_real(dentry, inode);
+		return dentry->d_op->d_real(dentry, inode, pmnt);
 	else
 		return dentry;
 }
@@ -589,7 +592,7 @@ static inline struct dentry *d_real(struct dentry *dentry,
 static inline struct inode *d_real_inode(const struct dentry *dentry)
 {
 	/* This usage of d_real() results in const dentry */
-	return d_backing_inode(d_real((struct dentry *) dentry, NULL));
+	return d_inode(d_real((struct dentry *) dentry, NULL, NULL));
 }
 
 struct name_snapshot {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 13eec1e8ca86..d0129e9e0ae5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1042,7 +1042,7 @@ static inline struct inode *file_inode(const struct file *f)
 
 static inline struct dentry *file_dentry(const struct file *file)
 {
-	return d_real(file->f_path.dentry, file_inode(file));
+	return d_real(file->f_path.dentry, file_inode(file), NULL);
 }
 
 struct fasync_struct {
@@ -2354,6 +2354,8 @@ extern struct file *dentry_create(const struct path *path, int flags,
 				  umode_t mode, const struct cred *cred);
 extern struct file * open_with_fake_path(const struct path *, int,
 					 struct inode*, const struct cred *);
+extern struct path f_real_path(const struct file *f);
+
 static inline struct file *file_clone_open(struct file *file)
 {
 	return dentry_open(&file->f_path, file->f_flags, file->f_cred);
-- 
2.34.1

Re: [PATCH v2 2/3] fs: introduce f_real_path() helper

[Christoph Hellwig]

On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote:
> Overlayfs knows the real path of underlying dentries.  Add an optional
> struct vfsmount out argument to ->d_real(), so callers could compose the
> real path.
> 
> Add a helper f_real_path() that uses this new interface to return the
> real path of f_inode, for overlayfs internal files whose f_path if a
> "fake" overlayfs path and f_inode is the underlying real inode.

I really don't like this ->d_real nagic.  Most callers of it
really can't ever be on overlayfs.  So I'd suggest we do an audit
of the callers of file_dentry and drop all the pointless ones
first, and improve the documentation on when to use it.

Re: [PATCH v2 2/3] fs: introduce f_real_path() helper

[Amir Goldstein]

On Mon, Jun 12, 2023 at 7:36 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote:
> > Overlayfs knows the real path of underlying dentries.  Add an optional
> > struct vfsmount out argument to ->d_real(), so callers could compose the
> > real path.
> >
> > Add a helper f_real_path() that uses this new interface to return the
> > real path of f_inode, for overlayfs internal files whose f_path if a
> > "fake" overlayfs path and f_inode is the underlying real inode.
>
> I really don't like this ->d_real nagic.  Most callers of it
> really can't ever be on overlayfs.

Which callers are you referring to?

> So I'd suggest we do an audit
> of the callers of file_dentry and drop all the pointless ones
> first, and improve the documentation on when to use it.

Well, v3 is trying to reduce ->d_real() magic and the step
after introducing the alternative path container is to convert
file_dentry() to use the stored real_path instead of ->d_real().

But I agree that the documentation about this black magic is
missing. Will try to improve that with the move to the "fake"
file container.

Thanks,
Amir.

Re: [PATCH v2 2/3] fs: introduce f_real_path() helper

[Christoph Hellwig]

On Mon, Jun 12, 2023 at 09:28:40AM +0300, Amir Goldstein wrote:
> On Mon, Jun 12, 2023 at 7:36 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote:
> > > Overlayfs knows the real path of underlying dentries.  Add an optional
> > > struct vfsmount out argument to ->d_real(), so callers could compose the
> > > real path.
> > >
> > > Add a helper f_real_path() that uses this new interface to return the
> > > real path of f_inode, for overlayfs internal files whose f_path if a
> > > "fake" overlayfs path and f_inode is the underlying real inode.
> >
> > I really don't like this ->d_real nagic.  Most callers of it
> > really can't ever be on overlayfs.
> 
> Which callers are you referring to?

Most users of file_dentry are inside file systems and will never
see the overlayfs path.

> > So I'd suggest we do an audit
> > of the callers of file_dentry and drop all the pointless ones
> > first, and improve the documentation on when to use it.
> 
> Well, v3 is trying to reduce ->d_real() magic and the step
> after introducing the alternative path container is to convert
> file_dentry() to use the stored real_path instead of ->d_real().

Yeah, that makes this comment kinda irrelevant.

Re: [PATCH v2 2/3] fs: introduce f_real_path() helper

[Amir Goldstein]

On Mon, Jun 12, 2023 at 9:36 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jun 12, 2023 at 09:28:40AM +0300, Amir Goldstein wrote:
> > On Mon, Jun 12, 2023 at 7:36 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Sun, Jun 11, 2023 at 04:27:31PM +0300, Amir Goldstein wrote:
> > > > Overlayfs knows the real path of underlying dentries.  Add an optional
> > > > struct vfsmount out argument to ->d_real(), so callers could compose the
> > > > real path.
> > > >
> > > > Add a helper f_real_path() that uses this new interface to return the
> > > > real path of f_inode, for overlayfs internal files whose f_path if a
> > > > "fake" overlayfs path and f_inode is the underlying real inode.
> > >
> > > I really don't like this ->d_real nagic.  Most callers of it
> > > really can't ever be on overlayfs.
> >
> > Which callers are you referring to?
>
> Most users of file_dentry are inside file systems and will never
> see the overlayfs path.
>

Ay ay ay.
I suspected that this is what you meant and I do not blame you.
There is no documentation and it is hard to understand what is going on
even harder to understand why that is going on...

Before "ovl: stack file ops" series, a file opened from ovl (over xfs) path
would have ovl f_path and xfs f_inode, as well as xfs f_ops, so indeed xfs
code had to be careful, but so did a lot of generic vfs code.

After ovl stacked f_ops, a file opened from ovl (over xfs) path has an
ovl f_path and an ovl f_inode, so a lot of hacks could be removed from
generic vfs code (e.g. locks_inode() macro).

Alas, for every ovl file, there is an "internal/real" file (stashed in
file->f_private)
which is opened by open_with_fake_path().
This "internal/real" file has ovl f_path and xfs f_inode, so xfs could
not get rid
of file_dentry() just yet.

Currently, the reason for the fake f_path hack is that the same internal file
is assigned as ->vm_file in ovl_mmap(), ovl does not implement stacked aops
and some users of ->vm_file need the "fake" ovl path.

Anyway, the first step is to introduce the ovl internal file container.
Next, we will use the real_path for file_dentry() and we can see if we
can get rid of some of these file_dentry() uses.

Thanks,
Amir.

[PATCH v2 3/3] ovl: enable fsnotify events on underlying real files

[Amir Goldstein]

Overlayfs creates the real underlying files with fake f_path, whose
f_inode is on the underlying fs and f_path on overlayfs.

Those real files were open with FMODE_NONOTIFY, because fsnotify code was
not prapared to handle fsnotify hooks on files with fake path correctly
and fanotify would report unexpected event->fd with fake overlayfs path,
when the underlying fs was being watched.

Teach fsnotify to handle events on the real files, and do not set real
files to FMODE_NONOTIFY to allow operations on real file (e.g. open,
access, modify, close) to generate async and permission events.

Because fsnotify does not have notifications on address space
operations, we do not need to worry about ->vm_file not reporting
events to a watched overlayfs when users are accessing a mapped
overlayfs file.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/file.c      | 4 ++--
 include/linux/fsnotify.h | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 39737c2aaa84..61b5faa2b10f 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -34,8 +34,8 @@ static char ovl_whatisit(struct inode *inode, struct inode *realinode)
 		return 'm';
 }
 
-/* No atime modification nor notify on underlying */
-#define OVL_OPEN_FLAGS (O_NOATIME | FMODE_NONOTIFY)
+/* No atime modification on underlying */
+#define OVL_OPEN_FLAGS (O_NOATIME)
 
 static struct file *ovl_open_realfile(const struct file *file,
 				      const struct path *realpath)
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index bb8467cd11ae..f4f5db765ec2 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -91,12 +91,14 @@ static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
 
 static inline int fsnotify_file(struct file *file, __u32 mask)
 {
-	const struct path *path = &file->f_path;
+	struct path path;
 
 	if (file->f_mode & FMODE_NONOTIFY)
 		return 0;
 
-	return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH);
+	/* Overlayfs internal files have fake f_path */
+	path = f_real_path(file);
+	return fsnotify_parent(path.dentry, mask, &path, FSNOTIFY_EVENT_PATH);
 }
 
 /* Simple call site for access decisions */
-- 
2.34.1

Re: [PATCH v2 0/3] Handle notifications on overlayfs fake path files

[Miklos Szeredi]

On Sun, 11 Jun 2023 at 15:27, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Miklos,
>
> The first solution that we discussed for removing FMODE_NONOTIFY
> from overlayfs real files using file_fake container got complicated.
>
> This alternative solution is less intrusive to vfs and all the vfs
> code should remian unaffected expect for the special fsnotify case
> that we want to fix.
>
> Thanks,
> Amir.
>
> Changes since v1:
> - Drop the file_fake container

Why?

Thanks,
Miklos

Re: [PATCH v2 0/3] Handle notifications on overlayfs fake path files

[Amir Goldstein]

On Sun, Jun 11, 2023 at 5:23 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, 11 Jun 2023 at 15:27, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Miklos,
> >
> > The first solution that we discussed for removing FMODE_NONOTIFY
> > from overlayfs real files using file_fake container got complicated.
> >
> > This alternative solution is less intrusive to vfs and all the vfs
> > code should remian unaffected expect for the special fsnotify case
> > that we want to fix.
> >
> > Thanks,
> > Amir.
> >
> > Changes since v1:
> > - Drop the file_fake container
>
> Why?

See my question on v1.
The fake file objects are used both as vm_file and in read/write iter
how do we know which path to use in low level functions?

If we allocate file_fake container and still store the fake path
in f_path, then we have no need to store also the real path
because ovl knows how to get from fake path to real path.

This is what v2 does.

What am I missing?

Thanks,
Amir.

Re: [PATCH v2 0/3] Handle notifications on overlayfs fake path files

[Amir Goldstein]

On Sun, Jun 11, 2023 at 7:55 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sun, Jun 11, 2023 at 5:23 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sun, 11 Jun 2023 at 15:27, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Miklos,
> > >
> > > The first solution that we discussed for removing FMODE_NONOTIFY
> > > from overlayfs real files using file_fake container got complicated.
> > >
> > > This alternative solution is less intrusive to vfs and all the vfs
> > > code should remian unaffected expect for the special fsnotify case
> > > that we want to fix.
> > >
> > > Thanks,
> > > Amir.
> > >
> > > Changes since v1:
> > > - Drop the file_fake container
> >
> > Why?
>
> See my question on v1.
> The fake file objects are used both as vm_file and in read/write iter
> how do we know which path to use in low level functions?
>
> If we allocate file_fake container and still store the fake path
> in f_path, then we have no need to store also the real path
> because ovl knows how to get from fake path to real path.
>
> This is what v2 does.
>
> What am I missing?
>

Is it because getting f_real_path() and file_dentry() via d_real()
is more expensive?
and caching this information in file_fake container would be
more efficient?

I will restore the file_fake container and post v3.

Thanks,
Amir.

Re: [PATCH v2 0/3] Handle notifications on overlayfs fake path files

[Miklos Szeredi]

On Sun, 11 Jun 2023 at 19:52, Amir Goldstein <amir73il@gmail.com> wrote:

> Is it because getting f_real_path() and file_dentry() via d_real()
> is more expensive?
> and caching this information in file_fake container would be
> more efficient?
>
> I will restore the file_fake container and post v3.

I simply dislike the fact that ->d_real() is getting more complex.
I'd prefer d_real to die, which is unfortunately not so easy, as
you've explained.

But if we can make it somewhat less complex (remove the inode
parameter) instead of more complex (add a vfsmount * parameter) then
that's already a big win in my eyes.

Thanks,
Miklos

Re: [PATCH v2 0/3] Handle notifications on overlayfs fake path files

[Amir Goldstein]

On Sun, Jun 11, 2023 at 10:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, 11 Jun 2023 at 19:52, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > Is it because getting f_real_path() and file_dentry() via d_real()
> > is more expensive?
> > and caching this information in file_fake container would be
> > more efficient?
> >
> > I will restore the file_fake container and post v3.
>
> I simply dislike the fact that ->d_real() is getting more complex.
> I'd prefer d_real to die, which is unfortunately not so easy, as
> you've explained.
>
> But if we can make it somewhat less complex (remove the inode
> parameter) instead of more complex (add a vfsmount * parameter) then
> that's already a big win in my eyes.
>

OK, I can relate to that.

Here is file_fake restored, now with fsnotify fix also tested:

https://github.com/amir73il/linux/commits/ovl_fake_path

IIUC, you would now want to change file_dentry(f) to using
f_real_file(f)->dentry and get rid of the inode argument to d_real().

Do you want that change also in v3 or should we save some fun for later
and just fix fsnotify for now?

Thanks,
Amir.

Re: [PATCH v2 0/3] Handle notifications on overlayfs fake path files

[Miklos Szeredi]

On Sun, 11 Jun 2023 at 21:25, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sun, Jun 11, 2023 at 10:12 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sun, 11 Jun 2023 at 19:52, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > Is it because getting f_real_path() and file_dentry() via d_real()
> > > is more expensive?
> > > and caching this information in file_fake container would be
> > > more efficient?
> > >
> > > I will restore the file_fake container and post v3.
> >
> > I simply dislike the fact that ->d_real() is getting more complex.
> > I'd prefer d_real to die, which is unfortunately not so easy, as
> > you've explained.
> >
> > But if we can make it somewhat less complex (remove the inode
> > parameter) instead of more complex (add a vfsmount * parameter) then
> > that's already a big win in my eyes.
> >
>
> OK, I can relate to that.
>
> Here is file_fake restored, now with fsnotify fix also tested:
>
> https://github.com/amir73il/linux/commits/ovl_fake_path
>
> IIUC, you would now want to change file_dentry(f) to using
> f_real_file(f)->dentry and get rid of the inode argument to d_real().
>
> Do you want that change also in v3 or should we save some fun for later
> and just fix fsnotify for now?

Can do that later.  Small steps are good.

Thanks,
Miklos