[PATCH 0/2] userns: clean up dead code on error paths

[PATCH 0/2] userns: clean up dead code on error paths

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Two small cleanups on the create_user_ns() and map_write() error
paths.  Both remove code that provably cannot have an effect; no
behavior change.

  user_namespace.c:164-167: the key_put() at fail_keyring has been
  dead since it was copy-pasted from free_user_ns() in 2016
  (dbec28460a89); persistent_keyring_register is only ever assigned
  lazily from key_get_persistent(), long after create_user_ns().

  user_namespace.c:1110-1112: the map clearing at the out: label of
  map_write() only ever zeroes an already-zero map; the destination
  map is untouched on every error path, and a second write to a
  mapped namespace is rejected before parsing.

Compile-tested and A/B-tested on next-20260827: identical behavior
on vanilla and patched kernels for single-write map failures,
recovery writes and 300 rounds of userns create/destroy.

Tao Cui (2):
  userns: remove dead key_put() on the create_user_ns() error path
  userns: don't clear the install target map on map_write() failure

 kernel/user_namespace.c | 6 ------
 1 file changed, 6 deletions(-)

-- 
2.43.0

[PATCH 1/2] userns: remove dead key_put() on the create_user_ns() error path

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

create_user_ns() jumps to fail_keyring when setup_userns_sysctls()
fails.  At that point ns was freshly allocated with
kmem_cache_zalloc() and ns->persistent_keyring_register is only ever
assigned later, lazily, from key_get_persistent()
(security/keys/persistent.c).  The key_put() therefore always
receives NULL.

Remove the dead call.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 kernel/user_namespace.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 0bed462e9b2a..e9e04ce167df 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -162,9 +162,6 @@ int create_user_ns(struct cred *new)
 	ns_tree_add(ns);
 	return 0;
 fail_keyring:
-#ifdef CONFIG_PERSISTENT_KEYRINGS
-	key_put(ns->persistent_keyring_register);
-#endif
 	ns_common_free(ns);
 fail_free:
 	kmem_cache_free(user_ns_cachep, ns);
-- 
2.43.0

Re: [PATCH 1/2] userns: remove dead key_put() on the create_user_ns() error path

[Jan Kara]

On Fri 28-08-26 17:16:58, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> create_user_ns() jumps to fail_keyring when setup_userns_sysctls()
> fails.  At that point ns was freshly allocated with
> kmem_cache_zalloc() and ns->persistent_keyring_register is only ever
> assigned later, lazily, from key_get_persistent()
> (security/keys/persistent.c).  The key_put() therefore always
> receives NULL.
> 
> Remove the dead call.
> 
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>

Adding Eric to CC for verification but yes, that code seems to be dead
since its introduction by Eric 10 years ago. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  kernel/user_namespace.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 0bed462e9b2a..e9e04ce167df 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -162,9 +162,6 @@ int create_user_ns(struct cred *new)
>  	ns_tree_add(ns);
>  	return 0;
>  fail_keyring:
> -#ifdef CONFIG_PERSISTENT_KEYRINGS
> -	key_put(ns->persistent_keyring_register);
> -#endif
>  	ns_common_free(ns);
>  fail_free:
>  	kmem_cache_free(user_ns_cachep, ns);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] userns: remove dead key_put() on the create_user_ns() error path

[Bradley Morgan]

On 28 August 2026 10:16:58 BST, Tao Cui <cui.tao@linux.dev> wrote:
>From: Tao Cui <cuitao@kylinos.cn>
>
>create_user_ns() jumps to fail_keyring when setup_userns_sysctls()
>fails.  At that point ns was freshly allocated with
>kmem_cache_zalloc() and ns->persistent_keyring_register is only ever
>assigned later, lazily, from key_get_persistent()
>(security/keys/persistent.c).  The key_put() therefore always
>receives NULL.
>
>Remove the dead call.
>

Agreed,

Reviewed-by: Bradley Morgan <brads@mainlining.org>

>Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>---
> kernel/user_namespace.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>index 0bed462e9b2a..e9e04ce167df 100644
>--- a/kernel/user_namespace.c
>+++ b/kernel/user_namespace.c
>@@ -162,9 +162,6 @@ int create_user_ns(struct cred *new)
> 	ns_tree_add(ns);
> 	return 0;
> fail_keyring:
>-#ifdef CONFIG_PERSISTENT_KEYRINGS
>-	key_put(ns->persistent_keyring_register);
>-#endif
> 	ns_common_free(ns);
> fail_free:
> 	kmem_cache_free(user_ns_cachep, ns);
>


--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/

[PATCH 2/2] userns: don't clear the install target map on map_write() failure

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

At the out: label of map_write() the destination map is also cleared
(map->forward = NULL; map->reverse = NULL; map->nr_extents = 0)
whenever the write failed and the extent arrays had to be freed.

However, the destination map is written by the successful install
block above, which has no failure exit; every error path reaches
out: without having touched it.  A second write to a mapped
namespace is also rejected with -EPERM before any parsing happens,
so the clearing can never roll back a previously installed map
either.

The three assignments just zero an already-zero map.  Remove them.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 kernel/user_namespace.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index e9e04ce167df..cef5e71779f6 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -1104,9 +1104,6 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 	if (ret < 0 && new_map.nr_extents > UID_GID_MAP_MAX_BASE_EXTENTS) {
 		kfree(new_map.forward);
 		kfree(new_map.reverse);
-		map->forward = NULL;
-		map->reverse = NULL;
-		map->nr_extents = 0;
 	}
 
 	mutex_unlock(&userns_state_mutex);
-- 
2.43.0

Re: [PATCH 2/2] userns: don't clear the install target map on map_write() failure

[Jan Kara]

On Fri 28-08-26 17:16:59, Tao Cui wrote:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> At the out: label of map_write() the destination map is also cleared
> (map->forward = NULL; map->reverse = NULL; map->nr_extents = 0)
> whenever the write failed and the extent arrays had to be freed.
> 
> However, the destination map is written by the successful install
> block above, which has no failure exit; every error path reaches
> out: without having touched it.  A second write to a mapped
> namespace is also rejected with -EPERM before any parsing happens,
> so the clearing can never roll back a previously installed map
> either.
> 
> The three assignments just zero an already-zero map.  Remove them.
> 
> Signed-off-by: Tao Cui <cuitao@kylinos.cn>

I agree this is a dead code but it kind of makes it easier to argue about
the correctness of the error handling branch. I guess I'll leave it to for
Christian to decide whether he wants to take this or not.

								Honza

> ---
>  kernel/user_namespace.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index e9e04ce167df..cef5e71779f6 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -1104,9 +1104,6 @@ static ssize_t map_write(struct file *file, const char __user *buf,
>  	if (ret < 0 && new_map.nr_extents > UID_GID_MAP_MAX_BASE_EXTENTS) {
>  		kfree(new_map.forward);
>  		kfree(new_map.reverse);
> -		map->forward = NULL;
> -		map->reverse = NULL;
> -		map->nr_extents = 0;
>  	}
>  
>  	mutex_unlock(&userns_state_mutex);
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] userns: don't clear the install target map on map_write() failure

[Bradley Morgan]

On 28 August 2026 10:16:59 BST, Tao Cui <cui.tao@linux.dev> wrote:
>From: Tao Cui <cuitao@kylinos.cn>
>
>At the out: label of map_write() the destination map is also cleared
>(map->forward = NULL; map->reverse = NULL; map->nr_extents = 0)
>whenever the write failed and the extent arrays had to be freed.
>
>However, the destination map is written by the successful install
>block above, which has no failure exit; every error path reaches
>out: without having touched it.  A second write to a mapped
>namespace is also rejected with -EPERM before any parsing happens,
>so the clearing can never roll back a previously installed map
>either.
>
>The three assignments just zero an already-zero map.  Remove them.
>

Thinking about NAKing, but I have comments.

>Signed-off-by: Tao Cui <cuitao@kylinos.cn>
>---
> kernel/user_namespace.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>index e9e04ce167df..cef5e71779f6 100644
>--- a/kernel/user_namespace.c
>+++ b/kernel/user_namespace.c
>@@ -1104,9 +1104,6 @@ static ssize_t map_write(struct file *file, const char __user *buf,
> 	if (ret < 0 && new_map.nr_extents > UID_GID_MAP_MAX_BASE_EXTENTS) {
> 		kfree(new_map.forward);
> 		kfree(new_map.reverse);
>-		map->forward = NULL;
>-		map->reverse = NULL;
>-		map->nr_extents = 0;

Yup, that's dead code, and I would add a tag here, but the thing is, this
code is a defensive pattern, so if some bloke decided "hmmmm let's add a
failure path", then that would be stupid if this code was gone.

If I was the maintainer, I wouldn't take it.


> 	}
> 
> 	mutex_unlock(&userns_state_mutex);
>

--- Thanks!
https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/