[Devel] [PATCH hci-8.1 0/4] Fix kernel crash in overlayfs

[Devel] [PATCH hci-8.1 0/4] Fix kernel crash in overlayfs

[Vladimir Riabchun]

When testing CRIU with containers with docker images,
we found an easily reproducible kernel crash:

[ 2300.870181] BUG: kernel NULL pointer dereference, address: 0000000000000005
[ 2300.871537] #PF: supervisor read access in kernel mode
[ 2300.872577] #PF: error_code(0x0000) - not-present page
[ 2300.873595] PGD 800000010a18f067 P4D 800000010a18f067 PUD 10730c067 PMD 0
[ 2300.874900] Oops: Oops: 0000 [#1] SMP PTI
[ 2300.875742] CPU: 1 UID: 0 PID: 5419 Comm: criu ve: 0 Kdump: loaded Not tainted 6.12.0-211.39.1.16.6.vz10.x86_64 #1 16.6 PREEMPT(voluntary)
[ 2300.878057] Hardware name: Acronis OpenStack Compute/Virtuozzo, BIOS 1.16.1-1.vz9.2 04/01/2014
[ 2300.879684] RIP: 0010:ovl_get_index_name_fh+0x18/0x70 [overlay]
[ 2300.880859] Code: ...
[ 2300.884128] RSP: 0018:ffffd32582e0f7c0 EFLAGS: 00010246
[ 2300.895807] Call Trace:
[ 2300.896397]  <TASK>
[ 2300.904435]  ovl_create_index+0x82/0x260 [overlay]
[ 2300.905406]  ovl_copy_up_workdir+0x30a/0x330 [overlay]
[ 2300.906427]  ovl_do_copy_up+0x10d/0x2e0 [overlay]
[ 2300.907387]  ovl_copy_up_one+0x456/0x720 [overlay]
[ 2300.910736]  ovl_copy_up_flags+0x81/0x110 [overlay]
[ 2300.911740]  ovl_create_object+0x51/0x130 [overlay]
[ 2300.912719]  vfs_mkdir+0x194/0x250
[ 2300.913470]  do_mkdirat+0x14c/0x180
[ 2300.914223]  __x64_sys_mkdir+0x46/0x70
[ 2300.915019]  do_syscall_64+0x7d/0x160
[ 2300.922814]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

The cause is that overlayfs does some feature checks on superblock
creation using capable/ve_capable and it stores current credentials.
But ve_capable check result depends on both current creds and
current VE. Later, overlayfs switches credentials only, leading
to unexpected feature disappearance.

This was reworked completely after v2, the approach is different now.
1. Patch 1 removes separate STOPPED state - VE goes directly to DEAD.
2. Patch 2 adds helpers for overriding tasks VE.
3. Patch 3 adds VE owner to overlay superblock.
4. Patch 4 fixes the bug by switching VE together with creds in overlayfs.

v2: http://lore.virtuozzo.com/all/20260920-ovl-use-orig-ve-v1-0-b3e63f6ff80d@virtuozzo.com/
https://virtuozzo.atlassian.net/browse/VSTOR-143259 - non-CRIU repro
https://virtuozzo.atlassian.net/browse/VSTOR-142405

Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
Vladimir Riabchun (4):
      ve: Remove STOPPED state
      ve: Introduce override_ve/revert_ve helpers
      ovl: Keep creator VE in overlayfs superblock
      ve/overlayfs: Override VE together with creds

 fs/overlayfs/copy_up.c   |  4 ++--
 fs/overlayfs/dir.c       | 28 ++++++++++++++--------------
 fs/overlayfs/file.c      | 28 ++++++++++++++--------------
 fs/overlayfs/inode.c     | 38 +++++++++++++++++++-------------------
 fs/overlayfs/namei.c     | 18 +++++++++---------
 fs/overlayfs/overlayfs.h |  9 ++++++++-
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/params.c    |  3 +++
 fs/overlayfs/readdir.c   | 16 ++++++++--------
 fs/overlayfs/super.c     |  3 +++
 fs/overlayfs/util.c      | 22 ++++++++++++++++------
 fs/overlayfs/xattrs.c    | 14 +++++++-------
 include/linux/ve.h       | 24 ++++++++++++++++++++++--
 kernel/ve/ve.c           |  6 ++----
 14 files changed, 128 insertions(+), 86 deletions(-)
---
base-commit: f5d1fdcfffe101fe2bafa7df09d95d2e811c5c5e
change-id: 20260918-ovl-use-orig-ve-2dc365976202

Best regards,
--  
Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 1/4] ve: Remove STOPPED state

[Vladimir Riabchun]

We really don't want to reuse VE after its stop.
Some resources from the old run can reference this CT, so new
start may see leftovers.

Fix this by transitioning from STOPPING to DEAD directly.

https://virtuozzo.atlassian.net/browse/VSTOR-142405
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 include/linux/ve.h | 3 +--
 kernel/ve/ve.c     | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/include/linux/ve.h b/include/linux/ve.h
index a400d21593b2..717567775f58 100644
--- a/include/linux/ve.h
+++ b/include/linux/ve.h
@@ -40,8 +40,7 @@ struct vfsmount;
 #define VE_STATE_STARTING	0
 #define VE_STATE_RUNNING	1
 #define VE_STATE_STOPPING	2
-#define VE_STATE_STOPPED	3
-#define VE_STATE_DEAD		4
+#define VE_STATE_DEAD		3
 
 #define VE_IS_RUNNING(ve) ((ve)->state == VE_STATE_RUNNING)
 
diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
index 579628a8a310..d74f74ca958a 100644
--- a/kernel/ve/ve.c
+++ b/kernel/ve/ve.c
@@ -604,7 +604,7 @@ static int ve_start_container(struct ve_struct *ve)
 err_mark_ve:
 	ve_list_del(ve);
 err_list:
-	ve_set_state(ve, VE_STATE_STOPPED);
+	ve_set_state(ve, VE_STATE_DEAD);
 	ve_drop_context(ve);
 	return err;
 }
@@ -680,7 +680,7 @@ void ve_exit_ns(struct pid_namespace *pid_ns)
 	ve_hook_iterate_fini(VE_SS_CHAIN, ve);
 	ve_list_del(ve);
 	ve_drop_context(ve);
-	ve_set_state(ve, VE_STATE_STOPPED);
+	ve_set_state(ve, VE_STATE_DEAD);
 	up_write(&ve->op_sem);
 
 	printk(KERN_INFO "CT: %s: stopped\n", ve_name(ve));
@@ -935,7 +935,6 @@ static void ve_destroy(struct cgroup_subsys_state *css)
 	ve_free_vdso(ve);
 	mntput(ve->devtmpfs_mnt);
 	free_percpu(ve->sched_lat_ve.cur);
-	ve_set_state(ve, VE_STATE_DEAD);
 	kmem_cache_free(ve_cachep, ve);
 }
 
@@ -955,7 +954,6 @@ static int ve_state_show(struct seq_file *sf, void *v)
 	case VE_STATE_STOPPING:
 		seq_puts(sf, "STOPPING");
 		break;
-	case VE_STATE_STOPPED:
 	case VE_STATE_DEAD:
 		seq_puts(sf, "STOPPED");
 		break;

-- 
2.52.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 2/4] ve: Introduce override_ve/revert_ve helpers

[Vladimir Riabchun]

These helpers mirror override_creds/revert_creds and are supposed to
be used together in cases when VE creds may be stored and later used
by the different VE task.

https://virtuozzo.atlassian.net/browse/VSTOR-142405
Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 include/linux/ve.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/ve.h b/include/linux/ve.h
index 717567775f58..157326ef91b9 100644
--- a/include/linux/ve.h
+++ b/include/linux/ve.h
@@ -295,6 +295,19 @@ static inline struct vdso_image *ve_get_vdso_64(struct ve_struct *ve)
 	return ve->vdso_64;
 }
 
+static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
+{
+	struct ve_struct *old_ve = current->task_ve;
+
+	current->task_ve = new_ve;
+	return old_ve;
+}
+
+static inline void revert_ve(struct ve_struct *orig_ve)
+{
+	current->task_ve = orig_ve;
+}
+
 #else	/* CONFIG_VE */
 #include <linux/init_task.h>
 #include <net/net_namespace.h>
@@ -403,6 +416,14 @@ static inline struct vdso_image *ve_get_vdso_64(struct ve_struct *ve)
 {
 	return (struct vdso_image *)&vdso_image_64;
 }
+
+static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
+{
+	return NULL;
+}
+
+static inline void revert_ve(struct ve_struct *orig_ve) {}
+
 #endif /* CONFIG_X86 */
 
 #endif	/* CONFIG_VE */

-- 
2.52.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 3/4] ovl: Keep creator VE in overlayfs superblock

[Vladimir Riabchun]

Overlayfs superblock stores creator creds in it, but these creds
should go together with the VE they were taken from.

Store the VE in the superblock. We can't use mount->owner_ve,
because several mounts can reference one superblock.

https://virtuozzo.atlassian.net/browse/VSTOR-142405

Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 fs/overlayfs/ovl_entry.h | 1 +
 fs/overlayfs/params.c    | 3 +++
 fs/overlayfs/super.c     | 3 +++
 3 files changed, 7 insertions(+)

diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index ab78001635e7..a6ad8b8de48c 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -77,6 +77,7 @@ struct ovl_fs {
 	struct ovl_config config;
 	/* creds of process who forced instantiation of super block */
 	const struct cred *creator_cred;
+	struct ve_struct *creator_ve;
 	bool tmpfile;
 	bool noxattr;
 	bool nofh;
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 32c58fcfe52f..c047f1265b8e 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -8,6 +8,7 @@
 #include <linux/posix_acl_xattr.h>
 #include <linux/seq_file.h>
 #include <linux/xattr.h>
+#include <linux/ve.h>
 #include "overlayfs.h"
 #include "params.h"
 
@@ -761,6 +762,8 @@ void ovl_free_fs(struct ovl_fs *ofs)
 	kfree(ofs->config.workdir);
 	if (ofs->creator_cred)
 		put_cred(ofs->creator_cred);
+	if (ofs->creator_ve)
+		put_ve(ofs->creator_ve);
 	kfree(ofs);
 }
 
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index fb47711c8fb9..8ab39a35a873 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -18,6 +18,7 @@
 #include <linux/file.h>
 #include <linux/fs_context.h>
 #include <linux/fs_parser.h>
+#include <linux/ve.h>
 #include "overlayfs.h"
 #include "params.h"
 
@@ -1328,6 +1329,8 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	sb->s_d_op = &ovl_dentry_operations;
 
+	ofs->creator_ve = get_ve(get_exec_env());
+
 	err = -ENOMEM;
 	ofs->creator_cred = cred = prepare_creds();
 	if (!cred)

-- 
2.52.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 4/4] ve/overlayfs: Override VE together with creds

[Vladimir Riabchun]

Host users might want to access overlayfs, owned by CT. In such case
ve_capable checks will fail - they will perform VE cred checks against
host user_ns.

Modify ovl_override_creds so that it returns both old cred and old VE.
This will also protect us from future rebase issues - all new users of
old_cred and ovl_override_creds will fail with compilation error.

This fixes an observed crash in overlayfs when checkpointing
a container with a docker running inside:

[ 2300.870181] BUG: kernel NULL pointer dereference, address: 0000000000000005
[ 2300.871537] #PF: supervisor read access in kernel mode
[ 2300.872577] #PF: error_code(0x0000) - not-present page
[ 2300.873595] PGD 800000010a18f067 P4D 800000010a18f067 PUD 10730c067 PMD 0
[ 2300.874900] Oops: Oops: 0000 [#1] SMP PTI
[ 2300.875742] CPU: 1 UID: 0 PID: 5419 Comm: criu ve: 0 Kdump: loaded Not tainted 6.12.0-211.39.1.16.6.vz10.x86_64 #1 16.6 PREEMPT(voluntary)
[ 2300.878057] Hardware name: Acronis OpenStack Compute/Virtuozzo, BIOS 1.16.1-1.vz9.2 04/01/2014
[ 2300.879684] RIP: 0010:ovl_get_index_name_fh+0x18/0x70 [overlay]
[ 2300.880859] Code: ...
[ 2300.884128] RSP: 0018:ffffd32582e0f7c0 EFLAGS: 00010246
[ 2300.885153] RAX: ...
[ 2300.895807] Call Trace:
[ 2300.896397]
[ 2300.903298] ? ovl_get_index_name_fh+0x18/0x70 [overlay]
[ 2300.904435] ovl_create_index+0x82/0x260 [overlay]
[ 2300.905406] ovl_copy_up_workdir+0x30a/0x330 [overlay]
[ 2300.906427] ovl_do_copy_up+0x10d/0x2e0 [overlay]
[ 2300.907387] ovl_copy_up_one+0x456/0x720 [overlay]
[ 2300.910736] ovl_copy_up_flags+0x81/0x110 [overlay]
[ 2300.911740] ovl_create_object+0x51/0x130 [overlay]
[ 2300.912719] vfs_mkdir+0x194/0x250
[ 2300.913470] do_mkdirat+0x14c/0x180
[ 2300.914223] __x64_sys_mkdir+0x46/0x70
[ 2300.915019] do_syscall_64+0x7d/0x160
[ 2300.922814] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 2300.923823] RIP: 0033:0x7f5a2f8de3eb

Overlayfs was created inside CT, so ovl_fill_super stored
in-container credentials to ofs->creator_cred.

Those creds were later used in ovl_override_creds to temporarily
swich creds and perform checks using them.

One of these checks happened in ovl_get_origin_fh/ovl_can_decode_fh.
On fs creation this check passed because we had container VE
and container creds, so overlayfs thought that we support
file handles (ovl_can_decode_fh call in ovl_make_workdir).

When we tried to do something with the overlayfs from host VE,
ve_capable check was performed with credentials from CT
and with host VE.

ovl_get_origin_fh returned unexpected NULL and all exploded.

Stroing ve_capable check result on creation is a possible fix,
but it just masks the problem. We have other paths from overlay to
ve_capable checks:

ovl_fileattr_set
  ovl_override_creds
  ovl_real_fileattr_set
    vfs_fileattr_set
      fileattr_set_prepare
        ve_capable

And storing capability check result will make rebase dangerous - we
would have to track new added code - not funny.

https://virtuozzo.atlassian.net/browse/VSTOR-142405

Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
---
 fs/overlayfs/copy_up.c   |  4 ++--
 fs/overlayfs/dir.c       | 28 ++++++++++++++--------------
 fs/overlayfs/file.c      | 28 ++++++++++++++--------------
 fs/overlayfs/inode.c     | 38 +++++++++++++++++++-------------------
 fs/overlayfs/namei.c     | 18 +++++++++---------
 fs/overlayfs/overlayfs.h |  9 ++++++++-
 fs/overlayfs/readdir.c   | 16 ++++++++--------
 fs/overlayfs/util.c      | 22 ++++++++++++++++------
 fs/overlayfs/xattrs.c    | 14 +++++++-------
 9 files changed, 97 insertions(+), 80 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 4388004a319d..0b1270bc6d8d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -1215,7 +1215,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	bool disconnected = (dentry->d_flags & DCACHE_DISCONNECTED);
 
 	/*
@@ -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 1c8009bf194b..fe91fd4b04f0 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -554,7 +554,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 }
 
 static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
-				     umode_t mode, const struct cred *old_cred)
+				     umode_t mode, const struct ovl_creds_desc *old_cred)
 {
 	int err;
 	struct cred *override_cred;
@@ -566,7 +566,7 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
 	override_cred->fsuid = inode->i_uid;
 	override_cred->fsgid = inode->i_gid;
 	err = security_dentry_create_files_as(dentry, mode, &dentry->d_name,
-					      old_cred, override_cred);
+					      old_cred->cred, override_cred);
 	if (err) {
 		put_cred(override_cred);
 		return err;
@@ -581,7 +581,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 			      struct ovl_cattr *attr, bool origin)
 {
 	int err;
-	const struct cred *old_cred;
+	struct ovl_creds_desc old_cred;
 	struct dentry *parent = dentry->d_parent;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
@@ -610,7 +610,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		 * create a new inode, so just use the ovl mounter's
 		 * fs{u,g}id.
 		 */
-		err = ovl_setup_cred_for_create(dentry, inode, attr->mode, old_cred);
+		err = ovl_setup_cred_for_create(dentry, inode, attr->mode, &old_cred);
 		if (err)
 			goto out_revert_creds;
 	}
@@ -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;
 }
 
@@ -697,12 +697,12 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int err;
 
 	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;
 }
@@ -888,7 +888,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	bool lower_positive = ovl_lower_positive(dentry);
 	LIST_HEAD(list);
 
@@ -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);
@@ -1095,7 +1095,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	bool new_is_dir = d_is_dir(new);
 	bool samedir = olddir == newdir;
 	struct dentry *opaquedir = NULL;
-	const struct cred *old_cred = NULL;
+	struct ovl_creds_desc old_cred = {};
 	struct ovl_fs *ofs = OVL_FS(old->d_sb);
 	LIST_HEAD(list);
 
@@ -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
@@ -1306,7 +1306,7 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 			      struct inode *inode, umode_t mode)
 {
-	const struct cred *old_cred;
+	struct ovl_creds_desc old_cred;
 	struct path realparentpath;
 	struct file *realfile;
 	struct dentry *newdentry;
@@ -1315,7 +1315,7 @@ static int ovl_create_tmpfile(struct file *file, struct dentry *dentry,
 	int err;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
-	err = ovl_setup_cred_for_create(dentry, inode, mode, old_cred);
+	err = ovl_setup_cred_for_create(dentry, inode, mode, &old_cred);
 	if (err)
 		goto out_revert_creds;
 
@@ -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 84aad9d19bdd..55b4589e895a 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -31,7 +31,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int flags = file->f_flags | OVL_OPEN_FLAGS;
 	int acc_mode = ACC_MODE(flags);
 	int err;
@@ -51,7 +51,7 @@ static struct file *ovl_open_realfile(const struct file *file,
 		realfile = backing_file_open(file, 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,
@@ -184,7 +184,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	loff_t ret;
 
 	/*
@@ -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);
@@ -392,7 +392,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int ret;
 
 	ret = ovl_sync_status(OVL_FS(file_inode(file)->i_sb));
@@ -407,7 +407,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);
@@ -431,7 +431,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int ret;
 
 	inode_lock(inode);
@@ -447,7 +447,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);
@@ -463,7 +463,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int ret;
 
 	ret = ovl_real_fdget(file, &real);
@@ -472,7 +472,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);
 
@@ -491,7 +491,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	loff_t ret;
 
 	inode_lock(inode_out);
@@ -531,7 +531,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);
@@ -583,7 +583,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int err;
 
 	err = ovl_real_fdget(file, &real);
@@ -593,7 +593,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 8b31f44c12cd..6a8d35e8c14a 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -25,7 +25,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 
 	err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
 	if (err)
@@ -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);
@@ -159,7 +159,7 @@ 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 ovl_creds_desc old_cred;
 	struct inode *inode = d_inode(dentry);
 	bool is_dir = S_ISDIR(inode->i_mode);
 	int fsid = 0;
@@ -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;
 }
@@ -291,7 +291,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int err;
 
 	/* Careful in RCU walk mode */
@@ -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;
 }
@@ -326,7 +326,7 @@ static const char *ovl_get_link(struct dentry *dentry,
 				struct inode *inode,
 				struct delayed_call *done)
 {
-	const struct cred *old_cred;
+	struct ovl_creds_desc old_cred;
 	const char *p;
 
 	if (!dentry)
@@ -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;
 }
 
@@ -465,11 +465,11 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
 
 		acl = get_cached_acl_rcu(realinode, type);
 	} else {
-		const struct cred *old_cred;
+		struct ovl_creds_desc old_cred;
 
 		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;
@@ -481,7 +481,7 @@ 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_creds_desc 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);
@@ -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 */
@@ -590,7 +590,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 
 	if (!realinode)
 		return -EIO;
@@ -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;
 }
@@ -648,7 +648,7 @@ int ovl_fileattr_set(struct mnt_idmap *idmap,
 {
 	struct inode *inode = d_inode(dentry);
 	struct path upperpath;
-	const struct cred *old_cred;
+	struct ovl_creds_desc old_cred;
 	unsigned int flags;
 	int 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);
 
 		/*
@@ -725,7 +725,7 @@ int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa)
 {
 	struct inode *inode = d_inode(dentry);
 	struct path realpath;
-	const struct cred *old_cred;
+	struct ovl_creds_desc old_cred;
 	int err;
 
 	ovl_path_real(dentry, &realpath);
@@ -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 efa2a072aa69..6af0712fcb83 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -969,7 +969,7 @@ static int ovl_maybe_validate_verity(struct dentry *dentry)
 		return err;
 
 	if (!ovl_test_flag(OVL_VERIFIED_DIGEST, inode)) {
-		const struct cred *old_cred;
+		struct ovl_creds_desc old_cred;
 
 		old_cred = ovl_override_creds(dentry->d_sb);
 
@@ -977,7 +977,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);
@@ -991,7 +991,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int err;
 
 	if (!redirect || ovl_dentry_lowerdata(dentry))
@@ -1011,7 +1011,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;
 
@@ -1071,7 +1071,7 @@ 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_creds_desc 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);
@@ -1378,7 +1378,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);
@@ -1402,7 +1402,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	kfree(d.upperredirect);
 out:
 	kfree(d.redirect);
-	revert_creds(old_cred);
+	ovl_revert_creds(&old_cred);
 	return ERR_PTR(err);
 }
 
@@ -1410,7 +1410,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	unsigned int i;
 	bool positive = false;
 	bool done = false;
@@ -1459,7 +1459,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 8577c7884fb9..b2ae8c810898 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -164,6 +164,12 @@ struct ovl_metacopy {
 	u8 digest[FS_VERITY_MAX_DIGEST_SIZE];  /* Only the used part on disk */
 } __packed;
 
+/* Credential inforamtion used by overlay */
+struct ovl_creds_desc {
+	const struct cred *cred;
+	struct ve_struct *ve;
+};
+
 #define OVL_METACOPY_MAX_SIZE (sizeof(struct ovl_metacopy))
 #define OVL_METACOPY_MIN_SIZE (OVL_METACOPY_MAX_SIZE - FS_VERITY_MAX_DIGEST_SIZE)
 #define OVL_METACOPY_INIT { 0, OVL_METACOPY_MIN_SIZE }
@@ -420,7 +426,8 @@ void ovl_end_write(struct dentry *dentry);
 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);
+struct ovl_creds_desc ovl_override_creds(struct super_block *sb);
+void ovl_revert_creds(struct ovl_creds_desc *old_creds);
 
 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..aca2dd6d4e2f 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -273,7 +273,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 
 	old_cred = ovl_override_creds(rdd->dentry->d_sb);
 
@@ -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;
 }
@@ -753,7 +753,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int err;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
@@ -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;
 }
 
@@ -856,11 +856,11 @@ static struct file *ovl_dir_open_realfile(const struct file *file,
 					  const struct path *realpath)
 {
 	struct file *res;
-	const struct cred *old_cred;
+	struct ovl_creds_desc old_cred;
 
 	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;
 }
@@ -983,11 +983,11 @@ 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;
+	struct ovl_creds_desc old_cred;
 
 	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 f95090f0d29c..e3cbdf7259d1 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -63,11 +63,21 @@ struct dentry *ovl_workdir(struct dentry *dentry)
 	return ofs->workdir;
 }
 
-const struct cred *ovl_override_creds(struct super_block *sb)
+struct ovl_creds_desc ovl_override_creds(struct super_block *sb)
 {
 	struct ovl_fs *ofs = OVL_FS(sb);
+	struct ovl_creds_desc desc;
 
-	return override_creds(ofs->creator_cred);
+	desc.cred = override_creds(ofs->creator_cred);
+	desc.ve = override_ve(ofs->creator_ve);
+
+	return desc;
+}
+
+void ovl_revert_creds(struct ovl_creds_desc *old_creds)
+{
+	revert_ve(old_creds->ve);
+	revert_creds(old_creds->cred);
 }
 
 /*
@@ -1150,7 +1160,7 @@ 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;
+	struct ovl_creds_desc old_cred;
 	int err;
 
 	if (WARN_ON(!inode))
@@ -1195,7 +1205,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;
 
@@ -1216,11 +1226,11 @@ 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;
+		struct ovl_creds_desc old_cred;
 
 		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 0ed2b049056d..0bd5835061fb 100644
--- a/fs/overlayfs/xattrs.c
+++ b/fs/overlayfs/xattrs.c
@@ -41,13 +41,13 @@ 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;
+	struct ovl_creds_desc old_cred;
 
 	if (!value && !upperdentry) {
 		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 */
@@ -85,13 +85,13 @@ 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 ovl_creds_desc old_cred;
 	struct path realpath;
 
 	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;
 }
 
@@ -116,12 +116,12 @@ 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;
+	struct ovl_creds_desc old_cred;
 	size_t prefix_len, name_len;
 
 	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;
 

-- 
2.52.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 2/4] ve: Introduce override_ve/revert_ve helpers

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] ve: override_ve()/revert_ve() !CONFIG_VE stubs hidden inside CONFIG_X86 block
- [Low] ve: override_ve()/revert_ve() store task_ve without WRITE_ONCE() or task_lock
--

commit f94943d7cf0e7cf13a560e724d14ae8e02ad7271
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve: Introduce override_ve/revert_ve helpers

This commit adds override_ve() and revert_ve() helpers to include/linux/ve.h.
They temporarily replace current->task_ve, in the same way that
override_creds() and revert_creds() handle credentials. Stubs for
CONFIG_VE=n are added as well.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405

> diff --git a/include/linux/ve.h b/include/linux/ve.h
> index 717567775f58..157326ef91b9 100644
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -295,6 +295,19 @@ static inline struct vdso_image *ve_get_vdso_64(struct ve_struct *ve)
>  	return ve->vdso_64;
>  }
>  
> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
> +{
> +	struct ve_struct *old_ve = current->task_ve;
> +
> +	current->task_ve = new_ve;
> +	return old_ve;
> +}
> +
> +static inline void revert_ve(struct ve_struct *orig_ve)
> +{
> +	current->task_ve = orig_ve;
> +}

[Severity: Low]
Can these plain stores to current->task_ve race with remote readers?

task_ve() reads the field with READ_ONCE(), and get_task_ve() reads it
under task_lock:

include/linux/ve.h:
    return READ_ONCE(task->task_ve);
    ...
    guard(task_lock)(task);
    return get_ve(task_ve(task));

override_ve() and revert_ve() use neither WRITE_ONCE() nor task_lock, so
the lock gives get_ve() no exclusion against them. The existing writer
follows a different convention:

kernel/ve/ve_namespace.c:switch_ve_namespace() {
    scoped_guard(task_lock, p) {
        old = p->ve_ns;
        p->ve_ns = new;
        p->task_ve = new ? new->ve : &ve0;
    }
}

A remote task can hit this window while the overlayfs task is between
ovl_override_creds() and ovl_revert_creds(). For example, /proc code
may call task_pid_ve_nr(tsk) or get_task_ve(tsk) on that task. The reader
can then see creator_ve instead of the task's real VE, while ve_ns still
refers to the original one. task_pid_ve_nr() could then transiently
return a wrong or zero pid.

Should both helpers use WRITE_ONCE(), and should the store be done under
task_lock so that get_task_ve() cannot observe the transient value?

> +
>  #else	/* CONFIG_VE */
>  #include <linux/init_task.h>
>  #include <net/net_namespace.h>

[ ... ]

> @@ -403,6 +416,14 @@ static inline struct vdso_image *ve_get_vdso_64(struct vdso_image *ve)
>  {
>  	return (struct vdso_image *)&vdso_image_64;
>  }
> +
> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
> +{
> +	return NULL;
> +}
> +
> +static inline void revert_ve(struct ve_struct *orig_ve) {}
> +
>  #endif /* CONFIG_X86 */
>  
>  #endif	/* CONFIG_VE */

[Severity: Low]
Are these CONFIG_VE=n stubs placed correctly? They sit inside the
CONFIG_X86 block, before the closing line:

#endif /* CONFIG_X86 */

With CONFIG_VE=n and CONFIG_X86 unset (for example arm64 or UML),
override_ve() and revert_ve() are never defined. The CONFIG_VE=y versions
are defined unconditionally.

Patch 4/4 of the series calls them from the generic file
fs/overlayfs/util.c, in ovl_override_creds() and ovl_revert_creds():

    desc.ve = override_ve(ofs->creator_ve);
    ...
    revert_ve(old_creds->ve);

Would that fail to build on those configurations with an implicit
declaration error? Neither patch 3 nor patch 4 touches include/linux/ve.h,
so the series does not fix this later. Could the stubs be moved after the
closing line of the CONFIG_X86 block?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022-ovl-use-orig-ve-v1-0-da84c00c8065@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 1/4] ve: Remove STOPPED state

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] ve: renumbering VE_STATE_DEAD from 4 to 3 changes the state value logged by setns
- [Low] ve: commit message claims STOPPED removal blocks VE reuse, but it is only a cleanup
--

commit afa07824d11d984ebc29eb501787f9b9b4ecea47
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve: Remove STOPPED state

This patch merges VE_STATE_STOPPED into VE_STATE_DEAD. The stop path in
ve_exit_ns() and the error path in ve_start_container() now set DEAD
directly. It also drops the ve_set_state(DEAD) call in ve_destroy() and
renumbers VE_STATE_DEAD from 4 to 3.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405

> diff --git a/include/linux/ve.h b/include/linux/ve.h
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -40,8 +40,7 @@ struct vfsmount;
>  #define VE_STATE_STARTING	0
>  #define VE_STATE_RUNNING	1
>  #define VE_STATE_STOPPING	2
> -#define VE_STATE_STOPPED	3
> -#define VE_STATE_DEAD		4
> +#define VE_STATE_DEAD		3

[Severity: Low]
Is the renumbering of VE_STATE_DEAD from 4 to 3 intended to be visible in
logs?

The raw state integer is printed in the setns check in
kernel/ve/ve_namespace.c:

    ve_name(ve_ns->ve), ve_ns->ve->state,

and in the ve_set_state() WARN_ONCE messages. Anything that parses the
integer in the "setns to the VE namespace of a non-running CT (state %d)"
message now sees 3 for a stopped CT, where it could previously see 3 or 4.

The cgroup state file is unaffected, since ve_state_show() still prints
"STOPPED" for VE_STATE_DEAD.

[ ... ]

> @@ -680,7 +680,7 @@ void ve_exit_ns(struct pid_namespace *pid_ns)
>  	ve_hook_iterate_fini(VE_SS_CHAIN, ve);
>  	ve_list_del(ve);
>  	ve_drop_context(ve);
> -	ve_set_state(ve, VE_STATE_STOPPED);
> +	ve_set_state(ve, VE_STATE_DEAD);
>  	up_write(&ve->op_sem);

[Severity: Low]
This isn't a bug, but the commit message doesn't seem to match what the
patch does. It says:

    We really don't want to reuse VE after its stop.
    Some resources from the old run can reference this CT, so new
    start may see leftovers.

    Fix this by transitioning from STOPPING to DEAD directly.

Is that description accurate?

ve_start_container() already rejected a restart for any state other than
STARTING:

    if (ve->state != VE_STATE_STARTING || ve_nsproxy)
        return -EBUSY;

STARTING is only assigned in ve_create(). A VE in STOPPED could therefore
not be restarted before this patch, and a VE in DEAD cannot be restarted
now.

The ve_set_state(DEAD) call removed from ve_destroy() wrote to an object
that kmem_cache_free() releases immediately afterwards:

    ve_set_state(ve, VE_STATE_DEAD);
    kmem_cache_free(ve_cachep, ve);

So DEAD was never observable there.

The later patches in the series (override_ve/revert_ve and the overlayfs
creator VE changes) don't check VE_STATE_DEAD or VE_IS_RUNNING. They take
and drop references with get_ve() and put_ve(). It looks like the
"leftovers" problem is addressed by those reference changes and not by
this patch.

Could the commit message be reworded to say that this is a state machine
cleanup that merges STOPPED into DEAD and removes a dead store in
ve_destroy()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022-ovl-use-orig-ve-v1-0-da84c00c8065@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 3/4] ovl: Keep creator VE in overlayfs superblock

[no-reply]

> Overlayfs superblock stores creator creds in it, but these creds
> should go together with the VE they were taken from.
> 
> Store the VE in the superblock. We can't use mount->owner_ve,
> because several mounts can reference one superblock.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-142405
> 
> Signed-off-by: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022-ovl-use-orig-ve-v1-0-da84c00c8065@virtuozzo.com?part=3


_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 4/4] ve/overlayfs: Override VE together with creds

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] overlayfs: typo 'inforamtion' in new struct ovl_creds_desc comment
- [Low] ve: override_ve()/revert_ve() store current->task_ve without WRITE_ONCE or task_lock
- [Low] ve: override_ve()/revert_ve() !CONFIG_VE stubs are x86-only; overlayfs now needs them
--

commit f580a7811856a41f48d8fc37ebc70368ebd6d989
Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>

ve/overlayfs: Override VE together with creds

This patch changes ovl_override_creds() to return a struct ovl_creds_desc
holding both the old cred and the old VE. It adds ovl_revert_creds() to
restore both, and converts all overlayfs callers. The aim is to make
ve_capable checks behave correctly when a host user accesses an overlayfs
created inside a container.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405

> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h

[ ... ]

> +/* Credential inforamtion used by overlay */
> +struct ovl_creds_desc {
> +	const struct cred *cred;
> +	struct ve_struct *ve;
> +};

[Severity: Low]
This isn't a bug, but there is a typo in the new comment: "inforamtion"
should be "information".

The commit message also has typos: "swich" should be "switch", and
"Stroing" should be "Storing".

> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c

[ ... ]

> +struct ovl_creds_desc ovl_override_creds(struct super_block *sb)
>  {
>  	struct ovl_fs *ofs = OVL_FS(sb);
> +	struct ovl_creds_desc desc;
>  
> -	return override_creds(ofs->creator_cred);
> +	desc.cred = override_creds(ofs->creator_cred);
> +	desc.ve = override_ve(ofs->creator_ve);
> +
> +	return desc;
> +}
> +
> +void ovl_revert_creds(struct ovl_creds_desc *old_creds)
> +{
> +	revert_ve(old_creds->ve);
> +	revert_creds(old_creds->cred);
>  }

[Severity: Low]
Will this build when CONFIG_VE is disabled and CONFIG_X86 is not set, for
example on UML or on non-x86 architectures?

In include/linux/ve.h the !CONFIG_VE stubs for override_ve() and
revert_ve() sit inside the "#ifdef CONFIG_X86" block of the !CONFIG_VE
branch:

    #ifdef CONFIG_X86
    ...
    static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
    {
            return NULL;
    }

    static inline void revert_ve(struct ve_struct *orig_ve) {}

    #endif /* CONFIG_X86 */

    #endif  /* CONFIG_VE */

ovl_override_creds() and ovl_revert_creds() now call these helpers
unconditionally. fs/overlayfs/Kconfig has no dependency on VE or X86, and
CONFIG_VE is a bool that can be turned off.

With CONFIG_OVERLAY_FS enabled, CONFIG_VE off and CONFIG_X86 unset, the
helpers have no declaration, so fs/overlayfs/util.c would fail with an
implicit declaration error. Should the stubs be moved outside the
CONFIG_X86 block?

[Severity: Low]
The helpers called here, override_ve() and revert_ve() in
include/linux/ve.h, store current->task_ve with a plain write:

    struct ve_struct *old_ve = current->task_ve;

    current->task_ve = new_ve;
    return old_ve;

Remote readers such as get_task_ve() take task_lock() and use READ_ONCE()
on task_ve. The writers take neither task_lock nor use WRITE_ONCE().

Before this patch nothing outside ve.h used these helpers. Now every
ovl_override_creds() caller, for example ovl_lookup(), ovl_iterate() and
ovl_copy_up_flags(), flips task_ve for the whole duration of operations that
can sleep. During that window a remote get_task_ve() caller, for example
sched_show_task() or update_sched_lat(), can race with the store. KCSAN
would flag this. The remote reader can also see the creator VE instead of
the task's own VE.

Would it make sense to use WRITE_ONCE() in override_ve() and revert_ve(),
and possibly take task_lock() as well?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022-ovl-use-orig-ve-v1-0-da84c00c8065@virtuozzo.com?part=4

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 1/4] ve: Remove STOPPED state

[Vladimir Riabchun]

On 10/22/26 17:11, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] ve: renumbering VE_STATE_DEAD from 4 to 3 changes the state value logged by setns
> - [Low] ve: commit message claims STOPPED removal blocks VE reuse, but it is only a cleanup
> --
> 
> commit afa07824d11d984ebc29eb501787f9b9b4ecea47
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve: Remove STOPPED state
> 
> This patch merges VE_STATE_STOPPED into VE_STATE_DEAD. The stop path in
> ve_exit_ns() and the error path in ve_start_container() now set DEAD
> directly. It also drops the ve_set_state(DEAD) call in ve_destroy() and
> renumbers VE_STATE_DEAD from 4 to 3.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405
> 
>> diff --git a/include/linux/ve.h b/include/linux/ve.h
>> --- a/include/linux/ve.h
>> +++ b/include/linux/ve.h
>> @@ -40,8 +40,7 @@ struct vfsmount;
>>   #define VE_STATE_STARTING	0
>>   #define VE_STATE_RUNNING	1
>>   #define VE_STATE_STOPPING	2
>> -#define VE_STATE_STOPPED	3
>> -#define VE_STATE_DEAD		4
>> +#define VE_STATE_DEAD		3
> 
> [Severity: Low]
> Is the renumbering of VE_STATE_DEAD from 4 to 3 intended to be visible in
> logs?
> 
> The raw state integer is printed in the setns check in
> kernel/ve/ve_namespace.c:
> 
>      ve_name(ve_ns->ve), ve_ns->ve->state,
> 
> and in the ve_set_state() WARN_ONCE messages. Anything that parses the
> integer in the "setns to the VE namespace of a non-running CT (state %d)"
> message now sees 3 for a stopped CT, where it could previously see 3 or 4.
> 
> The cgroup state file is unaffected, since ve_state_show() still prints
> "STOPPED" for VE_STATE_DEAD.
> 

STOPPED left intentionally, vzctl checks it.
Numbers in warns - don't care, these are debug messages.

> [ ... ]
> 
>> @@ -680,7 +680,7 @@ void ve_exit_ns(struct pid_namespace *pid_ns)
>>   	ve_hook_iterate_fini(VE_SS_CHAIN, ve);
>>   	ve_list_del(ve);
>>   	ve_drop_context(ve);
>> -	ve_set_state(ve, VE_STATE_STOPPED);
>> +	ve_set_state(ve, VE_STATE_DEAD);
>>   	up_write(&ve->op_sem);
> 
> [Severity: Low]
> This isn't a bug, but the commit message doesn't seem to match what the
> patch does. It says:
> 
>      We really don't want to reuse VE after its stop.
>      Some resources from the old run can reference this CT, so new
>      start may see leftovers.
> 
>      Fix this by transitioning from STOPPING to DEAD directly.
> 
> Is that description accurate?
> 
> ve_start_container() already rejected a restart for any state other than
> STARTING:
> 
>      if (ve->state != VE_STATE_STARTING || ve_nsproxy)
>          return -EBUSY;
> 
> STARTING is only assigned in ve_create(). A VE in STOPPED could therefore
> not be restarted before this patch, and a VE in DEAD cannot be restarted
> now.
> 
> The ve_set_state(DEAD) call removed from ve_destroy() wrote to an object
> that kmem_cache_free() releases immediately afterwards:
> 
>      ve_set_state(ve, VE_STATE_DEAD);
>      kmem_cache_free(ve_cachep, ve);
> 
> So DEAD was never observable there.
> 
> The later patches in the series (override_ve/revert_ve and the overlayfs
> creator VE changes) don't check VE_STATE_DEAD or VE_IS_RUNNING. They take
> and drop references with get_ve() and put_ve(). It looks like the
> "leftovers" problem is addressed by those reference changes and not by
> this patch.
> 
> Could the commit message be reworded to say that this is a state machine
> cleanup that merges STOPPED into DEAD and removes a dead store in
> ve_destroy()?

Huh, true. So it just drops useless state. I'll change commit message
in v2.

> 
> [ ... ]
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 2/4] ve: Introduce override_ve/revert_ve helpers

[Vladimir Riabchun]

On 10/22/26 17:10, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] ve: override_ve()/revert_ve() !CONFIG_VE stubs hidden inside CONFIG_X86 block
> - [Low] ve: override_ve()/revert_ve() store task_ve without WRITE_ONCE() or task_lock
> --
> 
> commit f94943d7cf0e7cf13a560e724d14ae8e02ad7271
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve: Introduce override_ve/revert_ve helpers
> 
> This commit adds override_ve() and revert_ve() helpers to include/linux/ve.h.
> They temporarily replace current->task_ve, in the same way that
> override_creds() and revert_creds() handle credentials. Stubs for
> CONFIG_VE=n are added as well.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405
> 
>> diff --git a/include/linux/ve.h b/include/linux/ve.h
>> index 717567775f58..157326ef91b9 100644
>> --- a/include/linux/ve.h
>> +++ b/include/linux/ve.h
>> @@ -295,6 +295,19 @@ static inline struct vdso_image *ve_get_vdso_64(struct ve_struct *ve)
>>   	return ve->vdso_64;
>>   }
>>   
>> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
>> +{
>> +	struct ve_struct *old_ve = current->task_ve;
>> +
>> +	current->task_ve = new_ve;
>> +	return old_ve;
>> +}
>> +
>> +static inline void revert_ve(struct ve_struct *orig_ve)
>> +{
>> +	current->task_ve = orig_ve;
>> +}
> 
> [Severity: Low]
> Can these plain stores to current->task_ve race with remote readers?
> 
> task_ve() reads the field with READ_ONCE(), and get_task_ve() reads it
> under task_lock:
> 
> include/linux/ve.h:
>      return READ_ONCE(task->task_ve);
>      ...
>      guard(task_lock)(task);
>      return get_ve(task_ve(task));
> 
> override_ve() and revert_ve() use neither WRITE_ONCE() nor task_lock, so
> the lock gives get_ve() no exclusion against them. The existing writer
> follows a different convention:
> 
> kernel/ve/ve_namespace.c:switch_ve_namespace() {
>      scoped_guard(task_lock, p) {
>          old = p->ve_ns;
>          p->ve_ns = new;
>          p->task_ve = new ? new->ve : &ve0;
>      }
> }
> 
> A remote task can hit this window while the overlayfs task is between
> ovl_override_creds() and ovl_revert_creds(). For example, /proc code
> may call task_pid_ve_nr(tsk) or get_task_ve(tsk) on that task. The reader
> can then see creator_ve instead of the task's real VE, while ve_ns still
> refers to the original one. task_pid_ve_nr() could then transiently
> return a wrong or zero pid.
> 
> Should both helpers use WRITE_ONCE(), and should the store be done under
> task_lock so that get_task_ve() cannot observe the transient value?

Will add locking and WRITE_ONCE in v2.

> 
>> +
>>   #else	/* CONFIG_VE */
>>   #include <linux/init_task.h>
>>   #include <net/net_namespace.h>
> 
> [ ... ]
> 
>> @@ -403,6 +416,14 @@ static inline struct vdso_image *ve_get_vdso_64(struct vdso_image *ve)
>>   {
>>   	return (struct vdso_image *)&vdso_image_64;
>>   }
>> +
>> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline void revert_ve(struct ve_struct *orig_ve) {}
>> +
>>   #endif /* CONFIG_X86 */
>>   
>>   #endif	/* CONFIG_VE */
> 
> [Severity: Low]
> Are these CONFIG_VE=n stubs placed correctly? They sit inside the
> CONFIG_X86 block, before the closing line:

Nope, wrong place. Will fix in v2.

> 
> #endif /* CONFIG_X86 */
> 
> With CONFIG_VE=n and CONFIG_X86 unset (for example arm64 or UML),
> override_ve() and revert_ve() are never defined. The CONFIG_VE=y versions
> are defined unconditionally.
> 
> Patch 4/4 of the series calls them from the generic file
> fs/overlayfs/util.c, in ovl_override_creds() and ovl_revert_creds():
> 
>      desc.ve = override_ve(ofs->creator_ve);
>      ...
>      revert_ve(old_creds->ve);
> 
> Would that fail to build on those configurations with an implicit
> declaration error? Neither patch 3 nor patch 4 touches include/linux/ve.h,
> so the series does not fix this later. Could the stubs be moved after the
> closing line of the CONFIG_X86 block?
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 4/4] ve/overlayfs: Override VE together with creds

[Vladimir Riabchun]

On 10/22/26 17:17, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Low] overlayfs: typo 'inforamtion' in new struct ovl_creds_desc comment

Will fix in v2.

> - [Low] ve: override_ve()/revert_ve() store current->task_ve without WRITE_ONCE or task_lock
> - [Low] ve: override_ve()/revert_ve() !CONFIG_VE stubs are x86-only; overlayfs now needs them

Already reported, will fix.

> --
> 
> commit f580a7811856a41f48d8fc37ebc70368ebd6d989
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/overlayfs: Override VE together with creds
> 
> This patch changes ovl_override_creds() to return a struct ovl_creds_desc
> holding both the old cred and the old VE. It adds ovl_revert_creds() to
> restore both, and converts all overlayfs callers. The aim is to make
> ve_capable checks behave correctly when a host user accesses an overlayfs
> created inside a container.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405
> 
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
> 
> [ ... ]
> 
>> +/* Credential inforamtion used by overlay */
>> +struct ovl_creds_desc {
>> +	const struct cred *cred;
>> +	struct ve_struct *ve;
>> +};
> 
> [Severity: Low]
> This isn't a bug, but there is a typo in the new comment: "inforamtion"
> should be "information".
> 
> The commit message also has typos: "swich" should be "switch", and
> "Stroing" should be "Storing".
> 
>> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
>> --- a/fs/overlayfs/util.c
>> +++ b/fs/overlayfs/util.c
> 
> [ ... ]
> 
>> +struct ovl_creds_desc ovl_override_creds(struct super_block *sb)
>>   {
>>   	struct ovl_fs *ofs = OVL_FS(sb);
>> +	struct ovl_creds_desc desc;
>>   
>> -	return override_creds(ofs->creator_cred);
>> +	desc.cred = override_creds(ofs->creator_cred);
>> +	desc.ve = override_ve(ofs->creator_ve);
>> +
>> +	return desc;
>> +}
>> +
>> +void ovl_revert_creds(struct ovl_creds_desc *old_creds)
>> +{
>> +	revert_ve(old_creds->ve);
>> +	revert_creds(old_creds->cred);
>>   }
> 
> [Severity: Low]
> Will this build when CONFIG_VE is disabled and CONFIG_X86 is not set, for
> example on UML or on non-x86 architectures?
> 
> In include/linux/ve.h the !CONFIG_VE stubs for override_ve() and
> revert_ve() sit inside the "#ifdef CONFIG_X86" block of the !CONFIG_VE
> branch:
> 
>      #ifdef CONFIG_X86
>      ...
>      static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
>      {
>              return NULL;
>      }
> 
>      static inline void revert_ve(struct ve_struct *orig_ve) {}
> 
>      #endif /* CONFIG_X86 */
> 
>      #endif  /* CONFIG_VE */
> 
> ovl_override_creds() and ovl_revert_creds() now call these helpers
> unconditionally. fs/overlayfs/Kconfig has no dependency on VE or X86, and
> CONFIG_VE is a bool that can be turned off.
> 
> With CONFIG_OVERLAY_FS enabled, CONFIG_VE off and CONFIG_X86 unset, the
> helpers have no declaration, so fs/overlayfs/util.c would fail with an
> implicit declaration error. Should the stubs be moved outside the
> CONFIG_X86 block?
> 
> [Severity: Low]
> The helpers called here, override_ve() and revert_ve() in
> include/linux/ve.h, store current->task_ve with a plain write:
> 
>      struct ve_struct *old_ve = current->task_ve;
> 
>      current->task_ve = new_ve;
>      return old_ve;
> 
> Remote readers such as get_task_ve() take task_lock() and use READ_ONCE()
> on task_ve. The writers take neither task_lock nor use WRITE_ONCE().
> 
> Before this patch nothing outside ve.h used these helpers. Now every
> ovl_override_creds() caller, for example ovl_lookup(), ovl_iterate() and
> ovl_copy_up_flags(), flips task_ve for the whole duration of operations that
> can sleep. During that window a remote get_task_ve() caller, for example
> sched_show_task() or update_sched_lat(), can race with the store. KCSAN
> would flag this. The remote reader can also see the creator VE instead of
> the task's own VE.
> 
> Would it make sense to use WRITE_ONCE() in override_ve() and revert_ve(),
> and possibly take task_lock() as well?
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel