[PATCH v4 0/2] Handle notifications on overlayfs fake path files

[PATCH v4 0/2] Handle notifications on overlayfs fake path files

[Amir Goldstein]

Christain,

A little while ago, Jan and I realized that an unprivileged overlayfs
mount could be used to avert fanotify permission events that were
requested for an inode or sb on the underlying fs.

The [v1] patch set was an attempt to implement Miklos' suggestion
(opt-in to query the fake path) which turned out to affet the vfs in
many places, so Miklos and I agreed on a solution that will be less
intrusive for vfs (opt-in to query the real path).

The [v2] patch set took the less intrusive approach to vfs, but it
also tried a different approach of extending the d_real() interface,
which Miklos did not like.

The [v3] patch goes back to the less intrusive approach to vfs without
complicating d_real() interface, that Miklso and I agreed on during the
[v1] patch set review, so hopefully everyone can be happy with it.

This v4 patch set addresses review comments from yourself and from
Christoph on [v3].

Since the patches are 95% vfs, I think it is better if they are merged
through the vfs tree.

I am hoping to solicit an ACK from Jan and Miklos on the minor changes
in ovl and fsnotify in the 2nd patch.

Thanks,
Amir.

Changes since [v3]:
- Rename struct file_fake to backing_file
- Rename helpers to open_backing_file(), backing_file_real_path()
- Rename FMODE_FAKE_PATH to FMODE_BACKING
- Separate flag from FMODE_NOACCOUNT
- inline the fast-path branch of f_real_path()

Changes since [v2]:
- Restore the file_fake container (Miklos)
- Re-arrange the v1 helpers (Christian)

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

[v3] https://lore.kernel.org/linux-unionfs/20230611194706.1583818-1-amir73il@gmail.com/
[v2] https://lore.kernel.org/linux-unionfs/20230611132732.1502040-1-amir73il@gmail.com/
[v1] https://lore.kernel.org/linux-unionfs/20230609073239.957184-1-amir73il@gmail.com/

Amir Goldstein (2):
  fs: use backing_file container for internal files with "fake" f_path
  ovl: enable fsnotify events on underlying real files

 fs/cachefiles/namei.c    |  4 +--
 fs/file_table.c          | 74 +++++++++++++++++++++++++++++++++++-----
 fs/internal.h            |  5 +--
 fs/open.c                | 30 +++++++++-------
 fs/overlayfs/file.c      |  8 ++---
 include/linux/fs.h       | 24 ++++++++++---
 include/linux/fsnotify.h |  3 +-
 7 files changed, 113 insertions(+), 35 deletions(-)

-- 
2.34.1

[PATCH v4 1/2] fs: use backing_file container for internal files with "fake" f_path

[Amir Goldstein]

Overlayfs and cachefiles use open_with_fake_path() to allocate internal
files, where overlayfs also puts a "fake" path in f_path - a path which
is not on the same fs as f_inode.

Allocate a container struct backing_file for those internal files, that
is used to hold the "fake" ovl path along with the real path.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/cachefiles/namei.c |  4 +--
 fs/file_table.c       | 74 +++++++++++++++++++++++++++++++++++++------
 fs/internal.h         |  5 +--
 fs/open.c             | 30 +++++++++++-------
 fs/overlayfs/file.c   |  4 +--
 include/linux/fs.h    | 24 +++++++++++---
 6 files changed, 109 insertions(+), 32 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 82219a8f6084..283534c6bc8d 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -560,8 +560,8 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
 	 */
 	path.mnt = cache->mnt;
 	path.dentry = dentry;
-	file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
-				   d_backing_inode(dentry), cache->cache_cred);
+	file = open_backing_file(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
+				 &path, cache->cache_cred);
 	if (IS_ERR(file)) {
 		trace_cachefiles_vfs_error(object, d_backing_inode(dentry),
 					   PTR_ERR(file),
diff --git a/fs/file_table.c b/fs/file_table.c
index 372653b92617..138d5d405df7 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -44,18 +44,40 @@ 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 */
+struct backing_file {
+	struct file file;
+	struct path real_path;
+};
+
+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)
+{
+	return &backing_file(f)->real_path;
+}
+EXPORT_SYMBOL_GPL(backing_file_real_path);
+
 static void file_free_rcu(struct rcu_head *head)
 {
 	struct file *f = container_of(head, struct file, f_rcuhead);
 
 	put_cred(f->f_cred);
-	kmem_cache_free(filp_cachep, f);
+	if (unlikely(f->f_mode & FMODE_BACKING))
+		kfree(backing_file(f));
+	else
+		kmem_cache_free(filp_cachep, f);
 }
 
 static inline void file_free(struct file *f)
 {
 	security_file_free(f);
-	if (!(f->f_mode & FMODE_NOACCOUNT))
+	if (unlikely(f->f_mode & FMODE_BACKING))
+		path_put(backing_file_real_path(f));
+	else
 		percpu_counter_dec(&nr_files);
 	call_rcu(&f->f_rcuhead, file_free_rcu);
 }
@@ -131,20 +153,15 @@ static int __init init_fs_stat_sysctls(void)
 fs_initcall(init_fs_stat_sysctls);
 #endif
 
-static struct file *__alloc_file(int flags, const struct cred *cred)
+static int init_file(struct file *f, int flags, const struct cred *cred)
 {
-	struct file *f;
 	int error;
 
-	f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
-	if (unlikely(!f))
-		return ERR_PTR(-ENOMEM);
-
 	f->f_cred = get_cred(cred);
 	error = security_file_alloc(f);
 	if (unlikely(error)) {
 		file_free_rcu(&f->f_rcuhead);
-		return ERR_PTR(error);
+		return error;
 	}
 
 	atomic_long_set(&f->f_count, 1);
@@ -155,6 +172,22 @@ static struct file *__alloc_file(int flags, const struct cred *cred)
 	f->f_mode = OPEN_FMODE(flags);
 	/* f->f_version: 0 */
 
+	return 0;
+}
+
+static struct file *__alloc_file(int flags, const struct cred *cred)
+{
+	struct file *f;
+	int error;
+
+	f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
+	if (unlikely(!f))
+		return ERR_PTR(-ENOMEM);
+
+	error = init_file(f, flags, cred);
+	if (unlikely(error))
+		return ERR_PTR(error);
+
 	return f;
 }
 
@@ -215,6 +248,29 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
 	return f;
 }
 
+/*
+ * Variant of alloc_empty_file() that allocates a backing_file container
+ * and doesn't check and modify nr_files.
+ *
+ * Should not be used unless there's a very good reason to do so.
+ */
+struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
+{
+	struct backing_file *ff;
+	int error;
+
+	ff = kzalloc(sizeof(struct backing_file), GFP_KERNEL);
+	if (unlikely(!ff))
+		return ERR_PTR(-ENOMEM);
+
+	error = init_file(&ff->file, flags, cred);
+	if (unlikely(error))
+		return ERR_PTR(error);
+
+	ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT;
+	return &ff->file;
+}
+
 /**
  * alloc_file - allocate and initialize a 'struct file'
  *
diff --git a/fs/internal.h b/fs/internal.h
index bd3b2810a36b..9c31078e0d16 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 *);
+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);
 
 static inline void put_file_access(struct file *file)
 {
diff --git a/fs/open.c b/fs/open.c
index 005ca91a173b..8f745e07a037 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1121,23 +1121,29 @@ struct file *dentry_create(const struct path *path, int flags, umode_t mode,
 }
 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 *open_backing_file(const struct path *path, int flags,
+			       const struct path *real_path,
+			       const struct cred *cred)
 {
-	struct file *f = alloc_empty_file_noaccount(flags, cred);
-	if (!IS_ERR(f)) {
-		int error;
+	struct file *f;
+	int error;
 
-		f->f_path = *path;
-		error = do_dentry_open(f, inode, NULL);
-		if (error) {
-			fput(f);
-			f = ERR_PTR(error);
-		}
+	f = alloc_empty_backing_file(flags, cred);
+	if (IS_ERR(f))
+		return f;
+
+	f->f_path = *path;
+	path_get(real_path);
+	*backing_file_real_path(f) = *real_path;
+	error = do_dentry_open(f, d_inode(real_path->dentry), NULL);
+	if (error) {
+		fput(f);
+		f = ERR_PTR(error);
 	}
+
 	return f;
 }
-EXPORT_SYMBOL(open_with_fake_path);
+EXPORT_SYMBOL(open_backing_file);
 
 #define WILL_CREATE(flags)	(flags & (O_CREAT | __O_TMPFILE))
 #define O_PATH_FLAGS		(O_DIRECTORY | O_NOFOLLOW | O_PATH | O_CLOEXEC)
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 39737c2aaa84..8cf099aa97de 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -61,8 +61,8 @@ static struct file *ovl_open_realfile(const struct file *file,
 		if (!inode_owner_or_capable(real_idmap, realinode))
 			flags &= ~O_NOATIME;
 
-		realfile = open_with_fake_path(&file->f_path, flags, realinode,
-					       current_cred());
+		realfile = open_backing_file(&file->f_path, flags, realpath,
+					     current_cred());
 	}
 	revert_creds(old_cred);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21a981680856..8393408b9885 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -171,6 +171,9 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 /* File supports non-exclusive O_DIRECT writes from multiple threads */
 #define FMODE_DIO_PARALLEL_WRITE	((__force fmode_t)0x1000000)
 
+/* File is embedded in backing_file object */
+#define FMODE_BACKING		((__force fmode_t)0x2000000)
+
 /* File was opened by fanotify and shouldn't generate fanotify events */
 #define FMODE_NONOTIFY		((__force fmode_t)0x4000000)
 
@@ -2349,11 +2352,22 @@ static inline struct file *file_open_root_mnt(struct vfsmount *mnt,
 	return file_open_root(&(struct path){.mnt = mnt, .dentry = mnt->mnt_root},
 			      name, flags, mode);
 }
-extern struct file * dentry_open(const struct path *, int, const struct cred *);
-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 *);
+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 *open_backing_file(const struct path *path, int flags,
+			       const struct path *real_path,
+			       const struct cred *cred);
+struct path *backing_file_real_path(struct file *f);
+static inline const struct path *f_real_path(struct file *f)
+{
+	if (unlikely(f->f_mode & FMODE_BACKING))
+		return backing_file_real_path(f);
+	else
+		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);
-- 
2.34.1

[PATCH v4 2/2] 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 | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 8cf099aa97de..1fdfc53f1207 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..6f6cbc2dc49b 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -91,7 +91,8 @@ 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;
+	/* Overlayfs internal files have fake f_path */
+	const struct path *path = f_real_path(file);
 
 	if (file->f_mode & FMODE_NONOTIFY)
 		return 0;
-- 
2.34.1

Re: [PATCH v4 2/2] ovl: enable fsnotify events on underlying real files

[Jan Kara]

On Wed 14-06-23 10:49:07, Amir Goldstein wrote:
> 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>

Looks good to me. Feel free to add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/overlayfs/file.c      | 4 ++--
>  include/linux/fsnotify.h | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 8cf099aa97de..1fdfc53f1207 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..6f6cbc2dc49b 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -91,7 +91,8 @@ 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;
> +	/* Overlayfs internal files have fake f_path */
> +	const struct path *path = f_real_path(file);
>  
>  	if (file->f_mode & FMODE_NONOTIFY)
>  		return 0;
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 1/2] fs: use backing_file container for internal files with "fake" f_path

[Christian Brauner]

On Wed, Jun 14, 2023 at 10:49:06AM +0300, Amir Goldstein wrote:
> Overlayfs and cachefiles use open_with_fake_path() to allocate internal
> files, where overlayfs also puts a "fake" path in f_path - a path which
> is not on the same fs as f_inode.
> 
> Allocate a container struct backing_file for those internal files, that
> is used to hold the "fake" ovl path along with the real path.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/cachefiles/namei.c |  4 +--
>  fs/file_table.c       | 74 +++++++++++++++++++++++++++++++++++++------
>  fs/internal.h         |  5 +--
>  fs/open.c             | 30 +++++++++++-------
>  fs/overlayfs/file.c   |  4 +--
>  include/linux/fs.h    | 24 +++++++++++---
>  6 files changed, 109 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index 82219a8f6084..283534c6bc8d 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -560,8 +560,8 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
>  	 */
>  	path.mnt = cache->mnt;
>  	path.dentry = dentry;
> -	file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> -				   d_backing_inode(dentry), cache->cache_cred);
> +	file = open_backing_file(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> +				 &path, cache->cache_cred);
>  	if (IS_ERR(file)) {
>  		trace_cachefiles_vfs_error(object, d_backing_inode(dentry),
>  					   PTR_ERR(file),
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 372653b92617..138d5d405df7 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -44,18 +44,40 @@ 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 */
> +struct backing_file {
> +	struct file file;
> +	struct path real_path;
> +};
> +
> +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)
> +{
> +	return &backing_file(f)->real_path;
> +}
> +EXPORT_SYMBOL_GPL(backing_file_real_path);
> +
>  static void file_free_rcu(struct rcu_head *head)
>  {
>  	struct file *f = container_of(head, struct file, f_rcuhead);
>  
>  	put_cred(f->f_cred);
> -	kmem_cache_free(filp_cachep, f);
> +	if (unlikely(f->f_mode & FMODE_BACKING))
> +		kfree(backing_file(f));
> +	else
> +		kmem_cache_free(filp_cachep, f);
>  }
>  
>  static inline void file_free(struct file *f)
>  {
>  	security_file_free(f);
> -	if (!(f->f_mode & FMODE_NOACCOUNT))
> +	if (unlikely(f->f_mode & FMODE_BACKING))
> +		path_put(backing_file_real_path(f));
> +	else
>  		percpu_counter_dec(&nr_files);

I think this needs to be:

if (unlikely(f->f_mode & FMODE_BACKING))
        path_put(backing_file_real_path(f));

if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
        percpu_counter_dec(&nr_files);

as we do have FMODE_NOACCOUNT without FMODE_BACKING.

No need to resend though.

Re: [PATCH v4 1/2] fs: use backing_file container for internal files with "fake" f_path

[Amir Goldstein]

On Wed, Jun 14, 2023 at 4:26 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Wed, Jun 14, 2023 at 10:49:06AM +0300, Amir Goldstein wrote:
> > Overlayfs and cachefiles use open_with_fake_path() to allocate internal
> > files, where overlayfs also puts a "fake" path in f_path - a path which
> > is not on the same fs as f_inode.
> >
> > Allocate a container struct backing_file for those internal files, that
> > is used to hold the "fake" ovl path along with the real path.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/cachefiles/namei.c |  4 +--
> >  fs/file_table.c       | 74 +++++++++++++++++++++++++++++++++++++------
> >  fs/internal.h         |  5 +--
> >  fs/open.c             | 30 +++++++++++-------
> >  fs/overlayfs/file.c   |  4 +--
> >  include/linux/fs.h    | 24 +++++++++++---
> >  6 files changed, 109 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index 82219a8f6084..283534c6bc8d 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -560,8 +560,8 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
> >        */
> >       path.mnt = cache->mnt;
> >       path.dentry = dentry;
> > -     file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> > -                                d_backing_inode(dentry), cache->cache_cred);
> > +     file = open_backing_file(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> > +                              &path, cache->cache_cred);
> >       if (IS_ERR(file)) {
> >               trace_cachefiles_vfs_error(object, d_backing_inode(dentry),
> >                                          PTR_ERR(file),
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 372653b92617..138d5d405df7 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -44,18 +44,40 @@ 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 */
> > +struct backing_file {
> > +     struct file file;
> > +     struct path real_path;
> > +};
> > +
> > +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)
> > +{
> > +     return &backing_file(f)->real_path;
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_real_path);
> > +
> >  static void file_free_rcu(struct rcu_head *head)
> >  {
> >       struct file *f = container_of(head, struct file, f_rcuhead);
> >
> >       put_cred(f->f_cred);
> > -     kmem_cache_free(filp_cachep, f);
> > +     if (unlikely(f->f_mode & FMODE_BACKING))
> > +             kfree(backing_file(f));
> > +     else
> > +             kmem_cache_free(filp_cachep, f);
> >  }
> >
> >  static inline void file_free(struct file *f)
> >  {
> >       security_file_free(f);
> > -     if (!(f->f_mode & FMODE_NOACCOUNT))
> > +     if (unlikely(f->f_mode & FMODE_BACKING))
> > +             path_put(backing_file_real_path(f));
> > +     else
> >               percpu_counter_dec(&nr_files);
>
> I think this needs to be:
>
> if (unlikely(f->f_mode & FMODE_BACKING))
>         path_put(backing_file_real_path(f));
>
> if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
>         percpu_counter_dec(&nr_files);
>
> as we do have FMODE_NOACCOUNT without FMODE_BACKING.
>

Ouch! good catch!

Thanks,
Amir.

Re: [PATCH v4 1/2] fs: use backing_file container for internal files with "fake" f_path

[Christoph Hellwig]

On Wed, Jun 14, 2023 at 10:49:06AM +0300, Amir Goldstein wrote:
> +static struct file *__alloc_file(int flags, const struct cred *cred)
> +{
> +	struct file *f;
> +	int error;
> +
> +	f = kmem_cache_zalloc(filp_cachep, GFP_KERNEL);
> +	if (unlikely(!f))
> +		return ERR_PTR(-ENOMEM);
> +
> +	error = init_file(f, flags, cred);
> +	if (unlikely(error))
> +		return ERR_PTR(error);
> +
>  	return f;

Nit: is there much of a point in keeping this now very trivial helper
instead of open coding it in the two callers?

> +/*
> + * Variant of alloc_empty_file() that allocates a backing_file container
> + * and doesn't check and modify nr_files.
> + *
> + * Should not be used unless there's a very good reason to do so.

I'm not sure this comment is all that helpful..  I'd rather explain
when it should be used (i.e. only from open_backing_file) then when
it should not..

> -struct file *open_with_fake_path(const struct path *path, int flags,
> -				struct inode *inode, const struct cred *cred)
> +struct file *open_backing_file(const struct path *path, int flags,
> +			       const struct path *real_path,
> +			       const struct cred *cred)

Please write a big fat comment on where this function should and should
not be used and how it works.

> +struct path *backing_file_real_path(struct file *f);
> +static inline const struct path *f_real_path(struct file *f)
> +{
> +	if (unlikely(f->f_mode & FMODE_BACKING))
> +		return backing_file_real_path(f);
> +	else
> +		return &f->f_path;
> +}

Nit: no need for the else here.

Re: [PATCH v4 1/2] fs: use backing_file container for internal files with "fake" f_path

[Amir Goldstein]

On Wed, Jun 14, 2023 at 4:26 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Wed, Jun 14, 2023 at 10:49:06AM +0300, Amir Goldstein wrote:
> > Overlayfs and cachefiles use open_with_fake_path() to allocate internal
> > files, where overlayfs also puts a "fake" path in f_path - a path which
> > is not on the same fs as f_inode.
> >
> > Allocate a container struct backing_file for those internal files, that
> > is used to hold the "fake" ovl path along with the real path.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/cachefiles/namei.c |  4 +--
> >  fs/file_table.c       | 74 +++++++++++++++++++++++++++++++++++++------
> >  fs/internal.h         |  5 +--
> >  fs/open.c             | 30 +++++++++++-------
> >  fs/overlayfs/file.c   |  4 +--
> >  include/linux/fs.h    | 24 +++++++++++---
> >  6 files changed, 109 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index 82219a8f6084..283534c6bc8d 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -560,8 +560,8 @@ static bool cachefiles_open_file(struct cachefiles_object *object,
> >        */
> >       path.mnt = cache->mnt;
> >       path.dentry = dentry;
> > -     file = open_with_fake_path(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> > -                                d_backing_inode(dentry), cache->cache_cred);
> > +     file = open_backing_file(&path, O_RDWR | O_LARGEFILE | O_DIRECT,
> > +                              &path, cache->cache_cred);
> >       if (IS_ERR(file)) {
> >               trace_cachefiles_vfs_error(object, d_backing_inode(dentry),
> >                                          PTR_ERR(file),
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 372653b92617..138d5d405df7 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -44,18 +44,40 @@ 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 */
> > +struct backing_file {
> > +     struct file file;
> > +     struct path real_path;
> > +};
> > +
> > +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)
> > +{
> > +     return &backing_file(f)->real_path;
> > +}
> > +EXPORT_SYMBOL_GPL(backing_file_real_path);
> > +
> >  static void file_free_rcu(struct rcu_head *head)
> >  {
> >       struct file *f = container_of(head, struct file, f_rcuhead);
> >
> >       put_cred(f->f_cred);
> > -     kmem_cache_free(filp_cachep, f);
> > +     if (unlikely(f->f_mode & FMODE_BACKING))
> > +             kfree(backing_file(f));
> > +     else
> > +             kmem_cache_free(filp_cachep, f);
> >  }
> >
> >  static inline void file_free(struct file *f)
> >  {
> >       security_file_free(f);
> > -     if (!(f->f_mode & FMODE_NOACCOUNT))
> > +     if (unlikely(f->f_mode & FMODE_BACKING))
> > +             path_put(backing_file_real_path(f));
> > +     else
> >               percpu_counter_dec(&nr_files);
>
> I think this needs to be:
>
> if (unlikely(f->f_mode & FMODE_BACKING))
>         path_put(backing_file_real_path(f));
>
> if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
>         percpu_counter_dec(&nr_files);
>
> as we do have FMODE_NOACCOUNT without FMODE_BACKING.
>
> No need to resend though.

Ay! forgot to include this in v5.
Please fix it for me again.

Thanks,
Amir.