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

[Devel] [PATCH hci-8.1 v3 0/5] 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>
---
Changes in v3:
- Patch 2: added a separate field for ve_override to fix problems Pasha spotted.
- Patch 3: new one, delays init_cred destroy so dead CT can still use it.
- Link to v2: http://lore.virtuozzo.com/20261026-ovl-use-orig-ve-v2-0-eab58bfc29ca@virtuozzo.com

Changes in v2:
- Patch 1: updated description as Sashiko suggested..
- Patch 2: moved stubs outside CONFIG_X86 section,
  added task_lock protection in helpers. Verified via lockdep,
  no splats after vzctl tests, C/R and crash reproducer.
- Patch 4: fixed typo in comment.
- Link to v1: http://lore.virtuozzo.com/20261022-ovl-use-orig-ve-v1-0-da84c00c8065@virtuozzo.com

---
Vladimir Riabchun (5):
      ve: Remove STOPPED state
      ve: Introduce override_ve/revert_ve helpers
      ve: Defer ve context drop until ve is completely destroyed
      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/sched.h    |  1 +
 include/linux/ve.h       | 27 +++++++++++++++++++++++++--
 kernel/capability.c      |  6 ++++--
 kernel/ve/ve.c           | 10 ++++++----
 16 files changed, 140 insertions(+), 88 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 v3 1/5] ve: Remove STOPPED state

[Vladimir Riabchun]

We don't need extra STOPPED state. It makes us feel that CT
could be started again from it, but it is actually not true.

Separate STOPPED stated doesn't bring any value. Drop it and
transition VE to DEAD state directly on the stop.

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

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

[no-reply]

> We don't need extra STOPPED state. It makes us feel that CT
> could be started again from it, but it is actually not true.
> 
> Separate STOPPED stated doesn't bring any value. Drop it and
> transition VE to DEAD state directly on the stop.
> 
> 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/20261103-ovl-use-orig-ve-v3-0-add042cdd087@virtuozzo.com?part=1


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

[Devel] [PATCH hci-8.1 v3 2/5] 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.

To keep task_ve consistent for other users, introduce a separate
task->overrided_ve field. Nobody should race on it since its
updates are supposed to go with override_creds, and non-current
processes shouldn't do this.

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

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 10545e470d3d..7f4e6b1fde38 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1065,6 +1065,7 @@ struct task_struct {
 #ifdef CONFIG_VE
 	struct ve_struct		*task_ve;
 	struct ve_namespace		*ve_ns;
+	struct ve_struct		*overrided_ve; /* NULL when no overrides */
 #endif
 #ifdef CONFIG_PREEMPT_RT
 	struct netdev_xmit		net_xmit;
diff --git a/include/linux/ve.h b/include/linux/ve.h
index 717567775f58..c07484b4eee2 100644
--- a/include/linux/ve.h
+++ b/include/linux/ve.h
@@ -295,6 +295,23 @@ 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->overrided_ve;
+
+	/* CT tasks are not supposed to switch VE context, this is a breach */
+	if (WARN_ON(new_ve != get_exec_env() && !ve_is_super(get_exec_env())))
+		return ERR_PTR(-EPERM);
+
+	current->overrided_ve = new_ve;
+	return old_ve;
+}
+
+static inline void revert_ve(struct ve_struct *orig_ve)
+{
+	current->overrided_ve = orig_ve;
+}
+
 #else	/* CONFIG_VE */
 #include <linux/init_task.h>
 #include <net/net_namespace.h>
@@ -405,6 +422,13 @@ static inline struct vdso_image *ve_get_vdso_64(struct ve_struct *ve)
 }
 #endif /* 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_VE */
 
 struct seq_file;
diff --git a/kernel/capability.c b/kernel/capability.c
index 34716847c6b8..6de4dcba2283 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -389,10 +389,11 @@ EXPORT_SYMBOL(ns_capable);
 bool ve_capable(int cap)
 {
 	struct cred *cred;
+	struct ve_struct *ve = current->overrided_ve ?: get_exec_env();
 	bool ret;
 
 	rcu_read_lock();
-	cred = get_exec_env()->init_cred;
+	cred = ve->init_cred;
 
 	if (cred == NULL) /* ve isn't running */
 		cred = ve0.init_cred;
@@ -406,10 +407,11 @@ bool ve_capable(int cap)
 bool ve_capable_noaudit(int cap)
 {
 	struct cred *cred;
+	struct ve_struct *ve = current->overrided_ve ?: get_exec_env();
 	bool ret;
 
 	rcu_read_lock();
-	cred = get_exec_env()->init_cred;
+	cred = ve->init_cred;
 
 	if (cred == NULL) /* ve isn't running */
 		cred = ve0.init_cred;

-- 
2.52.0

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ve: revert_ve() can store ERR_PTR from override_ve() into overrided_ve
- [High] ve: override_ve() ERR_PTR(-EPERM) stored into current->overrided_ve by revert_ve()
- [Medium] ve: task->overrided_ve is inherited across fork and never reset in copy_process()
--

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

ve: Introduce override_ve/revert_ve helpers

This adds override_ve() and revert_ve(), which mirror
override_creds()/revert_creds(). It also adds a task->overrided_ve field.
ve_capable() and ve_capable_noaudit() now use that field in preference to
get_exec_env().

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

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h

[ ... ]

> @@ -1065,6 +1065,7 @@ struct task_struct {
>  #ifdef CONFIG_VE
>  	struct ve_struct		*task_ve;
>  	struct ve_namespace		*ve_ns;
> +	struct ve_struct		*overrided_ve; /* NULL when no overrides */
>  #endif

[Severity: Medium]
Can this pointer be inherited across fork? dup_task_struct() copies the
whole task_struct, so a child created while the parent is between
override_ve() and revert_ve() gets the parent's non-NULL overrided_ve.

copy_process()->copy_ve_ns() only sets these fields:

    p->ve_ns = new_ve_ns;
    p->task_ve = new_ve_ns->ve;

It never clears overrided_ve, so the comment "NULL when no overrides" may
not hold for the child. The child never runs revert_ve(), because that
only acts on current.

ve_capable() and ve_capable_noaudit() in the child would then use the
parent's overriding VE init_cred instead of the child's own
get_exec_env().

I did not find a concrete in-tree caller that forks inside the window.
Should overrided_ve be reset to NULL at fork time?

> diff --git a/include/linux/ve.h b/include/linux/ve.h
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h

[ ... ]

> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
> +{
> +	struct ve_struct *old_ve = current->overrided_ve;
> +
> +	/* CT tasks are not supposed to switch VE context, this is a breach */
> +	if (WARN_ON(new_ve != get_exec_env() && !ve_is_super(get_exec_env())))
> +		return ERR_PTR(-EPERM);
> +
> +	current->overrided_ve = new_ve;
> +	return old_ve;
> +}
> +
> +static inline void revert_ve(struct ve_struct *orig_ve)
> +{
> +	current->overrided_ve = orig_ve;
> +}

[Severity: High]
Can revert_ve() store an ERR_PTR into current->overrided_ve? When the
WARN_ON condition holds, override_ve() returns ERR_PTR(-EPERM) and leaves
overrided_ve unchanged. That return value has the same type as a valid
previous-override cookie.

revert_ve() has no IS_ERR() check and stores whatever it is given:

    current->overrided_ve = orig_ve;

In the final series state, ovl_override_creds() does this:

fs/overlayfs/util.c:ovl_override_creds() {
    ...
    desc.ve = override_ve(ofs->creator_ve);
    ...
}

It does not check the result with IS_ERR(). ovl_revert_creds() then calls
revert_ve(old_creds->ve) unconditionally.

A container task that uses an overlayfs superblock whose creator_ve
differs from its own VE, for example one created by the host, would hit
the WARN_ON and end up with ERR_PTR(-EPERM) in current->overrided_ve.

> diff --git a/kernel/capability.c b/kernel/capability.c
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -389,10 +389,11 @@ EXPORT_SYMBOL(ns_capable);
>  bool ve_capable(int cap)
>  {
>  	struct cred *cred;
> +	struct ve_struct *ve = current->overrided_ve ?: get_exec_env();
>  	bool ret;
>  
>  	rcu_read_lock();
> -	cred = get_exec_env()->init_cred;
> +	cred = ve->init_cred;

[Severity: High]
Can this dereference an ERR_PTR? ERR_PTR(-EPERM) is non-NULL, so the ?:
expression selects it. The next ve_capable() or ve_capable_noaudit() call
by that task then reads ve->init_cred through an invalid pointer.
ve_capable_noaudit() has the same pattern.

Should override_ve() callers check IS_ERR(), and should revert_ve() skip
error pointers?

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

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

[Devel] [PATCH hci-8.1 v3 3/5] ve: Defer ve context drop until ve is completely destroyed

[Vladimir Riabchun]

We may have a case, when VE is dead, but there are some users of it,
overlayfs superblock for example.

This user may want to use VE creds, so we need to keep ve->init_cred alive.
Drop creds in ve_destroy. We need to drop ve_nsproxy early because
ploop waits for refcount going to zero, dropping ve_nsproxy in destroy
causes CT suspend/resume hangs.

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

diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
index d74f74ca958a..0dc3778b6ba0 100644
--- a/kernel/ve/ve.c
+++ b/kernel/ve/ve.c
@@ -377,7 +377,10 @@ static void ve_drop_context(struct ve_struct *ve)
 	rcu_assign_pointer(ve->ve_nsproxy, NULL);
 	synchronize_rcu();
 	put_nsproxy(ve_nsproxy);
+}
 
+static void ve_drop_cred(struct ve_struct *ve)
+{
 	put_cred(ve->init_cred);
 	ve->init_cred = NULL;
 }
@@ -925,6 +928,7 @@ static void ve_destroy(struct cgroup_subsys_state *css)
 {
 	struct ve_struct *ve = css_to_ve(css);
 
+	ve_drop_cred(ve);
 	free_ve_devmnts(ve);
 
 	kmapset_unlink(&ve->sysfs_perms_key, &sysfs_ve_perms_set);

-- 
2.52.0

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

Re: [Devel] [PATCH hci-8.1 v3 3/5] ve: Defer ve context drop until ve is completely destroyed

[no-reply]

> We may have a case, when VE is dead, but there are some users of it,
> overlayfs superblock for example.
> 
> This user may want to use VE creds, so we need to keep ve->init_cred alive.
> Drop creds in ve_destroy. We need to drop ve_nsproxy early because
> ploop waits for refcount going to zero, dropping ve_nsproxy in destroy
> causes CT suspend/resume hangs.
> 
> 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/20261103-ovl-use-orig-ve-v3-0-add042cdd087@virtuozzo.com?part=3


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

[Devel] [PATCH hci-8.1 v3 4/5] 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

Re: [Devel] [PATCH hci-8.1 v3 4/5] 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/20261103-ovl-use-orig-ve-v3-0-add042cdd087@virtuozzo.com?part=4


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

[Devel] [PATCH hci-8.1 v3 5/5] 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..eb2fb473a64b 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 information 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 v3 5/5] ve/overlayfs: Override VE together with creds

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] overlayfs: ovl_override_creds() ignores ERR_PTR from override_ve()
--

commit da24c0ddbfb00d7b0ed397f4ffa68728b9aecd62
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 also adds ovl_revert_creds()
to restore both. All overlayfs callers are converted to the new pair of
helpers.

> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> --- 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);

[Severity: High]
Can override_ve() return an error pointer here that goes unchecked?

override_ve() in include/linux/ve.h has this early return:

    if (WARN_ON(new_ve != get_exec_env() && !ve_is_super(get_exec_env())))
        return ERR_PTR(-EPERM);

ofs->creator_ve is set at mount time, so it can differ from the VE of the
task using the filesystem. One example is a CT task using an overlay mounted
by the host or by another CT, through a bind mount or setns().

In that case override_ve() returns ERR_PTR(-EPERM) without setting
current->overrided_ve. The error pointer is stored in desc.ve, and the
overlay operation carries on.

> +
> +	return desc;
> +}
> +
> +void ovl_revert_creds(struct ovl_creds_desc *old_creds)
> +{
> +	revert_ve(old_creds->ve);

Can this store the error pointer in current->overrided_ve?

Every call site pairs ovl_override_creds() with ovl_revert_creds(). On the
path above, revert_ve(old_creds->ve) would then do:

    current->overrided_ve = ERR_PTR(-EPERM);

A nested ovl_override_creds() would also save this bogus value as its old VE.

ve_capable() and ve_capable_noaudit() in kernel/capability.c compute:

    struct ve_struct *ve = current->overrided_ve ?: get_exec_env();
    ...
    cred = ve->init_cred;

The error pointer is non-NULL, so it would be dereferenced as a ve_struct.
That would be an oops on any later capability check by that task, including
paths that never touch overlayfs.

The WARN_ON in override_ve() would also fire on every overlay operation that
takes this path.

Would it make sense to check IS_ERR() in ovl_override_creds() and record that
no VE override was done? ovl_revert_creds() could then skip revert_ve() in
that case. Alternatively, the VE override could be limited to tasks running
in ve0.

> +	revert_creds(old_creds->cred);
>  }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-ovl-use-orig-ve-v3-0-add042cdd087@virtuozzo.com?part=5

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