[PATCH 0/6] ovl: convert creation credential override to cred guard

[PATCH 0/6] ovl: convert creation credential override to cred guard

[Christian Brauner]

Hey,

This is on top of the overlayfs cleanup guard work I already sent out.
This cleans up the creation specific credential override.

The current code to override credentials for creation operations is
pretty difficult to understand as we override the credentials twice:

(1) override with the mounter's credentials
(2) copy the mounts credentials and override the fs{g,u}id with the inode {u,g}id

And then we elide the revert_creds() because it would be an idempotent
revert. That elision doesn't buy us anything anymore though because it's
all reference count less anyway.

The fact that this is done in a function and that the revert is
happening in the original override makes this a lot to grasp.

By introducing a cleanup guard for the creation case we can make this a
lot easier to understand and extremely visually prevalent:

with_ovl_creds(dentry->d_sb) {
	scoped_class(prepare_creds_ovl, cred, dentry, inode, mode) {
		if (IS_ERR(cred))
			return PTR_ERR(cred);

		ovl_path_upper(dentry->d_parent, &realparentpath);

		/* more stuff you want to do */
}

I think this is a big improvement over what we have now.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Christian Brauner (6):
      ovl: add prepare_creds_ovl cleanup guard
      ovl: port ovl_create_tmpfile() to new prepare_creds_ovl cleanup guard
      ovl: reflow ovl_create_or_link()
      ovl: mark ovl_setup_cred_for_create() as unused temporarily
      ovl: port ovl_create_or_link() to new prepare_creds_ovl cleanup guard
      ovl: drop ovl_setup_cred_for_create()

 fs/overlayfs/dir.c | 151 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 80 insertions(+), 71 deletions(-)
---
base-commit: b4f90b838f462d46522e17de86431b171937adc2
change-id: 20251114-work-ovl-cred-guard-prepare-53210e7e41f8

[PATCH 1/6] ovl: add prepare_creds_ovl cleanup guard

[Christian Brauner]

The current code to override credentials for creation operations is
pretty difficult to understand. We effectively override the credentials
twice:

(1) override with the mounter's credentials
(2) copy the mounts credentials and override the fs{g,u}id with the inode {u,g}id

And then we elide the revert because it would be an idempotent revert.
That elision doesn't buy us anything anymore though because I've made it
all work without any reference counting anyway. All it does is mix the
two credential overrides together.

We can use a cleanup guard to clarify the creation codepaths and make
them easier to understand.

This just introduces the cleanup guard keeping the patch reviewable.
We'll convert the caller in follow-up patches and then drop the
duplicated code.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 0030f5a69d22..87f6c5ea6ce0 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -575,6 +575,42 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	goto out_dput;
 }
 
+static const struct cred *ovl_prepare_creds(struct dentry *dentry, struct inode *inode, umode_t mode)
+{
+	int err;
+
+	if (WARN_ON_ONCE(current->cred != ovl_creds(dentry->d_sb)))
+		return ERR_PTR(-EINVAL);
+
+	CLASS(prepare_creds, override_cred)();
+	if (!override_cred)
+		return ERR_PTR(-ENOMEM);
+
+	override_cred->fsuid = inode->i_uid;
+	override_cred->fsgid = inode->i_gid;
+
+	err = security_dentry_create_files_as(dentry, mode, &dentry->d_name,
+					      current->cred, override_cred);
+	if (err)
+		return ERR_PTR(err);
+
+	return override_creds(no_free_ptr(override_cred));
+}
+
+static void ovl_revert_creds(const struct cred *old_cred)
+{
+	const struct cred *override_cred;
+
+	override_cred = revert_creds(old_cred);
+	put_cred(override_cred);
+}
+
+DEFINE_CLASS(prepare_creds_ovl,
+	     const struct cred *,
+	     if (!IS_ERR(_T)) ovl_revert_creds(_T),
+	     ovl_prepare_creds(dentry, inode, mode),
+	     struct dentry *dentry, struct inode *inode, umode_t mode)
+
 static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
 						    struct inode *inode,
 						    umode_t mode,

-- 
2.47.3

[PATCH 2/6] ovl: port ovl_create_tmpfile() to new prepare_creds_ovl cleanup guard

[Christian Brauner]

This clearly indicates the double-credential override and makes the code
a lot easier to grasp with one glance.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/dir.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 87f6c5ea6ce0..a276eafb5e78 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -1372,7 +1372,6 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 			      struct inode *inode, umode_t mode)
 {
-	const struct cred *new_cred __free(put_cred) = NULL;
 	struct path realparentpath;
 	struct file *realfile;
 	struct ovl_file *of;
@@ -1381,10 +1380,10 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 	int flags = file->f_flags | OVL_OPEN_FLAGS;
 	int err;
 
-	scoped_class(override_creds_ovl, old_cred, dentry->d_sb) {
-		new_cred = ovl_setup_cred_for_create(dentry, inode, mode, old_cred);
-		if (IS_ERR(new_cred))
-			return PTR_ERR(new_cred);
+	with_ovl_creds(dentry->d_sb) {
+		scoped_class(prepare_creds_ovl, cred, dentry, inode, mode) {
+			if (IS_ERR(cred))
+				return PTR_ERR(cred);
 
 			ovl_path_upper(dentry->d_parent, &realparentpath);
 			realfile = backing_tmpfile_open(&file->f_path, flags,
@@ -1412,6 +1411,7 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 				ovl_file_free(of);
 			}
 		}
+	}
 	return err;
 }
 

-- 
2.47.3

[PATCH 3/6] ovl: reflow ovl_create_or_link()

[Christian Brauner]

Reflow the creation routine in preparation of porting it to a guard.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/dir.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index a276eafb5e78..ff30a91e07f8 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -644,14 +644,23 @@ static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
 	return override_cred;
 }
 
+static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode,
+				 struct ovl_cattr *attr)
+{
+	if (!ovl_dentry_is_whiteout(dentry))
+		return ovl_create_upper(dentry, inode, attr);
+
+	return ovl_create_over_whiteout(dentry, inode, attr);
+}
+
 static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 			      struct ovl_cattr *attr, bool origin)
 {
 	int err;
-	const struct cred *new_cred __free(put_cred) = NULL;
 	struct dentry *parent = dentry->d_parent;
 
 	scoped_class(override_creds_ovl, old_cred, dentry->d_sb) {
+		const struct cred *new_cred __free(put_cred) = NULL;
 		/*
 		 * When linking a file with copy up origin into a new parent, mark the
 		 * new parent dir "impure".
@@ -662,7 +671,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 				return err;
 		}
 
-		if (!attr->hardlink) {
 		/*
 		 * In the creation cases(create, mkdir, mknod, symlink),
 		 * ovl should transfer current's fs{u,g}id to underlying
@@ -676,16 +684,15 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		 * create a new inode, so just use the ovl mounter's
 		 * fs{u,g}id.
 		 */
+
+		if (attr->hardlink)
+			return do_ovl_create_or_link(dentry, inode, attr);
+
 		new_cred = ovl_setup_cred_for_create(dentry, inode, attr->mode, old_cred);
 		if (IS_ERR(new_cred))
 			return PTR_ERR(new_cred);
-		}
-
-		if (!ovl_dentry_is_whiteout(dentry))
-			return ovl_create_upper(dentry, inode, attr);
-
-		return ovl_create_over_whiteout(dentry, inode, attr);
 
+		return do_ovl_create_or_link(dentry, inode, attr);
 	}
 	return err;
 }

-- 
2.47.3

[PATCH 4/6] ovl: mark ovl_setup_cred_for_create() as unused temporarily

[Christian Brauner]

The function will become unused in the next patch.
We'll remove it in later patches to keep the diff legible.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index ff30a91e07f8..f42e1a22bcb8 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -611,7 +611,7 @@ DEFINE_CLASS(prepare_creds_ovl,
 	     ovl_prepare_creds(dentry, inode, mode),
 	     struct dentry *dentry, struct inode *inode, umode_t mode)
 
-static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
+static const __maybe_unused struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
 						    struct inode *inode,
 						    umode_t mode,
 						    const struct cred *old_cred)

-- 
2.47.3

[PATCH 5/6] ovl: port ovl_create_or_link() to new prepare_creds_ovl cleanup guard

[Christian Brauner]

This clearly indicates the double-credential override and makes the code
a lot easier to grasp with one glance.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/dir.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index f42e1a22bcb8..d6a3589c0da7 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -659,8 +659,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 	int err;
 	struct dentry *parent = dentry->d_parent;
 
-	scoped_class(override_creds_ovl, old_cred, dentry->d_sb) {
-		const struct cred *new_cred __free(put_cred) = NULL;
+	with_ovl_creds(dentry->d_sb) {
 		/*
 		 * When linking a file with copy up origin into a new parent, mark the
 		 * new parent dir "impure".
@@ -688,12 +687,12 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		if (attr->hardlink)
 			return do_ovl_create_or_link(dentry, inode, attr);
 
-		new_cred = ovl_setup_cred_for_create(dentry, inode, attr->mode, old_cred);
-		if (IS_ERR(new_cred))
-			return PTR_ERR(new_cred);
-
+		scoped_class(prepare_creds_ovl, cred, dentry, inode, attr->mode) {
+			if (IS_ERR(cred))
+				return PTR_ERR(cred);
 			return do_ovl_create_or_link(dentry, inode, attr);
 		}
+	}
 	return err;
 }
 

-- 
2.47.3

[PATCH 6/6] ovl: drop ovl_setup_cred_for_create()

[Christian Brauner]

It is now unused and can be removed.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/overlayfs/dir.c | 33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index d6a3589c0da7..7d365203dd0e 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -611,39 +611,6 @@ DEFINE_CLASS(prepare_creds_ovl,
 	     ovl_prepare_creds(dentry, inode, mode),
 	     struct dentry *dentry, struct inode *inode, umode_t mode)
 
-static const __maybe_unused struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
-						    struct inode *inode,
-						    umode_t mode,
-						    const struct cred *old_cred)
-{
-	int err;
-	struct cred *override_cred;
-
-	override_cred = prepare_creds();
-	if (!override_cred)
-		return ERR_PTR(-ENOMEM);
-
-	override_cred->fsuid = inode->i_uid;
-	override_cred->fsgid = inode->i_gid;
-	err = security_dentry_create_files_as(dentry, mode, &dentry->d_name,
-					      old_cred, override_cred);
-	if (err) {
-		put_cred(override_cred);
-		return ERR_PTR(err);
-	}
-
-	/*
-	 * Caller is going to match this with revert_creds() and drop
-	 * referenec on the returned creds.
-	 * We must be called with creator creds already, otherwise we risk
-	 * leaking creds.
-	 */
-	old_cred = override_creds(override_cred);
-	WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb));
-
-	return override_cred;
-}
-
 static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 				 struct ovl_cattr *attr)
 {

-- 
2.47.3

Re: [PATCH 3/6] ovl: reflow ovl_create_or_link()

[Amir Goldstein]

On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:
>
> Reflow the creation routine in preparation of porting it to a guard.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/overlayfs/dir.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index a276eafb5e78..ff30a91e07f8 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -644,14 +644,23 @@ static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
>         return override_cred;
>  }
>
> +static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> +                                struct ovl_cattr *attr)

Trying to avert the bikesheding over do_ovl_ helper name...

> +{
> +       if (!ovl_dentry_is_whiteout(dentry))
> +               return ovl_create_upper(dentry, inode, attr);
> +
> +       return ovl_create_over_whiteout(dentry, inode, attr);
> +}
> +
>  static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>                               struct ovl_cattr *attr, bool origin)
>  {
>         int err;
> -       const struct cred *new_cred __free(put_cred) = NULL;
>         struct dentry *parent = dentry->d_parent;
>
>         scoped_class(override_creds_ovl, old_cred, dentry->d_sb) {
> +               const struct cred *new_cred __free(put_cred) = NULL;
>                 /*
>                  * When linking a file with copy up origin into a new parent, mark the
>                  * new parent dir "impure".
> @@ -662,7 +671,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>                                 return err;
>                 }
>
> -               if (!attr->hardlink) {
>                 /*
>                  * In the creation cases(create, mkdir, mknod, symlink),
>                  * ovl should transfer current's fs{u,g}id to underlying
> @@ -676,16 +684,15 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
>                  * create a new inode, so just use the ovl mounter's
>                  * fs{u,g}id.
>                  */
> +
> +               if (attr->hardlink)
> +                       return do_ovl_create_or_link(dentry, inode, attr);
> +

^^^ This looks like an optimization (don't setup cred for hardlink).
Is it really an important optimization that is worth complicating the code flow?
What if we just drop the optimization instead?
Would that creak anything?

Thanks,
Amir.

Re: [PATCH 3/6] ovl: reflow ovl_create_or_link()

[Christian Brauner]

On Fri, Nov 14, 2025 at 12:52:58PM +0100, Amir Goldstein wrote:
> On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > Reflow the creation routine in preparation of porting it to a guard.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  fs/overlayfs/dir.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index a276eafb5e78..ff30a91e07f8 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -644,14 +644,23 @@ static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
> >         return override_cred;
> >  }
> >
> > +static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > +                                struct ovl_cattr *attr)
> 
> Trying to avert the bikesheding over do_ovl_ helper name...
> 
> > +{
> > +       if (!ovl_dentry_is_whiteout(dentry))
> > +               return ovl_create_upper(dentry, inode, attr);
> > +
> > +       return ovl_create_over_whiteout(dentry, inode, attr);
> > +}
> > +
> >  static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> >                               struct ovl_cattr *attr, bool origin)
> >  {
> >         int err;
> > -       const struct cred *new_cred __free(put_cred) = NULL;
> >         struct dentry *parent = dentry->d_parent;
> >
> >         scoped_class(override_creds_ovl, old_cred, dentry->d_sb) {
> > +               const struct cred *new_cred __free(put_cred) = NULL;
> >                 /*
> >                  * When linking a file with copy up origin into a new parent, mark the
> >                  * new parent dir "impure".
> > @@ -662,7 +671,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> >                                 return err;
> >                 }
> >
> > -               if (!attr->hardlink) {
> >                 /*
> >                  * In the creation cases(create, mkdir, mknod, symlink),
> >                  * ovl should transfer current's fs{u,g}id to underlying
> > @@ -676,16 +684,15 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> >                  * create a new inode, so just use the ovl mounter's
> >                  * fs{u,g}id.
> >                  */
> > +
> > +               if (attr->hardlink)
> > +                       return do_ovl_create_or_link(dentry, inode, attr);
> > +
> 
> ^^^ This looks like an optimization (don't setup cred for hardlink).
> Is it really an important optimization that is worth complicating the code flow?

It elides a bunch of allocations and an rcu cycle from put_cred().
So yes, I think it's worth it. You can always remove the special-case
later yourself.

Re: [PATCH 1/6] ovl: add prepare_creds_ovl cleanup guard

[Amir Goldstein]

On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:
>
> The current code to override credentials for creation operations is
> pretty difficult to understand. We effectively override the credentials
> twice:
>
> (1) override with the mounter's credentials
> (2) copy the mounts credentials and override the fs{g,u}id with the inode {u,g}id
>
> And then we elide the revert because it would be an idempotent revert.
> That elision doesn't buy us anything anymore though because I've made it
> all work without any reference counting anyway. All it does is mix the
> two credential overrides together.
>
> We can use a cleanup guard to clarify the creation codepaths and make
> them easier to understand.
>
> This just introduces the cleanup guard keeping the patch reviewable.
> We'll convert the caller in follow-up patches and then drop the
> duplicated code.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 0030f5a69d22..87f6c5ea6ce0 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -575,6 +575,42 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>         goto out_dput;
>  }
>
> +static const struct cred *ovl_prepare_creds(struct dentry *dentry, struct inode *inode, umode_t mode)
> +{
> +       int err;
> +
> +       if (WARN_ON_ONCE(current->cred != ovl_creds(dentry->d_sb)))
> +               return ERR_PTR(-EINVAL);
> +
> +       CLASS(prepare_creds, override_cred)();
> +       if (!override_cred)
> +               return ERR_PTR(-ENOMEM);
> +
> +       override_cred->fsuid = inode->i_uid;
> +       override_cred->fsgid = inode->i_gid;
> +
> +       err = security_dentry_create_files_as(dentry, mode, &dentry->d_name,
> +                                             current->cred, override_cred);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       return override_creds(no_free_ptr(override_cred));
> +}
> +
> +static void ovl_revert_creds(const struct cred *old_cred)
> +{
> +       const struct cred *override_cred;
> +
> +       override_cred = revert_creds(old_cred);
> +       put_cred(override_cred);
> +}
> +

Earlier patch removed a helper by the same name that does not put_cred()
That's a backporting trap.

Maybe something like ovl_revert_create_creds()?

And ovl_prepare_create_creds()?

> +DEFINE_CLASS(prepare_creds_ovl,
> +            const struct cred *,
> +            if (!IS_ERR(_T)) ovl_revert_creds(_T),
> +            ovl_prepare_creds(dentry, inode, mode),
> +            struct dentry *dentry, struct inode *inode, umode_t mode)
> +

Maybe also matching CLASS name.

Thanks,
Amir.

Re: [PATCH 3/6] ovl: reflow ovl_create_or_link()

[Amir Goldstein]

On Fri, Nov 14, 2025 at 1:00 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Fri, Nov 14, 2025 at 12:52:58PM +0100, Amir Goldstein wrote:
> > On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > Reflow the creation routine in preparation of porting it to a guard.
> > >
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > ---
> > >  fs/overlayfs/dir.c | 23 +++++++++++++++--------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > > index a276eafb5e78..ff30a91e07f8 100644
> > > --- a/fs/overlayfs/dir.c
> > > +++ b/fs/overlayfs/dir.c
> > > @@ -644,14 +644,23 @@ static const struct cred *ovl_setup_cred_for_create(struct dentry *dentry,
> > >         return override_cred;
> > >  }
> > >
> > > +static int do_ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > > +                                struct ovl_cattr *attr)
> >
> > Trying to avert the bikesheding over do_ovl_ helper name...
> >
> > > +{
> > > +       if (!ovl_dentry_is_whiteout(dentry))
> > > +               return ovl_create_upper(dentry, inode, attr);
> > > +
> > > +       return ovl_create_over_whiteout(dentry, inode, attr);
> > > +}
> > > +
> > >  static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > >                               struct ovl_cattr *attr, bool origin)
> > >  {
> > >         int err;
> > > -       const struct cred *new_cred __free(put_cred) = NULL;
> > >         struct dentry *parent = dentry->d_parent;
> > >
> > >         scoped_class(override_creds_ovl, old_cred, dentry->d_sb) {
> > > +               const struct cred *new_cred __free(put_cred) = NULL;
> > >                 /*
> > >                  * When linking a file with copy up origin into a new parent, mark the
> > >                  * new parent dir "impure".
> > > @@ -662,7 +671,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > >                                 return err;
> > >                 }
> > >
> > > -               if (!attr->hardlink) {
> > >                 /*
> > >                  * In the creation cases(create, mkdir, mknod, symlink),
> > >                  * ovl should transfer current's fs{u,g}id to underlying
> > > @@ -676,16 +684,15 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> > >                  * create a new inode, so just use the ovl mounter's
> > >                  * fs{u,g}id.
> > >                  */
> > > +
> > > +               if (attr->hardlink)
> > > +                       return do_ovl_create_or_link(dentry, inode, attr);
> > > +
> >
> > ^^^ This looks like an optimization (don't setup cred for hardlink).
> > Is it really an important optimization that is worth complicating the code flow?
>
> It elides a bunch of allocations and an rcu cycle from put_cred().
> So yes, I think it's worth it.

I have no doubt that ovl_setup_cred_for_create() has a price.
The question is whether hardlinking over ovl is an interesting use case
to optimize for.

Miklos? WDYT?

> You can always remove the special-case later yourself.

Sure. Just as we touch and improve the code, it's worth asking those
questions.

Thanks,
Amir.

Re: [PATCH 0/6] ovl: convert creation credential override to cred guard

[Amir Goldstein]

On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:
>
> Hey,
>
> This is on top of the overlayfs cleanup guard work I already sent out.
> This cleans up the creation specific credential override.
>
> The current code to override credentials for creation operations is
> pretty difficult to understand as we override the credentials twice:
>
> (1) override with the mounter's credentials
> (2) copy the mounts credentials and override the fs{g,u}id with the inode {u,g}id
>
> And then we elide the revert_creds() because it would be an idempotent
> revert. That elision doesn't buy us anything anymore though because it's
> all reference count less anyway.
>
> The fact that this is done in a function and that the revert is
> happening in the original override makes this a lot to grasp.
>
> By introducing a cleanup guard for the creation case we can make this a
> lot easier to understand and extremely visually prevalent:
>
> with_ovl_creds(dentry->d_sb) {
>         scoped_class(prepare_creds_ovl, cred, dentry, inode, mode) {
>                 if (IS_ERR(cred))
>                         return PTR_ERR(cred);
>
>                 ovl_path_upper(dentry->d_parent, &realparentpath);
>
>                 /* more stuff you want to do */
> }
>
> I think this is a big improvement over what we have now.
>

I agree!

This bonus cleanup looks very good and helps with hairy parts of the
ovl code.

Overall, apart from the reuse of ovl_revert_creds() helper name,
I had only minor comments about suggestions for
CLASS name and helpers, take it or leave it.

Personally, I think I can leave with the minor confusion of the
static do_ovl_ helpers vs. ovl_do_ helpers, so this one is up to Miklos
to stand ground or not.

After rename of ovl_revert_creds(),
Feel free to add to this series as well:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

This series also passed the ovl sanity tests.

Thanks!
Amir.

Re: [PATCH 1/6] ovl: add prepare_creds_ovl cleanup guard

[Christian Brauner]

On Fri, Nov 14, 2025 at 01:04:22PM +0100, Amir Goldstein wrote:
> On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > The current code to override credentials for creation operations is
> > pretty difficult to understand. We effectively override the credentials
> > twice:
> >
> > (1) override with the mounter's credentials
> > (2) copy the mounts credentials and override the fs{g,u}id with the inode {u,g}id
> >
> > And then we elide the revert because it would be an idempotent revert.
> > That elision doesn't buy us anything anymore though because I've made it
> > all work without any reference counting anyway. All it does is mix the
> > two credential overrides together.
> >
> > We can use a cleanup guard to clarify the creation codepaths and make
> > them easier to understand.
> >
> > This just introduces the cleanup guard keeping the patch reviewable.
> > We'll convert the caller in follow-up patches and then drop the
> > duplicated code.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 0030f5a69d22..87f6c5ea6ce0 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -575,6 +575,42 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
> >         goto out_dput;
> >  }
> >
> > +static const struct cred *ovl_prepare_creds(struct dentry *dentry, struct inode *inode, umode_t mode)
> > +{
> > +       int err;
> > +
> > +       if (WARN_ON_ONCE(current->cred != ovl_creds(dentry->d_sb)))
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       CLASS(prepare_creds, override_cred)();
> > +       if (!override_cred)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       override_cred->fsuid = inode->i_uid;
> > +       override_cred->fsgid = inode->i_gid;
> > +
> > +       err = security_dentry_create_files_as(dentry, mode, &dentry->d_name,
> > +                                             current->cred, override_cred);
> > +       if (err)
> > +               return ERR_PTR(err);
> > +
> > +       return override_creds(no_free_ptr(override_cred));
> > +}
> > +
> > +static void ovl_revert_creds(const struct cred *old_cred)
> > +{
> > +       const struct cred *override_cred;
> > +
> > +       override_cred = revert_creds(old_cred);
> > +       put_cred(override_cred);
> > +}
> > +
> 
> Earlier patch removed a helper by the same name that does not put_cred()
> That's a backporting trap.
> 
> Maybe something like ovl_revert_create_creds()?
> 
> And ovl_prepare_create_creds()?

Ok.

> 
> > +DEFINE_CLASS(prepare_creds_ovl,
> > +            const struct cred *,
> > +            if (!IS_ERR(_T)) ovl_revert_creds(_T),
> > +            ovl_prepare_creds(dentry, inode, mode),
> > +            struct dentry *dentry, struct inode *inode, umode_t mode)
> > +
> 
> Maybe also matching CLASS name.

Ok.

Re: [PATCH 3/6] ovl: reflow ovl_create_or_link()

[Miklos Szeredi]

On Fri, 14 Nov 2025 at 13:07, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Nov 14, 2025 at 1:00 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Fri, Nov 14, 2025 at 12:52:58PM +0100, Amir Goldstein wrote:
> > > On Fri, Nov 14, 2025 at 11:15 AM Christian Brauner <brauner@kernel.org> wrote:

> > > > +
> > > > +               if (attr->hardlink)
> > > > +                       return do_ovl_create_or_link(dentry, inode, attr);
> > > > +
> > >
> > > ^^^ This looks like an optimization (don't setup cred for hardlink).
> > > Is it really an important optimization that is worth complicating the code flow?
> >
> > It elides a bunch of allocations and an rcu cycle from put_cred().
> > So yes, I think it's worth it.
>
> I have no doubt that ovl_setup_cred_for_create() has a price.
> The question is whether hardlinking over ovl is an interesting use case
> to optimize for.
>
> Miklos? WDYT?

Hard link is special cased in several places because it's only
creating the directory entry, but no the inode.  As I see it it's not
about optimization, more about hardlink simply being special.

Thanks,
Miklos