[PATCH v4 0/4] overlayfs: Optimize override/revert creds

[PATCH v4 0/4] overlayfs: Optimize override/revert creds

[Vinicius Costa Gomes]

Hi,

Changes from v3:
 - Another reorganization of the series: separate the pure mechanical
   changes into their own (Amir Goldstein)

The series now reads:

Patch 1: Introduce the _light() version of the override/revert cred operations;
Patch 2: Convert backing-file.c to use those;
Patch 3: Mechanical change to introduce the ovl_revert_creds() helper;
Patch 4: Make the ovl_{override,convert}_creds() use the _light()
  creds helpers, and fix the reference counting issue that would happen;

Changes from v2:
 - Removed the "convert to guard()/scoped_guard()" patches (Miklos Szeredi);
 - In the overlayfs code, convert all users of override_creds()/revert_creds() to the _light() versions by:
      1. making ovl_override_creds() use override_creds_light();
      2. introduce ovl_revert_creds() which calls revert_creds_light();
      3. convert revert_creds() to ovl_revert_creds()
   (Amir Goldstein);
 - Fix an potential reference counting issue, as the lifetime
   expectations of the mounter credentials are different (Christian
   Brauner);

The series is now much simpler:

Patch 1: Introduce the _light() version of the override/revert cred operations;
Patch 2: Convert backing-file.c to use those;
Patch 3: Do the conversion to use the _light() version internally;
Patch 4: Fix a potential refcounting issue

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 (4):
  cred: Add a light version of override/revert_creds()
  fs/backing-file: Convert to revert/override_creds_light()
  ovl: use wrapper ovl_revert_creds()
  ovl: Optimize override/revert creds

 fs/backing-file.c        | 20 ++++++++++----------
 fs/overlayfs/copy_up.c   |  2 +-
 fs/overlayfs/dir.c       | 17 +++++++++++------
 fs/overlayfs/file.c      | 14 +++++++-------
 fs/overlayfs/inode.c     | 20 ++++++++++----------
 fs/overlayfs/namei.c     | 10 +++++-----
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/readdir.c   |  8 ++++----
 fs/overlayfs/util.c      | 11 ++++++++---
 fs/overlayfs/xattrs.c    |  9 ++++-----
 include/linux/cred.h     | 18 ++++++++++++++++++
 kernel/cred.c            |  6 +++---
 12 files changed, 82 insertions(+), 54 deletions(-)

-- 
2.47.0

[PATCH v4 1/4] 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.47.0

[PATCH v4 2/4] 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 a38737592ec7..526ddb4d6f76 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(iocb->ki_filp);
@@ -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, struct kiocb *iocb,
 	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, &iocb->ki_pos, pipe, len, flags);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	if (ctx->accessed)
 		ctx->accessed(iocb->ki_filp);
@@ -310,11 +310,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 = out->f_op->splice_write(pipe, out, &iocb->ki_pos, len, flags);
 	file_end_write(out);
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 
 	if (ctx->end_write)
 		ctx->end_write(iocb, ret);
@@ -337,9 +337,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(vma->vm_file);
-- 
2.47.0

[PATCH v4 3/4] ovl: use wrapper ovl_revert_creds()

[Vinicius Costa Gomes]

Introduce ovl_revert_creds() wrapper of revert_creds() to
match callers of ovl_override_creds().

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c   |  2 +-
 fs/overlayfs/dir.c       | 10 +++++-----
 fs/overlayfs/file.c      | 14 +++++++-------
 fs/overlayfs/inode.c     | 20 ++++++++++----------
 fs/overlayfs/namei.c     | 10 +++++-----
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/readdir.c   |  8 ++++----
 fs/overlayfs/util.c      |  9 +++++++--
 fs/overlayfs/xattrs.c    |  9 ++++-----
 9 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 2ed6ad641a20..dafd1c71b977 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1260,7 +1260,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		dput(parent);
 		dput(next);
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index ab65e98a1def..09db5eb19242 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -621,7 +621,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		err = ovl_create_over_whiteout(dentry, inode, attr);
 
 out_revert_creds:
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return err;
 }
 
@@ -702,7 +702,7 @@ static int ovl_set_link_redirect(struct dentry *dentry)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_set_redirect(dentry, false);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -912,7 +912,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 		err = ovl_remove_upper(dentry, is_dir, &list);
 	else
 		err = ovl_remove_and_whiteout(dentry, &list);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (!err) {
 		if (is_dir)
 			clear_nlink(dentry->d_inode);
@@ -1292,7 +1292,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 out_unlock:
 	unlock_rename(new_upperdir, old_upperdir);
 out_revert_creds:
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (update_nlink)
 		ovl_nlink_end(new);
 	else
@@ -1337,7 +1337,7 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 		fput(realfile);
 	}
 out_revert_creds:
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return err;
 }
 
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 12c4d502ff91..608a88ff8d81 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -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);
+	ovl_revert_creds(old_cred);
 
 	pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
 		 file, file, ovl_whatisit(inode, realinode), file->f_flags,
@@ -215,7 +215,7 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	ret = vfs_llseek(fd_file(real), offset, whence);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	file->f_pos = fd_file(real)->f_pos;
 	ovl_inode_unlock(inode);
@@ -412,7 +412,7 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
 		old_cred = ovl_override_creds(file_inode(file)->i_sb);
 		ret = vfs_fsync_range(fd_file(real), start, end, datasync);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 	}
 
 	fdput(real);
@@ -451,7 +451,7 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	ret = vfs_fallocate(fd_file(real), mode, offset, len);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	/* Update size */
 	ovl_file_modified(file);
@@ -476,7 +476,7 @@ static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	ret = vfs_fadvise(fd_file(real), offset, len, advice);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	fdput(real);
 
@@ -535,7 +535,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 						flags);
 		break;
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	/* Update size */
 	ovl_file_modified(file_out);
@@ -597,7 +597,7 @@ static int ovl_flush(struct file *file, fl_owner_t id)
 	if (fd_file(real)->f_op->flush) {
 		old_cred = ovl_override_creds(file_inode(file)->i_sb);
 		err = fd_file(real)->f_op->flush(fd_file(real), id);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 	}
 	fdput(real);
 
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 35fd3e3e1778..6c3add3801be 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -80,7 +80,7 @@ int ovl_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 		inode_lock(upperdentry->d_inode);
 		old_cred = ovl_override_creds(dentry->d_sb);
 		err = ovl_do_notify_change(ofs, upperdentry, attr);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 		if (!err)
 			ovl_copyattr(dentry->d_inode);
 		inode_unlock(upperdentry->d_inode);
@@ -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);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -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);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -334,7 +334,7 @@ static const char *ovl_get_link(struct dentry *dentry,
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	p = vfs_get_link(ovl_dentry_real(dentry), done);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return p;
 }
 
@@ -469,7 +469,7 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
 
 		old_cred = ovl_override_creds(inode->i_sb);
 		acl = ovl_get_acl_path(&realpath, posix_acl_xattr_name(type), noperm);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 	}
 
 	return acl;
@@ -498,7 +498,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
 		old_cred = ovl_override_creds(dentry->d_sb);
 		real_acl = vfs_get_acl(mnt_idmap(realpath.mnt), realdentry,
 				       acl_name);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 		if (IS_ERR(real_acl)) {
 			err = PTR_ERR(real_acl);
 			goto out;
@@ -523,7 +523,7 @@ static int ovl_set_or_remove_acl(struct dentry *dentry, struct inode *inode,
 		err = ovl_do_set_acl(ofs, realdentry, acl_name, acl);
 	else
 		err = ovl_do_remove_acl(ofs, realdentry, acl_name);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	ovl_drop_write(dentry);
 
 	/* copy c/mtime */
@@ -600,7 +600,7 @@ static int ovl_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 
 	old_cred = ovl_override_creds(inode->i_sb);
 	err = realinode->i_op->fiemap(realinode, fieinfo, start, len);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -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);
+		ovl_revert_creds(old_cred);
 		ovl_drop_write(dentry);
 
 		/*
@@ -733,7 +733,7 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 	old_cred = ovl_override_creds(inode->i_sb);
 	err = ovl_real_fileattr_get(&realpath, fa);
 	ovl_fileattr_prot_flags(inode, fa);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5764f91d283e..7e27b7d4adee 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -961,7 +961,7 @@ static int ovl_maybe_validate_verity(struct dentry *dentry)
 		if (err == 0)
 			ovl_set_flag(OVL_VERIFIED_DIGEST, inode);
 
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 	}
 
 	ovl_inode_unlock(inode);
@@ -995,7 +995,7 @@ static int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_lookup_data_layers(dentry, redirect, &datapath);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (err)
 		goto out_err;
 
@@ -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);
+	ovl_revert_creds(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);
+	ovl_revert_creds(old_cred);
 	return ERR_PTR(err);
 }
 
@@ -1423,7 +1423,7 @@ bool ovl_lower_positive(struct dentry *dentry)
 			dput(this);
 		}
 	}
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return positive;
 }
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 0bfe35da4b7b..7b7a6e3a43e2 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);
+void ovl_revert_creds(const struct cred *old_cred);
 
 static inline const struct cred *ovl_creds(struct super_block *sb)
 {
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 0ca8af060b0c..881ec5592da5 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -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);
+	ovl_revert_creds(old_cred);
 
 	return err;
 }
@@ -808,7 +808,7 @@ static int ovl_iterate(struct file *file, struct dir_context *ctx)
 	}
 	err = 0;
 out:
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return err;
 }
 
@@ -860,7 +860,7 @@ static struct file *ovl_dir_open_realfile(const struct file *file,
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	res = ovl_path_open(realpath, O_RDONLY | (file->f_flags & O_LARGEFILE));
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 
 	return res;
 }
@@ -987,7 +987,7 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	err = ovl_dir_read_merged(dentry, list, &root);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	if (err)
 		return err;
 
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index edc9216f6e27..9408046f4f41 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -68,6 +68,11 @@ const struct cred *ovl_override_creds(struct super_block *sb)
 	return override_creds(ofs->creator_cred);
 }
 
+void ovl_revert_creds(const struct cred *old_cred)
+{
+	revert_creds(old_cred);
+}
+
 /*
  * Check if underlying fs supports file handles and try to determine encoding
  * type, in order to deduce maximum inode number used by fs.
@@ -1178,7 +1183,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);
+	ovl_revert_creds(old_cred);
 	if (err)
 		goto out_drop_write;
 
@@ -1203,7 +1208,7 @@ void ovl_nlink_end(struct dentry *dentry)
 
 		old_cred = ovl_override_creds(dentry->d_sb);
 		ovl_cleanup_index(dentry);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 	}
 
 	ovl_inode_unlock(inode);
diff --git a/fs/overlayfs/xattrs.c b/fs/overlayfs/xattrs.c
index 383978e4663c..88055deca936 100644
--- a/fs/overlayfs/xattrs.c
+++ b/fs/overlayfs/xattrs.c
@@ -47,7 +47,7 @@ static int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char
 		ovl_path_lower(dentry, &realpath);
 		old_cred = ovl_override_creds(dentry->d_sb);
 		err = vfs_getxattr(mnt_idmap(realpath.mnt), realdentry, name, NULL, 0);
-		revert_creds(old_cred);
+		ovl_revert_creds(old_cred);
 		if (err < 0)
 			goto out;
 	}
@@ -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);
+	ovl_revert_creds(old_cred);
 	ovl_drop_write(dentry);
 
 	/* copy c/mtime */
@@ -91,7 +91,7 @@ static int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char
 	ovl_i_path_real(inode, &realpath);
 	old_cred = ovl_override_creds(dentry->d_sb);
 	res = vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, value, size);
-	revert_creds(old_cred);
+	ovl_revert_creds(old_cred);
 	return res;
 }
 
@@ -121,7 +121,7 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
 
 	old_cred = ovl_override_creds(dentry->d_sb);
 	res = vfs_listxattr(realdentry, list, size);
-	revert_creds(old_cred);
+	ovl_revert_creds(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.47.0

[PATCH v4 4/4] ovl: Optimize override/revert creds

[Vinicius Costa Gomes]

Use override_creds_light() in ovl_override_creds() and
revert_creds_light() in ovl_revert_creds_light().

The _light() functions do not change the 'usage' of the credentials in
question, as they refer to the credentials associated with the
mounter, which have a longer lifetime.

In ovl_setup_cred_for_create(), do not need to modify the mounter
credentials (returned by override_creds()) 'usage' counter. Add a
warning to verify that we are indeed working with the mounter
credentials (stored in the superblock). Failure in this assumption
means that creds may leak.

Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 fs/overlayfs/dir.c  | 7 ++++++-
 fs/overlayfs/util.c | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 09db5eb19242..136a2c7fb9e5 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;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 9408046f4f41..3bb107471fb4 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -65,12 +65,12 @@ const struct cred *ovl_override_creds(struct super_block *sb)
 {
 	struct ovl_fs *ofs = OVL_FS(sb);
 
-	return override_creds(ofs->creator_cred);
+	return override_creds_light(ofs->creator_cred);
 }
 
 void ovl_revert_creds(const struct cred *old_cred)
 {
-	revert_creds(old_cred);
+	revert_creds_light(old_cred);
 }
 
 /*
-- 
2.47.0

Re: [PATCH v4 4/4] ovl: Optimize override/revert creds

[Amir Goldstein]

On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Use override_creds_light() in ovl_override_creds() and
> revert_creds_light() in ovl_revert_creds_light().
>
> The _light() functions do not change the 'usage' of the credentials in
> question, as they refer to the credentials associated with the
> mounter, which have a longer lifetime.
>
> In ovl_setup_cred_for_create(), do not need to modify the mounter
> credentials (returned by override_creds()) 'usage' counter. Add a
> warning to verify that we are indeed working with the mounter
> credentials (stored in the superblock). Failure in this assumption
> means that creds may leak.
>
> Suggested-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  fs/overlayfs/dir.c  | 7 ++++++-
>  fs/overlayfs/util.c | 4 ++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 09db5eb19242..136a2c7fb9e5 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;

Vinicius,

While testing fanotify with LTP tests (some are using overlayfs),
kmemleak consistently reports the problems below.

Can you see the bug, because I don't see it.
Maybe it is a false positive...

Christian, Miklos,

Can you see a problem?

Thanks,
Amir.


unreferenced object 0xffff888008ad8240 (size 192):
  comm "fanotify06", pid 1803, jiffies 4294890084
  hex dump (first 32 bytes):
    01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace (crc ee6a93ea):
    [<00000000ab4340a4>] __create_object+0x22/0x83
    [<0000000053dcaf3b>] kmem_cache_alloc_noprof+0x156/0x1e6
    [<00000000b4a08c1d>] prepare_creds+0x1d/0xf9
    [<00000000c55dfb6c>] ovl_setup_cred_for_create+0x27/0x93
    [<00000000f82af4ee>] ovl_create_or_link+0x73/0x1bd
    [<0000000040a439db>] ovl_create_object+0xda/0x11d
    [<00000000fbbadf17>] lookup_open.isra.0+0x3a0/0x3ff
    [<0000000007a2faf0>] open_last_lookups+0x160/0x223
    [<00000000e7d8243a>] path_openat+0x136/0x1b5
    [<0000000004e51585>] do_filp_open+0x57/0xb8
    [<0000000053871b92>] do_sys_openat2+0x6f/0xc0
    [<000000004d76b8b7>] do_sys_open+0x3f/0x60
    [<000000009b0be238>] do_syscall_64+0x96/0xf8
    [<000000006ff466ad>] entry_SYSCALL_64_after_hwframe+0x76/0x7e

Re: [PATCH v4 4/4] ovl: Optimize override/revert creds

[Vinicius Costa Gomes]

Amir Goldstein <amir73il@gmail.com> writes:

> On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:

[...]

>
> Vinicius,
>
> While testing fanotify with LTP tests (some are using overlayfs),
> kmemleak consistently reports the problems below.
>
> Can you see the bug, because I don't see it.
> Maybe it is a false positive...

Hm, if the leak wasn't there before and we didn't touch anything related to
prepare_creds(), I think that points to the leak being real.

But I see your point, still not seeing it.

This code should be equivalent to the code we have now (just boot
tested):

----
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 136a2c7fb9e5..7ebc2fd3097a 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -576,8 +576,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
         * 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);
+       WARN_ON_ONCE(override_creds_light(override_cred) != ovl_creds(dentry->d_sb));

        return 0;
 }
----

Does it change anything? (I wouldn't think so, just to try something)

>
> Christian, Miklos,
>
> Can you see a problem?
>
> Thanks,
> Amir.
>
>
> unreferenced object 0xffff888008ad8240 (size 192):
>   comm "fanotify06", pid 1803, jiffies 4294890084
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace (crc ee6a93ea):
>     [<00000000ab4340a4>] __create_object+0x22/0x83
>     [<0000000053dcaf3b>] kmem_cache_alloc_noprof+0x156/0x1e6
>     [<00000000b4a08c1d>] prepare_creds+0x1d/0xf9
>     [<00000000c55dfb6c>] ovl_setup_cred_for_create+0x27/0x93
>     [<00000000f82af4ee>] ovl_create_or_link+0x73/0x1bd
>     [<0000000040a439db>] ovl_create_object+0xda/0x11d
>     [<00000000fbbadf17>] lookup_open.isra.0+0x3a0/0x3ff
>     [<0000000007a2faf0>] open_last_lookups+0x160/0x223
>     [<00000000e7d8243a>] path_openat+0x136/0x1b5
>     [<0000000004e51585>] do_filp_open+0x57/0xb8
>     [<0000000053871b92>] do_sys_openat2+0x6f/0xc0
>     [<000000004d76b8b7>] do_sys_open+0x3f/0x60
>     [<000000009b0be238>] do_syscall_64+0x96/0xf8
>     [<000000006ff466ad>] entry_SYSCALL_64_after_hwframe+0x76/0x7e


Cheers,
-- 
Vinicius

Re: [PATCH v4 4/4] ovl: Optimize override/revert creds

[Amir Goldstein]

On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
> > <vinicius.gomes@intel.com> wrote:
>
> [...]
>
> >
> > Vinicius,
> >
> > While testing fanotify with LTP tests (some are using overlayfs),
> > kmemleak consistently reports the problems below.
> >
> > Can you see the bug, because I don't see it.
> > Maybe it is a false positive...
>
> Hm, if the leak wasn't there before and we didn't touch anything related to
> prepare_creds(), I think that points to the leak being real.
>
> But I see your point, still not seeing it.
>
> This code should be equivalent to the code we have now (just boot
> tested):
>
> ----
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 136a2c7fb9e5..7ebc2fd3097a 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -576,8 +576,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
>          * 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);
> +       WARN_ON_ONCE(override_creds_light(override_cred) != ovl_creds(dentry->d_sb));
>
>         return 0;
>  }
> ----
>
> Does it change anything? (I wouldn't think so, just to try something)

No, but I think this does:

--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -576,7 +576,8 @@ static int ovl_setup_cred_for_create(struct dentry
*dentry, struct inode *inode,
         * 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));
+       old_cred = override_creds(override_cred);
+       WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb));
        put_cred(override_cred);

        return 0;

Compiler optimized out override_creds(override_cred)? :-/

However, this is not enough.

Dropping the ref of the new creds is going to drop the refcount to zero,
so that is incorrect, we need to return the reference to the new creds
explicitly to the callers. I will send a patch.

Thanks,
Amir.

Re: [PATCH v4 4/4] ovl: Optimize override/revert creds

[Amir Goldstein]

On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
> >
> > Amir Goldstein <amir73il@gmail.com> writes:
> >
> > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
> > > <vinicius.gomes@intel.com> wrote:
> >
> > [...]
> >
> > >
> > > Vinicius,
> > >
> > > While testing fanotify with LTP tests (some are using overlayfs),
> > > kmemleak consistently reports the problems below.
> > >
> > > Can you see the bug, because I don't see it.
> > > Maybe it is a false positive...
> >
> > Hm, if the leak wasn't there before and we didn't touch anything related to
> > prepare_creds(), I think that points to the leak being real.
> >
> > But I see your point, still not seeing it.
> >
> > This code should be equivalent to the code we have now (just boot
> > tested):
> >
> > ----
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 136a2c7fb9e5..7ebc2fd3097a 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -576,8 +576,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
> >          * 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);
> > +       WARN_ON_ONCE(override_creds_light(override_cred) != ovl_creds(dentry->d_sb));
> >
> >         return 0;
> >  }
> > ----
> >
> > Does it change anything? (I wouldn't think so, just to try something)
>
> No, but I think this does:
>
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -576,7 +576,8 @@ static int ovl_setup_cred_for_create(struct dentry
> *dentry, struct inode *inode,
>          * 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));
> +       old_cred = override_creds(override_cred);
> +       WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb));
>         put_cred(override_cred);
>
>         return 0;
>
> Compiler optimized out override_creds(override_cred)? :-/

Nope, this voodoo did not help either.

>
> However, this is not enough.
>
> Dropping the ref of the new creds is going to drop the refcount to zero,
> so that is incorrect, we need to return the reference to the new creds
> explicitly to the callers. I will send a patch.

And neither did this.
The suspect memleak is still reported.

Any other ideas?

Thanks,
Amir.

Re: [PATCH v4 4/4] ovl: Optimize override/revert creds

[Amir Goldstein]

On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
> >
> > Amir Goldstein <amir73il@gmail.com> writes:
> >
> > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
> > > <vinicius.gomes@intel.com> wrote:
> >
> > [...]
> >
> > >
> > > Vinicius,
> > >
> > > While testing fanotify with LTP tests (some are using overlayfs),
> > > kmemleak consistently reports the problems below.
> > >
> > > Can you see the bug, because I don't see it.
> > > Maybe it is a false positive...
> >
> > Hm, if the leak wasn't there before and we didn't touch anything related to
> > prepare_creds(), I think that points to the leak being real.
> >
> > But I see your point, still not seeing it.
> >
> > This code should be equivalent to the code we have now (just boot
> > tested):
> >
> > ----
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 136a2c7fb9e5..7ebc2fd3097a 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -576,8 +576,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
> >          * 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);
> > +       WARN_ON_ONCE(override_creds_light(override_cred) != ovl_creds(dentry->d_sb));
> >
> >         return 0;
> >  }
> > ----
> >
> > Does it change anything? (I wouldn't think so, just to try something)
>
> No, but I think this does:
>

Vinicius,

Sorry, your fix is correct. I did not apply it properly when I tested.

I edited the comment as follows and applied on top of the patch
that I just sent [1]:


-       put_cred(override_creds(override_cred));
+
+       /*
+        * Caller is going to match this with revert_creds_light() and drop
+        * reference on the returned creds.
+        * We must be called with creator creds already, otherwise we risk
+        * leaking creds.
+        */
+       old_cred = override_creds_light(override_cred);
+       WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb));

        return override_cred;

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/20241114100536.628162-1-amir73il@gmail.com/

Re: [PATCH v4 4/4] ovl: Optimize override/revert creds

[Vinicius Costa Gomes]

Amir Goldstein <amir73il@gmail.com> writes:

> On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes
>> <vinicius.gomes@intel.com> wrote:
>> >
>> > Amir Goldstein <amir73il@gmail.com> writes:
>> >
>> > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
>> > > <vinicius.gomes@intel.com> wrote:
>> >
>> > [...]
>> >
>> > >
>> > > Vinicius,
>> > >
>> > > While testing fanotify with LTP tests (some are using overlayfs),
>> > > kmemleak consistently reports the problems below.
>> > >
>> > > Can you see the bug, because I don't see it.
>> > > Maybe it is a false positive...
>> >
>> > Hm, if the leak wasn't there before and we didn't touch anything related to
>> > prepare_creds(), I think that points to the leak being real.
>> >
>> > But I see your point, still not seeing it.
>> >
>> > This code should be equivalent to the code we have now (just boot
>> > tested):
>> >
>> > ----
>> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> > index 136a2c7fb9e5..7ebc2fd3097a 100644
>> > --- a/fs/overlayfs/dir.c
>> > +++ b/fs/overlayfs/dir.c
>> > @@ -576,8 +576,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
>> >          * 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);
>> > +       WARN_ON_ONCE(override_creds_light(override_cred) != ovl_creds(dentry->d_sb));
>> >
>> >         return 0;
>> >  }
>> > ----
>> >
>> > Does it change anything? (I wouldn't think so, just to try something)
>>
>> No, but I think this does:
>>
>
> Vinicius,
>
> Sorry, your fix is correct. I did not apply it properly when I tested.
>
> I edited the comment as follows and applied on top of the patch
> that I just sent [1]:
>

I just noticed there's a typo in the first sentence of the commit
message, the function name that we are using revert_creds_light() is
ovl_revert_creds(). Could you fix that while you are at it?

Glad that the fix works:

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

>
> -       put_cred(override_creds(override_cred));
> +
> +       /*
> +        * Caller is going to match this with revert_creds_light() and drop
> +        * reference on the returned creds.
> +        * We must be called with creator creds already, otherwise we risk
> +        * leaking creds.
> +        */
> +       old_cred = override_creds_light(override_cred);
> +       WARN_ON_ONCE(old_cred != ovl_creds(dentry->d_sb));
>
>         return override_cred;
>
> Thanks,
> Amir.
>
> [1] https://lore.kernel.org/linux-unionfs/20241114100536.628162-1-amir73il@gmail.com/

-- 
Vinicius

Re: [PATCH v4 4/4] ovl: Optimize override/revert creds

[Amir Goldstein]

On Thu, Nov 14, 2024 at 10:01 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Thu, Nov 14, 2024 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >>
> >> On Wed, Nov 13, 2024 at 8:30 PM Vinicius Costa Gomes
> >> <vinicius.gomes@intel.com> wrote:
> >> >
> >> > Amir Goldstein <amir73il@gmail.com> writes:
> >> >
> >> > > On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
> >> > > <vinicius.gomes@intel.com> wrote:
> >> >
> >> > [...]
> >> >
> >> > >
> >> > > Vinicius,
> >> > >
> >> > > While testing fanotify with LTP tests (some are using overlayfs),
> >> > > kmemleak consistently reports the problems below.
> >> > >
> >> > > Can you see the bug, because I don't see it.
> >> > > Maybe it is a false positive...
> >> >
> >> > Hm, if the leak wasn't there before and we didn't touch anything related to
> >> > prepare_creds(), I think that points to the leak being real.
> >> >
> >> > But I see your point, still not seeing it.
> >> >
> >> > This code should be equivalent to the code we have now (just boot
> >> > tested):
> >> >
> >> > ----
> >> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> >> > index 136a2c7fb9e5..7ebc2fd3097a 100644
> >> > --- a/fs/overlayfs/dir.c
> >> > +++ b/fs/overlayfs/dir.c
> >> > @@ -576,8 +576,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
> >> >          * 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);
> >> > +       WARN_ON_ONCE(override_creds_light(override_cred) != ovl_creds(dentry->d_sb));
> >> >
> >> >         return 0;
> >> >  }
> >> > ----
> >> >
> >> > Does it change anything? (I wouldn't think so, just to try something)
> >>
> >> No, but I think this does:
> >>
> >
> > Vinicius,
> >
> > Sorry, your fix is correct. I did not apply it properly when I tested.
> >
> > I edited the comment as follows and applied on top of the patch
> > that I just sent [1]:
> >
>
> I just noticed there's a typo in the first sentence of the commit
> message, the function name that we are using revert_creds_light() is
> ovl_revert_creds(). Could you fix that while you are at it?
>

fixed.

Thanks,
Amir.

Re: [PATCH v4 0/4] overlayfs: Optimize override/revert creds

[Amir Goldstein]

On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Hi,
>
> Changes from v3:
>  - Another reorganization of the series: separate the pure mechanical
>    changes into their own (Amir Goldstein)
>
> The series now reads:
>
> Patch 1: Introduce the _light() version of the override/revert cred operations;
> Patch 2: Convert backing-file.c to use those;
> Patch 3: Mechanical change to introduce the ovl_revert_creds() helper;
> Patch 4: Make the ovl_{override,convert}_creds() use the _light()
>   creds helpers, and fix the reference counting issue that would happen;
>

For the record, this series depends on backing_file API cleanup patch by Miklos:
https://lore.kernel.org/linux-fsdevel/20241021103340.260731-1-mszeredi@redhat.com/

> Changes from v2:
>  - Removed the "convert to guard()/scoped_guard()" patches (Miklos Szeredi);
>  - In the overlayfs code, convert all users of override_creds()/revert_creds() to the _light() versions by:
>       1. making ovl_override_creds() use override_creds_light();
>       2. introduce ovl_revert_creds() which calls revert_creds_light();
>       3. convert revert_creds() to ovl_revert_creds()
>    (Amir Goldstein);
>  - Fix an potential reference counting issue, as the lifetime
>    expectations of the mounter credentials are different (Christian
>    Brauner);
>

I pushed these patches to:
https://github.com/amir73il/linux/commits/ovl_creds

rebased overlayfs-next on top of them and tested.

Christian,

Since this work is mostly based on your suggestions,
I thought that you might want to author and handle this PR?

Would you like to take the patches from ovl_creds (including the backing_file
API cleanup) to a stable branch in your tree for me to base overlayfs-next on?
Or would you rather I include them in the overlayfs PR for v6.13 myself?

Thanks,
Amir.

P.S. some of the info below is relevant for the PR message and
some of it is completely stale...

> The series is now much simpler:
>
> Patch 1: Introduce the _light() version of the override/revert cred operations;
> Patch 2: Convert backing-file.c to use those;
> Patch 3: Do the conversion to use the _light() version internally;
> Patch 4: Fix a potential refcounting issue
>
> 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.

this info is stale and confusing in the context of this series.

>
> [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 (4):
>   cred: Add a light version of override/revert_creds()
>   fs/backing-file: Convert to revert/override_creds_light()
>   ovl: use wrapper ovl_revert_creds()
>   ovl: Optimize override/revert creds
>
>  fs/backing-file.c        | 20 ++++++++++----------
>  fs/overlayfs/copy_up.c   |  2 +-
>  fs/overlayfs/dir.c       | 17 +++++++++++------
>  fs/overlayfs/file.c      | 14 +++++++-------
>  fs/overlayfs/inode.c     | 20 ++++++++++----------
>  fs/overlayfs/namei.c     | 10 +++++-----
>  fs/overlayfs/overlayfs.h |  1 +
>  fs/overlayfs/readdir.c   |  8 ++++----
>  fs/overlayfs/util.c      | 11 ++++++++---
>  fs/overlayfs/xattrs.c    |  9 ++++-----
>  include/linux/cred.h     | 18 ++++++++++++++++++
>  kernel/cred.c            |  6 +++---
>  12 files changed, 82 insertions(+), 54 deletions(-)
>
> --
> 2.47.0
>

Re: [PATCH v4 0/4] overlayfs: Optimize override/revert creds

[Amir Goldstein]

On Thu, Nov 7, 2024 at 11:19 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Nov 7, 2024 at 1:57 AM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
> >
> > Hi,
> >
> > Changes from v3:
> >  - Another reorganization of the series: separate the pure mechanical
> >    changes into their own (Amir Goldstein)
> >
> > The series now reads:
> >
> > Patch 1: Introduce the _light() version of the override/revert cred operations;
> > Patch 2: Convert backing-file.c to use those;
> > Patch 3: Mechanical change to introduce the ovl_revert_creds() helper;
> > Patch 4: Make the ovl_{override,convert}_creds() use the _light()
> >   creds helpers, and fix the reference counting issue that would happen;
> >
>
> For the record, this series depends on backing_file API cleanup patch by Miklos:
> https://lore.kernel.org/linux-fsdevel/20241021103340.260731-1-mszeredi@redhat.com/
>
> > Changes from v2:
> >  - Removed the "convert to guard()/scoped_guard()" patches (Miklos Szeredi);
> >  - In the overlayfs code, convert all users of override_creds()/revert_creds() to the _light() versions by:
> >       1. making ovl_override_creds() use override_creds_light();
> >       2. introduce ovl_revert_creds() which calls revert_creds_light();
> >       3. convert revert_creds() to ovl_revert_creds()
> >    (Amir Goldstein);
> >  - Fix an potential reference counting issue, as the lifetime
> >    expectations of the mounter credentials are different (Christian
> >    Brauner);
> >
>
> I pushed these patches to:
> https://github.com/amir73il/linux/commits/ovl_creds
>
> rebased overlayfs-next on top of them and tested.
>
> Christian,
>
> Since this work is mostly based on your suggestions,
> I thought that you might want to author and handle this PR?
>
> Would you like to take the patches from ovl_creds (including the backing_file
> API cleanup) to a stable branch in your tree for me to base overlayfs-next on?
> Or would you rather I include them in the overlayfs PR for v6.13 myself?
>

For now, there patches are queued in overlayfs-next.

Thanks Vinicius,
for being patient and working through all the different revisions!

Amir.