[PATCH 0/2] Decomplicate file_dentry()

[PATCH 0/2] Decomplicate file_dentry()

[Amir Goldstein]

Miklos,

When posting the patches for file_user_path(), I wrote [1]:

"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()."

I was going to follow your suggestion and make this change to ->d_real(),
but a recent discussion on EVM signature verification on overlay files [2]
raised the need to get the real metacopy inode instead of the real data
inode.

So instead of removing the inode argument, I replaced it with a type.
Currently, type can take D_REAL_{DATA,METADATA}, but in the future,
if the need arises, it could grow other types like D_REAL_{UPPER,LOWER}.

Please let me know what you think of the proposed API.

This work is destined to be a pull request to Christian's vfs tree,
unless someone has any objections to this route.

Thanks,
Amir.

[1] https://lore.kernel.org/r/20231009153712.1566422-1-amir73il@gmail.com/
[2] https://lore.kernel.org/r/20240130214620.3155380-5-stefanb@linux.ibm.com/

Amir Goldstein (2):
  fs: make file_dentry() a simple accessor
  fs: remove the inode argument to ->d_real() method

 Documentation/filesystems/locking.rst |  2 +-
 Documentation/filesystems/vfs.rst     | 16 ++++-----
 fs/overlayfs/super.c                  | 52 ++++++++++++---------------
 include/linux/dcache.h                | 18 ++++++----
 include/linux/fs.h                    | 13 ++++++-
 5 files changed, 53 insertions(+), 48 deletions(-)

-- 
2.34.1

[PATCH 1/2] fs: make file_dentry() a simple accessor

[Amir Goldstein]

file_dentry() is a relic from the days that overlayfs was using files with
a "fake" path, meaning, f_path on overlayfs and f_inode on underlying fs.

In those days, file_dentry() was needed to get the underlying fs dentry
that matches f_inode.

Files with "fake" path should not exist nowadays, so make file_dentry() a
simple accessor and use an assertion to make sure that file_dentry() was
not papering over filesystem bugs.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fs.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 023f37c60709..de9aa86d2624 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1078,9 +1078,20 @@ static inline struct inode *file_inode(const struct file *f)
 	return f->f_inode;
 }
 
+/*
+ * file_dentry() is a relic from the days that overlayfs was using files with a
+ * "fake" path, meaning, f_path on overlayfs and f_inode on underlying fs.
+ * In those days, file_dentry() was needed to get the underlying fs dentry that
+ * matches f_inode.
+ * Files with "fake" path should not exist nowadays, so use an assertion to make
+ * sure that file_dentry() was not papering over filesystem bugs.
+ */
 static inline struct dentry *file_dentry(const struct file *file)
 {
-	return d_real(file->f_path.dentry, file_inode(file));
+	struct dentry *dentry = file->f_path.dentry;
+
+	WARN_ON_ONCE(d_inode(dentry) != file_inode(file));
+	return dentry;
 }
 
 struct fasync_struct {
-- 
2.34.1

Re: [PATCH 1/2] fs: make file_dentry() a simple accessor

[Stefan Berger]

On 2/2/24 06:01, Amir Goldstein wrote:
> file_dentry() is a relic from the days that overlayfs was using files with
> a "fake" path, meaning, f_path on overlayfs and f_inode on underlying fs.
> 
> In those days, file_dentry() was needed to get the underlying fs dentry
> that matches f_inode.
> 
> Files with "fake" path should not exist nowadays, so make file_dentry() a
> simple accessor and use an assertion to make sure that file_dentry() was
> not papering over filesystem bugs.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Tested-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   include/linux/fs.h | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 023f37c60709..de9aa86d2624 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1078,9 +1078,20 @@ static inline struct inode *file_inode(const struct file *f)
>   	return f->f_inode;
>   }
>   
> +/*
> + * file_dentry() is a relic from the days that overlayfs was using files with a
> + * "fake" path, meaning, f_path on overlayfs and f_inode on underlying fs.
> + * In those days, file_dentry() was needed to get the underlying fs dentry that
> + * matches f_inode.
> + * Files with "fake" path should not exist nowadays, so use an assertion to make
> + * sure that file_dentry() was not papering over filesystem bugs.
> + */
>   static inline struct dentry *file_dentry(const struct file *file)
>   {
> -	return d_real(file->f_path.dentry, file_inode(file));
> +	struct dentry *dentry = file->f_path.dentry;
> +
> +	WARN_ON_ONCE(d_inode(dentry) != file_inode(file));
> +	return dentry;
>   }
>   
>   struct fasync_struct {

[PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Amir Goldstein]

The only remaining user of ->d_real() method is d_real_inode(), which
passed NULL inode argument to get the real data dentry.

There are no longer any users that call ->d_real() with a non-NULL
inode argument for getting a detry from a specific underlying layer.

Remove the inode argument of the method and replace it with an integer
'type' argument, to allow callers to request the real metadata dentry
instead of the real data dentry.

All the current users of d_real_inode() (e.g. uprobe) continue to get
the real data inode.  Caller that need to get the real metadata inode
(e.g. IMA/EVM) can use d_inode(d_real(dentry, D_REAL_METADATA)).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/locking.rst |  2 +-
 Documentation/filesystems/vfs.rst     | 16 ++++-----
 fs/overlayfs/super.c                  | 52 ++++++++++++---------------
 include/linux/dcache.h                | 18 ++++++----
 4 files changed, 41 insertions(+), 47 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index d5bf4b6b7509..453039a2e49b 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -29,7 +29,7 @@ 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 *, int type);
 
 locking rules:
 
diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
index eebcc0f9e2bc..2a39e718fdf8 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -1264,7 +1264,7 @@ 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 *, int type);
 	};
 
 ``d_revalidate``
@@ -1419,16 +1419,14 @@ defined:
 	the dentry being transited from.
 
 ``d_real``
-	overlay/union type filesystems implement this method to return
-	one of the underlying dentries hidden by the overlay.  It is
-	used in two different modes:
+	overlay/union type filesystems implement this method to return one
+	of the underlying dentries of a regular file hidden by the overlay.
 
-	Called from file_dentry() it returns the real dentry matching
-	the inode argument.  The real dentry may be from a lower layer
-	already copied up, but still referenced from the file.  This
-	mode is selected with a non-NULL inode argument.
+	The 'type' argument takes the values D_REAL_DATA or D_REAL_METADATA
+	for returning the real underlying dentry that refers to the inode
+	hosting the file's data or metadata respectively.
 
-	With NULL inode the topmost real underlying dentry is returned.
+	For non-regular files, the 'dentry' argument is returned.
 
 Each dentry has a pointer to its parent dentry, as well as a hash list
 of child dentries.  Child dentries are basically like files in a
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 2eef6c70b2ae..938852a0a92b 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -28,41 +28,38 @@ MODULE_LICENSE("GPL");
 
 struct ovl_dir_cache;
 
-static struct dentry *ovl_d_real(struct dentry *dentry,
-				 const struct inode *inode)
+static struct dentry *ovl_d_real(struct dentry *dentry, int type)
 {
-	struct dentry *real = NULL, *lower;
+	struct dentry *upper, *lower;
 	int err;
 
-	/*
-	 * 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)
+	switch (type) {
+	case D_REAL_DATA:
+	case D_REAL_METADATA:
+		break;
+	default:
 		goto bug;
+	}
 
 	if (!d_is_reg(dentry)) {
 		/* d_real_inode() is only relevant for regular files */
 		return dentry;
 	}
 
-	real = ovl_dentry_upper(dentry);
-	if (real && (inode == d_inode(real)))
-		return real;
+	upper = ovl_dentry_upper(dentry);
+	if (upper && (type == D_REAL_METADATA ||
+		      ovl_has_upperdata(d_inode(dentry))))
+		return upper;
 
-	if (real && !inode && ovl_has_upperdata(d_inode(dentry)))
-		return real;
+	if (type == D_REAL_METADATA) {
+		lower = ovl_dentry_lower(dentry);
+		goto real_lower;
+	}
 
 	/*
-	 * Best effort lazy lookup of lowerdata for !inode case to return
+	 * Best effort lazy lookup of lowerdata for D_REAL_DATA case to return
 	 * the real lowerdata dentry.  The only current caller of d_real() with
-	 * NULL inode is d_real_inode() from trace_uprobe and this caller is
+	 * D_REAL_DATA is d_real_inode() from trace_uprobe and this caller is
 	 * likely going to be followed reading from the file, before placing
 	 * uprobes on offset within the file, so lowerdata should be available
 	 * when setting the uprobe.
@@ -73,18 +70,13 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	lower = ovl_dentry_lowerdata(dentry);
 	if (!lower)
 		goto bug;
-	real = lower;
 
-	/* Handle recursion */
-	real = d_real(real, inode);
+real_lower:
+	/* Handle recursion into stacked lower fs */
+	return d_real(lower, type);
 
-	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",
-	     inode ? inode->i_ino : 0, real,
-	     real && d_inode(real) ? d_inode(real)->i_ino : 0);
+	WARN(1, "%s(%pd4, %d): real dentry not found\n", __func__, dentry, type);
 	return dentry;
 }
 
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 1666c387861f..019ad02f2b7e 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -139,7 +139,7 @@ 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 *, int type);
 } ____cacheline_aligned;
 
 /*
@@ -543,27 +543,31 @@ static inline struct inode *d_backing_inode(const struct dentry *upper)
 	return inode;
 }
 
+enum d_real_type {
+	D_REAL_DATA,
+	D_REAL_METADATA,
+};
+
 /**
  * d_real - Return the real dentry
  * @dentry: the dentry to query
- * @inode: inode to select the dentry from multiple layers (can be NULL)
+ * @type: the type of real dentry (data or metadata)
  *
  * If dentry is on a union/overlay, then return the underlying, real dentry.
  * Otherwise return the dentry itself.
  *
  * See also: Documentation/filesystems/vfs.rst
  */
-static inline struct dentry *d_real(struct dentry *dentry,
-				    const struct inode *inode)
+static inline struct dentry *d_real(struct dentry *dentry, int type)
 {
 	if (unlikely(dentry->d_flags & DCACHE_OP_REAL))
-		return dentry->d_op->d_real(dentry, inode);
+		return dentry->d_op->d_real(dentry, type);
 	else
 		return dentry;
 }
 
 /**
- * d_real_inode - Return the real inode
+ * d_real_inode - Return the real inode hosting the data
  * @dentry: The dentry to query
  *
  * If dentry is on a union/overlay, then return the underlying, real inode.
@@ -572,7 +576,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, D_REAL_DATA));
 }
 
 struct name_snapshot {
-- 
2.34.1

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Miklos Szeredi]

On Fri, 2 Feb 2024 at 12:01, Amir Goldstein <amir73il@gmail.com> wrote:

> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> index d5bf4b6b7509..453039a2e49b 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -29,7 +29,7 @@ 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 *, int type);

Why not use the specific enum type for the argument?

Thanks,
Miklos

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Amir Goldstein]

On Fri, Feb 2, 2024 at 2:19 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 2 Feb 2024 at 12:01, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> > index d5bf4b6b7509..453039a2e49b 100644
> > --- a/Documentation/filesystems/locking.rst
> > +++ b/Documentation/filesystems/locking.rst
> > @@ -29,7 +29,7 @@ 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 *, int type);
>
> Why not use the specific enum type for the argument?

No reason, we can do enum d_real_type.

Thanks,
Amir.

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Christian Brauner]

On Fri, Feb 02, 2024 at 02:41:16PM +0200, Amir Goldstein wrote:
> On Fri, Feb 2, 2024 at 2:19 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Fri, 2 Feb 2024 at 12:01, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > > diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> > > index d5bf4b6b7509..453039a2e49b 100644
> > > --- a/Documentation/filesystems/locking.rst
> > > +++ b/Documentation/filesystems/locking.rst
> > > @@ -29,7 +29,7 @@ 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 *, int type);
> >
> > Why not use the specific enum type for the argument?
> 
> No reason, we can do enum d_real_type.

Fwiw, I'm happy to just change this. No need to resend as far as I'm concerned.

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Amir Goldstein]

On Fri, Feb 2, 2024 at 3:55 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Fri, Feb 02, 2024 at 02:41:16PM +0200, Amir Goldstein wrote:
> > On Fri, Feb 2, 2024 at 2:19 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Fri, 2 Feb 2024 at 12:01, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > > diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> > > > index d5bf4b6b7509..453039a2e49b 100644
> > > > --- a/Documentation/filesystems/locking.rst
> > > > +++ b/Documentation/filesystems/locking.rst
> > > > @@ -29,7 +29,7 @@ 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 *, int type);
> > >
> > > Why not use the specific enum type for the argument?
> >
> > No reason, we can do enum d_real_type.
>
> Fwiw, I'm happy to just change this. No need to resend as far as I'm concerned.

FWIW, I'd be happy if you do that :)
Note to move enum d_real_type definition above dentry_operations.

Please add Stefan's Tested-by to both patches.

Thanks,
Amir.

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Stefan Berger]

On 2/2/24 06:01, Amir Goldstein wrote:
> The only remaining user of ->d_real() method is d_real_inode(), which
> passed NULL inode argument to get the real data dentry.
> 
> There are no longer any users that call ->d_real() with a non-NULL
> inode argument for getting a detry from a specific underlying layer.
> 
> Remove the inode argument of the method and replace it with an integer
> 'type' argument, to allow callers to request the real metadata dentry
> instead of the real data dentry.
> 
> All the current users of d_real_inode() (e.g. uprobe) continue to get
> the real data inode.  Caller that need to get the real metadata inode
> (e.g. IMA/EVM) can use d_inode(d_real(dentry, D_REAL_METADATA)).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Tested-by: Stefan Berger <stefanb@linux.ibm.com>


> ---
>   Documentation/filesystems/locking.rst |  2 +-
>   Documentation/filesystems/vfs.rst     | 16 ++++-----
>   fs/overlayfs/super.c                  | 52 ++++++++++++---------------
>   include/linux/dcache.h                | 18 ++++++----
>   4 files changed, 41 insertions(+), 47 deletions(-)
> 
> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> index d5bf4b6b7509..453039a2e49b 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -29,7 +29,7 @@ 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 *, int type);
>   
>   locking rules:
>   
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index eebcc0f9e2bc..2a39e718fdf8 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -1264,7 +1264,7 @@ 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 *, int type);
>   	};
>   
>   ``d_revalidate``
> @@ -1419,16 +1419,14 @@ defined:
>   	the dentry being transited from.
>   
>   ``d_real``
> -	overlay/union type filesystems implement this method to return
> -	one of the underlying dentries hidden by the overlay.  It is
> -	used in two different modes:
> +	overlay/union type filesystems implement this method to return one
> +	of the underlying dentries of a regular file hidden by the overlay.
>   
> -	Called from file_dentry() it returns the real dentry matching
> -	the inode argument.  The real dentry may be from a lower layer
> -	already copied up, but still referenced from the file.  This
> -	mode is selected with a non-NULL inode argument.
> +	The 'type' argument takes the values D_REAL_DATA or D_REAL_METADATA
> +	for returning the real underlying dentry that refers to the inode
> +	hosting the file's data or metadata respectively.
>   
> -	With NULL inode the topmost real underlying dentry is returned.
> +	For non-regular files, the 'dentry' argument is returned.
>   
>   Each dentry has a pointer to its parent dentry, as well as a hash list
>   of child dentries.  Child dentries are basically like files in a
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 2eef6c70b2ae..938852a0a92b 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -28,41 +28,38 @@ MODULE_LICENSE("GPL");
>   
>   struct ovl_dir_cache;
>   
> -static struct dentry *ovl_d_real(struct dentry *dentry,
> -				 const struct inode *inode)
> +static struct dentry *ovl_d_real(struct dentry *dentry, int type)
>   {
> -	struct dentry *real = NULL, *lower;
> +	struct dentry *upper, *lower;
>   	int err;
>   
> -	/*
> -	 * 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)
> +	switch (type) {
> +	case D_REAL_DATA:
> +	case D_REAL_METADATA:
> +		break;
> +	default:
>   		goto bug;
> +	}
>   
>   	if (!d_is_reg(dentry)) {
>   		/* d_real_inode() is only relevant for regular files */
>   		return dentry;
>   	}
>   
> -	real = ovl_dentry_upper(dentry);
> -	if (real && (inode == d_inode(real)))
> -		return real;
> +	upper = ovl_dentry_upper(dentry);
> +	if (upper && (type == D_REAL_METADATA ||
> +		      ovl_has_upperdata(d_inode(dentry))))
> +		return upper;
>   
> -	if (real && !inode && ovl_has_upperdata(d_inode(dentry)))
> -		return real;
> +	if (type == D_REAL_METADATA) {
> +		lower = ovl_dentry_lower(dentry);
> +		goto real_lower;
> +	}
>   
>   	/*
> -	 * Best effort lazy lookup of lowerdata for !inode case to return
> +	 * Best effort lazy lookup of lowerdata for D_REAL_DATA case to return
>   	 * the real lowerdata dentry.  The only current caller of d_real() with
> -	 * NULL inode is d_real_inode() from trace_uprobe and this caller is
> +	 * D_REAL_DATA is d_real_inode() from trace_uprobe and this caller is
>   	 * likely going to be followed reading from the file, before placing
>   	 * uprobes on offset within the file, so lowerdata should be available
>   	 * when setting the uprobe.
> @@ -73,18 +70,13 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>   	lower = ovl_dentry_lowerdata(dentry);
>   	if (!lower)
>   		goto bug;
> -	real = lower;
>   
> -	/* Handle recursion */
> -	real = d_real(real, inode);
> +real_lower:
> +	/* Handle recursion into stacked lower fs */
> +	return d_real(lower, type);
>   
> -	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",
> -	     inode ? inode->i_ino : 0, real,
> -	     real && d_inode(real) ? d_inode(real)->i_ino : 0);
> +	WARN(1, "%s(%pd4, %d): real dentry not found\n", __func__, dentry, type);
>   	return dentry;
>   }
>   
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 1666c387861f..019ad02f2b7e 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -139,7 +139,7 @@ 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 *, int type);
>   } ____cacheline_aligned;
>   
>   /*
> @@ -543,27 +543,31 @@ static inline struct inode *d_backing_inode(const struct dentry *upper)
>   	return inode;
>   }
>   
> +enum d_real_type {
> +	D_REAL_DATA,
> +	D_REAL_METADATA,
> +};
> +
>   /**
>    * d_real - Return the real dentry
>    * @dentry: the dentry to query
> - * @inode: inode to select the dentry from multiple layers (can be NULL)
> + * @type: the type of real dentry (data or metadata)
>    *
>    * If dentry is on a union/overlay, then return the underlying, real dentry.
>    * Otherwise return the dentry itself.
>    *
>    * See also: Documentation/filesystems/vfs.rst
>    */
> -static inline struct dentry *d_real(struct dentry *dentry,
> -				    const struct inode *inode)
> +static inline struct dentry *d_real(struct dentry *dentry, int type)
>   {
>   	if (unlikely(dentry->d_flags & DCACHE_OP_REAL))
> -		return dentry->d_op->d_real(dentry, inode);
> +		return dentry->d_op->d_real(dentry, type);
>   	else
>   		return dentry;
>   }
>   
>   /**
> - * d_real_inode - Return the real inode
> + * d_real_inode - Return the real inode hosting the data
>    * @dentry: The dentry to query
>    *
>    * If dentry is on a union/overlay, then return the underlying, real inode.
> @@ -572,7 +576,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, D_REAL_DATA));
>   }
>   
>   struct name_snapshot {

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Al Viro]

On Fri, Feb 02, 2024 at 01:01:32PM +0200, Amir Goldstein wrote:
> The only remaining user of ->d_real() method is d_real_inode(), which
> passed NULL inode argument to get the real data dentry.
> 
> There are no longer any users that call ->d_real() with a non-NULL
> inode argument for getting a detry from a specific underlying layer.
> 
> Remove the inode argument of the method and replace it with an integer
> 'type' argument, to allow callers to request the real metadata dentry
> instead of the real data dentry.
> 
> All the current users of d_real_inode() (e.g. uprobe) continue to get
> the real data inode.  Caller that need to get the real metadata inode
> (e.g. IMA/EVM) can use d_inode(d_real(dentry, D_REAL_METADATA)).

Hmm...  Speaking of the callers, could somebody try explain to IMA
folks that they _still_ have a blatant UAF in ima_collect_measurement()?
I gave up after several attempts years ago...

int ima_collect_measurement(struct integrity_iint_cache *iint,
                            struct file *file, void *buf, loff_t size,
                            enum hash_algo algo, struct modsig *modsig)
{
        const char *audit_cause = "failed";
        struct inode *inode = file_inode(file);
        struct inode *real_inode = d_real_inode(file_dentry(file));
        const char *filename = file->f_path.dentry->d_name.name;

The name is longer than 40 characters, and thus separately allocated.

	...
Somebody renames the file, now the name is short and ->d_name.name points to
embedded array.  The reference to external name is dropped and it's freed
after an RCU delay.
	...
        tmpbuf = krealloc(iint->ima_hash, length, GFP_NOFS);
We block, RCU delay expires and filename points to freed memory object.
	...

                integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
                                    filename, "collect_data", audit_cause,
                                    result, 0);

Which calls integrity_audit_message(), where we hit
                audit_log_untrustedstring(ab, fname);
with fname being our dangling pointer.

Use After Free.  Really.  And "untrusted" in the function name does not
refer to "it might be pointing to unmapped page" - it's just "don't
expect anything from the characters you might find there, including
the presence of NUL".

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Al Viro]

On Fri, Feb 02, 2024 at 04:05:09PM +0000, Al Viro wrote:

> Use After Free.  Really.  And "untrusted" in the function name does not
> refer to "it might be pointing to unmapped page" - it's just "don't
> expect anything from the characters you might find there, including
> the presence of NUL".

Argh...  s/including/beyond the/ - sorry.  Messed up rewriting the
sentence.

"Untrusted" refers to the lack of whitespaces, control characters, '"',
etc.  What audit_log_untrustedstring(ab, string) expects is
	* string pointing to readable memory object
	* the object remaining unchanged through the call
	* NUL existing somewhere in that object.

All of those assertions can be violated once the object string
used to point to has been passed to kmem_cache_free().  Which is what
can very well happen to filename pointer in this case.

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Stefan Berger]

On 2/2/24 11:16, Al Viro wrote:
> On Fri, Feb 02, 2024 at 04:05:09PM +0000, Al Viro wrote:
> 
>> Use After Free.  Really.  And "untrusted" in the function name does not
>> refer to "it might be pointing to unmapped page" - it's just "don't
>> expect anything from the characters you might find there, including
>> the presence of NUL".
> 
> Argh...  s/including/beyond the/ - sorry.  Messed up rewriting the
> sentence.
> 
> "Untrusted" refers to the lack of whitespaces, control characters, '"',
> etc.  What audit_log_untrustedstring(ab, string) expects is
> 	* string pointing to readable memory object
> 	* the object remaining unchanged through the call
> 	* NUL existing somewhere in that object.
> 
> All of those assertions can be violated once the object string
> used to point to has been passed to kmem_cache_free().  Which is what
> can very well happen to filename pointer in this case.

I suppose this would provide a stable name?

diff --git a/security/integrity/ima/ima_api.c 
b/security/integrity/ima/ima_api.c
index 597ea0c4d72f..48ae6911139b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -244,7 +244,6 @@ int ima_collect_measurement(struct 
integrity_iint_cache *iint,
         const char *audit_cause = "failed";
         struct inode *inode = file_inode(file);
         struct inode *real_inode = d_real_inode(file_dentry(file));
-       const char *filename = file->f_path.dentry->d_name.name;
         struct ima_max_digest_data hash;
         struct kstat stat;
         int result = 0;
@@ -313,11 +312,17 @@ int ima_collect_measurement(struct 
integrity_iint_cache *iint,
                 iint->flags |= IMA_COLLECTED;
  out:
         if (result) {
+               struct qstr *qstr = &file->f_path.dentry->d_name;
+               char buf[NAME_MAX + 1];
+
                 if (file->f_flags & O_DIRECT)
                         audit_cause = "failed(directio)";

+               memcpy(buf, qstr->name, qstr->len);
+               buf[qstr->len] = 0;
+
                 integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
-                                   filename, "collect_data", audit_cause,
+                                   buf, "collect_data", audit_cause,
                                     result, 0);
         }
         return result;


Regards,
    Stefan

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Al Viro]

On Fri, Feb 02, 2024 at 12:34:15PM -0500, Stefan Berger wrote:

> I suppose this would provide a stable name?
> 
> diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> index 597ea0c4d72f..48ae6911139b 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -244,7 +244,6 @@ int ima_collect_measurement(struct integrity_iint_cache
> *iint,
>         const char *audit_cause = "failed";
>         struct inode *inode = file_inode(file);
>         struct inode *real_inode = d_real_inode(file_dentry(file));
> -       const char *filename = file->f_path.dentry->d_name.name;
>         struct ima_max_digest_data hash;
>         struct kstat stat;
>         int result = 0;
> @@ -313,11 +312,17 @@ int ima_collect_measurement(struct
> integrity_iint_cache *iint,
>                 iint->flags |= IMA_COLLECTED;
>  out:
>         if (result) {
> +               struct qstr *qstr = &file->f_path.dentry->d_name;
> +               char buf[NAME_MAX + 1];
> +
>                 if (file->f_flags & O_DIRECT)
>                         audit_cause = "failed(directio)";
> 
> +               memcpy(buf, qstr->name, qstr->len);
> +               buf[qstr->len] = 0;

	Think what happens if you fetch ->len in state prior to
rename and ->name - after.  memcpy() from one memory object
with length that matches another, UAF right there.

	If you want to take a snapshot of the name, just use
take_dentry_name_snapshot() - that will copy name if it's
short or grab a reference to external name if the length is
>= 40, all of that under ->d_lock to stabilize things.

	Paired with release_dentry_name_snapshot(), to
drop the reference to external name if one had been taken.
No need to copy in long case (external names are never
rewritten) and it's kinder on the stack footprint that
way (56 bytes vs. 256).

	Something like this (completely untested):

fix a UAF in ima_collect_measurement()

->d_name.name can change on rename and the earlier value can get freed;
there are conditions sufficient to stabilize it (->d_lock on denty,
->d_lock on its parent, ->i_rwsem exclusive on the parent's inode,
rename_lock), but none of those are met in ima_collect_measurement().
Take a stable snapshot of name instead.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 597ea0c4d72f..d8be2280d97b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -244,7 +244,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 	const char *audit_cause = "failed";
 	struct inode *inode = file_inode(file);
 	struct inode *real_inode = d_real_inode(file_dentry(file));
-	const char *filename = file->f_path.dentry->d_name.name;
 	struct ima_max_digest_data hash;
 	struct kstat stat;
 	int result = 0;
@@ -313,12 +312,16 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 		iint->flags |= IMA_COLLECTED;
 out:
 	if (result) {
+		struct name_snapshot filename;
+
+		take_dentry_name_snapshot(&filename, file->f_path.dentry);
 		if (file->f_flags & O_DIRECT)
 			audit_cause = "failed(directio)";
 
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
-				    filename, "collect_data", audit_cause,
-				    result, 0);
+				    filename.name.name, "collect_data",
+				    audit_cause, result, 0);
+		release_dentry_name_snapshot(&filename);
 	}
 	return result;
 }

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Stefan Berger]

On 2/2/24 13:27, Al Viro wrote:
> On Fri, Feb 02, 2024 at 12:34:15PM -0500, Stefan Berger wrote:
> 
>> I suppose this would provide a stable name?
>>
>> diff --git a/security/integrity/ima/ima_api.c
>> b/security/integrity/ima/ima_api.c
>> index 597ea0c4d72f..48ae6911139b 100644
>> --- a/security/integrity/ima/ima_api.c
>> +++ b/security/integrity/ima/ima_api.c
>> @@ -244,7 +244,6 @@ int ima_collect_measurement(struct integrity_iint_cache
>> *iint,
>>          const char *audit_cause = "failed";
>>          struct inode *inode = file_inode(file);
>>          struct inode *real_inode = d_real_inode(file_dentry(file));
>> -       const char *filename = file->f_path.dentry->d_name.name;
>>          struct ima_max_digest_data hash;
>>          struct kstat stat;
>>          int result = 0;
>> @@ -313,11 +312,17 @@ int ima_collect_measurement(struct
>> integrity_iint_cache *iint,
>>                  iint->flags |= IMA_COLLECTED;
>>   out:
>>          if (result) {
>> +               struct qstr *qstr = &file->f_path.dentry->d_name;
>> +               char buf[NAME_MAX + 1];
>> +
>>                  if (file->f_flags & O_DIRECT)
>>                          audit_cause = "failed(directio)";
>>
>> +               memcpy(buf, qstr->name, qstr->len);
>> +               buf[qstr->len] = 0;
> 
> 	Think what happens if you fetch ->len in state prior to
> rename and ->name - after.  memcpy() from one memory object
> with length that matches another, UAF right there.
> 
> 	If you want to take a snapshot of the name, just use
> take_dentry_name_snapshot() - that will copy name if it's
> short or grab a reference to external name if the length is
>> = 40, all of that under ->d_lock to stabilize things.

I had exactly what you show below (inspired by overlayfs) but then 
looked around whether there's another way of doing it and I saw copies 
being made.

Thanks.


> 
> 	Paired with release_dentry_name_snapshot(), to
> drop the reference to external name if one had been taken.
> No need to copy in long case (external names are never
> rewritten) and it's kinder on the stack footprint that
> way (56 bytes vs. 256).
> 
> 	Something like this (completely untested):
> 
> fix a UAF in ima_collect_measurement()
> 
> ->d_name.name can change on rename and the earlier value can get freed;
> there are conditions sufficient to stabilize it (->d_lock on denty,
> ->d_lock on its parent, ->i_rwsem exclusive on the parent's inode,
> rename_lock), but none of those are met in ima_collect_measurement().
> Take a stable snapshot of name instead.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 597ea0c4d72f..d8be2280d97b 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -244,7 +244,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   	const char *audit_cause = "failed";
>   	struct inode *inode = file_inode(file);
>   	struct inode *real_inode = d_real_inode(file_dentry(file));
> -	const char *filename = file->f_path.dentry->d_name.name;
>   	struct ima_max_digest_data hash;
>   	struct kstat stat;
>   	int result = 0;
> @@ -313,12 +312,16 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   		iint->flags |= IMA_COLLECTED;
>   out:
>   	if (result) {
> +		struct name_snapshot filename;
> +
> +		take_dentry_name_snapshot(&filename, file->f_path.dentry);
>   		if (file->f_flags & O_DIRECT)
>   			audit_cause = "failed(directio)";
>   
>   		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,
> -				    filename, "collect_data", audit_cause,
> -				    result, 0);
> +				    filename.name.name, "collect_data",
> +				    audit_cause, result, 0);
> +		release_dentry_name_snapshot(&filename);
>   	}
>   	return result;
>   }

I can take it from here unless you want to formally post it.

    Stefan

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Al Viro]

On Fri, Feb 02, 2024 at 01:32:39PM -0500, Stefan Berger wrote:

> I can take it from here unless you want to formally post it.

Less headache for me that way, but you want to test it - all
I've checked is that the damn thing compiles, and while it's
hard to fuck up and I don't see any brainos in there...
I'm way too low on caffeine at the moment.  It should be
safe from races, just verifying that it prints the right thing
when you hit that codepath would be enough.

Re: [PATCH 2/2] fs: remove the inode argument to ->d_real() method

[Al Viro]

On Fri, Feb 02, 2024 at 06:27:32PM +0000, Al Viro wrote:

> 	Think what happens if you fetch ->len in state prior to
> rename and ->name - after.  memcpy() from one memory object
> with length that matches another, UAF right there.

	s/UAF/fairly easy oops/ - you can end up fetching past the end of
page that hosts kmalloc'ed object, and there's no promise that anything
will be mapped there.  I really need more coffee...

Re: [PATCH 0/2] Decomplicate file_dentry()

[Christian Brauner]

On Fri, 02 Feb 2024 13:01:30 +0200, Amir Goldstein wrote:
> Miklos,
> 
> When posting the patches for file_user_path(), I wrote [1]:
> 
> "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.
> 
> [...]

Applied. Let me know in case this needs to go somewhere else.
Added the enum change as discussed. Please double check ofc.

---

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/2] fs: make file_dentry() a simple accessor
      https://git.kernel.org/vfs/vfs/c/c6c14f926fbe
[2/2] fs: remove the inode argument to ->d_real() method
      https://git.kernel.org/vfs/vfs/c/2109cc619e73