[PATCH v2 00/16] overlayfs: Optimize override/revert creds

[PATCH v2 00/16] overlayfs: Optimize override/revert creds

[Vinicius Costa Gomes]

Hi,

Changes from v1:
 - Re-organized the series to be easier to follow, more details below
   (Miklos Szeredi and Amir Goldstein);

The series now reads as follows:

Patch 1: Introduce the _light() version of the override/revert cred operations;
Patch 2: Convert backing-file.c to use those;
Patch 3: Introduce the overlayfs specific _light() helper;
Patch 4: Document the cases that the helper cannot be used (critical
      section may change the cred->usage counter);
Patch 5: Convert the "rest" of overlayfs to the _light() helpers (mostly mechanical);
Patch 6: Introduce the GUARD() helpers;
Patch 7: Convert backing-file.c to the GUARD() helpers;
Patch 8-15: Convert each overlayfs/ file to use the GUARD() helpers,
      also explain the cases in which the scoped_guard() helper is
      used. Note that a 'goto' jump that crosses the guard() should
      fail to compile, gcc has a bug that fails to detect the
      error[1].
Patch 16: Remove the helper introduced in Patch 3 to close the series,
      as it is no longer used, everything was converted to use the
      safer/shorter GUARD() helpers.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951

This bug was also noticed here:

https://lore.kernel.org/all/20240730050927.GC5334@ZenIV/

Link to v1:

https://lore.kernel.org/r/20240403021808.309900-1-vinicius.gomes@intel.com/

Changes from RFC v3:
 - Removed the warning "fixes" patches, as they could hide potencial
   bugs (Christian Brauner);
 - Added "cred-specific" macros (Christian Brauner), from my side,
   added a few '_' to the guards to signify that the newly introduced
   helper macros are preferred.
 - Changed a few guard() to scoped_guard() to fix the clang (17.0.6)
   compilation error about 'goto' bypassing variable initialization;

Link to RFC v3:

https://lore.kernel.org/r/20240216051640.197378-1-vinicius.gomes@intel.com/

Changes from RFC v2:
 - Added separate patches for the warnings for the discarded const
   when using the cleanup macros: one for DEFINE_GUARD() and one for
   DEFINE_LOCK_GUARD_1() (I am uncertain if it's better to squash them
   together);
 - Reordered the series so the backing file patch is the first user of
   the introduced helpers (Amir Goldstein);
 - Change the definition of the cleanup "class" from a GUARD to a
   LOCK_GUARD_1, which defines an implicit container, that allows us
   to remove some variable declarations to store the overriden
   credentials (Amir Goldstein);
 - Replaced most of the uses of scoped_guard() with guard(), to reduce
   the code churn, the remaining ones I wasn't sure if I was changing
   the behavior: either they were nested (overrides "inside"
   overrides) or something calls current_cred() (Amir Goldstein).

New questions:
 - The backing file callbacks are now called with the "light"
   overriden credentials, so they are kind of restricted in what they
   can do with their credentials, is this acceptable in general?
 - in ovl_rename() I had to manually call the "light" the overrides,
   both using the guard() macro or using the non-light version causes
   the workload to crash the kernel. I still have to investigate why
   this is happening. Hints are appreciated.

Link to the RFC v2:

https://lore.kernel.org/r/20240125235723.39507-1-vinicius.gomes@intel.com/

Original cover letter (lightly edited):

It was noticed that some workloads suffer from contention on
increasing/decrementing the ->usage counter in their credentials,
those refcount operations are associated with overriding/reverting the
current task credentials. (the linked thread adds more context)

In some specialized cases, overlayfs is one of them, the credentials
in question have a longer lifetime than the override/revert "critical
section". In the overlayfs case, the credentials are created when the
fs is mounted and destroyed when it's unmounted. In this case of long
lived credentials, the usage counter doesn't need to be
incremented/decremented.

Add a lighter version of credentials override/revert to be used in
these specialized cases. To make sure that the override/revert calls
are paired, add a cleanup guard macro. This was suggested here:

https://lore.kernel.org/all/20231219-marken-pochen-26d888fb9bb9@brauner/

With a small number of tweaks:
 - Used inline functions instead of macros;
 - A small change to store the credentials into the passed argument,
   the guard is now defined as (note the added '_T ='):

      DEFINE_GUARD(cred, const struct cred *, _T = override_creds_light(_T),
		  revert_creds_light(_T));

 - Allow "const" arguments to be used with these kind of guards;

Some comments:
 - If patch 1/5 and 2/5 are not a good idea (adding the cast), the
   alternative I can see is using some kind of container for the
   credentials;
 - The only user for the backing file ops is overlayfs, so these
   changes make sense, but may not make sense in the most general
   case;

For the numbers, some from 'perf c2c', before this series:
(edited to fit)

#
#        ----- HITM -----                                        Shared                          
#   Num  RmtHitm  LclHitm                      Symbol            Object         Source:Line  Node
# .....  .......  .......  ..........................  ................  ..................  ....
#
  -------------------------
      0      412     1028  
  -------------------------
	  41.50%   42.22%  [k] revert_creds            [kernel.vmlinux]  atomic64_64.h:39     0  1
	  15.05%   10.60%  [k] override_creds          [kernel.vmlinux]  atomic64_64.h:25     0  1
	   0.73%    0.58%  [k] init_file               [kernel.vmlinux]  atomic64_64.h:25     0  1
	   0.24%    0.10%  [k] revert_creds            [kernel.vmlinux]  cred.h:266           0  1
	  32.28%   37.16%  [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
	   9.47%    8.75%  [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
	   0.49%    0.58%  [k] inode_owner_or_capable  [kernel.vmlinux]  mnt_idmapping.h:81   0  1
	   0.24%    0.00%  [k] generic_permission      [kernel.vmlinux]  namei.c:354          0

  -------------------------
      1       50      103  
  -------------------------
	 100.00%  100.00%  [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1

  -------------------------
      2       50       98  
  -------------------------
	  96.00%   96.94%  [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
	   2.00%    1.02%  [k] update_load_avg   [kernel.vmlinux]  atomic64_64.h:25   0  1
	   0.00%    2.04%  [k] update_load_avg   [kernel.vmlinux]  fair.c:4118        0
	   2.00%    0.00%  [k] update_cfs_group  [kernel.vmlinux]  fair.c:3932        0  1

after this series:

#
#        ----- HITM -----                                   Shared                        
#   Num  RmtHitm  LclHitm                 Symbol            Object       Source:Line  Node
# .....  .......  .......   ....................  ................  ................  ....
#
  -------------------------
      0       54       88  
  -------------------------
	 100.00%  100.00%   [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1

  -------------------------
      1       48       83  
  -------------------------
	  97.92%   97.59%   [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
	   2.08%    1.20%   [k] update_load_avg   [kernel.vmlinux]  atomic64_64.h:25   0  1
	   0.00%    1.20%   [k] update_load_avg   [kernel.vmlinux]  fair.c:4118        0  1

  -------------------------
      2       28       44  
  -------------------------
	  85.71%   79.55%   [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
	  14.29%   20.45%   [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1

The contention is practically gone.

Link: https://lore.kernel.org/all/20231018074553.41333-1-hu1.chen@intel.com/

Vinicius Costa Gomes (16):
  cred: Add a light version of override/revert_creds()
  fs/backing-file: Convert to revert/override_creds_light()
  fs/overlayfs: Introduce ovl_override_creds_light()
  overlayfs: Document critical override_creds() operations
  overlayfs: Use ovl_override_creds_light()/revert_creds_light()
  cred: Introduce cred_guard() and cred_scoped_guard() helpers
  fs/backing-file: Convert to cred_guard()
  overlayfs/copy_up: Convert to cred_guard()
  overlayfs/dir: Convert to cred_guard()
  overlayfs/file: Convert to cred_guard()
  overlayfs/inode: Convert to cred_guard()
  overlayfs/namei: Convert to cred_guard()
  overlayfs/readdir: Convert to cred_guard()
  overlayfs/xattrs: Convert to cred_guard()
  overlayfs/util: Convert to cred_guard()
  overlayfs: Remove ovl_override_creds_light()

 fs/backing-file.c      | 22 +++----------
 fs/overlayfs/copy_up.c | 10 +++---
 fs/overlayfs/dir.c     | 20 ++++++------
 fs/overlayfs/file.c    | 64 +++++++++++++++---------------------
 fs/overlayfs/inode.c   | 73 ++++++++++++++++--------------------------
 fs/overlayfs/namei.c   | 20 +++---------
 fs/overlayfs/readdir.c | 16 +++------
 fs/overlayfs/util.c    | 24 ++++++--------
 fs/overlayfs/xattrs.c  | 32 ++++++++----------
 include/linux/cred.h   | 25 +++++++++++++++
 kernel/cred.c          |  6 ++--
 11 files changed, 134 insertions(+), 178 deletions(-)

-- 
2.46.0

[PATCH v2 01/16] cred: Add a light version of override/revert_creds()

[Vinicius Costa Gomes]

Add a light version of override/revert_creds(), this should only be
used when the credentials in question will outlive the critical
section and the critical section doesn't change the ->usage of the
credentials.

Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 include/linux/cred.h | 18 ++++++++++++++++++
 kernel/cred.c        |  6 +++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/linux/cred.h b/include/linux/cred.h
index 2976f534a7a3..e4a3155fe409 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -172,6 +172,24 @@ static inline bool cap_ambient_invariant_ok(const struct cred *cred)
 					  cred->cap_inheritable));
 }
 
+/*
+ * Override creds without bumping reference count. Caller must ensure
+ * reference remains valid or has taken reference. Almost always not the
+ * interface you want. Use override_creds()/revert_creds() instead.
+ */
+static inline const struct cred *override_creds_light(const struct cred *override_cred)
+{
+	const struct cred *old = current->cred;
+
+	rcu_assign_pointer(current->cred, override_cred);
+	return old;
+}
+
+static inline void revert_creds_light(const struct cred *revert_cred)
+{
+	rcu_assign_pointer(current->cred, revert_cred);
+}
+
 /**
  * get_new_cred_many - Get references on a new set of credentials
  * @cred: The new credentials to reference
diff --git a/kernel/cred.c b/kernel/cred.c
index 075cfa7c896f..da7da250f7c8 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -485,7 +485,7 @@ EXPORT_SYMBOL(abort_creds);
  */
 const struct cred *override_creds(const struct cred *new)
 {
-	const struct cred *old = current->cred;
+	const struct cred *old;
 
 	kdebug("override_creds(%p{%ld})", new,
 	       atomic_long_read(&new->usage));
@@ -499,7 +499,7 @@ const struct cred *override_creds(const struct cred *new)
 	 * visible to other threads under RCU.
 	 */
 	get_new_cred((struct cred *)new);
-	rcu_assign_pointer(current->cred, new);
+	old = override_creds_light(new);
 
 	kdebug("override_creds() = %p{%ld}", old,
 	       atomic_long_read(&old->usage));
@@ -521,7 +521,7 @@ void revert_creds(const struct cred *old)
 	kdebug("revert_creds(%p{%ld})", old,
 	       atomic_long_read(&old->usage));
 
-	rcu_assign_pointer(current->cred, old);
+	revert_creds_light(old);
 	put_cred(override);
 }
 EXPORT_SYMBOL(revert_creds);
-- 
2.46.0

[PATCH v2 02/16] fs/backing-file: Convert to revert/override_creds_light()

[Vinicius Costa Gomes]

As the credentials used by backing-file are long lived in relation to
the critical section (override_creds() -> revert_creds()) we can
replace them by their lighter alternatives.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/backing-file.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/backing-file.c b/fs/backing-file.c
index afb557446c27..bc19e8e28e58 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -176,7 +176,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
 	    !(file->f_mode & FMODE_CAN_ODIRECT))
 		return -EINVAL;
 
-	old_cred = override_creds(ctx->cred);
+	old_cred = override_creds_light(ctx->cred);
 	if (is_sync_kiocb(iocb)) {
 		rwf_t rwf = iocb_to_rw_flags(flags);
 
@@ -197,7 +197,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
 			backing_aio_cleanup(aio, ret);
 	}
 out:
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	if (ctx->accessed)
 		ctx->accessed(ctx->user_file);
@@ -233,7 +233,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 	 */
 	flags &= ~IOCB_DIO_CALLER_COMP;
 
-	old_cred = override_creds(ctx->cred);
+	old_cred = override_creds_light(ctx->cred);
 	if (is_sync_kiocb(iocb)) {
 		rwf_t rwf = iocb_to_rw_flags(flags);
 
@@ -264,7 +264,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 			backing_aio_cleanup(aio, ret);
 	}
 out:
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return ret;
 }
@@ -281,9 +281,9 @@ ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
 	if (WARN_ON_ONCE(!(in->f_mode & FMODE_BACKING)))
 		return -EIO;
 
-	old_cred = override_creds(ctx->cred);
+	old_cred = override_creds_light(ctx->cred);
 	ret = vfs_splice_read(in, ppos, pipe, len, flags);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	if (ctx->accessed)
 		ctx->accessed(ctx->user_file);
@@ -307,11 +307,11 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
 	if (ret)
 		return ret;
 
-	old_cred = override_creds(ctx->cred);
+	old_cred = override_creds_light(ctx->cred);
 	file_start_write(out);
 	ret = iter_file_splice_write(pipe, out, ppos, len, flags);
 	file_end_write(out);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	if (ctx->end_write)
 		ctx->end_write(ctx->user_file);
@@ -335,9 +335,9 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
 
 	vma_set_file(vma, file);
 
-	old_cred = override_creds(ctx->cred);
+	old_cred = override_creds_light(ctx->cred);
 	ret = call_mmap(vma->vm_file, vma);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	if (ctx->accessed)
 		ctx->accessed(ctx->user_file);
-- 
2.46.0

[PATCH v2 03/16] fs/overlayfs: Introduce ovl_override_creds_light()

[Vinicius Costa Gomes]

Will be used when there are guarantees that the credentials usage
count is not modified in the critical section.

This is a temporary helper, that will be removed when all users are
converted to use the credentials GUARD() helpers.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/overlayfs.h | 1 +
 fs/overlayfs/util.c      | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 0bfe35da4b7b..557d8c4e3a01 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -429,6 +429,7 @@ int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
 struct dentry *ovl_workdir(struct dentry *dentry);
 const struct cred *ovl_override_creds(struct super_block *sb);
+const struct cred *ovl_override_creds_light(struct super_block *sb);
 
 static inline const struct cred *ovl_creds(struct super_block *sb)
 {
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index edc9216f6e27..3525ede21600 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -68,6 +68,13 @@ const struct cred *ovl_override_creds(struct super_block *sb)
 	return override_creds(ofs->creator_cred);
 }
 
+const struct cred *ovl_override_creds_light(struct super_block *sb)
+{
+	struct ovl_fs *ofs = OVL_FS(sb);
+
+	return override_creds_light(ofs->creator_cred);
+}
+
 /*
  * Check if underlying fs supports file handles and try to determine encoding
  * type, in order to deduce maximum inode number used by fs.
-- 
2.46.0

[PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Vinicius Costa Gomes]

Add a comment to these operations that cannot use the _light version
of override_creds()/revert_creds(), because during the critical
section the struct cred .usage counter might be modified.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/copy_up.c | 6 +++++-
 fs/overlayfs/dir.c     | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index a5ef2005a2cc..7dec275e08cd 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -718,8 +718,12 @@ static int ovl_prep_cu_creds(struct dentry *dentry, struct ovl_cu_creds *cc)
 	if (err < 0)
 		return err;
 
-	if (cc->new)
+	if (cc->new) {
+		/* Do not use the _light version, the credentials
+		 * ->usage might be modified.
+		 */
 		cc->old = override_creds(cc->new);
+	}
 
 	return 0;
 }
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index ab65e98a1def..851945904385 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -584,6 +584,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 	const struct cred *old_cred;
 	struct dentry *parent = dentry->d_parent;
 
+	/* Do not use the _light version, cred->usage might be modified */
 	old_cred = ovl_override_creds(dentry->d_sb);
 
 	/*
@@ -1159,6 +1160,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 			goto out;
 	}
 
+	/* Do not use the _light version, cred->usage might be modified */
 	old_cred = ovl_override_creds(old->d_sb);
 
 	if (!list_empty(&list)) {
@@ -1314,6 +1316,7 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 	int flags = file->f_flags | OVL_OPEN_FLAGS;
 	int err;
 
+	/* Do not use the _light version, cred->usage might be modified */
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_setup_cred_for_create(dentry, inode, mode, old_cred);
 	if (err)
-- 
2.46.0

[PATCH v2 05/16] overlayfs: Use ovl_override_creds_light()/revert_creds_light()

[Vinicius Costa Gomes]

Convert to use ovl_override_creds_light()/revert_creds_light(), these
functions assume that the critical section won't modify the usage
counter of the credentials in question.

In most overlayfs instances, the credentials lifetime is the duration
of the filesystem being mounted, so it's safe to assume that it will
continue to exist for that duration.

NOTE: that in copy up there was an instance in which that the
conversion is not safe, because there's a risk of the cred refcount
being changed in the critical section.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/copy_up.c |  4 ++--
 fs/overlayfs/dir.c     |  8 ++++----
 fs/overlayfs/file.c    | 28 ++++++++++++++--------------
 fs/overlayfs/inode.c   | 40 ++++++++++++++++++++--------------------
 fs/overlayfs/namei.c   | 18 +++++++++---------
 fs/overlayfs/readdir.c | 16 ++++++++--------
 fs/overlayfs/util.c    |  8 ++++----
 fs/overlayfs/xattrs.c  | 17 ++++++++---------
 8 files changed, 69 insertions(+), 70 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 7dec275e08cd..7b1679ce996e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1204,7 +1204,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	if (err)
 		return err;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	while (!err) {
 		struct dentry *next;
 		struct dentry *parent = NULL;
@@ -1229,7 +1229,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		dput(parent);
 		dput(next);
 	}
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return err;
 }
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 851945904385..52021e56b235 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -701,9 +701,9 @@ static int ovl_set_link_redirect(struct dentry *dentry)
 	const struct cred *old_cred;
 	int err;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	err = ovl_set_redirect(dentry, false);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -908,12 +908,12 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 	if (err)
 		goto out;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	if (!lower_positive)
 		err = ovl_remove_upper(dentry, is_dir, &list);
 	else
 		err = ovl_remove_and_whiteout(dentry, &list);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	if (!err) {
 		if (is_dir)
 			clear_nlink(dentry->d_inode);
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 1a411cae57ed..5533fedcbc47 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -39,7 +39,7 @@ static struct file *ovl_open_realfile(const struct file *file,
 	if (flags & O_APPEND)
 		acc_mode |= MAY_APPEND;
 
-	old_cred = ovl_override_creds(inode->i_sb);
+	old_cred = ovl_override_creds_light(inode->i_sb);
 	real_idmap = mnt_idmap(realpath->mnt);
 	err = inode_permission(real_idmap, realinode, MAY_OPEN | acc_mode);
 	if (err) {
@@ -51,7 +51,7 @@ static struct file *ovl_open_realfile(const struct file *file,
 		realfile = backing_file_open(&file->f_path, flags, realpath,
 					     current_cred());
 	}
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
 		 file, file, ovl_whatisit(inode, realinode), file->f_flags,
@@ -211,9 +211,9 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 	ovl_inode_lock(inode);
 	real.file->f_pos = file->f_pos;
 
-	old_cred = ovl_override_creds(inode->i_sb);
+	old_cred = ovl_override_creds_light(inode->i_sb);
 	ret = vfs_llseek(real.file, offset, whence);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	file->f_pos = real.file->f_pos;
 	ovl_inode_unlock(inode);
@@ -398,9 +398,9 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 
 	/* Don't sync lower file for fear of receiving EROFS error */
 	if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
-		old_cred = ovl_override_creds(file_inode(file)->i_sb);
+		old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
 		ret = vfs_fsync_range(real.file, start, end, datasync);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 	}
 
 	fdput(real);
@@ -438,9 +438,9 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 	if (ret)
 		goto out_unlock;
 
-	old_cred = ovl_override_creds(file_inode(file)->i_sb);
+	old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
 	ret = vfs_fallocate(real.file, mode, offset, len);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	/* Update size */
 	ovl_file_modified(file);
@@ -463,9 +463,9 @@ static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 	if (ret)
 		return ret;
 
-	old_cred = ovl_override_creds(file_inode(file)->i_sb);
+	old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
 	ret = vfs_fadvise(real.file, offset, len, advice);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	fdput(real);
 
@@ -506,7 +506,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 		goto out_unlock;
 	}
 
-	old_cred = ovl_override_creds(file_inode(file_out)->i_sb);
+	old_cred = ovl_override_creds_light(file_inode(file_out)->i_sb);
 	switch (op) {
 	case OVL_COPY:
 		ret = vfs_copy_file_range(real_in.file, pos_in,
@@ -524,7 +524,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 						flags);
 		break;
 	}
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	/* Update size */
 	ovl_file_modified(file_out);
@@ -584,9 +584,9 @@ static int ovl_flush(struct file *file, fl_owner_t id)
 		return err;
 
 	if (real.file->f_op->flush) {
-		old_cred = ovl_override_creds(file_inode(file)->i_sb);
+		old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
 		err = real.file->f_op->flush(real.file, id);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 	}
 	fdput(real);
 
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 35fd3e3e1778..30460d718605 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -78,9 +78,9 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 			goto out_put_write;
 
 		inode_lock(upperdentry->d_inode);
-		old_cred = ovl_override_creds(dentry->d_sb);
+		old_cred = ovl_override_creds_light(dentry->d_sb);
 		err = ovl_do_notify_change(ofs, upperdentry, attr);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 		if (!err)
 			ovl_copyattr(dentry->d_inode);
 		inode_unlock(upperdentry->d_inode);
@@ -169,7 +169,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 	metacopy_blocks = ovl_is_metacopy_dentry(dentry);
 
 	type = ovl_path_real(dentry, &realpath);
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	err = ovl_do_getattr(&realpath, stat, request_mask, flags);
 	if (err)
 		goto out;
@@ -280,7 +280,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 		stat->nlink = dentry->d_inode->i_nlink;
 
 out:
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -309,7 +309,7 @@ int ovl_permission(struct mnt_idmap *idmap,
 	if (err)
 		return err;
 
-	old_cred = ovl_override_creds(inode->i_sb);
+	old_cred = ovl_override_creds_light(inode->i_sb);
 	if (!upperinode &&
 	    !special_file(realinode->i_mode) && mask & MAY_WRITE) {
 		mask &= ~(MAY_WRITE | MAY_APPEND);
@@ -317,7 +317,7 @@ int ovl_permission(struct mnt_idmap *idmap,
 		mask |= MAY_READ;
 	}
 	err = inode_permission(mnt_idmap(realpath.mnt), realinode, mask);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -332,9 +332,9 @@ static const char *ovl_get_link(struct dentry *dentry,
 	if (!dentry)
 		return ERR_PTR(-ECHILD);
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	p = vfs_get_link(ovl_dentry_real(dentry), done);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	return p;
 }
 
@@ -467,9 +467,9 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
 	} else {
 		const struct cred *old_cred;
 
-		old_cred = ovl_override_creds(inode->i_sb);
+		old_cred = ovl_override_creds_light(inode->i_sb);
 		acl = ovl_get_acl_path(&realpath, posix_acl_xattr_name(type), noperm);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 	}
 
 	return acl;
@@ -495,10 +495,10 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
 		struct posix_acl *real_acl;
 
 		ovl_path_lower(dentry, &realpath);
-		old_cred = ovl_override_creds(dentry->d_sb);
+		old_cred = ovl_override_creds_light(dentry->d_sb);
 		real_acl = vfs_get_acl(mnt_idmap(realpath.mnt), realdentry,
 				       acl_name);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 		if (IS_ERR(real_acl)) {
 			err = PTR_ERR(real_acl);
 			goto out;
@@ -518,12 +518,12 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
 	if (err)
 		goto out;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	if (acl)
 		err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
 	else
 		err = ovl_do_remove_acl(ofs, realdentry, acl_name);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	ovl_drop_write(dentry);
 
 	/* copy c/mtime */
@@ -598,9 +598,9 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	if (!realinode->i_op->fiemap)
 		return -EOPNOTSUPP;
 
-	old_cred = ovl_override_creds(inode->i_sb);
+	old_cred = ovl_override_creds_light(inode->i_sb);
 	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -660,7 +660,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
 		if (err)
 			goto out;
 
-		old_cred = ovl_override_creds(inode->i_sb);
+		old_cred = ovl_override_creds_light(inode->i_sb);
 		/*
 		 * Store immutable/append-only flags in xattr and clear them
 		 * in upper fileattr (in case they were set by older kernel)
@@ -671,7 +671,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
 		err = ovl_set_protattr(inode, upperpath.dentry, fa);
 		if (!err)
 			err = ovl_real_fileattr_set(&upperpath, fa);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 		ovl_drop_write(dentry);
 
 		/*
@@ -730,10 +730,10 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 
 	ovl_path_real(dentry, &realpath);
 
-	old_cred = ovl_override_creds(inode->i_sb);
+	old_cred = ovl_override_creds_light(inode->i_sb);
 	err = ovl_real_fileattr_get(&realpath, fa);
 	ovl_fileattr_prot_flags(inode, fa);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return err;
 }
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5764f91d283e..e52d6ae8eeb5 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -955,13 +955,13 @@ static int ovl_maybe_validate_verity(struct dentry *dentry)
 	if (!ovl_test_flag(OVL_VERIFIED_DIGEST, inode)) {
 		const struct cred *old_cred;
 
-		old_cred = ovl_override_creds(dentry->d_sb);
+		old_cred = ovl_override_creds_light(dentry->d_sb);
 
 		err = ovl_validate_verity(ofs, &metapath, &datapath);
 		if (err == 0)
 			ovl_set_flag(OVL_VERIFIED_DIGEST, inode);
 
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 	}
 
 	ovl_inode_unlock(inode);
@@ -993,9 +993,9 @@ static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 	if (ovl_dentry_lowerdata(dentry))
 		goto out;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	err = ovl_lookup_data_layers(dentry, redirect, &datapath);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	if (err)
 		goto out_err;
 
@@ -1061,7 +1061,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	if (dentry->d_name.len > ofs->namelen)
 		return ERR_PTR(-ENAMETOOLONG);
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	upperdir = ovl_dentry_upper(dentry->d_parent);
 	if (upperdir) {
 		d.layer = &ofs->layers[0];
@@ -1342,7 +1342,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 	ovl_dentry_init_reval(dentry, upperdentry, OVL_I_E(inode));
 
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	if (origin_path) {
 		dput(origin_path->dentry);
 		kfree(origin_path);
@@ -1366,7 +1366,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	kfree(upperredirect);
 out:
 	kfree(d.redirect);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	return ERR_PTR(err);
 }
 
@@ -1390,7 +1390,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 	if (!ovl_dentry_upper(dentry))
 		return true;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	/* Positive upper -> have to look up lower to see whether it exists */
 	for (i = 0; !done && !positive && i < ovl_numlower(poe); i++) {
 		struct dentry *this;
@@ -1423,7 +1423,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 			dput(this);
 		}
 	}
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return positive;
 }
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 0ca8af060b0c..c8bf681f5cf0 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -275,7 +275,7 @@ static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data
 	struct dentry *dentry, *dir = path->dentry;
 	const struct cred *old_cred;
 
-	old_cred = ovl_override_creds(rdd->dentry->d_sb);
+	old_cred = ovl_override_creds_light(rdd->dentry->d_sb);
 
 	err = down_write_killable(&dir->d_inode->i_rwsem);
 	if (!err) {
@@ -290,7 +290,7 @@ static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data
 		}
 		inode_unlock(dir->d_inode);
 	}
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -756,7 +756,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	const struct cred *old_cred;
 	int err;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	if (!ctx->pos)
 		ovl_dir_reset(file);
 
@@ -808,7 +808,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	}
 	err = 0;
 out:
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	return err;
 }
 
@@ -858,9 +858,9 @@ static struct file *ovl_dir_open_realfile(const struct file *file,
 	struct file *res;
 	const struct cred *old_cred;
 
-	old_cred = ovl_override_creds(file_inode(file)->i_sb);
+	old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
 	res = ovl_path_open(realpath, O_RDONLY | (file->f_flags & O_LARGEFILE));
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	return res;
 }
@@ -985,9 +985,9 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
 	struct rb_root root = RB_ROOT;
 	const struct cred *old_cred;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	err = ovl_dir_read_merged(dentry, list, &root);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 3525ede21600..80caeb81c727 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1177,7 +1177,7 @@ int ovl_nlink_start(struct dentry *dentry)
 	if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
 		return 0;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	/*
 	 * The overlay inode nlink should be incremented/decremented IFF the
 	 * upper operation succeeds, along with nlink change of upper inode.
@@ -1185,7 +1185,7 @@ int ovl_nlink_start(struct dentry *dentry)
 	 * value relative to the upper inode nlink in an upper inode xattr.
 	 */
 	err = ovl_set_nlink_upper(dentry);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	if (err)
 		goto out_drop_write;
 
@@ -1208,9 +1208,9 @@ void ovl_nlink_end(struct dentry *dentry)
 	if (ovl_test_flag(OVL_INDEX, inode) && inode->i_nlink == 0) {
 		const struct cred *old_cred;
 
-		old_cred = ovl_override_creds(dentry->d_sb);
+		old_cred = ovl_override_creds_light(dentry->d_sb);
 		ovl_cleanup_index(dentry);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 	}
 
 	ovl_inode_unlock(inode);
diff --git a/fs/overlayfs/xattrs.c b/fs/overlayfs/xattrs.c
index 383978e4663c..0d315d0dd89e 100644
--- a/fs/overlayfs/xattrs.c
+++ b/fs/overlayfs/xattrs.c
@@ -45,9 +45,9 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char
 
 	if (!value && !upperdentry) {
 		ovl_path_lower(dentry, &realpath);
-		old_cred = ovl_override_creds(dentry->d_sb);
+		old_cred = ovl_override_creds_light(dentry->d_sb);
 		err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
-		revert_creds(old_cred);
+		revert_creds_light(old_cred);
 		if (err < 0)
 			goto out;
 	}
@@ -64,7 +64,7 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char
 	if (err)
 		goto out;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	if (value) {
 		err = ovl_do_setxattr(ofs, realdentry, name, value, size,
 				      flags);
@@ -72,7 +72,7 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char
 		WARN_ON(flags != XATTR_REPLACE);
 		err = ovl_do_removexattr(ofs, realdentry, name);
 	}
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	ovl_drop_write(dentry);
 
 	/* copy c/mtime */
@@ -89,9 +89,9 @@ static int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char
 	struct path realpath;
 
 	ovl_i_path_real(inode, &realpath);
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	res = vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, value, size);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	return res;
 }
 
@@ -119,9 +119,9 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
 	const struct cred *old_cred;
 	size_t prefix_len, name_len;
 
-	old_cred = ovl_override_creds(dentry->d_sb);
+	old_cred = ovl_override_creds_light(dentry->d_sb);
 	res = vfs_listxattr(realdentry, list, size);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 	if (res <= 0 || size == 0)
 		return res;
 
@@ -268,4 +268,3 @@ const struct xattr_handler * const *ovl_xattr_handlers(struct ovl_fs *ofs)
 	return ofs->config.userxattr ? ovl_user_xattr_handlers :
 		ovl_trusted_xattr_handlers;
 }
-
-- 
2.46.0

[PATCH v2 06/16] cred: Introduce cred_guard() and cred_scoped_guard() helpers

[Vinicius Costa Gomes]

These helpers will make it less error prone to use
override_creds_light()/revert_creds_light(). They make sure that they
are paired.

As they use the _light() version of the credentials override/revert
operations, they should only be used when there are guarantees that
the lifetime of the credentials in question is not modified during the
critical section.

Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 include/linux/cred.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/cred.h b/include/linux/cred.h
index e4a3155fe409..f4f3d55cd6a2 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -190,6 +190,13 @@ static inline void revert_creds_light(const struct cred *revert_cred)
 	rcu_assign_pointer(current->cred, revert_cred);
 }
 
+DEFINE_LOCK_GUARD_1(__cred, struct cred,
+		    _T->lock = (struct cred *)override_creds_light(_T->lock),
+		    revert_creds_light(_T->lock));
+
+#define cred_guard(_cred) guard(__cred)(((struct cred *)_cred))
+#define cred_scoped_guard(_cred) scoped_guard(__cred, ((struct cred *)_cred))
+
 /**
  * get_new_cred_many - Get references on a new set of credentials
  * @cred: The new credentials to reference
-- 
2.46.0

[PATCH v2 07/16] fs/backing-file: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations to cred_guard().

For fs/backing-file.c, backing_file_open() and backing_tmpfile_open()
are not converted because they increase the usage counter of the
credentials in question.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/backing-file.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/fs/backing-file.c b/fs/backing-file.c
index bc19e8e28e58..29fe207a2032 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -163,7 +163,6 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
 			       struct backing_file_ctx *ctx)
 {
 	struct backing_aio *aio = NULL;
-	const struct cred *old_cred;
 	ssize_t ret;
 
 	if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
@@ -176,7 +175,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
 	    !(file->f_mode & FMODE_CAN_ODIRECT))
 		return -EINVAL;
 
-	old_cred = override_creds_light(ctx->cred);
+	cred_guard(ctx->cred);
 	if (is_sync_kiocb(iocb)) {
 		rwf_t rwf = iocb_to_rw_flags(flags);
 
@@ -197,8 +196,6 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
 			backing_aio_cleanup(aio, ret);
 	}
 out:
-	revert_creds_light(old_cred);
-
 	if (ctx->accessed)
 		ctx->accessed(ctx->user_file);
 
@@ -210,7 +207,6 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 				struct kiocb *iocb, int flags,
 				struct backing_file_ctx *ctx)
 {
-	const struct cred *old_cred;
 	ssize_t ret;
 
 	if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
@@ -233,7 +229,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 	 */
 	flags &= ~IOCB_DIO_CALLER_COMP;
 
-	old_cred = override_creds_light(ctx->cred);
+	cred_guard(ctx->cred);
 	if (is_sync_kiocb(iocb)) {
 		rwf_t rwf = iocb_to_rw_flags(flags);
 
@@ -264,7 +260,6 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 			backing_aio_cleanup(aio, ret);
 	}
 out:
-	revert_creds_light(old_cred);
 
 	return ret;
 }
@@ -275,15 +270,13 @@ ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
 				 unsigned int flags,
 				 struct backing_file_ctx *ctx)
 {
-	const struct cred *old_cred;
 	ssize_t ret;
 
 	if (WARN_ON_ONCE(!(in->f_mode & FMODE_BACKING)))
 		return -EIO;
 
-	old_cred = override_creds_light(ctx->cred);
+	cred_guard(ctx->cred);
 	ret = vfs_splice_read(in, ppos, pipe, len, flags);
-	revert_creds_light(old_cred);
 
 	if (ctx->accessed)
 		ctx->accessed(ctx->user_file);
@@ -297,7 +290,6 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
 				  unsigned int flags,
 				  struct backing_file_ctx *ctx)
 {
-	const struct cred *old_cred;
 	ssize_t ret;
 
 	if (WARN_ON_ONCE(!(out->f_mode & FMODE_BACKING)))
@@ -306,12 +298,10 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
 	ret = file_remove_privs(ctx->user_file);
 	if (ret)
 		return ret;
-
-	old_cred = override_creds_light(ctx->cred);
+	cred_guard(ctx->cred);
 	file_start_write(out);
 	ret = iter_file_splice_write(pipe, out, ppos, len, flags);
 	file_end_write(out);
-	revert_creds_light(old_cred);
 
 	if (ctx->end_write)
 		ctx->end_write(ctx->user_file);
@@ -323,7 +313,6 @@ EXPORT_SYMBOL_GPL(backing_file_splice_write);
 int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
 		      struct backing_file_ctx *ctx)
 {
-	const struct cred *old_cred;
 	int ret;
 
 	if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
@@ -335,9 +324,8 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
 
 	vma_set_file(vma, file);
 
-	old_cred = override_creds_light(ctx->cred);
+	cred_guard(ctx->cred);
 	ret = call_mmap(vma->vm_file, vma);
-	revert_creds_light(old_cred);
 
 	if (ctx->accessed)
 		ctx->accessed(ctx->user_file);
-- 
2.46.0

[PATCH v2 08/16] overlayfs/copy_up: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations to cred_guard().

Only ovl_copy_up_flags() was converted to use cred_guard().

ovl_copy_up_workdir() and ovl_copy_up_tmpfile() use their own
credentials helpers that may change the usage of creds in question. So
these are not modified.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/copy_up.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 7b1679ce996e..cab87d390b54 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1184,7 +1184,6 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 {
 	int err = 0;
-	const struct cred *old_cred;
 	bool disconnected = (dentry->d_flags & DCACHE_DISCONNECTED);
 
 	/*
@@ -1204,7 +1203,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 	if (err)
 		return err;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	while (!err) {
 		struct dentry *next;
 		struct dentry *parent = NULL;
@@ -1229,7 +1228,6 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		dput(parent);
 		dput(next);
 	}
-	revert_creds_light(old_cred);
 
 	return err;
 }
-- 
2.46.0

[PATCH v2 09/16] overlayfs/dir: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations with cred_guard()/cred_scoped_guard().

In ovl_do_remove(), cred_scoped_guard() was used because mixing
cred_guard() with 'goto' can cause the cleanup part of the guard to
run with garbage values.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/dir.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 52021e56b235..28ea6bc0a298 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -698,12 +698,10 @@ static int ovl_symlink(struct mnt_idmap *idmap, struct inode *dir,
 
 static int ovl_set_link_redirect(struct dentry *dentry)
 {
-	const struct cred *old_cred;
 	int err;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	err = ovl_set_redirect(dentry, false);
-	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -889,7 +887,6 @@ static void ovl_drop_nlink(struct dentry *dentry)
 static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 {
 	int err;
-	const struct cred *old_cred;
 	bool lower_positive = ovl_lower_positive(dentry);
 	LIST_HEAD(list);
 
@@ -908,12 +905,12 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 	if (err)
 		goto out;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
-	if (!lower_positive)
-		err = ovl_remove_upper(dentry, is_dir, &list);
-	else
-		err = ovl_remove_and_whiteout(dentry, &list);
-	revert_creds_light(old_cred);
+	cred_scoped_guard(ovl_creds(dentry->d_sb)) {
+		if (!lower_positive)
+			err = ovl_remove_upper(dentry, is_dir, &list);
+		else
+			err = ovl_remove_and_whiteout(dentry, &list);
+	}
 	if (!err) {
 		if (is_dir)
 			clear_nlink(dentry->d_inode);
-- 
2.46.0

[PATCH v2 10/16] overlayfs/file: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations with cred_guard()/cred_scoped_guard().

Only ovl_copyfile() and ovl_fallocate() use cred_scoped_guard(),
because of 'goto', which can cause the cleanup flow to run on garbage
memory.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/file.c | 64 ++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 39 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 5533fedcbc47..97aa657e6916 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -31,7 +31,6 @@ static struct file *ovl_open_realfile(const struct file *file,
 	struct inode *inode = file_inode(file);
 	struct mnt_idmap *real_idmap;
 	struct file *realfile;
-	const struct cred *old_cred;
 	int flags = file->f_flags | OVL_OPEN_FLAGS;
 	int acc_mode = ACC_MODE(flags);
 	int err;
@@ -39,7 +38,7 @@ static struct file *ovl_open_realfile(const struct file *file,
 	if (flags & O_APPEND)
 		acc_mode |= MAY_APPEND;
 
-	old_cred = ovl_override_creds_light(inode->i_sb);
+	cred_guard(ovl_creds(inode->i_sb));
 	real_idmap = mnt_idmap(realpath->mnt);
 	err = inode_permission(real_idmap, realinode, MAY_OPEN | acc_mode);
 	if (err) {
@@ -51,7 +50,6 @@ static struct file *ovl_open_realfile(const struct file *file,
 		realfile = backing_file_open(&file->f_path, flags, realpath,
 					     current_cred());
 	}
-	revert_creds_light(old_cred);
 
 	pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
 		 file, file, ovl_whatisit(inode, realinode), file->f_flags,
@@ -182,7 +180,6 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 {
 	struct inode *inode = file_inode(file);
 	struct fd real;
-	const struct cred *old_cred;
 	loff_t ret;
 
 	/*
@@ -211,9 +208,8 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 	ovl_inode_lock(inode);
 	real.file->f_pos = file->f_pos;
 
-	old_cred = ovl_override_creds_light(inode->i_sb);
+	cred_guard(ovl_creds(inode->i_sb));
 	ret = vfs_llseek(real.file, offset, whence);
-	revert_creds_light(old_cred);
 
 	file->f_pos = real.file->f_pos;
 	ovl_inode_unlock(inode);
@@ -385,7 +381,6 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 {
 	struct fd real;
-	const struct cred *old_cred;
 	int ret;
 
 	ret = ovl_sync_status(OVL_FS(file_inode(file)->i_sb));
@@ -398,9 +393,8 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 
 	/* Don't sync lower file for fear of receiving EROFS error */
 	if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
-		old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
+		cred_guard(ovl_creds(file_inode(file)->i_sb));
 		ret = vfs_fsync_range(real.file, start, end, datasync);
-		revert_creds_light(old_cred);
 	}
 
 	fdput(real);
@@ -424,7 +418,6 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 {
 	struct inode *inode = file_inode(file);
 	struct fd real;
-	const struct cred *old_cred;
 	int ret;
 
 	inode_lock(inode);
@@ -438,9 +431,8 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 	if (ret)
 		goto out_unlock;
 
-	old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
-	ret = vfs_fallocate(real.file, mode, offset, len);
-	revert_creds_light(old_cred);
+	cred_scoped_guard(ovl_creds(file_inode(file)->i_sb))
+		ret = vfs_fallocate(real.file, mode, offset, len);
 
 	/* Update size */
 	ovl_file_modified(file);
@@ -456,16 +448,14 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 {
 	struct fd real;
-	const struct cred *old_cred;
 	int ret;
 
 	ret = ovl_real_fdget(file, &real);
 	if (ret)
 		return ret;
 
-	old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
+	cred_guard(ovl_creds(file_inode(file)->i_sb));
 	ret = vfs_fadvise(real.file, offset, len, advice);
-	revert_creds_light(old_cred);
 
 	fdput(real);
 
@@ -484,7 +474,6 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 {
 	struct inode *inode_out = file_inode(file_out);
 	struct fd real_in, real_out;
-	const struct cred *old_cred;
 	loff_t ret;
 
 	inode_lock(inode_out);
@@ -506,26 +495,25 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 		goto out_unlock;
 	}
 
-	old_cred = ovl_override_creds_light(file_inode(file_out)->i_sb);
-	switch (op) {
-	case OVL_COPY:
-		ret = vfs_copy_file_range(real_in.file, pos_in,
-					  real_out.file, pos_out, len, flags);
-		break;
-
-	case OVL_CLONE:
-		ret = vfs_clone_file_range(real_in.file, pos_in,
-					   real_out.file, pos_out, len, flags);
-		break;
-
-	case OVL_DEDUPE:
-		ret = vfs_dedupe_file_range_one(real_in.file, pos_in,
-						real_out.file, pos_out, len,
-						flags);
-		break;
+	cred_scoped_guard(ovl_creds(file_inode(file_out)->i_sb)) {
+		switch (op) {
+		case OVL_COPY:
+			ret = vfs_copy_file_range(real_in.file, pos_in,
+						  real_out.file, pos_out, len, flags);
+			break;
+
+		case OVL_CLONE:
+			ret = vfs_clone_file_range(real_in.file, pos_in,
+						   real_out.file, pos_out, len, flags);
+			break;
+
+		case OVL_DEDUPE:
+			ret = vfs_dedupe_file_range_one(real_in.file, pos_in,
+							real_out.file, pos_out, len,
+							flags);
+			break;
+		}
 	}
-	revert_creds_light(old_cred);
-
 	/* Update size */
 	ovl_file_modified(file_out);
 
@@ -576,7 +564,6 @@ static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
 static int ovl_flush(struct file *file, fl_owner_t id)
 {
 	struct fd real;
-	const struct cred *old_cred;
 	int err;
 
 	err = ovl_real_fdget(file, &real);
@@ -584,9 +571,8 @@ static int ovl_flush(struct file *file, fl_owner_t id)
 		return err;
 
 	if (real.file->f_op->flush) {
-		old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
+		cred_guard(ovl_creds(file_inode(file)->i_sb));
 		err = real.file->f_op->flush(real.file, id);
-		revert_creds_light(old_cred);
 	}
 	fdput(real);
 
-- 
2.46.0

[PATCH v2 11/16] overlayfs/inode: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations with cred_guard()/cred_scoped_guard().

In ovl_setattr(), ovl_set_or_remove_acl() and ovl_fileattr_set() use
cred_scoped_guard(), because of 'goto', which can cause the cleanup
flow to run on garbage memory.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/inode.c | 73 +++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 45 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 30460d718605..a597e748397f 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -25,7 +25,6 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	bool full_copy_up = false;
 	struct dentry *upperdentry;
-	const struct cred *old_cred;
 
 	err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
 	if (err)
@@ -78,9 +77,8 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 			goto out_put_write;
 
 		inode_lock(upperdentry->d_inode);
-		old_cred = ovl_override_creds_light(dentry->d_sb);
-		err = ovl_do_notify_change(ofs, upperdentry, attr);
-		revert_creds_light(old_cred);
+		cred_scoped_guard(ovl_creds(dentry->d_sb))
+			err = ovl_do_notify_change(ofs, upperdentry, attr);
 		if (!err)
 			ovl_copyattr(dentry->d_inode);
 		inode_unlock(upperdentry->d_inode);
@@ -159,7 +157,6 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 	struct dentry *dentry = path->dentry;
 	enum ovl_path_type type;
 	struct path realpath;
-	const struct cred *old_cred;
 	struct inode *inode = d_inode(dentry);
 	bool is_dir = S_ISDIR(inode->i_mode);
 	int fsid = 0;
@@ -169,7 +166,7 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 	metacopy_blocks = ovl_is_metacopy_dentry(dentry);
 
 	type = ovl_path_real(dentry, &realpath);
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	err = ovl_do_getattr(&realpath, stat, request_mask, flags);
 	if (err)
 		goto out;
@@ -280,7 +277,6 @@ int ovl_getattr(struct mnt_idmap *idmap, const struct path *path,
 		stat->nlink = dentry->d_inode->i_nlink;
 
 out:
-	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -291,7 +287,6 @@ int ovl_permission(struct mnt_idmap *idmap,
 	struct inode *upperinode = ovl_inode_upper(inode);
 	struct inode *realinode;
 	struct path realpath;
-	const struct cred *old_cred;
 	int err;
 
 	/* Careful in RCU walk mode */
@@ -309,7 +304,7 @@ int ovl_permission(struct mnt_idmap *idmap,
 	if (err)
 		return err;
 
-	old_cred = ovl_override_creds_light(inode->i_sb);
+	cred_guard(ovl_creds(inode->i_sb));
 	if (!upperinode &&
 	    !special_file(realinode->i_mode) && mask & MAY_WRITE) {
 		mask &= ~(MAY_WRITE | MAY_APPEND);
@@ -317,7 +312,6 @@ int ovl_permission(struct mnt_idmap *idmap,
 		mask |= MAY_READ;
 	}
 	err = inode_permission(mnt_idmap(realpath.mnt), realinode, mask);
-	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -326,15 +320,13 @@ static const char *ovl_get_link(struct dentry *dentry,
 				struct inode *inode,
 				struct delayed_call *done)
 {
-	const struct cred *old_cred;
 	const char *p;
 
 	if (!dentry)
 		return ERR_PTR(-ECHILD);
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	p = vfs_get_link(ovl_dentry_real(dentry), done);
-	revert_creds_light(old_cred);
 	return p;
 }
 
@@ -465,11 +457,9 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
 
 		acl = get_cached_acl_rcu(realinode, type);
 	} else {
-		const struct cred *old_cred;
 
-		old_cred = ovl_override_creds_light(inode->i_sb);
+		cred_guard(ovl_creds(inode->i_sb));
 		acl = ovl_get_acl_path(&realpath, posix_acl_xattr_name(type), noperm);
-		revert_creds_light(old_cred);
 	}
 
 	return acl;
@@ -481,7 +471,6 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
 	int err;
 	struct path realpath;
 	const char *acl_name;
-	const struct cred *old_cred;
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct dentry *upperdentry = ovl_dentry_upper(dentry);
 	struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
@@ -495,10 +484,9 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
 		struct posix_acl *real_acl;
 
 		ovl_path_lower(dentry, &realpath);
-		old_cred = ovl_override_creds_light(dentry->d_sb);
+		cred_guard(ovl_creds(dentry->d_sb));
 		real_acl = vfs_get_acl(mnt_idmap(realpath.mnt), realdentry,
 				       acl_name);
-		revert_creds_light(old_cred);
 		if (IS_ERR(real_acl)) {
 			err = PTR_ERR(real_acl);
 			goto out;
@@ -518,12 +506,12 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
 	if (err)
 		goto out;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
-	if (acl)
-		err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
-	else
-		err = ovl_do_remove_acl(ofs, realdentry, acl_name);
-	revert_creds_light(old_cred);
+	cred_scoped_guard(ovl_creds(dentry->d_sb)) {
+		if (acl)
+			err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
+		else
+			err = ovl_do_remove_acl(ofs, realdentry, acl_name);
+	}
 	ovl_drop_write(dentry);
 
 	/* copy c/mtime */
@@ -590,7 +578,6 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 {
 	int err;
 	struct inode *realinode = ovl_inode_realdata(inode);
-	const struct cred *old_cred;
 
 	if (!realinode)
 		return -EIO;
@@ -598,9 +585,8 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	if (!realinode->i_op->fiemap)
 		return -EOPNOTSUPP;
 
-	old_cred = ovl_override_creds_light(inode->i_sb);
+	cred_guard(ovl_creds(inode->i_sb));
 	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
-	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -648,7 +634,6 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
 {
 	struct inode *inode = d_inode(dentry);
 	struct path upperpath;
-	const struct cred *old_cred;
 	unsigned int flags;
 	int err;
 
@@ -660,19 +645,19 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
 		if (err)
 			goto out;
 
-		old_cred = ovl_override_creds_light(inode->i_sb);
-		/*
-		 * Store immutable/append-only flags in xattr and clear them
-		 * in upper fileattr (in case they were set by older kernel)
-		 * so children of "ovl-immutable" directories lower aliases of
-		 * "ovl-immutable" hardlinks could be copied up.
-		 * Clear xattr when flags are cleared.
-		 */
-		err = ovl_set_protattr(inode, upperpath.dentry, fa);
-		if (!err)
-			err = ovl_real_fileattr_set(&upperpath, fa);
-		revert_creds_light(old_cred);
-		ovl_drop_write(dentry);
+		cred_scoped_guard(ovl_creds(inode->i_sb)) {
+			/*
+			 * Store immutable/append-only flags in xattr and clear them
+			 * in upper fileattr (in case they were set by older kernel)
+			 * so children of "ovl-immutable" directories lower aliases of
+			 * "ovl-immutable" hardlinks could be copied up.
+			 * Clear xattr when flags are cleared.
+			 */
+			err = ovl_set_protattr(inode, upperpath.dentry, fa);
+			if (!err)
+				err = ovl_real_fileattr_set(&upperpath, fa);
+			ovl_drop_write(dentry);
+		}
 
 		/*
 		 * Merge real inode flags with inode flags read from
@@ -725,15 +710,13 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
 	struct path realpath;
-	const struct cred *old_cred;
 	int err;
 
 	ovl_path_real(dentry, &realpath);
 
-	old_cred = ovl_override_creds_light(inode->i_sb);
+	cred_guard(ovl_creds(inode->i_sb));
 	err = ovl_real_fileattr_get(&realpath, fa);
 	ovl_fileattr_prot_flags(inode, fa);
-	revert_creds_light(old_cred);
 
 	return err;
 }
-- 
2.46.0

[PATCH v2 12/16] overlayfs/namei: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations with cred_guard()/cred_scoped_guard().

Only ovl_maybe_lookup_lowerdata() use cred_scoped_guard(), because of
'goto', which can cause the cleanup flow to run on garbage memory.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/namei.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index e52d6ae8eeb5..723731bc3678 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -953,15 +953,12 @@ static int ovl_maybe_validate_verity(struct dentry *dentry)
 		return err;
 
 	if (!ovl_test_flag(OVL_VERIFIED_DIGEST, inode)) {
-		const struct cred *old_cred;
-
-		old_cred = ovl_override_creds_light(dentry->d_sb);
+		cred_guard(ovl_creds(dentry->d_sb));
 
 		err = ovl_validate_verity(ofs, &metapath, &datapath);
 		if (err == 0)
 			ovl_set_flag(OVL_VERIFIED_DIGEST, inode);
 
-		revert_creds_light(old_cred);
 	}
 
 	ovl_inode_unlock(inode);
@@ -975,7 +972,6 @@ static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 	struct inode *inode = d_inode(dentry);
 	const char *redirect = ovl_lowerdata_redirect(inode);
 	struct ovl_path datapath = {};
-	const struct cred *old_cred;
 	int err;
 
 	if (!redirect || ovl_dentry_lowerdata(dentry))
@@ -993,9 +989,8 @@ static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 	if (ovl_dentry_lowerdata(dentry))
 		goto out;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
-	err = ovl_lookup_data_layers(dentry, redirect, &datapath);
-	revert_creds_light(old_cred);
+	cred_scoped_guard(ovl_creds(dentry->d_sb))
+		err = ovl_lookup_data_layers(dentry, redirect, &datapath);
 	if (err)
 		goto out_err;
 
@@ -1030,7 +1025,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			  unsigned int flags)
 {
 	struct ovl_entry *oe = NULL;
-	const struct cred *old_cred;
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct ovl_entry *poe = OVL_E(dentry->d_parent);
 	struct ovl_entry *roe = OVL_E(dentry->d_sb->s_root);
@@ -1061,7 +1055,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	if (dentry->d_name.len > ofs->namelen)
 		return ERR_PTR(-ENAMETOOLONG);
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	upperdir = ovl_dentry_upper(dentry->d_parent);
 	if (upperdir) {
 		d.layer = &ofs->layers[0];
@@ -1342,7 +1336,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 
 	ovl_dentry_init_reval(dentry, upperdentry, OVL_I_E(inode));
 
-	revert_creds_light(old_cred);
 	if (origin_path) {
 		dput(origin_path->dentry);
 		kfree(origin_path);
@@ -1366,7 +1359,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	kfree(upperredirect);
 out:
 	kfree(d.redirect);
-	revert_creds_light(old_cred);
 	return ERR_PTR(err);
 }
 
@@ -1374,7 +1366,6 @@ bool ovl_lower_positive(struct dentry *dentry)
 {
 	struct ovl_entry *poe = OVL_E(dentry->d_parent);
 	const struct qstr *name = &dentry->d_name;
-	const struct cred *old_cred;
 	unsigned int i;
 	bool positive = false;
 	bool done = false;
@@ -1390,7 +1381,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 	if (!ovl_dentry_upper(dentry))
 		return true;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	/* Positive upper -> have to look up lower to see whether it exists */
 	for (i = 0; !done && !positive && i < ovl_numlower(poe); i++) {
 		struct dentry *this;
@@ -1423,7 +1414,6 @@ bool ovl_lower_positive(struct dentry *dentry)
 			dput(this);
 		}
 	}
-	revert_creds_light(old_cred);
 
 	return positive;
 }
-- 
2.46.0

[PATCH v2 13/16] overlayfs/readdir: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations with cred_guard()/cred_scoped_guard().

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/readdir.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index c8bf681f5cf0..41e01fe3ae4a 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -273,9 +273,8 @@ static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data
 	int err;
 	struct ovl_cache_entry *p;
 	struct dentry *dentry, *dir = path->dentry;
-	const struct cred *old_cred;
 
-	old_cred = ovl_override_creds_light(rdd->dentry->d_sb);
+	cred_guard(ovl_creds(rdd->dentry->d_sb));
 
 	err = down_write_killable(&dir->d_inode->i_rwsem);
 	if (!err) {
@@ -290,7 +289,6 @@ static int ovl_check_whiteouts(const struct path *path, struct ovl_readdir_data
 		}
 		inode_unlock(dir->d_inode);
 	}
-	revert_creds_light(old_cred);
 
 	return err;
 }
@@ -753,10 +751,9 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	struct dentry *dentry = file->f_path.dentry;
 	struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
 	struct ovl_cache_entry *p;
-	const struct cred *old_cred;
 	int err;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	if (!ctx->pos)
 		ovl_dir_reset(file);
 
@@ -808,7 +805,6 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	}
 	err = 0;
 out:
-	revert_creds_light(old_cred);
 	return err;
 }
 
@@ -856,11 +852,9 @@ static struct file *ovl_dir_open_realfile(const struct file *file,
 					  const struct path *realpath)
 {
 	struct file *res;
-	const struct cred *old_cred;
 
-	old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
+	cred_guard(ovl_creds(file_inode(file)->i_sb));
 	res = ovl_path_open(realpath, O_RDONLY | (file->f_flags & O_LARGEFILE));
-	revert_creds_light(old_cred);
 
 	return res;
 }
@@ -983,11 +977,9 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
 	int err;
 	struct ovl_cache_entry *p, *n;
 	struct rb_root root = RB_ROOT;
-	const struct cred *old_cred;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	err = ovl_dir_read_merged(dentry, list, &root);
-	revert_creds_light(old_cred);
 	if (err)
 		return err;
 
-- 
2.46.0

[PATCH v2 14/16] overlayfs/xattrs: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations with cred_guard()/cred_scoped_guard().

In ovl_xattr_set() use cred_scoped_guard(), because of 'goto', which
can cause the cleanup flow to run on garbage memory.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/xattrs.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/fs/overlayfs/xattrs.c b/fs/overlayfs/xattrs.c
index 0d315d0dd89e..c7eb3d06a9b8 100644
--- a/fs/overlayfs/xattrs.c
+++ b/fs/overlayfs/xattrs.c
@@ -41,15 +41,14 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char
 	struct dentry *upperdentry = ovl_i_dentry_upper(inode);
 	struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
 	struct path realpath;
-	const struct cred *old_cred;
 
 	if (!value && !upperdentry) {
 		ovl_path_lower(dentry, &realpath);
-		old_cred = ovl_override_creds_light(dentry->d_sb);
-		err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
-		revert_creds_light(old_cred);
+		cred_scoped_guard(ovl_creds(dentry->d_sb))
+			err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
 		if (err < 0)
 			goto out;
+
 	}
 
 	if (!upperdentry) {
@@ -64,15 +63,15 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char
 	if (err)
 		goto out;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
-	if (value) {
-		err = ovl_do_setxattr(ofs, realdentry, name, value, size,
-				      flags);
-	} else {
-		WARN_ON(flags != XATTR_REPLACE);
-		err = ovl_do_removexattr(ofs, realdentry, name);
+	cred_scoped_guard(ovl_creds(dentry->d_sb)) {
+		if (value) {
+			err = ovl_do_setxattr(ofs, realdentry, name, value, size,
+					      flags);
+		} else {
+			WARN_ON(flags != XATTR_REPLACE);
+			err = ovl_do_removexattr(ofs, realdentry, name);
+		}
 	}
-	revert_creds_light(old_cred);
 	ovl_drop_write(dentry);
 
 	/* copy c/mtime */
@@ -85,13 +84,11 @@ static int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char
 			 void *value, size_t size)
 {
 	ssize_t res;
-	const struct cred *old_cred;
 	struct path realpath;
 
 	ovl_i_path_real(inode, &realpath);
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	res = vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, value, size);
-	revert_creds_light(old_cred);
 	return res;
 }
 
@@ -116,12 +113,10 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
 	ssize_t res;
 	size_t len;
 	char *s;
-	const struct cred *old_cred;
 	size_t prefix_len, name_len;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
+	cred_guard(ovl_creds(dentry->d_sb));
 	res = vfs_listxattr(realdentry, list, size);
-	revert_creds_light(old_cred);
 	if (res <= 0 || size == 0)
 		return res;
 
-- 
2.46.0

[PATCH v2 15/16] overlayfs/util: Convert to cred_guard()

[Vinicius Costa Gomes]

Replace the override_creds_light()/revert_creds_light() pairs of
operations with cred_guard()/cred_scoped_guard().

In ovl_nlink_start() use cred_scoped_guard(), because of
'goto', which can cause the cleanup flow to run on garbage memory.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/util.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 80caeb81c727..77b8d01829a4 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1140,7 +1140,6 @@ static void ovl_cleanup_index(struct dentry *dentry)
 int ovl_nlink_start(struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
-	const struct cred *old_cred;
 	int err;
 
 	if (WARN_ON(!inode))
@@ -1177,15 +1176,15 @@ int ovl_nlink_start(struct dentry *dentry)
 	if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
 		return 0;
 
-	old_cred = ovl_override_creds_light(dentry->d_sb);
-	/*
-	 * The overlay inode nlink should be incremented/decremented IFF the
-	 * upper operation succeeds, along with nlink change of upper inode.
-	 * Therefore, before link/unlink/rename, we store the union nlink
-	 * value relative to the upper inode nlink in an upper inode xattr.
-	 */
-	err = ovl_set_nlink_upper(dentry);
-	revert_creds_light(old_cred);
+	cred_scoped_guard(ovl_creds(dentry->d_sb)) {
+		/*
+		 * The overlay inode nlink should be incremented/decremented IFF the
+		 * upper operation succeeds, along with nlink change of upper inode.
+		 * Therefore, before link/unlink/rename, we store the union nlink
+		 * value relative to the upper inode nlink in an upper inode xattr.
+		 */
+		err = ovl_set_nlink_upper(dentry);
+	}
 	if (err)
 		goto out_drop_write;
 
@@ -1206,11 +1205,8 @@ void ovl_nlink_end(struct dentry *dentry)
 	ovl_drop_write(dentry);
 
 	if (ovl_test_flag(OVL_INDEX, inode) && inode->i_nlink == 0) {
-		const struct cred *old_cred;
-
-		old_cred = ovl_override_creds_light(dentry->d_sb);
+		cred_guard(ovl_creds(dentry->d_sb));
 		ovl_cleanup_index(dentry);
-		revert_creds_light(old_cred);
 	}
 
 	ovl_inode_unlock(inode);
-- 
2.46.0

[PATCH v2 16/16] overlayfs: Remove ovl_override_creds_light()

[Vinicius Costa Gomes]

Remove the declaration of this unsafe helper.

As the GUARD() helper guarantees that the cleanup will run, it is less
error prone. Future usages should either use the GUARD helpers or the
non "light" versions.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/overlayfs.h | 1 -
 fs/overlayfs/util.c      | 7 -------
 2 files changed, 8 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 557d8c4e3a01..0bfe35da4b7b 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -429,7 +429,6 @@ int ovl_want_write(struct dentry *dentry);
 void ovl_drop_write(struct dentry *dentry);
 struct dentry *ovl_workdir(struct dentry *dentry);
 const struct cred *ovl_override_creds(struct super_block *sb);
-const struct cred *ovl_override_creds_light(struct super_block *sb);
 
 static inline const struct cred *ovl_creds(struct super_block *sb)
 {
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 77b8d01829a4..fafd8b709f64 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -68,13 +68,6 @@ const struct cred *ovl_override_creds(struct super_block *sb)
 	return override_creds(ofs->creator_cred);
 }
 
-const struct cred *ovl_override_creds_light(struct super_block *sb)
-{
-	struct ovl_fs *ofs = OVL_FS(sb);
-
-	return override_creds_light(ofs->creator_cred);
-}
-
 /*
  * Check if underlying fs supports file handles and try to determine encoding
  * type, in order to deduce maximum inode number used by fs.
-- 
2.46.0

Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Add a comment to these operations that cannot use the _light version
> of override_creds()/revert_creds(), because during the critical
> section the struct cred .usage counter might be modified.

Why is it a problem if the usage counter is modified?  Why is the
counter modified in each of these cases?

Thanks,
Miklos

Re: [PATCH v2 05/16] overlayfs: Use ovl_override_creds_light()/revert_creds_light()

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Convert to use ovl_override_creds_light()/revert_creds_light(), these
> functions assume that the critical section won't modify the usage
> counter of the credentials in question.
>
> In most overlayfs instances, the credentials lifetime is the duration

Why most instances?  AFAICS the creds have the same lifetime as the
overlayfs superblock.

Thanks,
Miklos

Re: [PATCH v2 07/16] fs/backing-file: Convert to cred_guard()

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Replace the override_creds_light()/revert_creds_light() pairs of
> operations to cred_guard().

I'd note here, that in some cases the revert will happen later than
previously, but (hopefully) you have verified that in these cases it
won't make a difference.

Thanks,
Miklos

Re: [PATCH v2 10/16] overlayfs/file: Convert to cred_guard()

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Replace the override_creds_light()/revert_creds_light() pairs of
> operations with cred_guard()/cred_scoped_guard().
>
> Only ovl_copyfile() and ovl_fallocate() use cred_scoped_guard(),
> because of 'goto', which can cause the cleanup flow to run on garbage
> memory.

This doesn't sound good.  Is this a compiler bug or a limitation of guards?

> @@ -211,9 +208,8 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
>         ovl_inode_lock(inode);
>         real.file->f_pos = file->f_pos;
>
> -       old_cred = ovl_override_creds_light(inode->i_sb);
> +       cred_guard(ovl_creds(inode->i_sb));
>         ret = vfs_llseek(real.file, offset, whence);
> -       revert_creds_light(old_cred);

Why not use scoped guard, like in fallocate?

> @@ -398,9 +393,8 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>
>         /* Don't sync lower file for fear of receiving EROFS error */
>         if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
> -               old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
> +               cred_guard(ovl_creds(file_inode(file)->i_sb));
>                 ret = vfs_fsync_range(real.file, start, end, datasync);
> -               revert_creds_light(old_cred);

Same here.

> @@ -584,9 +571,8 @@ static int ovl_flush(struct file *file, fl_owner_t id)
>                 return err;
>
>         if (real.file->f_op->flush) {
> -               old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
> +               cred_guard(ovl_creds(file_inode(file)->i_sb));

What's the scope of this?  The function or the inner block?

Thanks,
Miklos

Re: [PATCH v2 16/16] overlayfs: Remove ovl_override_creds_light()

[Miklos Szeredi]

On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Remove the declaration of this unsafe helper.
>
> As the GUARD() helper guarantees that the cleanup will run, it is less
> error prone.

This statement is somewhat dubious.

I suggest that unless and until the goto issue can be fixed the
conversion to guards is postponed.

Thanks,
Miklos

Re: [PATCH v2 10/16] overlayfs/file: Convert to cred_guard()

[Amir Goldstein]

On Thu, Aug 22, 2024 at 3:25 AM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Replace the override_creds_light()/revert_creds_light() pairs of
> operations with cred_guard()/cred_scoped_guard().
>
> Only ovl_copyfile() and ovl_fallocate() use cred_scoped_guard(),
> because of 'goto', which can cause the cleanup flow to run on garbage
> memory.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  fs/overlayfs/file.c | 64 ++++++++++++++++++---------------------------
>  1 file changed, 25 insertions(+), 39 deletions(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 5533fedcbc47..97aa657e6916 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -31,7 +31,6 @@ static struct file *ovl_open_realfile(const struct file *file,
>         struct inode *inode = file_inode(file);
>         struct mnt_idmap *real_idmap;
>         struct file *realfile;
> -       const struct cred *old_cred;
>         int flags = file->f_flags | OVL_OPEN_FLAGS;
>         int acc_mode = ACC_MODE(flags);
>         int err;
> @@ -39,7 +38,7 @@ static struct file *ovl_open_realfile(const struct file *file,
>         if (flags & O_APPEND)
>                 acc_mode |= MAY_APPEND;
>
> -       old_cred = ovl_override_creds_light(inode->i_sb);
> +       cred_guard(ovl_creds(inode->i_sb));
>         real_idmap = mnt_idmap(realpath->mnt);
>         err = inode_permission(real_idmap, realinode, MAY_OPEN | acc_mode);
>         if (err) {
> @@ -51,7 +50,6 @@ static struct file *ovl_open_realfile(const struct file *file,
>                 realfile = backing_file_open(&file->f_path, flags, realpath,
>                                              current_cred());
>         }
> -       revert_creds_light(old_cred);
>
>         pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
>                  file, file, ovl_whatisit(inode, realinode), file->f_flags,
> @@ -182,7 +180,6 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
>  {
>         struct inode *inode = file_inode(file);
>         struct fd real;
> -       const struct cred *old_cred;
>         loff_t ret;
>
>         /*
> @@ -211,9 +208,8 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
>         ovl_inode_lock(inode);
>         real.file->f_pos = file->f_pos;
>
> -       old_cred = ovl_override_creds_light(inode->i_sb);
> +       cred_guard(ovl_creds(inode->i_sb));
>         ret = vfs_llseek(real.file, offset, whence);
> -       revert_creds_light(old_cred);
>
>         file->f_pos = real.file->f_pos;
>         ovl_inode_unlock(inode);
> @@ -385,7 +381,6 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
>  static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  {
>         struct fd real;
> -       const struct cred *old_cred;
>         int ret;
>
>         ret = ovl_sync_status(OVL_FS(file_inode(file)->i_sb));
> @@ -398,9 +393,8 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>
>         /* Don't sync lower file for fear of receiving EROFS error */
>         if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
> -               old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
> +               cred_guard(ovl_creds(file_inode(file)->i_sb));
>                 ret = vfs_fsync_range(real.file, start, end, datasync);
> -               revert_creds_light(old_cred);
>         }
>
>         fdput(real);
> @@ -424,7 +418,6 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
>  {
>         struct inode *inode = file_inode(file);
>         struct fd real;
> -       const struct cred *old_cred;
>         int ret;
>
>         inode_lock(inode);
> @@ -438,9 +431,8 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
>         if (ret)
>                 goto out_unlock;
>
> -       old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
> -       ret = vfs_fallocate(real.file, mode, offset, len);
> -       revert_creds_light(old_cred);
> +       cred_scoped_guard(ovl_creds(file_inode(file)->i_sb))
> +               ret = vfs_fallocate(real.file, mode, offset, len);
>

I find this syntax confusing. Even though it is a valid syntax,
I prefer that if there is a scope we use explicit brackets for it even
if the scope is
a single line.

How about using:
       {
               cred_guard(ovl_creds(file_inode(file)->i_sb));
               ret = vfs_fallocate(real.file, mode, offset, len);
       }

It is more clear and helps averting the compiler bug(?).

>         /* Update size */
>         ovl_file_modified(file);
> @@ -456,16 +448,14 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
>  static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
>  {
>         struct fd real;
> -       const struct cred *old_cred;
>         int ret;
>
>         ret = ovl_real_fdget(file, &real);
>         if (ret)
>                 return ret;
>
> -       old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
> +       cred_guard(ovl_creds(file_inode(file)->i_sb));
>         ret = vfs_fadvise(real.file, offset, len, advice);
> -       revert_creds_light(old_cred);
>
>         fdput(real);
>
> @@ -484,7 +474,6 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
>  {
>         struct inode *inode_out = file_inode(file_out);
>         struct fd real_in, real_out;
> -       const struct cred *old_cred;
>         loff_t ret;
>
>         inode_lock(inode_out);
> @@ -506,26 +495,25 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
>                 goto out_unlock;
>         }
>
> -       old_cred = ovl_override_creds_light(file_inode(file_out)->i_sb);
> -       switch (op) {
> -       case OVL_COPY:
> -               ret = vfs_copy_file_range(real_in.file, pos_in,
> -                                         real_out.file, pos_out, len, flags);
> -               break;
> -
> -       case OVL_CLONE:
> -               ret = vfs_clone_file_range(real_in.file, pos_in,
> -                                          real_out.file, pos_out, len, flags);
> -               break;
> -
> -       case OVL_DEDUPE:
> -               ret = vfs_dedupe_file_range_one(real_in.file, pos_in,
> -                                               real_out.file, pos_out, len,
> -                                               flags);
> -               break;
> +       cred_scoped_guard(ovl_creds(file_inode(file_out)->i_sb)) {
> +               switch (op) {
> +               case OVL_COPY:
> +                       ret = vfs_copy_file_range(real_in.file, pos_in,
> +                                                 real_out.file, pos_out, len, flags);
> +                       break;
> +
> +               case OVL_CLONE:
> +                       ret = vfs_clone_file_range(real_in.file, pos_in,
> +                                                  real_out.file, pos_out, len, flags);
> +                       break;
> +
> +               case OVL_DEDUPE:
> +                       ret = vfs_dedupe_file_range_one(real_in.file, pos_in,
> +                                                       real_out.file, pos_out, len,
> +                                                       flags);
> +                       break;
> +               }
>         }
> -       revert_creds_light(old_cred);
> -
>         /* Update size */
>         ovl_file_modified(file_out);
>

Maybe we should just place cred_guard(ovl_creds(file_inode(file_out)->i_sb))
in ovl_copy_file_range()?

I don't think that the order of ovl_override_creds() vs. inode_lock()
really matters?

Thanks,
Amir.

Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Vinicius Costa Gomes]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> Add a comment to these operations that cannot use the _light version
>> of override_creds()/revert_creds(), because during the critical
>> section the struct cred .usage counter might be modified.
>
> Why is it a problem if the usage counter is modified?  Why is the
> counter modified in each of these cases?
>

Working on getting some logs from the crash that I get when I convert
the remaining cases to use the _light() functions.

Perhaps I was wrong on my interpretation of the crash.

Thanks for raising this, I should have added more information about this.


Cheers,
-- 
Vinicius

Re: [PATCH v2 05/16] overlayfs: Use ovl_override_creds_light()/revert_creds_light()

[Vinicius Costa Gomes]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> Convert to use ovl_override_creds_light()/revert_creds_light(), these
>> functions assume that the critical section won't modify the usage
>> counter of the credentials in question.
>>
>> In most overlayfs instances, the credentials lifetime is the duration
>
> Why most instances?  AFAICS the creds have the same lifetime as the
> overlayfs superblock.
>

I may be reading the code wrong, but on file creation some of the
credentials that are worked on come from the task(?).


Cheers,
-- 
Vinicius

Re: [PATCH v2 07/16] fs/backing-file: Convert to cred_guard()

[Vinicius Costa Gomes]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> Replace the override_creds_light()/revert_creds_light() pairs of
>> operations to cred_guard().
>
> I'd note here, that in some cases the revert will happen later than
> previously, but (hopefully) you have verified that in these cases it
> won't make a difference.
>

Will add this note to the commit message.

> Thanks,
> Miklos


Cheers,
-- 
Vinicius

Re: [PATCH v2 10/16] overlayfs/file: Convert to cred_guard()

[Vinicius Costa Gomes]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> Replace the override_creds_light()/revert_creds_light() pairs of
>> operations with cred_guard()/cred_scoped_guard().
>>
>> Only ovl_copyfile() and ovl_fallocate() use cred_scoped_guard(),
>> because of 'goto', which can cause the cleanup flow to run on garbage
>> memory.
>
> This doesn't sound good.  Is this a compiler bug or a limitation of guards?
>

This is a gcc bug, that it accepts invalid code: i.e. with a goto you
can skip the declaration of a variable and as the cleanup is inserted by
the compiler unconditionally, the cleanup will run with garbage value.
clang refuses to compile and emits an error.

Link to a simpler version of the bug I am seeing:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951

>> @@ -211,9 +208,8 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
>>         ovl_inode_lock(inode);
>>         real.file->f_pos = file->f_pos;
>>
>> -       old_cred = ovl_override_creds_light(inode->i_sb);
>> +       cred_guard(ovl_creds(inode->i_sb));
>>         ret = vfs_llseek(real.file, offset, whence);
>> -       revert_creds_light(old_cred);
>
> Why not use scoped guard, like in fallocate?

No reason. I was only under the impression that cred_guard() was
preferred over cred_scoped_guard(). 

>
>> @@ -398,9 +393,8 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>>
>>         /* Don't sync lower file for fear of receiving EROFS error */
>>         if (file_inode(real.file) == ovl_inode_upper(file_inode(file))) {
>> -               old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
>> +               cred_guard(ovl_creds(file_inode(file)->i_sb));
>>                 ret = vfs_fsync_range(real.file, start, end, datasync);
>> -               revert_creds_light(old_cred);
>
> Same here.
>

Will keep it consistent whatever version is chosen.

>> @@ -584,9 +571,8 @@ static int ovl_flush(struct file *file, fl_owner_t id)
>>                 return err;
>>
>>         if (real.file->f_op->flush) {
>> -               old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
>> +               cred_guard(ovl_creds(file_inode(file)->i_sb));
>
> What's the scope of this?  The function or the inner block?
>

As far as I understand, the inner block.

> Thanks,
> Miklos


Cheers,
-- 
Vinicius

Re: [PATCH v2 10/16] overlayfs/file: Convert to cred_guard()

[Vinicius Costa Gomes]

Amir Goldstein <amir73il@gmail.com> writes:
>> -       old_cred = ovl_override_creds_light(file_inode(file)->i_sb);
>> -       ret = vfs_fallocate(real.file, mode, offset, len);
>> -       revert_creds_light(old_cred);
>> +       cred_scoped_guard(ovl_creds(file_inode(file)->i_sb))
>> +               ret = vfs_fallocate(real.file, mode, offset, len);
>>
>
> I find this syntax confusing. Even though it is a valid syntax,
> I prefer that if there is a scope we use explicit brackets for it even
> if the scope is
> a single line.
>

Will add the brackets.

> How about using:
>        {
>                cred_guard(ovl_creds(file_inode(file)->i_sb));
>                ret = vfs_fallocate(real.file, mode, offset, len);
>        }
>
> It is more clear and helps averting the compiler bug(?).

I prefer the scoped_cred_guard() idiom, having it spelled out sounds
better to me. But a new block should avoid the bug as well.

>
> Maybe we should just place cred_guard(ovl_creds(file_inode(file_out)->i_sb))
> in ovl_copy_file_range()?
>
> I don't think that the order of ovl_override_creds() vs. inode_lock()
> really matters?
>

Most probably the order should not matter. Will change this.

> Thanks,
> Amir.


Cheers,
-- 
Vinicius

Re: [PATCH v2 16/16] overlayfs: Remove ovl_override_creds_light()

[Vinicius Costa Gomes]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> Remove the declaration of this unsafe helper.
>>
>> As the GUARD() helper guarantees that the cleanup will run, it is less
>> error prone.
>
> This statement is somewhat dubious.
>
> I suggest that unless and until the goto issue can be fixed the
> conversion to guards is postponed.

That's a good point. I just want to point out that the issue is only
with the combination of the "plain" (not scoped) GUARD() statements and
'goto'.

But I would be happy to postpone that, if the trouble is not worth it.
As all the performance gains come from the conversion to the _light()
helpers.

>
> Thanks,
> Miklos

Cheers,
-- 
Vinicius

Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Vinicius Costa Gomes]

Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:

> Miklos Szeredi <miklos@szeredi.hu> writes:
>
>> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
>> <vinicius.gomes@intel.com> wrote:
>>>
>>> Add a comment to these operations that cannot use the _light version
>>> of override_creds()/revert_creds(), because during the critical
>>> section the struct cred .usage counter might be modified.
>>
>> Why is it a problem if the usage counter is modified?  Why is the
>> counter modified in each of these cases?
>>
>
> Working on getting some logs from the crash that I get when I convert
> the remaining cases to use the _light() functions.
>

See the log below.

> Perhaps I was wrong on my interpretation of the crash.
>

What I am seeing is that ovl_setup_cred_for_create() has a "side
effect", it creates another set of credentials, runs the security hooks
with this new credentials, and the side effect is that when it returns,
by design, 'current->cred' is this new credentials (a third set of
credentials).

And this implies that refcounting for this is somewhat tricky, as said
in commit d0e13f5bbe4b ("ovl: fix uid/gid when creating over whiteout").

I see two ways forward:

1. Keep using the non _light() versions in functions that call
   ovl_setup_cred_for_create().
2. Change ovl_setup_cred_for_create() so it doesn't drop the "extra"
   refcount.

I went with (1), and it still sounds to me like the best way, but I
agree that my explanation was not good enough, will add the information
I just learned to the commit message and to the code.

Do you see another way forward? Or do you think that I should go with
(2)?

> Thanks for raising this, I should have added more information about this.
>
>
> Cheers,
> -- 
> Vinicius

[    4.646955] [touch 1512] commit_creds(0000000009e62474{1})
[    4.648637] [touch 1512] __put_cred(00000000200a9944{0})
[    4.648844] [virtm 1502] prepare_creds() alloc 0000000050563530
[    4.651631] [virtm 1513] prepare_creds() alloc 00000000da716e80
[    4.652515] [mktem 1513] commit_creds(00000000da716e80{1})
[    4.654056] ovl_create_or_link: [override] cred 0000000007112f42
[    4.654108] ovl_override_creds_light: new cred 0000000007112f42{1}
[    4.654155] ovl_override_creds_light: old cred 00000000da716e80{3}
[    4.654199] [mktem 1513] prepare_creds() alloc 000000003c8d17b7
[    4.654246] [mktem 1513] override_creds(000000003c8d17b7{1})
[    4.654292] [mktem 1513] override_creds() = 0000000007112f42{1}
[    4.654337] [mktem 1513] __put_cred(0000000007112f42{0})
[    4.654388] [mktem 1513] __put_cred(0000000007112f42{0})
[    4.654431] ------------[ cut here ]------------
[    4.654470] ODEBUG: activate active (active state 1) object: 00000000ad88840d object type: rcu_head hint: 0x0
[    4.654484] [swapp    0] exit_creds(1507,00000000efafcffd,00000000efafcffd,{2})
[    4.654575] WARNING: CPU: 23 PID: 1513 at lib/debugobjects.c:515 debug_print_object+0x7d/0xb0
[    4.654596] [swapp    0] __put_cred(00000000efafcffd{0})
[    4.654674] Modules linked in: sha512_ssse3(E) isst_if_common(E-) crct10dif_pclmul(E) sha256_ssse3(E) skx_edac_common(E) nfit(E) virtio_net(E) net_failover(E) i2c_piix4(E) input_leds(E) psmouse(E) serio_raw(E) failover(E) i2c_smbus(E) pata_acpi(E) floppy(E) qemu_fw_cfg(E) mac_hid(E) overlay(E) 9pnet_virtio(E) virtiofs(E) 9p(E) 9pnet(E) netfs(E)
[    4.654686] CPU: 23 UID: 0 PID: 1513 Comm: mktemp Tainted: G            E      6.11.0-rc5+ #4
[    4.654689] Tainted: [E]=UNSIGNED_MODULE
[    4.654689] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
[    4.654690] RIP: 0010:debug_print_object+0x7d/0xb0
[    4.654692] Code: 01 8b 4b 14 48 c7 c7 d8 ce 3a 99 89 15 ec 53 77 02 8b 53 10 50 4c 8b 4d 00 48 8b 14 d5 80 32 e8 98 4c 8b 43 18 e8 73 fb a0 ff <0f> 0b 58 83 05 dd 35 c6 01 01 48 83 c4 08 5b 5d c3 cc cc cc cc 83
[    4.654693] RSP: 0018:ff5fa086c391bd28 EFLAGS: 00010282
[    4.654695] RAX: 0000000000000000 RBX: ff5fa086c391bd60 RCX: 0000000000140017
[    4.654696] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
[    4.654697] RBP: ffffffff98e28c40 R08: 0000000000000000 R09: ff4deb8a8531a0a8
[    4.654697] R10: 0000000000000000 R11: 0000000000000001 R12: ff4deb8a87d29de8
[    4.654698] R13: ffffffff98e28c40 R14: 0000000000000202 R15: ffffffff9aa64e58
[    4.654699] FS:  00007ff8543af740(0000) GS:ff4deb8ab8580000(0000) knlGS:0000000000000000
[    4.654700] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    4.654701] CR2: 00007ff8540ec040 CR3: 0000000005784002 CR4: 0000000000771ef0
[    4.654704] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    4.654705] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
[    4.654706] PKRU: 55555554
[    4.654707] Call Trace:
[    4.654709]  <TASK>
[    4.654710]  ? __warn+0x83/0x130
[    4.654725]  ? debug_print_object+0x7d/0xb0
[    4.654726]  ? report_bug+0x18e/0x1a0
[    4.654773] [swapp    0] exit_creds(1508,00000000b957e777,00000000b957e777,{2})


Cheers,
-- 
Vinicius

Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Christian Brauner]

On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
> Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
> 
> > Miklos Szeredi <miklos@szeredi.hu> writes:
> >
> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> >> <vinicius.gomes@intel.com> wrote:
> >>>
> >>> Add a comment to these operations that cannot use the _light version
> >>> of override_creds()/revert_creds(), because during the critical
> >>> section the struct cred .usage counter might be modified.
> >>
> >> Why is it a problem if the usage counter is modified?  Why is the
> >> counter modified in each of these cases?
> >>
> >
> > Working on getting some logs from the crash that I get when I convert
> > the remaining cases to use the _light() functions.
> >
> 
> See the log below.
> 
> > Perhaps I was wrong on my interpretation of the crash.
> >
> 
> What I am seeing is that ovl_setup_cred_for_create() has a "side
> effect", it creates another set of credentials, runs the security hooks
> with this new credentials, and the side effect is that when it returns,
> by design, 'current->cred' is this new credentials (a third set of
> credentials).

Well yes, during ovl_setup_cred_for_create() the fs{g,u}id needs to be
overwritten. But I'm stil confused what the exact problem is as it was
always clear that ovl_setup_cred_for_create() wouldn't be ported to
light variants.

/me looks...

> 
> And this implies that refcounting for this is somewhat tricky, as said
> in commit d0e13f5bbe4b ("ovl: fix uid/gid when creating over whiteout").
> 
> I see two ways forward:
> 
> 1. Keep using the non _light() versions in functions that call
>    ovl_setup_cred_for_create().
> 2. Change ovl_setup_cred_for_create() so it doesn't drop the "extra"
>    refcount.
> 
> I went with (1), and it still sounds to me like the best way, but I
> agree that my explanation was not good enough, will add the information
> I just learned to the commit message and to the code.
> 
> Do you see another way forward? Or do you think that I should go with
> (2)?

... ok, I understand. Say we have:

ovl_create_tmpfile()
/* current->cred == ovl->creator_cred without refcount bump /*
old_cred = ovl_override_creds_light()
-> ovl_setup_cred_for_create()
   /* Copy current->cred == ovl->creator_cred */
   modifiable_cred = prepare_creds()

   /* Override current->cred == modifiable_cred */
   mounter_creds = override_creds(modifiable_cred)

   /*
    * And here's the BUG BUG BUG where we decrement the refcount on the
    * constant mounter_creds.
    */
   put_cred(mounter_creds) // BUG BUG BUG

   put_cred(modifiable_creds)

So (1) is definitely the wrong option given that we can get rid of
refcount decs and incs in the creation path.

Imo, you should do (2) and add a WARN_ON_ONC(). Something like the
__completely untested__:

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index ab65e98a1def..e246e0172bb6 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
                put_cred(override_cred);
                return err;
        }
-       put_cred(override_creds(override_cred));
+
+       /*
+        * We must be called with creator creds already, otherwise we risk
+        * leaking creds.
+        */
+       WARN_ON_ONCE(override_creds(override_cred) != ovl_creds(dentry->d_sb));
        put_cred(override_cred);

        return 0;

> 
> > Thanks for raising this, I should have added more information about this.
> >
> >
> > Cheers,
> > -- 
> > Vinicius
> 
> [    4.646955] [touch 1512] commit_creds(0000000009e62474{1})
> [    4.648637] [touch 1512] __put_cred(00000000200a9944{0})
> [    4.648844] [virtm 1502] prepare_creds() alloc 0000000050563530
> [    4.651631] [virtm 1513] prepare_creds() alloc 00000000da716e80
> [    4.652515] [mktem 1513] commit_creds(00000000da716e80{1})
> [    4.654056] ovl_create_or_link: [override] cred 0000000007112f42
> [    4.654108] ovl_override_creds_light: new cred 0000000007112f42{1}
> [    4.654155] ovl_override_creds_light: old cred 00000000da716e80{3}
> [    4.654199] [mktem 1513] prepare_creds() alloc 000000003c8d17b7
> [    4.654246] [mktem 1513] override_creds(000000003c8d17b7{1})
> [    4.654292] [mktem 1513] override_creds() = 0000000007112f42{1}
> [    4.654337] [mktem 1513] __put_cred(0000000007112f42{0})
> [    4.654388] [mktem 1513] __put_cred(0000000007112f42{0})
> [    4.654431] ------------[ cut here ]------------
> [    4.654470] ODEBUG: activate active (active state 1) object: 00000000ad88840d object type: rcu_head hint: 0x0
> [    4.654484] [swapp    0] exit_creds(1507,00000000efafcffd,00000000efafcffd,{2})
> [    4.654575] WARNING: CPU: 23 PID: 1513 at lib/debugobjects.c:515 debug_print_object+0x7d/0xb0
> [    4.654596] [swapp    0] __put_cred(00000000efafcffd{0})
> [    4.654674] Modules linked in: sha512_ssse3(E) isst_if_common(E-) crct10dif_pclmul(E) sha256_ssse3(E) skx_edac_common(E) nfit(E) virtio_net(E) net_failover(E) i2c_piix4(E) input_leds(E) psmouse(E) serio_raw(E) failover(E) i2c_smbus(E) pata_acpi(E) floppy(E) qemu_fw_cfg(E) mac_hid(E) overlay(E) 9pnet_virtio(E) virtiofs(E) 9p(E) 9pnet(E) netfs(E)
> [    4.654686] CPU: 23 UID: 0 PID: 1513 Comm: mktemp Tainted: G            E      6.11.0-rc5+ #4
> [    4.654689] Tainted: [E]=UNSIGNED_MODULE
> [    4.654689] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> [    4.654690] RIP: 0010:debug_print_object+0x7d/0xb0
> [    4.654692] Code: 01 8b 4b 14 48 c7 c7 d8 ce 3a 99 89 15 ec 53 77 02 8b 53 10 50 4c 8b 4d 00 48 8b 14 d5 80 32 e8 98 4c 8b 43 18 e8 73 fb a0 ff <0f> 0b 58 83 05 dd 35 c6 01 01 48 83 c4 08 5b 5d c3 cc cc cc cc 83
> [    4.654693] RSP: 0018:ff5fa086c391bd28 EFLAGS: 00010282
> [    4.654695] RAX: 0000000000000000 RBX: ff5fa086c391bd60 RCX: 0000000000140017
> [    4.654696] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
> [    4.654697] RBP: ffffffff98e28c40 R08: 0000000000000000 R09: ff4deb8a8531a0a8
> [    4.654697] R10: 0000000000000000 R11: 0000000000000001 R12: ff4deb8a87d29de8
> [    4.654698] R13: ffffffff98e28c40 R14: 0000000000000202 R15: ffffffff9aa64e58
> [    4.654699] FS:  00007ff8543af740(0000) GS:ff4deb8ab8580000(0000) knlGS:0000000000000000
> [    4.654700] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    4.654701] CR2: 00007ff8540ec040 CR3: 0000000005784002 CR4: 0000000000771ef0
> [    4.654704] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    4.654705] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
> [    4.654706] PKRU: 55555554
> [    4.654707] Call Trace:
> [    4.654709]  <TASK>
> [    4.654710]  ? __warn+0x83/0x130
> [    4.654725]  ? debug_print_object+0x7d/0xb0
> [    4.654726]  ? report_bug+0x18e/0x1a0
> [    4.654773] [swapp    0] exit_creds(1508,00000000b957e777,00000000b957e777,{2})
> 
> 
> Cheers,
> -- 
> Vinicius

Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Vinicius Costa Gomes]

Christian Brauner <brauner@kernel.org> writes:

> On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
>> Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
>> 
>> > Miklos Szeredi <miklos@szeredi.hu> writes:
>> >
>> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
>> >> <vinicius.gomes@intel.com> wrote:
>> >>>
>> >>> Add a comment to these operations that cannot use the _light version
>> >>> of override_creds()/revert_creds(), because during the critical
>> >>> section the struct cred .usage counter might be modified.
>> >>
>> >> Why is it a problem if the usage counter is modified?  Why is the
>> >> counter modified in each of these cases?
>> >>
>> >
>> > Working on getting some logs from the crash that I get when I convert
>> > the remaining cases to use the _light() functions.
>> >
>> 
>> See the log below.
>> 
>> > Perhaps I was wrong on my interpretation of the crash.
>> >
>> 
>> What I am seeing is that ovl_setup_cred_for_create() has a "side
>> effect", it creates another set of credentials, runs the security hooks
>> with this new credentials, and the side effect is that when it returns,
>> by design, 'current->cred' is this new credentials (a third set of
>> credentials).
>
> Well yes, during ovl_setup_cred_for_create() the fs{g,u}id needs to be
> overwritten. But I'm stil confused what the exact problem is as it was
> always clear that ovl_setup_cred_for_create() wouldn't be ported to
> light variants.
>
> /me looks...
>
>> 
>> And this implies that refcounting for this is somewhat tricky, as said
>> in commit d0e13f5bbe4b ("ovl: fix uid/gid when creating over whiteout").
>> 
>> I see two ways forward:
>> 
>> 1. Keep using the non _light() versions in functions that call
>>    ovl_setup_cred_for_create().
>> 2. Change ovl_setup_cred_for_create() so it doesn't drop the "extra"
>>    refcount.
>> 
>> I went with (1), and it still sounds to me like the best way, but I
>> agree that my explanation was not good enough, will add the information
>> I just learned to the commit message and to the code.
>> 
>> Do you see another way forward? Or do you think that I should go with
>> (2)?
>
> ... ok, I understand. Say we have:
>
> ovl_create_tmpfile()
> /* current->cred == ovl->creator_cred without refcount bump /*
> old_cred = ovl_override_creds_light()
> -> ovl_setup_cred_for_create()
>    /* Copy current->cred == ovl->creator_cred */
>    modifiable_cred = prepare_creds()
>
>    /* Override current->cred == modifiable_cred */
>    mounter_creds = override_creds(modifiable_cred)
>
>    /*
>     * And here's the BUG BUG BUG where we decrement the refcount on the
>     * constant mounter_creds.
>     */
>    put_cred(mounter_creds) // BUG BUG BUG
>
>    put_cred(modifiable_creds)
>
> So (1) is definitely the wrong option given that we can get rid of
> refcount decs and incs in the creation path.
>
> Imo, you should do (2) and add a WARN_ON_ONC(). Something like the
> __completely untested__:
>

> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index ab65e98a1def..e246e0172bb6 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
>                 put_cred(override_cred);
>                 return err;
>         }
> -       put_cred(override_creds(override_cred));
> +
> +       /*
> +        * We must be called with creator creds already, otherwise we risk
> +        * leaking creds.
> +        */
> +       WARN_ON_ONCE(override_creds(override_cred) != ovl_creds(dentry->d_sb));
>         put_cred(override_cred);
>
>         return 0;
>

At first glance, looks good. Going to test it and see how it works.
Thank you.

For the next version of the series, my plan is to include this
suggestion/change and remove the guard()/scoped_guard() conversion
patches from the series.


Cheers,
-- 
Vinicius

Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Amir Goldstein]

On Wed, Sep 25, 2024 at 4:17 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Christian Brauner <brauner@kernel.org> writes:
>
> > On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
> >> Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
> >>
> >> > Miklos Szeredi <miklos@szeredi.hu> writes:
> >> >
> >> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
> >> >> <vinicius.gomes@intel.com> wrote:
> >> >>>
> >> >>> Add a comment to these operations that cannot use the _light version
> >> >>> of override_creds()/revert_creds(), because during the critical
> >> >>> section the struct cred .usage counter might be modified.
> >> >>
> >> >> Why is it a problem if the usage counter is modified?  Why is the
> >> >> counter modified in each of these cases?
> >> >>
> >> >
> >> > Working on getting some logs from the crash that I get when I convert
> >> > the remaining cases to use the _light() functions.
> >> >
> >>
> >> See the log below.
> >>
> >> > Perhaps I was wrong on my interpretation of the crash.
> >> >
> >>
> >> What I am seeing is that ovl_setup_cred_for_create() has a "side
> >> effect", it creates another set of credentials, runs the security hooks
> >> with this new credentials, and the side effect is that when it returns,
> >> by design, 'current->cred' is this new credentials (a third set of
> >> credentials).
> >
> > Well yes, during ovl_setup_cred_for_create() the fs{g,u}id needs to be
> > overwritten. But I'm stil confused what the exact problem is as it was
> > always clear that ovl_setup_cred_for_create() wouldn't be ported to
> > light variants.
> >
> > /me looks...
> >
> >>
> >> And this implies that refcounting for this is somewhat tricky, as said
> >> in commit d0e13f5bbe4b ("ovl: fix uid/gid when creating over whiteout").
> >>
> >> I see two ways forward:
> >>
> >> 1. Keep using the non _light() versions in functions that call
> >>    ovl_setup_cred_for_create().
> >> 2. Change ovl_setup_cred_for_create() so it doesn't drop the "extra"
> >>    refcount.
> >>
> >> I went with (1), and it still sounds to me like the best way, but I
> >> agree that my explanation was not good enough, will add the information
> >> I just learned to the commit message and to the code.
> >>
> >> Do you see another way forward? Or do you think that I should go with
> >> (2)?
> >
> > ... ok, I understand. Say we have:
> >
> > ovl_create_tmpfile()
> > /* current->cred == ovl->creator_cred without refcount bump /*
> > old_cred = ovl_override_creds_light()
> > -> ovl_setup_cred_for_create()
> >    /* Copy current->cred == ovl->creator_cred */
> >    modifiable_cred = prepare_creds()
> >
> >    /* Override current->cred == modifiable_cred */
> >    mounter_creds = override_creds(modifiable_cred)
> >
> >    /*
> >     * And here's the BUG BUG BUG where we decrement the refcount on the
> >     * constant mounter_creds.
> >     */
> >    put_cred(mounter_creds) // BUG BUG BUG
> >
> >    put_cred(modifiable_creds)
> >
> > So (1) is definitely the wrong option given that we can get rid of
> > refcount decs and incs in the creation path.
> >
> > Imo, you should do (2) and add a WARN_ON_ONC(). Something like the
> > __completely untested__:
> >
>
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index ab65e98a1def..e246e0172bb6 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
> >                 put_cred(override_cred);
> >                 return err;
> >         }
> > -       put_cred(override_creds(override_cred));
> > +
> > +       /*
> > +        * We must be called with creator creds already, otherwise we risk
> > +        * leaking creds.
> > +        */
> > +       WARN_ON_ONCE(override_creds(override_cred) != ovl_creds(dentry->d_sb));
> >         put_cred(override_cred);
> >
> >         return 0;
> >
>
> At first glance, looks good. Going to test it and see how it works.
> Thank you.
>
> For the next version of the series, my plan is to include this
> suggestion/change and remove the guard()/scoped_guard() conversion
> patches from the series.
>

Vinicius,

I have a request. Since the plan is to keep the _light() helpers around for the
time being, please make the existing helper ovl_override_creds() use the
light version and open code the non-light versions in the few places where
they are needed and please replace all the matching call sites of
revert_creds() to
a helper ovl_revert_creds() that is a wrapper for the light version.

Also, depending on when you intend to post your work for review,
I have a feeling that the review of my patches is going to be done
before your submit your patches for review, so you may want to consider
already basing your patches on top of my development branch [2] to avoid
conflicts later.

Anyway, the parts of my patches that conflict with yours (s/real.file/realfile/)
are not likely to change anymore.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/20241006082359.263755-1-amir73il@gmail.com/
[2] https://github.com/amir73il/linux/commits/ovl_real_file/

Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Vinicius Costa Gomes]

Hi,

Amir Goldstein <amir73il@gmail.com> writes:

> On Wed, Sep 25, 2024 at 4:17 PM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> Christian Brauner <brauner@kernel.org> writes:
>>
>> > On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
>> >> Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
>> >>
>> >> > Miklos Szeredi <miklos@szeredi.hu> writes:
>> >> >
>> >> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
>> >> >> <vinicius.gomes@intel.com> wrote:
>> >> >>>
>> >> >>> Add a comment to these operations that cannot use the _light version
>> >> >>> of override_creds()/revert_creds(), because during the critical
>> >> >>> section the struct cred .usage counter might be modified.
>> >> >>
>> >> >> Why is it a problem if the usage counter is modified?  Why is the
>> >> >> counter modified in each of these cases?
>> >> >>
>> >> >
>> >> > Working on getting some logs from the crash that I get when I convert
>> >> > the remaining cases to use the _light() functions.
>> >> >
>> >>
>> >> See the log below.
>> >>
>> >> > Perhaps I was wrong on my interpretation of the crash.
>> >> >
>> >>
>> >> What I am seeing is that ovl_setup_cred_for_create() has a "side
>> >> effect", it creates another set of credentials, runs the security hooks
>> >> with this new credentials, and the side effect is that when it returns,
>> >> by design, 'current->cred' is this new credentials (a third set of
>> >> credentials).
>> >
>> > Well yes, during ovl_setup_cred_for_create() the fs{g,u}id needs to be
>> > overwritten. But I'm stil confused what the exact problem is as it was
>> > always clear that ovl_setup_cred_for_create() wouldn't be ported to
>> > light variants.
>> >
>> > /me looks...
>> >
>> >>
>> >> And this implies that refcounting for this is somewhat tricky, as said
>> >> in commit d0e13f5bbe4b ("ovl: fix uid/gid when creating over whiteout").
>> >>
>> >> I see two ways forward:
>> >>
>> >> 1. Keep using the non _light() versions in functions that call
>> >>    ovl_setup_cred_for_create().
>> >> 2. Change ovl_setup_cred_for_create() so it doesn't drop the "extra"
>> >>    refcount.
>> >>
>> >> I went with (1), and it still sounds to me like the best way, but I
>> >> agree that my explanation was not good enough, will add the information
>> >> I just learned to the commit message and to the code.
>> >>
>> >> Do you see another way forward? Or do you think that I should go with
>> >> (2)?
>> >
>> > ... ok, I understand. Say we have:
>> >
>> > ovl_create_tmpfile()
>> > /* current->cred == ovl->creator_cred without refcount bump /*
>> > old_cred = ovl_override_creds_light()
>> > -> ovl_setup_cred_for_create()
>> >    /* Copy current->cred == ovl->creator_cred */
>> >    modifiable_cred = prepare_creds()
>> >
>> >    /* Override current->cred == modifiable_cred */
>> >    mounter_creds = override_creds(modifiable_cred)
>> >
>> >    /*
>> >     * And here's the BUG BUG BUG where we decrement the refcount on the
>> >     * constant mounter_creds.
>> >     */
>> >    put_cred(mounter_creds) // BUG BUG BUG
>> >
>> >    put_cred(modifiable_creds)
>> >
>> > So (1) is definitely the wrong option given that we can get rid of
>> > refcount decs and incs in the creation path.
>> >
>> > Imo, you should do (2) and add a WARN_ON_ONC(). Something like the
>> > __completely untested__:
>> >
>>
>> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> > index ab65e98a1def..e246e0172bb6 100644
>> > --- a/fs/overlayfs/dir.c
>> > +++ b/fs/overlayfs/dir.c
>> > @@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
>> >                 put_cred(override_cred);
>> >                 return err;
>> >         }
>> > -       put_cred(override_creds(override_cred));
>> > +
>> > +       /*
>> > +        * We must be called with creator creds already, otherwise we risk
>> > +        * leaking creds.
>> > +        */
>> > +       WARN_ON_ONCE(override_creds(override_cred) != ovl_creds(dentry->d_sb));
>> >         put_cred(override_cred);
>> >
>> >         return 0;
>> >
>>
>> At first glance, looks good. Going to test it and see how it works.
>> Thank you.
>>
>> For the next version of the series, my plan is to include this
>> suggestion/change and remove the guard()/scoped_guard() conversion
>> patches from the series.
>>
>
> Vinicius,
>
> I have a request. Since the plan is to keep the _light() helpers around for the
> time being, please make the existing helper ovl_override_creds() use the
> light version and open code the non-light versions in the few places where
> they are needed and please replace all the matching call sites of
> revert_creds() to
> a helper ovl_revert_creds() that is a wrapper for the light version.
>

Seems like a good idea. Will do that, and see how it looks.

> Also, depending on when you intend to post your work for review,
> I have a feeling that the review of my patches is going to be done
> before your submit your patches for review, so you may want to consider
> already basing your patches on top of my development branch [2] to avoid
> conflicts later.
>

Thanks for the heads up. Will rebase my code on top of your branch.

> Anyway, the parts of my patches that conflict with yours (s/real.file/realfile/)
> are not likely to change anymore.
>
> Thanks,
> Amir.
>
> [1] https://lore.kernel.org/linux-unionfs/20241006082359.263755-1-amir73il@gmail.com/
> [2] https://github.com/amir73il/linux/commits/ovl_real_file/


Cheers,
-- 
Vinicius