[Devel] [PATCH vz10] fs/kernfs, ve: pin the VE that mounted a kernfs instance

[Devel] [PATCH vz10] fs/kernfs, ve: pin the VE that mounted a kernfs instance

[Konstantin Khorenko]

kernfs_get_tree() stores a bare get_exec_env() pointer in
kernfs_super_info, and since commit 8d1abf587552 ("fs/kernfs, ve:
hide entries from a VE without invalidating the dentry")
kernfs_dop_revalidate() compares it against the caller's VE to decide
between dropping the dentry and answering -ENOENT. Nothing backs that
pointer with a reference: if the mount is pinned from outside - the
host bind-mounted a Container's sysfs, or holds an fd inside it - the
superblock outlives the Container, info->ve dangles, and a later
Container whose ve_struct is allocated at the same address is mistaken
for the creator of the instance. Its lookups then take the
d_invalidate() path on the dead instance and detach the mounts placed
on top of it.

Take a css reference on the VE for the lifetime of the superblock, so
the comparison always names the VE that really created the instance.

The assignment in kernfs_set_ve_perms() is dropped instead of
converted: it runs right after kernfs_get_tree() in the same mount
context, so it always rewrites the field with the same value.

Fixes: 8d1abf587552 ("fs/kernfs, ve: hide entries from a VE without invalidating the dentry")
https://virtuozzo.atlassian.net/browse/VSTOR-142552

Feature: kernfs: per-CT entries visibility and permissions configuration
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
---
kernfs_get_tree() stores a bare get_exec_env() pointer in
kernfs_super_info, and since commit 8d1abf587552 ("fs/kernfs, ve:
hide entries from a VE without invalidating the dentry")
kernfs_dop_revalidate() compares it against the caller's VE to decide
between dropping the dentry and answering -ENOENT. Nothing backs that
pointer with a reference: if the mount is pinned from outside - the
host bind-mounted a Container's sysfs, or holds an fd inside it - the
superblock outlives the Container, info->ve dangles, and a later
Container whose ve_struct is allocated at the same address is mistaken
for the creator of the instance. Its lookups then take the
d_invalidate() path on the dead instance and detach the mounts placed
on top of it.

Take a css reference on the VE for the lifetime of the superblock, so
the comparison always names the VE that really created the instance.

The assignment in kernfs_set_ve_perms() is dropped instead of
converted: it runs right after kernfs_get_tree() in the same mount
context, so it always rewrites the field with the same value.

Fixes: 8d1abf587552 ("fs/kernfs, ve: hide entries from a VE without invalidating the dentry")
https://virtuozzo.atlassian.net/browse/VSTOR-142552
---
 fs/kernfs/mount.c | 13 ++++++++++---
 fs/kernfs/ve.c    |  1 -
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index 14a08ba6ae57d..d4bf659b5f8fe 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -18,6 +18,7 @@
 #include <linux/exportfs.h>
 #include <linux/uuid.h>
 #include <linux/statfs.h>
+#include <linux/ve.h>
 
 #include "kernfs-internal.h"
 #include "kernfs-ve.h"
@@ -362,7 +363,7 @@ int kernfs_get_tree(struct fs_context *fc)
 	info->root = kfc->root;
 	info->ns = kfc->ns_tag;
 	INIT_LIST_HEAD(&info->node);
-	info->ve = get_exec_env();
+	info->ve = get_ve(get_exec_env());
 
 	fc->s_fs_info = info;
 	sb = sget_fc(fc, kernfs_test_super, kernfs_set_super);
@@ -397,9 +398,14 @@ int kernfs_get_tree(struct fs_context *fc)
 
 void kernfs_free_fs_context(struct fs_context *fc)
 {
+	struct kernfs_super_info *info = fc->s_fs_info;
+
 	/* Note that we don't deal with kfc->ns_tag here. */
-	kfree(fc->s_fs_info);
-	fc->s_fs_info = NULL;
+	if (info) {
+		put_ve(info->ve);
+		kfree(info);
+		fc->s_fs_info = NULL;
+	}
 }
 
 /**
@@ -424,6 +430,7 @@ void kernfs_kill_sb(struct super_block *sb)
 	 * so we can't find it, before freeing kernfs_super_info.
 	 */
 	kill_anon_super(sb);
+	put_ve(info->ve);
 	kfree(info);
 }
 
diff --git a/fs/kernfs/ve.c b/fs/kernfs/ve.c
index 630ea34d1ccab..1097fd30598e3 100644
--- a/fs/kernfs/ve.c
+++ b/fs/kernfs/ve.c
@@ -28,7 +28,6 @@ void kernfs_set_ve_perms(struct dentry *root, off_t key_off)
 	struct kernfs_super_info *info = kernfs_info(root->d_sb);
 
 	info->ve_perms_off = key_off;
-	info->ve = get_exec_env();
 }
 
 int kernfs_init_ve_perms(struct kernfs_root *root,

---
base-commit: 0679f0d70523972eec3a0643565521d93e26eb9c
change-id: 20260928-prep-kh-vstor-142552-kernfs-pin-ve-db8e7eb89201

Best regards,
--  
Konstantin Khorenko <khorenko@virtuozzo.com>

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

Re: [Devel] [PATCH vz10] fs/kernfs, ve: pin the VE that mounted a kernfs instance

[Pavel Tikhomirov]

> kernfs_get_tree() stores a bare get_exec_env() pointer in
> kernfs_super_info, and since commit 8d1abf587552 ("fs/kernfs, ve:
> hide entries from a VE without invalidating the dentry")
> kernfs_dop_revalidate() compares it against the caller's VE to decide
> between dropping the dentry and answering -ENOENT. Nothing backs that
> pointer with a reference: if the mount is pinned from outside - the
> host bind-mounted a Container's sysfs, or holds an fd inside it - the
> superblock outlives the Container, info->ve dangles, and a later
> Container whose ve_struct is allocated at the same address is mistaken
> for the creator of the instance. Its lookups then take the
> d_invalidate() path on the dead instance and detach the mounts placed
> on top of it.
> 
> Take a css reference on the VE for the lifetime of the superblock, so
> the comparison always names the VE that really created the instance.

This race of ve pointer reused and superblock being given to other ve
(the one which reused the pointer) is supper low probability. Each new
ve will get its own netns and thus it's own sysfs sb, it wont ever look
on the old ve superblock and thus on this stale matching ve pointer.

So practically we can avoid extra reference to ve here, as an Admin
giving old ve kernfs superblock to a new ve is not realy our usecase and
only breaks sysfs visibility.

Except that looks good.

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

-- 
Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH vz10] fs/kernfs, ve: pin the VE that mounted a kernfs instance

[Mirian Shilakadze]

On 9/28/26 8:41 PM, Konstantin Khorenko wrote:
> kernfs_get_tree() stores a bare get_exec_env() pointer in
> kernfs_super_info, and since commit 8d1abf587552 ("fs/kernfs, ve:
> hide entries from a VE without invalidating the dentry")
> kernfs_dop_revalidate() compares it against the caller's VE to decide
> between dropping the dentry and answering -ENOENT. Nothing backs that
> pointer with a reference: if the mount is pinned from outside - the
> host bind-mounted a Container's sysfs, or holds an fd inside it - the
> superblock outlives the Container, info->ve dangles, and a later
> Container whose ve_struct is allocated at the same address is mistaken
> for the creator of the instance. Its lookups then take the
> d_invalidate() path on the dead instance and detach the mounts placed
> on top of it.
> 
> Take a css reference on the VE for the lifetime of the superblock, so
> the comparison always names the VE that really created the instance.
> 
> The assignment in kernfs_set_ve_perms() is dropped instead of
> converted: it runs right after kernfs_get_tree() in the same mount
> context, so it always rewrites the field with the same value.
> 
> Fixes: 8d1abf587552 ("fs/kernfs, ve: hide entries from a VE without invalidating the dentry")
> https://virtuozzo.atlassian.net/browse/VSTOR-142552
> 
> Feature: kernfs: per-CT entries visibility and permissions configuration
> Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> ---
> kernfs_get_tree() stores a bare get_exec_env() pointer in
> kernfs_super_info, and since commit 8d1abf587552 ("fs/kernfs, ve:
> hide entries from a VE without invalidating the dentry")
> kernfs_dop_revalidate() compares it against the caller's VE to decide
> between dropping the dentry and answering -ENOENT. Nothing backs that
> pointer with a reference: if the mount is pinned from outside - the
> host bind-mounted a Container's sysfs, or holds an fd inside it - the
> superblock outlives the Container, info->ve dangles, and a later
> Container whose ve_struct is allocated at the same address is mistaken
> for the creator of the instance. Its lookups then take the
> d_invalidate() path on the dead instance and detach the mounts placed
> on top of it.
> 
> Take a css reference on the VE for the lifetime of the superblock, so
> the comparison always names the VE that really created the instance.

Nit: in the subject, "mounted" would better as "created". A later mount 
of an existing instance reuses the superblock, so only the creator's VE 
is pinned.

FWIW, the comparison was never needed for the fix. v1 returned -ENOENT 
for every hidden entry and fixed the container start on its own. The 
comparison came in v2, after Pavel suggested in review keeping the old 
invalidate inside the CT. He later wrote on the v1 cover letter that 
invalidating isn't really needed. It's the only reader of info->ve, so 
dropping it would also close this.

Reviewed-by: Mirian Shilakadze <mirian.shilakadze@virtuozzo.com>

-- 
Best regards, Mirian Shilakadze
Linux C Virtualization Developer, Virtuozzo.

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